launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04577
[Merge] lp:~wallyworld/launchpad/remove-answer-contact-823945 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/remove-answer-contact-823945 into lp:launchpad with lp:~wallyworld/launchpad/question-contacts-portlet-view as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #823945 in Launchpad itself: "Allow users with authority to remove answer contacts"
https://bugs.launchpad.net/launchpad/+bug/823945
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/remove-answer-contact-823945/+merge/71304
Wire up the new answer contacts portlet.
== Implementation ==
As with the question subscriptions portlet, provide some javascript to instantiate the generic subscribers_list with the correct config parameters.
The subscribers_list portlet has new configuration parameters so that text displayed to the user says "answer contacts" instead of "subscribers". Also, the web service api methods are configurable, since we need add/removeAnswerContact instead of subscribe/unsubscribe.
== QA ==
Goto a question target eg http://answers.launchpad.dev/ubuntu
The answer contacts portlet should load asynchronously and provide red "Remove" icons next to those people who the logged in user can remove.
== Tests ==
Add new yui test: lp/answers/javascript/tests/test_answercontacts.js
Update lp/app/javascript/subscribers/tests/test_subscribers_list.js
New tests:
test_SubscribersLoader_default_config_parameters
test_SubscribersLoader_default_config_override
test_addSubscriber_no_levels
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/answers/javascript/answercontacts.js
lib/lp/answers/javascript/tests/test_answercontacts.html
lib/lp/answers/javascript/tests/test_answercontacts.js
lib/lp/answers/templates/questiontarget-portlet-answercontacts.pt
lib/lp/app/javascript/subscribers/subscribers_list.js
lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js
--
https://code.launchpad.net/~wallyworld/launchpad/remove-answer-contact-823945/+merge/71304
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/remove-answer-contact-823945 into lp:launchpad.
=== added file 'lib/lp/answers/javascript/answercontacts.js'
--- lib/lp/answers/javascript/answercontacts.js 1970-01-01 00:00:00 +0000
+++ lib/lp/answers/javascript/answercontacts.js 2011-08-12 05:51:26 +0000
@@ -0,0 +1,45 @@
+/* Copyright 2011 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Setup for managing subscribers list for questions.
+ *
+ * @module answers
+ * @submodule subscribers
+ */
+
+YUI.add('lp.answers.answercontacts', function(Y) {
+
+var namespace = Y.namespace('lp.answers.answercontacts');
+
+/**
+ * Create the SubscribersLoader instance which will load answer contacts for
+ * a question and put them in the web page.
+ */
+function createQuestionAnswerContactsLoader(setup_config) {
+ var questiontarget = {
+ self_link: LP.cache.context.self_link,
+ web_link: LP.cache.context.web_link };
+ var default_config = {
+ container_box: '#answer-contacts',
+ subscribers_details_view:
+ '/+portlet-answercontacts-details',
+ subscriber_levels: {},
+ context: questiontarget,
+ display_me_in_list: true,
+ subscribers_label: 'answer contacts',
+ unsubscribe_label: 'Remove',
+ unsubscribe_api: 'removeAnswerContact'
+ };
+ var module = Y.lp.app.subscribers.subscribers_list;
+
+ if (Y.Lang.isValue(setup_config)) {
+ setup_config = Y.mix(setup_config, default_config);
+ } else {
+ setup_config = default_config;
+ }
+ return new module.SubscribersLoader(setup_config);
+}
+namespace.createQuestionAnswerContactsLoader
+ = createQuestionAnswerContactsLoader;
+
+}, "0.1", {"requires": ["lp.app.subscribers.subscribers_list"]});
=== added file 'lib/lp/answers/javascript/tests/test_answercontacts.html'
--- lib/lp/answers/javascript/tests/test_answercontacts.html 1970-01-01 00:00:00 +0000
+++ lib/lp/answers/javascript/tests/test_answercontacts.html 2011-08-12 05:51:26 +0000
@@ -0,0 +1,55 @@
+<html>
+ <head>
+ <title>Question answer contacts: answer contacts list</title>
+
+ <!-- YUI and test setup -->
+ <script type="text/javascript"
+ src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
+ </script>
+ <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
+ <script type="text/javascript"
+ src="../../../app/javascript/testing/testrunner.js"></script>
+
+ <script type="text/javascript"
+ src="../../../app/javascript/client.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/errors.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/lp-names.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/anim/anim.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/extras/extras.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/formoverlay/formoverlay.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/overlay/overlay.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/anim/anim.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/activator/activator.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/lazr/lazr.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/picker/picker_patcher.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/picker/picker.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/picker/person_picker.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/subscribers/subscribers_list.js"></script>
+
+ <!-- The module under test -->
+ <script type="text/javascript"
+ src="../subscribers.js"></script>
+
+ <!-- The test suite -->
+ <script type="text/javascript"
+ src="test_subscribers.js"></script>
+
+ </head>
+ <body class="yui3-skin-sam">
+ <!-- Example markup required by test suite -->
+ <div id="test-root"></div>
+ </body>
+</html>
=== added file 'lib/lp/answers/javascript/tests/test_answercontacts.js'
--- lib/lp/answers/javascript/tests/test_answercontacts.js 1970-01-01 00:00:00 +0000
+++ lib/lp/answers/javascript/tests/test_answercontacts.js 2011-08-12 05:51:26 +0000
@@ -0,0 +1,61 @@
+YUI().use('lp.testing.runner', 'test', 'console', 'node', 'lazr.picker',
+ 'lp.answers.answercontacts',
+ 'event', 'node-event-simulate', 'dump', function(Y) {
+
+var suite = new Y.Test.Suite("lp.answers.answercontacts Tests");
+var module = Y.lp.answers.subscribers;
+
+
+suite.add(new Y.Test.Case({
+ name: 'QuestionAnswerContactsList constructor test',
+
+ setUp: function() {
+ this.root = Y.Node.create('<div />');
+ Y.one('body').appendChild(this.root);
+ window.LP = {
+ links: {},
+ cache: { context: { web_link: "/~link", self_link: "/~link" }}
+ };
+ },
+
+ tearDown: function() {
+ this.root.remove();
+ delete window.LP;
+ },
+
+ setUpLoader: function() {
+ this.root.appendChild(
+ Y.Node.create('<div />').addClass('container'));
+ return new module.createQuestionSubscribersLoader({
+ container_box: '.container'});
+ },
+
+ test_contacts_list_instantiation: function() {
+ this.setUpLoader();
+ },
+
+ test_addContact: function() {
+ // Check that the contact list has been created and can accept
+ // new contacts. Answer contacts do not use subscription levels so
+ // pass in '' and check this works as expected.
+ var loader = this.setUpLoader(this.root);
+ var node = loader.subscribers_list.addSubscriber(
+ { name: 'user' }, '');
+
+ // Node is constructed using _createSubscriberNode.
+ Y.Assert.isTrue(node.hasClass('subscriber'));
+ // And the ID is set inside addSubscriber() method.
+ Y.Assert.areEqual('subscriber-user', node.get('id'));
+
+ // And it nested in the subscribers-list of a 'Direct' section.
+ var list_node = node.ancestor('.subscribers-list');
+ Y.Assert.isNotNull(list_node);
+ var section_node = list_node.ancestor(
+ '.subscribers-section-default');
+ Y.Assert.isNotNull(section_node);
+ }
+}));
+
+Y.lp.testing.Runner.run(suite);
+
+});
=== modified file 'lib/lp/answers/templates/questiontarget-portlet-answercontacts.pt'
--- lib/lp/answers/templates/questiontarget-portlet-answercontacts.pt 2009-07-17 17:59:07 +0000
+++ lib/lp/answers/templates/questiontarget-portlet-answercontacts.pt 2011-08-12 05:51:26 +0000
@@ -4,23 +4,17 @@
xmlns:i18n="http://xml.zope.org/namespaces/i18n";
omit-tag="">
+<script type="text/javascript">
+ LPS.use('base', 'node', 'event',
+ 'lp.comments.hide', 'lp.answers.answercontacts',
+ function(Y) {
+ Y.on('domready', function() {
+ new Y.lp.answers.answercontacts.createQuestionAnswerContactsLoader();
+ });
+ });
+</script>
<div class="portlet" id="portlet-answer-contacts">
-
<h2>Answer contacts for <span tal:replace="context/displayname"/></h2>
-
- <div class="portletBody portletContent"
- tal:define="answer_contacts context/direct_answer_contacts">
-
- <ul tal:condition="answer_contacts">
- <li tal:repeat="answer_contact answer_contacts">
- <a tal:replace="structure answer_contact/fmt:link">Ubuntu Team</a>
- </li>
- </ul>
-
- <p tal:condition="not: answer_contacts">
- <i>There are no answer contacts.</i>
- </p>
-
- </div>
+ <div id="answer-contacts"></div>
</div>
</tal:root>
=== modified file 'lib/lp/app/javascript/subscribers/subscribers_list.js'
--- lib/lp/app/javascript/subscribers/subscribers_list.js 2011-08-09 14:18:02 +0000
+++ lib/lp/app/javascript/subscribers/subscribers_list.js 2011-08-12 05:51:26 +0000
@@ -45,6 +45,15 @@
var MAX_DISPLAYNAME_LENGTH = 20;
+var CONFIG_DEFAULTS = {
+ default_subscriber_level: '',
+ subscribers_label: 'subscribers',
+ subscribe_label:'Subscribe',
+ unsubscribe_label:'Unsubscribe',
+ subscribe_api: 'subscribe',
+ unsubscribe_api: 'unsubscribe'
+};
+
/**
* Load subscribers for an entity from Launchpad and put them in the web page.
*
@@ -63,8 +72,15 @@
if (!Y.Lang.isValue(config.subscriber_levels)) {
Y.error("No subscriber levels specified in `config'.");
}
- if (Y.Lang.isString(config.default_subscriber_level)) {
- this.default_subscriber_level = config.default_subscriber_level;
+ var config_var;
+ for (config_var in CONFIG_DEFAULTS) {
+ if (CONFIG_DEFAULTS.hasOwnProperty(config_var)) {
+ if (Y.Lang.isString(config[config_var])) {
+ this[config_var] = config[config_var];
+ } else {
+ this[config_var] = CONFIG_DEFAULTS[config_var];
+ }
+ }
}
this.subscriber_levels = config.subscriber_levels;
var sl = this.subscribers_list = new SubscribersList(config);
@@ -115,13 +131,12 @@
if (Y.Lang.isString(config.unsubscribed_help_text)) {
this.unsubscribed_help_text = config.unsubscribed_help_text;
}
-
- // Should the current user be shown in the subscribers list if they
- // use the Subscribe Me link.
- this.display_me_in_list = false;
- if (Y.Lang.isBoolean(config.display_me_in_list)) {
- this.display_me_in_list = config.display_me_in_list;
- }
+ }
+ // Should the current user be shown in the subscribers list if they are in
+ // the subscriber results or use the Subscribe Me link.
+ this.display_me_in_list = false;
+ if (Y.Lang.isBoolean(config.display_me_in_list)) {
+ this.display_me_in_list = config.display_me_in_list;
}
this._unsubscribe_me = undefined;
@@ -252,7 +267,7 @@
failure: this.error_handler.getFailureHandler()
} };
- sl.startActivity("Loading subscribers...");
+ sl.startActivity("Loading " + this.subscribers_label + "...");
this.lp_client.get(this.subscribers_portlet_uri, config);
};
@@ -296,7 +311,7 @@
}
// If we have just unsubscribed ourselves, we need to update the
// "subscribe me" link.
- if (is_me) {
+ if (is_me && Y.Lang.isString(loader.subscribe_me_link)) {
loader._updateSubscribeMeLink(false);
}
}
@@ -323,7 +338,7 @@
subscribers_list.startActivity("Unsubscribing...");
}
loader.lp_client.named_post(
- loader.context.self_link, 'unsubscribe', config);
+ loader.context.self_link, loader.unsubscribe_api, config);
};
};
@@ -373,13 +388,13 @@
SubscribersLoader.prototype._updateSubscribeMeLink = function(is_subscribed) {
var link = Y.one(this.subscribe_me_link);
if (is_subscribed) {
- link.set('text', 'Unsubscribe')
+ link.set('text', this.unsubscribe_label)
.removeClass('add')
.addClass('remove')
.set('title', this.subscribed_help_text);
} else {
this._unsubscribe_me = undefined;
- link.set('text', 'Subscribe')
+ link.set('text', this.subscribe_label)
.removeClass('remove')
.addClass('add')
.set('title', this.unsubscribed_help_text);
@@ -504,7 +519,8 @@
on: { success: on_success,
failure: on_failure },
parameters: { person: subscriber.self_link } };
- this.lp_client.named_post(this.context.self_link, 'subscribe', config);
+ this.lp_client.named_post(
+ this.context.self_link, this.subscribe_api, config);
};
/**
@@ -524,6 +540,8 @@
"No subscriber levels specified in `config'.");
}
this.subscriber_levels = config.subscriber_levels;
+ this.subscribers_label = config.subscribers_label;
+ this.unsubscribe_label = config.unsubscribe_label;
if (!Y.Lang.isValue(config.subscriber_level_order)) {
// If no ordering is specified, we will create a default ordering.
this.subscriber_level_order = [];
@@ -574,7 +592,7 @@
} else {
no_subs = Y.Node.create('<div />')
.addClass(CSS_CLASSES.no_subscribers)
- .set('text', 'No other subscribers.');
+ .set('text', 'No other ' + this.subscribers_label + '.');
this.container_node.appendChild(no_subs);
}
};
@@ -689,6 +707,7 @@
* @return {String} CSS class to use for the section for the `level`.
*/
SubscribersList.prototype._getSectionCSSClass = function(level) {
+ level = (level===''?'default':level);
return CSS_CLASSES.section + '-' + level.toLowerCase();
};
@@ -717,9 +736,11 @@
.addClass(CSS_CLASSES.section)
.addClass(this._getSectionCSSClass(level));
// Header.
- node.appendChild(
- Y.Node.create('<h3 />')
- .set('text', this.subscriber_levels[level]));
+ if (level !== '') {
+ node.appendChild(
+ Y.Node.create('<h3 />')
+ .set('text', this.subscriber_levels[level]));
+ }
// Node listing the actual subscribers.
node.appendChild(
Y.Node.create('<ul />')
@@ -739,6 +760,11 @@
* the entire section).
*/
SubscribersList.prototype._insertSectionNode = function(level, section_node) {
+ // We have no ordering so just prepend.
+ if (this.subscriber_level_order.length === 0) {
+ this.container_node.prepend(section_node);
+ return;
+ }
var index, existing_level;
var existing_level_node = null;
for (index=0; index < this.subscriber_level_order.length; index++) {
@@ -904,7 +930,8 @@
* Throws an error if not, otherwise silently returns.
*/
SubscribersList.prototype._checkSubscriptionLevel = function(level) {
- if (!this.subscriber_levels.hasOwnProperty(level)) {
+ if (Y.Object.size(this.subscriber_levels) > 0
+ && !this.subscriber_levels.hasOwnProperty(level)) {
Y.error(
'Level "' + level + '" is not an acceptable subscription level.');
}
@@ -1047,7 +1074,8 @@
unsubscribe_node = Y.Node.create('<a />')
.addClass(CSS_CLASSES.unsubscribe)
.set('href', '+subscribe')
- .set('title', 'Unsubscribe ' + subscriber.display_name);
+ .set('title',
+ this.unsubscribe_label + ' ' + subscriber.display_name);
unsubscribe_node.appendChild(
Y.Node.create('<img />')
.set('src', '/@@/remove')
=== modified file 'lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js'
--- lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js 2011-08-09 14:18:02 +0000
+++ lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js 2011-08-12 05:51:26 +0000
@@ -24,7 +24,9 @@
root_node.appendChild(node);
var config = {
container_box: '#other-subscribers-container',
- subscriber_levels: subscriber_levels
+ subscriber_levels: subscriber_levels,
+ subscribers_label: "subscribers",
+ unsubscribe_label: "Unsubscribe"
};
return new module.SubscribersList(config);
}
@@ -89,7 +91,7 @@
},
test_subscriber_levels: function() {
- // Check that subscriber_levels are registered..
+ // Check that subscriber_levels are registered.
var container_node = Y.Node.create('<div />')
.set('id', 'container');
this.root.appendChild(container_node);
@@ -893,6 +895,38 @@
Y.Assert.isNotNull(section_node);
},
+ test_addSubscriber_no_levels: function() {
+ // Check that addSubscriber works if there are no subscription levels.
+ var container_node = Y.Node.create('<div />')
+ .set('id', 'other-subscribers-container');
+ this.root.appendChild(container_node);
+ var config = {
+ container_box: '#other-subscribers-container',
+ subscriber_levels: [],
+ subscribers_label: "subscribers",
+ unsubscribe_label: "Unsubscribe"
+ };
+ var subscribers_list = new module.SubscribersList(config);
+
+ var node = subscribers_list.addSubscriber(
+ { name: 'user' }, '');
+
+ // Node is constructed using _createSubscriberNode.
+ Y.Assert.isTrue(node.hasClass('subscriber'));
+ // And the ID is set inside addSubscriber() method.
+ Y.Assert.areEqual('subscriber-user', node.get('id'));
+
+ // And it nested in the subscribers-list of a 'Default' section with
+ // no header.
+ var list_node = node.ancestor('.subscribers-list');
+ Y.Assert.isNotNull(list_node);
+ var section_node = list_node.ancestor(
+ '.subscribers-section-default');
+ Y.Assert.isNotNull(section_node);
+ var header_node = section_node.one('h3');
+ Y.Assert.isNull(header_node);
+ },
+
test_addSubscriber_incorrect_level: function() {
// When an incorrect level is passed in, an exception is thrown.
var subscribers_list = setUpSubscribersList(this.root);
@@ -1428,6 +1462,86 @@
setUpLoader(this.root, config, true);
},
+ test_SubscribersLoader_default_config_parameters: function() {
+ // Check that CONFIG_DEFAULTS are used.
+ var node = Y.Node.create('<div />')
+ .set('id', 'other-subscribers-container');
+ var config = {
+ container_box: '#other-subscribers-container',
+ context: {web_link: '', self_link: ''},
+ subscribers_details_view: '/+details',
+ subscriber_levels: []
+ };
+ this.root.appendChild(node);
+ window.LP = { links: { me : "/~viewer" } };
+
+ // Save original method for restoring later.
+ var old_load = module.SubscribersLoader.prototype._loadSubscribers;
+ module.SubscribersLoader.prototype._loadSubscribers = function() {};
+
+ var loader = new module.SubscribersLoader(config);
+ var default_config = {
+ default_subscriber_level: '',
+ subscribers_label: 'subscribers',
+ subscribe_label:'Subscribe',
+ unsubscribe_label:'Unsubscribe',
+ subscribe_api: 'subscribe',
+ unsubscribe_api: 'unsubscribe'
+ };
+
+ var config_var;
+ for (config_var in default_config) {
+ if (default_config.hasOwnProperty(config_var)) {
+ Y.Assert.areEqual(
+ default_config[config_var], loader[config_var],
+ 'Unexpected config value for ' + config_var);
+ }
+ }
+ // Restore original method.
+ module.SubscribersLoader.prototype._loadSubscribers = old_load;
+ },
+
+ test_SubscribersLoader_default_config_override: function() {
+ // Check that CONFIG_DEFAULTS parameters can be overridden.
+ var node = Y.Node.create('<div />')
+ .set('id', 'other-subscribers-container');
+ var config = {
+ container_box: '#other-subscribers-container',
+ context: {web_link: '', self_link: ''},
+ subscribers_details_view: '/+details',
+ subscriber_levels: []
+ };
+ var override_config = {
+ default_subscriber_level: 'aaa',
+ subscribers_label: 'bbbb',
+ subscribe_label:'cccc',
+ unsubscribe_label:'dddd',
+ subscribe_api: 'eeee',
+ unsubscribe_api: 'ffff'
+ };
+
+ this.root.appendChild(node);
+ window.LP = { links: { me : "/~viewer" } };
+
+ // Save original method for restoring later.
+ var old_load = module.SubscribersLoader.prototype._loadSubscribers;
+ module.SubscribersLoader.prototype._loadSubscribers = function() {};
+
+ config = Y.mix(config, override_config);
+ var loader = new module.SubscribersLoader(config);
+
+ var config_var;
+ for (config_var in override_config) {
+ if (override_config.hasOwnProperty(config_var)) {
+ Y.Assert.areEqual(
+ override_config[config_var], loader[config_var],
+ 'Unexpected config value for ' + config_var);
+ }
+ }
+ // Restore original method.
+ module.SubscribersLoader.prototype._loadSubscribers = old_load;
+ },
+
test_SubscribersLoader: function() {
// With all the parameters specified, it returns an instance
// with subscribers_portlet_uri, subscribers_list, error_handler,
