Merge lp:~brian-murray/launchpad/bug-912137 into lp:launchpad

[Brian Murray]

Fixes sort ordering of subscribers in the subscriber portlet

Summary

If you look at a bug like bug 764701 you'll notice the subscribers are sorted in a reverse order of what you would expect. This makes it a bit harder to determine whether or not a specific team or person is subscribed to any bug report. This was reported in bug 912137.

Proposed fix

Instead of inserting subscribers at the start of the list, in subscribers_list.js, we should add them at the end of it.

Tests

bin/test -vv -t test_subscribers_list

Demo and Q/A

I also tested this using the sample data and viewing bug #9 which has two subscribers and confirmed that they were in a normal order.

[j.c.sackett (jcsackett)]

Brian--

Thanks for this--it's good, it just needs one fix.

As we discussed on IRC, this has the side effect of new subscribers added via the UI being added to the bottom of the list. Because subscriber lists can quite long, new ones should be added to the top so the user can see that they were in fact added.

You could update addSubscriber to accept a parameter that indicates whether it's adding subscribers from a list of existing ones--which should be appended, or is adding a brand new subscriber which should be prepended. If you default this argument to indicate appending, then you should just need to update the call site that is bound to the `Subscribe someone else` button.

review: Needs Fixing

[j.c.sackett (jcsackett)]

Thanks for addressing the points above!

review: Approve

=== modified file 'lib/lp/app/javascript/subscribers/subscribers_list.js'
--- lib/lp/app/javascript/subscribers/subscribers_list.js	2011-11-18 16:20:50 +0000
+++ lib/lp/app/javascript/subscribers/subscribers_list.js	2012-06-08 16:11:26 +0000
@@ -192,10 +192,11 @@
     }
     if (this._updateSubscribersList(subscriber)) {
         if (subscriber.can_edit === true) {
-            this.subscribers_list.addSubscriber(subscriber, level, {
+            this.subscribers_list.addSubscriber(subscriber, level, 
+            false, {
                 unsubscribe_callback: unsubscribe_callback});
         } else {
-            this.subscribers_list.addSubscriber(subscriber, level);
+            this.subscribers_list.addSubscriber(subscriber, level, false);
         }
     }
 };
@@ -478,7 +479,7 @@
     var is_me = this._subscriber_is_me(subscriber);
     var update_subscribers_list = this._updateSubscribersList(subscriber);
     if (update_subscribers_list) {
-        this.subscribers_list.addSubscriber(subscriber, level);
+        this.subscribers_list.addSubscriber(subscriber, level, true);
         this.subscribers_list.indicateSubscriberActivity(subscriber);
     }
     var loader = this;
@@ -967,9 +968,10 @@
  * @param level {String} Level of the subscription.
  * @param config {Object} Object containing potential 'unsubscribe' callback
  *     in the `unsubscribe_callback` attribute.
+ * @param add_new {Boolean} True if the subscriber was newly added
  */
 SubscribersList.prototype.addSubscriber = function(subscriber, level,
-                                                   config) {
+                                                   add_new, config) {
     this._checkSubscriptionLevel(level);
     subscriber = this._validateSubscriber(subscriber);
 
@@ -982,8 +984,13 @@
     if (subscriber_node === null) {
         subscriber_node = this._createSubscriberNode(subscriber);
         subscriber_node.set('id', subscriber_id);
-        // Insert the subscriber at the start of the list.
-        list_node.prepend(subscriber_node);
+        if (add_new === true) {
+            // Add new subscriber to the start of the list.
+            list_node.prepend(subscriber_node);
+        } else {
+            // Add the subscriber at the end of the list.
+            list_node.append(subscriber_node);
+        }
         // Add the unsubscribe action if needed.
         if (Y.Lang.isValue(config) &&
             Y.Lang.isFunction(config.unsubscribe_callback)) {

=== modified file 'lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js'
--- lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js	2012-04-06 17:28:25 +0000
+++ lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js	2012-06-08 16:11:26 +0000
@@ -1015,7 +1015,7 @@
 
         test_addSubscriber_ordering: function() {
             // With multiple subscribers being added to the same section,
-            // the last one is listed first.
+            // the last one is listed last.
             var subscribers_list = setUpSubscribersList(this.root);
             var node1 = subscribers_list.addSubscriber(
                 { name: 'user1' }, 'Level3');
@@ -1032,7 +1032,7 @@
                 returned_nodes.push(all_subscribers.item(index));
             }
             Y.ArrayAssert.itemsAreSame(
-                [node2, node1],
+                [node1, node2],
                 returned_nodes);
         },
 
@@ -1052,7 +1052,7 @@
                 Y.Assert.areSame(subscriber, unsub_subscriber);
                 Y.Assert.areSame(callback, unsub_callback);
             };
-            subscribers_list.addSubscriber(subscriber, 'Level3',
+            subscribers_list.addSubscriber(subscriber, 'Level3', false,
                                            { unsubscribe_callback: callback });
             // Setting up a callback was performed.
             Y.Assert.isTrue(callback_setup);
@@ -1627,7 +1627,7 @@
             // Save the old method for restoring later.
             var old_addSub = module.SubscribersList.prototype.addSubscriber;
             module.SubscribersList.prototype.addSubscriber = function(
-                passed_subscriber, passed_level) {
+                passed_subscriber, passed_level, passed_new) {
                 call_passed_through = true;
                 Y.Assert.areSame(subscriber, passed_subscriber);
                 Y.Assert.areSame(level, passed_level);
@@ -1649,7 +1649,7 @@
             // Save the old method for restoring later.
             var old_addSub = module.SubscribersList.prototype.addSubscriber;
             module.SubscribersList.prototype.addSubscriber = function(
-                passed_subscriber, passed_level, passed_config) {
+                passed_subscriber, passed_level, passed_new, passed_config) {
                 Y.Assert.areSame('Default', passed_level);
                 Y.Assert.isUndefined(passed_config);
             };
@@ -1682,7 +1682,7 @@
             // Assert in addSubscriber that it's being passed the new
             // callback in the config parameter.
             module.SubscribersList.prototype.addSubscriber = function(
-                passed_subscriber, passed_level, passed_config) {
+                passed_subscriber, passed_level, passed_new, passed_config) {
                 Y.Assert.areSame(unsubscribe_callback,
                                  passed_config.unsubscribe_callback);
             };