launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03471
[Merge] lp:~danilo/launchpad/bug-770241 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-770241 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #770241 in Launchpad itself: "Subscribe someone else fails after you unsubscribed the last subscriber"
https://bugs.launchpad.net/launchpad/+bug/770241
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-770241/+merge/59338
= Bug 770241 =
Using JS to empty the list of subscribers on a bug page (iow, unsubscribe everybody) and then try to "subscribe someone else" fails because "None" div is added to the subscribers-links instead of the parent node for it (as is done on reload of the page with no subscribers).
== Proposed fix ==
* Move set_none_for_empty_subscribers to a separate JS module for managing the subscribers list and rename it to reset()
* Instead of adding directly to #subscribers-links, add the "None" div to the parent node
* Provide tests for the behaviour of reset() in all these cases
== Implementation details ==
* test_subscribers_list.html is boiler-plate for test set-up
* reset() is mostly old code
* the plan is to move more of the subscribers list management methods
to the new module, to split it up from the other code.
== Tests ==
lib/lp/bugs/javascript/tests/test_subscribers_list.html
== Demo and Q/A ==
https://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/3
(unsubscribe everyone, and then try to "subscribe someone else")
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/javascript/tests/test_subscribers_list.html
lib/lp/bugs/javascript/bugtask_index_portlets.js
lib/lp/bugs/javascript/subscribers_list.js
lib/lp/bugs/javascript/tests/test_subscribers_list.js
--
https://code.launchpad.net/~danilo/launchpad/bug-770241/+merge/59338
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-770241 into lp:launchpad.
=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js 2011-04-27 15:10:58 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js 2011-04-28 09:35:43 +0000
@@ -592,7 +592,7 @@
var anim = Y.lazr.anim.green_flash({ node: icon_parent_div });
anim.on('end', function(e) {
remove_user_name_link(icon_parent_div);
- set_none_for_empty_subscribers();
+ Y.lp.bugs.subscribers_list.reset();
var person_link = Y.one('.' + person.get('css_name'));
if (Y.Lang.isNull(person_link) &&
subscription.is_current_user_subscribing()) {
@@ -843,7 +843,7 @@
var anim = Y.lazr.anim.green_flash({ node: flash_node });
anim.on('end', function(e) {
remove_user_name_link(flash_node);
- set_none_for_empty_subscribers();
+ Y.lp.bugs.subscribers_list.reset();
});
anim.run();
},
@@ -918,7 +918,7 @@
});
subscriber_anim.on('end', function(e) {
remove_user_name_link(subscriber_node);
- set_none_for_empty_subscribers();
+ Y.lp.bugs.subscribers_list.reset();
});
subscriber_anim.run();
}
@@ -1197,31 +1197,6 @@
}
/*
- * Add the "None" div to the subscribers list if
- * there aren't any subscribers left.
- *
- * @method set_none_for_empty_subscribers
- */
-function set_none_for_empty_subscribers() {
- var subscriber_list = Y.one('#subscribers-links');
- // Assume if subscriber_list has no child divs
- // then the list of subscribers is empty.
- if (!Y.Lang.isValue(subscriber_list.one('div')) &&
- !Y.Lang.isValue(Y.one('#none-subscribers'))) {
- var none_div = Y.Node.create('<div id="none-subscribers">None</div>');
- subscriber_list.appendChild(none_div);
- }
-
- // Clear the empty duplicate subscribers list if it exists.
- var dup_list = Y.one('#subscribers-from-duplicates');
- if (Y.Lang.isValue(dup_list) &&
- !Y.Lang.isValue(dup_list.one('div'))) {
- var parent = dup_list.get('parentNode');
- parent.removeChild(dup_list);
- }
-}
-
-/*
* Set the class on subscription link's parentNode.
*
* This is used to reset the class used by the
@@ -1529,4 +1504,5 @@
"lazr.overlay", "lazr.choiceedit", "lp.app.picker",
"lp.client",
"lp.client.plugins", "lp.bugs.subscriber",
+ "lp.bugs.subscribers_list",
"lp.bugs.bug_subscription", "lp.app.errors"]});
=== added file 'lib/lp/bugs/javascript/subscribers_list.js'
--- lib/lp/bugs/javascript/subscribers_list.js 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/javascript/subscribers_list.js 2011-04-28 09:35:43 +0000
@@ -0,0 +1,43 @@
+/* Copyright 2011 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Functions for managing the subscribers list.
+ *
+ * @module bugs
+ * @submodule subscribers_list
+ */
+
+YUI.add('lp.bugs.subscribers_list', function(Y) {
+
+var namespace = Y.namespace('lp.bugs.subscribers_list');
+
+/**
+ * Reset the subscribers list if needed.
+ *
+ * Adds the "None" div to the subscribers list if
+ * there aren't any subscribers left, and clears up
+ * the duplicate subscribers list if empty.
+ *
+ * @method reset
+ */
+function reset() {
+ var subscriber_list = Y.one('#subscribers-links');
+ // Assume if subscriber_list has no child divs
+ // then the list of subscribers is empty.
+ if (!Y.Lang.isValue(subscriber_list.one('div')) &&
+ !Y.Lang.isValue(Y.one('#none-subscribers'))) {
+ var none_div = Y.Node.create('<div id="none-subscribers">None</div>');
+ var subscribers = subscriber_list.get('parentNode');
+ subscribers.appendChild(none_div);
+ }
+
+ // Clear the empty duplicate subscribers list if it exists.
+ var dup_list = Y.one('#subscribers-from-duplicates');
+ if (Y.Lang.isValue(dup_list) &&
+ !Y.Lang.isValue(dup_list.one('div'))) {
+ dup_list.remove();
+ }
+}
+namespace.reset = reset;
+
+}, "0.1", {"requires": ["node"]});
=== added file 'lib/lp/bugs/javascript/tests/test_subscribers_list.html'
--- lib/lp/bugs/javascript/tests/test_subscribers_list.html 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/javascript/tests/test_subscribers_list.html 2011-04-28 09:35:43 +0000
@@ -0,0 +1,44 @@
+<html>
+ <head>
+ <title>Bug subscriptions: subscribers list</title>
+
+ <!-- YUI 3.0 Setup -->
+ <script type="text/javascript"
+ src="../../../../canonical/launchpad/icing/yui/yui/yui.js"></script>
+ <script type="text/javascript"
+ src="../../../../canonical/launchpad/icing/lazr/build/lazr.js"></script>
+ <link rel="stylesheet"
+ href="../../../../canonical/launchpad/icing/yui/cssreset/reset.css"/>
+ <link rel="stylesheet"
+ href="../../../../canonical/launchpad/icing/yui/cssfonts/fonts.css"/>
+ <link rel="stylesheet"
+ href="../../../../canonical/launchpad/icing/yui/cssbase/base.css"/>
+ <link rel="stylesheet"
+ href="../../../../canonical/launchpad/javascript/test.css" />
+
+ <script type="text/javascript"
+ src="../../../app/javascript/client.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/errors.js"></script>
+
+ <!-- The module under test -->
+ <script type="text/javascript"
+ src="../subscribers_list.js"></script>
+
+ <!-- The test suite -->
+ <script type="text/javascript"
+ src="test_subscribers_list.js"></script>
+
+ <!-- Pretty up the sample html -->
+ <style type="text/css">
+ div#sample {margin:15px; width:200px; border:1px solid #999; padding:10px;}
+ </style>
+ </head>
+ <body class="yui3-skin-sam">
+ <!-- Example markup required by test suite -->
+ <div id="test-root"></div>
+
+ <!-- The test output -->
+ <div id="log"></div>
+ </body>
+</html>
=== added file 'lib/lp/bugs/javascript/tests/test_subscribers_list.js'
--- lib/lp/bugs/javascript/tests/test_subscribers_list.js 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/javascript/tests/test_subscribers_list.js 2011-04-28 09:35:43 +0000
@@ -0,0 +1,121 @@
+YUI({
+ base: '../../../../canonical/launchpad/icing/yui/',
+ filter: 'raw', combine: false, fetchCSS: false
+ }).use('test', 'console', 'lp.bugs.subscribers_list',
+ 'node-event-simulate',
+ function(Y) {
+
+var suite = new Y.Test.Suite("lp.bugs.subscribers_list Tests");
+var module = Y.lp.bugs.subscribers_list;
+
+/**
+ * Test resetting of the subscribers list.
+ */
+suite.add(new Y.Test.Case({
+ name: 'Resetting subscribers list',
+
+ setUp: function() {
+ this.root = Y.Node.create('<div></div>');
+ Y.one('body').appendChild(this.root);
+ },
+
+ tearDown: function() {
+ this.root.remove();
+ },
+
+ test_no_subscribers: function() {
+ // There are no subscribers left in the subscribers_list
+ // (iow, subscribers_links is empty).
+ var subscribers_links = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-links');
+ var subscribers_list = Y.Node.create('<div></div>');
+ subscribers_list.appendChild(subscribers_links);
+ this.root.appendChild(subscribers_list);
+
+ // Resetting the list adds a 'None' div to the
+ // subscribers_list (and not to the subscriber_links).
+ module.reset();
+ var none_node = subscribers_list.one('#none-subscribers');
+ Y.Assert.isNotNull(none_node);
+ Y.Assert.areEqual('None', none_node.get('innerHTML'));
+ Y.Assert.areEqual(subscribers_list,
+ none_node.get('parentNode'));
+
+ },
+
+ test_subscribers: function() {
+ // When there is at least one subscriber, nothing
+ // happens when reset() is called.
+ var subscribers_links = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-links');
+ subscribers_links.appendChild(
+ Y.Node.create('<div>Subscriber</div>'));
+ var subscribers_list = Y.Node.create('<div></div>');
+ subscribers_list.appendChild(subscribers_links);
+ this.root.appendChild(subscribers_list);
+
+ // Resetting the list is a no-op.
+ module.reset();
+ var none_node = subscribers_list.one('#none-subscribers');
+ Y.Assert.isNull(none_node);
+ },
+
+
+ test_empty_duplicates: function() {
+ // There are no subscribers among the duplicate subscribers.
+ var dupe_subscribers = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-from-duplicates');
+ this.root.appendChild(dupe_subscribers);
+
+ // There must always be subscribers-links node for reset() to work.
+ var subscribers_links = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-links');
+ this.root.appendChild(subscribers_links);
+
+ // Resetting the list removes the entire duplicate subscribers node.
+ module.reset();
+ var dupes_node = Y.one('#subscribers-from-duplicates');
+ Y.Assert.isNull(dupes_node);
+
+ },
+
+ test_duplicates: function() {
+ // There are subscribers among the duplicate subscribers,
+ // and nothing changes.
+ var dupe_subscribers = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-from-duplicates');
+ dupe_subscribers.appendChild(
+ Y.Node.create('<div>Subscriber</div>'));
+ this.root.appendChild(dupe_subscribers);
+
+ // There must always be subscribers-links node for reset() to work.
+ var subscribers_links = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-links');
+ this.root.appendChild(subscribers_links);
+
+ // Resetting the list does nothing.
+ module.reset();
+
+ // The list is still there.
+ var dupes_node = this.root.one('#subscribers-from-duplicates');
+ Y.Assert.isNotNull(dupes_node);
+ Y.Assert.areEqual(1, dupes_node.all('div').size());
+ }
+}));
+
+
+var handle_complete = function(data) {
+ status_node = Y.Node.create(
+ '<p id="complete">Test status: complete</p>');
+ Y.one('body').appendChild(status_node);
+ };
+Y.Test.Runner.on('complete', handle_complete);
+Y.Test.Runner.add(suite);
+
+var console = new Y.Console({newestOnTop: false});
+console.render('#log');
+
+Y.on('domready', function() {
+ Y.Test.Runner.run();
+});
+});
