Merge lp:~dev-nigelj/launchpad/bug837290 into lp:launchpad

[Nigel Jones]

[r=gmb][bug=837290] SubscribersList() now handles default configuration options properly so as to prevent objects from being undefined.

Summary

Bug 837290 is an edge case, where if the only subscriber to a bug is logged-in and viewing the bug, the message 'No other undefined' appears instead of 'No other subscribers'. Further testing showed this was also the case for the answer tracker system.

Proposed Fix

The issue appears to be that within lib/lp/app/javascript/subscribers/subscribers_list.js SubscribersLoader receives a set of configuration variables (property name 'config') and then merges them with default variables (at the top of the file). However the fully merged variables aren't passed to SubscribersList() which is passed config.

My proposed fix is to merge 'config' with the defaults, to both 'this' and 'config' so that if not specified by callers (both bugs & answers don't pass all details and merging is required) of SubscribersLoader() then correct defaults will be passed to SubscribersList() and the sub-function that produces the 'No other subscribers' message.

Pre-implementation notes

I discussed the issue with Graham (gmb) on #launchpad-dev who agreed on IRC that it seems that this way is likely the best way to go.

Implementation details

I implemented the Proposed Fix to lib/lp/app/javascript/subscribers/subscribers_list.js and verified that the correct message was displayed.

Tests

I have repeated two unit tests to run from within SubscribersLoader() (instead of SubscribersList()) to test that configuration merging works as intended.

Testing against the devel branch, the two tests failed, against my patch, the all tests passed.

Demo & Q/A

On a clean Launchpad setup, I logged in with the default <email address hidden>/test user/pass combination created a new project, configured the bug tracker and created a bug against the new project.

Once the bug is created, the right sidebar displays "No other subscribers" on an un-patched Launchpad, this will say "No other undefined"

Lint

njones:~/launchpad/lp-branches/bug-837290$ make lint
bzr: ERROR: unknown command "pipes"
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/subscribers/subscribers_list.js
  lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js

(Not sure why 'bzr pipes' is missing but other than that, looks clean)

[Graham Binns (gmb)]

Hi Nigel,

Nice branch, thanks for doing the hard work! I'm happy for this to land and will run it through EC2 for you shortly.

review: Approve (code)

[Francis J. Lacoste (flacoste)]

Nigel signed the contributor agreement so gmb feel free to land this branch.

[Graham Binns (gmb)]

This branch is now playing in EC2. I'll let you know how it fares.

=== modified file 'lib/lp/app/javascript/subscribers/subscribers_list.js'
--- lib/lp/app/javascript/subscribers/subscribers_list.js	2011-08-15 03:42:02 +0000
+++ lib/lp/app/javascript/subscribers/subscribers_list.js	2011-08-30 13:08:25 +0000
@@ -79,6 +79,7 @@
                 this[config_var] = config[config_var];
             } else {
                 this[config_var] = CONFIG_DEFAULTS[config_var];
+                config[config_var] = CONFIG_DEFAULTS[config_var];
             }
         }
     }
 
=== modified file 'lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js'
--- lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js	2011-08-15 03:42:02 +0000
+++ lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js	2011-08-30 13:08:25 +0000
@@ -2734,6 +2734,56 @@
     }
 }));
 
+/**
+ * Re-test several resetNoSubscribers() tests that need config options merging
+ * (LP Bug 837290)
+ */
+suite.add(new Y.Test.Case({
+    name: 'SubscribersLoader.SubscribersList.resetNoSubscribers() re-test',
+
+    setUp: function() {
+        this.root = Y.Node.create('<div />');
+        Y.one('body').appendChild(this.root);
+    },
+
+    tearDown: function() {
+        this.root.remove();
+    },
+
+    test_no_subscribers: function() {
+        // When resetNoSubscribers() is called on an empty
+        // SubscribersList, indication of no subscribers is added.
+        // In addition, this checks that the default configuration options
+        // have been correctly merged without SubscribersLoader()
+        var loader = setUpLoader(this.root);
+        var subscribers_list = loader.subscribers_list;
+        subscribers_list.resetNoSubscribers();
+        var no_subs_nodes = this.root.all(
+            '.no-subscribers-indicator');
+        Y.Assert.areEqual(1, no_subs_nodes.size());
+        Y.Assert.areEqual('No other subscribers.',
+                          no_subs_nodes.item(0).get('text'));
+    },
+
+    test_no_subscribers_no_levels: function() {
+        // When resetNoSubscribers() is called on an empty
+        // SubscribersList, indication of no subscribers is added. If there
+        // are no subscriber_levels, a different message is displayed.
+        // In addition, this checks that the default configuration options
+        // have been correctly merged without SubscribersLoader()
+        var loader = setUpLoader(this.root);
+        var subscribers_list = loader.subscribers_list;
+        subscribers_list.subscriber_levels = [];
+        subscribers_list.resetNoSubscribers();
+        var no_subs_nodes = this.root.all(
+            '.no-subscribers-indicator');
+        Y.Assert.areEqual(1, no_subs_nodes.size());
+        Y.Assert.areEqual('No subscribers.',
+                          no_subs_nodes.item(0).get('text'));
+    }
+}));
+
+
 Y.lp.testing.Runner.run(suite);
 
 });