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

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

[r=bac][bug=761257] Allow teams to be unsubscribed from the advanced subscription overlay.

= Bug 761257 =

Advanced bug subscription overlay UI shows an option to unsubscribe any of the teams that you have privileges for. However, choosing those options doesn't really work.

== Proposed fix ==

We need to factor handle_advanced_subscription_overlay to actually properly handle all the possible cases:
 a. updating existing subscription
 b. unmuting and subscribing with a different notification level
 c. unmuting
 d. unsubscribing teams

Case b. is equivalent to a. and is treated as such, so we've got only a conditional for a/c/d.

== Implementation details ==

I also fixed a very jumpy animation (it always slided out a list of bug notification levels when you had a bug muted), and (not so) accidentally fixed the case of "unmuting" not really allowing to set a bug notification level even if the UI offered it.

== Tests ==

No tests. I am doing a refactoring branch that will include tests for this separately.

== Demo and Q/A ==

https://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/3
https://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/3/+subscribe

The demo at
  https://devpad.canonical.com/~danilo/screencasts/bug-subscription-advanced.ogv
shows approximatelly what steps need to be taken for proper QA. Refreshing between steps will help ensure you've got the data updated (and not just the UI), and will avoid a bug in existing code that when you get rid of all subscribers, subscribing someone else stops working.

= Launchpad lint =

Checking for conflicts and issues in changed files.

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

[Brad Crittenden (bac)]

Hi Danilos thanks for fixing this bug. I'm especially happy since it is one I opened.

On IRC we identified some issues that can be addressed in your follow-on branch:

- In the overlay form, change the value of the teams to have the '/~' prefixed which will prevent any future clashes between valid team names and the other checkbox values in the form.

- Rename and change the description of 'unsubscribe_current_user' to reflect the larger scope of what it is doing. In this case you're using it to unsubscribe a team.

With the promised follow-on branch work this one is ok.

review: Approve (code)

=== modified file 'lib/lp/bugs/javascript/bug_subscription.js'
--- lib/lp/bugs/javascript/bug_subscription.js	2011-04-19 15:16:58 +0000
+++ lib/lp/bugs/javascript/bug_subscription.js	2011-04-22 12:29:31 +0000
@@ -42,13 +42,19 @@
                 visibility: 'visible'
             });
         });
+        var checked_box = subscription_radio_buttons.filter(':checked').pop();
+        var current_value = checked_box.get('value');
         slide_in_anim.run();
         Y.each(subscription_radio_buttons, function(subscription_button) {
             subscription_button.on('click', function(e) {
-                if(e.target.get('value') === 'update-subscription') {
-                    slide_out_anim.run();
-                } else {
-                    slide_in_anim.run();
+                var value = e.target.get('value');
+                if (value !== current_value) {
+                    if(value === 'update-subscription') {
+                        slide_out_anim.run();
+                    } else {
+                        slide_in_anim.run();
+                    }
+                    current_value = value;
                 }
             });
         });

=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-04-14 16:37:22 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-04-22 12:29:31 +0000
@@ -379,6 +379,30 @@
     return subscription;
 }
 
+
+/*
+ * Set up and return a Subscription object for the team subscription
+ * link.
+ */
+function get_team_subscription(team_uri) {
+    setup_client_and_bug();
+    var subscription = new Y.lp.bugs.subscriber.Subscription({
+        link: Y.one('.menu-link-subscription'),
+        spinner: Y.one('#sub-unsub-spinner'),
+        subscriber: new Y.lp.bugs.subscriber.Subscriber({
+            uri: team_uri,
+            subscriber_ids: subscriber_ids
+        })
+    });
+
+    subscription.set('is_direct', true);
+    subscription.set('has_dupes', false);
+    subscription.set('can_be_unsubscribed', true);
+    subscription.set('person', subscription.get('subscriber'));
+    subscription.set('is_team', true);
+    return subscription;
+}
+
 /*
  * Initialize callbacks for subscribe/unsubscribe links.
  *
@@ -777,6 +801,8 @@
         Y.lp.app.errors.display_error(subscription_link, error_msg);
     };
 
+    var subscriber_link = Y.lp.client.get_absolute_uri(
+        subscriber.get('escaped_uri'));
     var config = {
         on: {
             success: function(client) {
@@ -815,9 +841,11 @@
             },
 
             failure: error_handler.getFailureHandler()
-        }
+        },
+
+        parameters: { person: subscriber_link }
     };
-    if (subscription.is_direct_subscription()) {
+    if (subscription.is_direct_subscription() || subscription.is_team()) {
         lp_client.named_post(bug_repr.self_link, 'unsubscribe', config);
     } else {
         lp_client.named_post(
@@ -1211,22 +1239,36 @@
  * @param form_data {Object} The data from the submitted form.
  */
 function handle_advanced_subscription_overlay(form_data) {
-    var subscription = get_subscribe_self_subscription();
+    var subscription;
+    var mute_subscription = get_mute_subscription();
+    var is_muted = (!Y.Lang.isNull(mute_subscription) &&
+                    mute_subscription.get('is_muted'));
+    var requested_subscriber;
+    var request_for_self = false;
+    // XXX Danilo 20110422: this is a very lousy "special string"
+    // that will make it break for a team/person named 'update-subscription'.
+    // We should probably use special characters not allowed in team names
+    // (maybe all-uppercase would work).
+    var UPDATE_ACTION = 'update-subscription';
+
+    if (form_data['field.subscription'][0] === UPDATE_ACTION) {
+        requested_subscriber = LP.links.me;
+        request_for_self = true;
+    } else {
+        requested_subscriber = '/~' + form_data['field.subscription'][0];
+        if (requested_subscriber === LP.links.me) {
+            request_for_self = true;
+        }
+    }
+    if (request_for_self) {
+        subscription = get_subscribe_self_subscription();
+    } else {
+        subscription = get_team_subscription(requested_subscriber);
+    }
     var link = subscription.get('link');
     var link_parent = link.get('parentNode');
-    var mute_subscription = get_mute_subscription();
-    var is_muted = (!Y.Lang.isNull(mute_subscription) &&
-                    mute_subscription.get('is_muted'));
-    if (link_parent.hasClass('subscribed-false') &&
-        link_parent.hasClass('dup-subscribed-false') &&
-        !is_muted) {
-        // The user isn't subscribed or muted, so subscribe them.
-        subscription.set(
-            'bug_notification_level',
-            form_data['field.bug_notification_level']);
-        subscribe_current_user(subscription);
-    } else if (
-        form_data['field.subscription'] === 'update-subscription') {
+
+    if (form_data['field.subscription'][0] === UPDATE_ACTION) {
         // The user is already subscribed or is muted and wants to
         // update their subscription.
         setup_client_and_bug();
@@ -1279,13 +1321,25 @@
             }
         };
         lp_client.get(subscription_url, config);
+
+    } else if (link_parent.hasClass('subscribed-false') &&
+               link_parent.hasClass('dup-subscribed-false') &&
+               !is_muted && request_for_self) {
+        // The user isn't subscribed or muted, and the request is
+        // for himself (iow, not for a team).
+        subscription.set(
+            'bug_notification_level',
+            form_data['field.bug_notification_level']);
+        subscribe_current_user(subscription);
+    } else if (is_muted && request_for_self) {
+        // When a person has a bug muted, we show 2+ options:
+        // a. unmute, b. unmute and subscribe, c. unsubscribe team1...
+        // "b" is treated as 'update-subscription' case, and for any
+        // of the teams, request_for_self won't be true.
+        unmute_current_user(mute_subscription);
     } else {
-        // The user is already subscribed and wants to unsubscribe.
-        if (is_muted) {
-            unmute_current_user(mute_subscription);
-        } else {
-            unsubscribe_current_user(subscription);
-        }
+        // Unsubscribe this person/team.
+        unsubscribe_current_user(subscription);
     }
 }