Merge lp:~danilo/launchpad/bug-770241 into lp:launchpad

[Данило Шеган]

[r=gmb][bug=770241] When the last subscriber is unsubscribed, put the "none" in appropriate div as expected by subscribe code.

= 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

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-04-28 07:55:44 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-04-28 13:50:50 +0000
@@ -612,7 +612,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()) {
@@ -836,7 +836,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();
             },
@@ -911,7 +911,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();
                 }
@@ -1190,31 +1190,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
@@ -1522,4 +1497,5 @@
                         "lazr.overlay", "lazr.choiceedit", "lp.app.picker",
                         "lp.client",
                         "lp.client.plugins", "lp.bugs.subscriber",
+                        "lp.bugs.subscribers_list",
                         "lp.bugs.bug_notification_level", "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 13:50:50 +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 13:50:50 +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 13:50:50 +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();
+});
+});