launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/subscribe-unsubscribe into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #770293 in Launchpad itself: "Clicking the minus sign next to my name on a bug page appears not to unsubscribe me"
  https://bugs.launchpad.net/launchpad/+bug/770293

For more details, see:
https://code.launchpad.net/~danilo/launchpad/subscribe-unsubscribe/+merge/59511

= Bug 770293 =

After one subscribes to a bug, his user link and unsubscription icon is added to the subscribers list. However, unsubscribing action doesn't work because user is tagged as the "dupe subscriber" on subscribing, which causes the unsubscribe action to call unsubscribeFromDupes which silently succeeds but doesn't do anything.

= Implementation details =

The actual fix for this is addition of "subscription.set('is_direct', true)" to subscribe_current_user().

Everything else is drive-by fixes:

 - lines ~486: hide_spinner emitted a "failed" event, and was also called from on_success handler which emitted a "success" event; this caused unsubscribe action to be called twice because two "click" handlers were set-up, which caused a lot of confusion during debugging - the fix is to split out hide_spinner from the failure handler
 - lines ~610: the check if there are no other entries works better if we do it before we remove one of the entries (there can be at most two person links: one for direct, another for duplicate subscription), especially in a debugger (which can block things like animations); so, instead of checking if there are no links anymore after removal, we check if there was exactly one link before removal
 - lines ~1054: typo fix
 - lines ~1357: when someone "subscribes someone else" to a bug, an entry for them is added to the subscribers list; if they are a team, and current user is a member of that team, they should be able to unsubscribe them. However, this code was using the attribute directly which doesn't exist, so when fixed to use get(), it started working.

= Tests =

There is no easy way to test this, and since I am still doing some refactoring of the code in bugtask_index_portlets.js, I don't want to introduce lousy tests that will soon break.

= Demo & QA =

With the lack of tests, QA is very important for this.  I seriously hope that we'll have enough time to introduce proper tests for this or it's inevitably going to break again in similar ways.

== For the actual bug ==

Go to https://bugs.launchpad.dev/bugs/1
"Subscribe" yourself, "Unsubscribe" yourself (eg. using the "(-)" icon next to your name in the subscribers list), and reload to make sure that you have been unsubscribed.

== For drive-by fixes ==

1. Try subscribing the same team to a bug, and to a duplicate of the bug. From the 'primary' bug, unsubscribe a team from the duplicate, and from the bug using "(-)" icons
2. Observe that the unsubscribe (-) icon is added for these teams even without page reload

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/bugtask_index_portlets.js

-- 
https://code.launchpad.net/~danilo/launchpad/subscribe-unsubscribe/+merge/59511
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/subscribe-unsubscribe into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-04-29 00:58:32 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-04-29 13:31:19 +0000
@@ -486,6 +486,10 @@
     function hide_spinner() {
         Y.one('#subscribers-portlet-dupe-spinner').setStyle(
             'display', 'none');
+    }
+
+    function on_failure(transactionid, response, args) {
+        hide_spinner();
         // Fire a custom event to signal failure, so that
         // any remaining unsub icons can be hooked up.
         namespace.portlet.fire('bugs:dupeportletloadfailed');
@@ -507,7 +511,7 @@
     }
 
     var config = {on: {success: on_success,
-                       failure: hide_spinner}};
+                       failure: on_failure}};
     var url = Y.one(
         '#subscribers-from-dupes-content-link').getAttribute(
             'href').replace('bugs.', '');
@@ -610,10 +614,10 @@
     var config = {
         on: {
             success: function(client) {
+                var person_links = Y.all('.' + person.get('css_name')).size();
                 Y.lp.bugs.subscribers_list.remove_user_link(person, is_dupe);
-                var person_link = Y.one('.' + person.get('css_name'));
                 var has_direct, has_dupes;
-                if (Y.Lang.isNull(person_link) &&
+                if (person_links === 1 &&
                     subscription.is_current_user_subscribing()) {
                     // Current user has been completely unsubscribed.
                     subscription.disable_spinner(
@@ -719,6 +723,9 @@
     var subscriber = subscription.get('subscriber');
     var bug_notification_level = subscription.get('bug_notification_level');
 
+    // This is always a direct subscription.
+    subscription.set('is_direct', true);
+
     var error_handler = new Y.lp.client.ErrorHandler();
     error_handler.clearProgressUI = function () {
         subscription.disable_spinner();
@@ -1054,7 +1061,7 @@
     var not_unsubscribables = [];
 
     // Use the list of subscribers pulled from the DOM to have sortable
-    // lists of unsubscribable vs. not unsubscribale person links.
+    // lists of unsubscribable vs. not unsubscribable person links.
     var all_subscribers = Y.all('#subscribers-links div');
     if (all_subscribers.size() > 0) {
         all_subscribers.each(function(sub_link) {
@@ -1357,7 +1364,7 @@
                             var team_member = false;
                             var i;
                             for (i=0; i<result.entries.length; i++) {
-                                 if (result.entries[i].member_link ===
+                                if (result.entries[i].get('member_link') ===
                                     Y.lp.client.get_absolute_uri(
                                         subscription.get(
                                             'subscriber').get('uri'))) {