launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/dupe-unsub-no-remove-name-775335 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #775335 in Launchpad itself: "Link no longer disappears from portlet when unsubscribing from a dupe"
  https://bugs.launchpad.net/launchpad/+bug/775335

For more details, see:
https://code.launchpad.net/~deryck/launchpad/dupe-unsub-no-remove-name-775335/+merge/60538

Bug 775335 notes that your name would not be removed from the subscribers portlet correctly if you clicked "Unsubscribe" and you were subscribed via a duplicate.  Everything worked fine if you clicked the unsubscribe icon in the portlet itself.  This was only going wrong when clicking the "Unsubscribe" link at the top of the portlet.

William's assessment in the bug that remove_user_link needed to be passed the is_dupe parameter was largely correct.  However, further digging revealed that the page state was being setup wrong for the page's Subscription object.  subscription.get('is_direct') or subscription.get('has_dupes') should be enough to know if is_dupe should be true or false, but this didn't work.  This branch fixes both issues.  The Subscription object is now correctly set up and remove_user_link gets passed an is_dupe parameter.

To fix the page state, I also had to move setup_subscription_link_handlers to be called after the dupe portlet was loaded.  It was being called after the direct subscribers portlet loaded and it needs all portlets loaded before it can work correctly.

There's also a minor drive by refactor to move the handler for bugs:dupeportletloaded around so that it lines up with the dupe portlet load failure handler too.

The Windmill test provided good coverage for this.  It's currently failing in the Jenkins builder, but this branch will fix that, too.  You can run it with:

./bin/test -cvvt test_bug_inline_subscriber --layer=WindmillLayer
-- 
https://code.launchpad.net/~deryck/launchpad/dupe-unsub-no-remove-name-775335/+merge/60538
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/dupe-unsub-no-remove-name-775335 into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-05-10 10:37:53 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-05-10 18:03:29 +0000
@@ -105,9 +105,6 @@
     namespace.portlet.subscribe('bugs:portletloaded', function() {
         load_subscriber_ids();
     });
-    namespace.portlet.subscribe('bugs:dupeportletloaded', function() {
-        setup_unsubscribe_icon_handlers();
-    });
     /*
      * If the subscribers portlet fails to load, clear any
      * click handlers, so the normal subscribe page can be reached.
@@ -118,6 +115,10 @@
             click_handler.detach();
         }
     });
+    namespace.portlet.subscribe('bugs:dupeportletloaded', function() {
+        setup_subscription_link_handlers();
+        setup_unsubscribe_icon_handlers();
+    });
     /* If the dupe subscribers portlet fails to load,
      * be sure to try to handle any unsub icons that may
      * exist for others.
@@ -125,6 +126,7 @@
     namespace.portlet.subscribe(
         'bugs:dupeportletloadfailed',
         function(handlers) {
+            setup_subscription_link_handlers();
             setup_unsubscribe_icon_handlers();
         });
 
@@ -134,7 +136,6 @@
     namespace.portlet.subscribe(
         'bugs:portletsubscriberidsloaded',
         function() {
-            setup_subscription_link_handlers();
             load_subscribers_from_duplicates();
         });
 
@@ -395,15 +396,18 @@
         })
     });
 
-    var is_direct = subscription.get(
-        'link').get('parentNode').hasClass('subscribed-true');
-    var has_dupes = subscription.get(
-        'link').get('parentNode').hasClass('dup-subscribed-true');
+    subscription.set('can_be_unsubscribed', true);
+    subscription.set('person', subscription.get('subscriber'));
+    subscription.set('is_team', false);
+    var css_name = subscription.get('person').get('css_name');
+    var direct_css_name = '#direct-' + css_name;
+    var direct_node = Y.one(direct_css_name);
+    var is_direct = direct_node !== null;
     subscription.set('is_direct', is_direct);
+    var dupe_css_name = '#dupe-' + css_name;
+    var dupe_node = Y.one(dupe_css_name);
+    var has_dupes = dupe_node !== null;
     subscription.set('has_dupes', has_dupes);
-    subscription.set('can_be_unsubscribed', true);
-    subscription.set('person', subscription.get('subscriber'));
-    subscription.set('is_team', false);
     return subscription;
 }
 
@@ -812,6 +816,10 @@
     var config = {
         on: {
             success: function(client) {
+                // We need to track if this is direct or not,
+                // before we toggle the is_direct flag.
+                var original_is_direct = subscription.get('is_direct');
+
                 if (subscription.is_direct_subscription() &&
                     subscription.has_duplicate_subscriptions()) {
                     // Don't change the 'Unsubscribe' text if
@@ -837,7 +845,9 @@
                     subscription.set('has_dupes', false);
                 }
 
-                Y.lp.bugs.subscribers_list.remove_user_link(subscriber);
+                var is_dupe = !original_is_direct;
+                Y.lp.bugs.subscribers_list.remove_user_link(
+                    subscriber, is_dupe);
             },
 
             failure: error_handler.getFailureHandler()