Merge lp:~danilo/launchpad/bug-772763-remove-unmute-dialog into lp:launchpad/db-devel

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

"Unmute bug mail" unmutes directly (and restores any previous subscription) without popping up a dialog.

= Bug 772763: remove unmute dialog =

As part of fixing 772763, we drop the pop-dialog when "unmute" is clicked and instead make it a direct action on click.

== Proposed fix ==

Turn "Unmute" link into a direct action (instead of popping up an "advanced overlay" with options to unmute and similar).

== Implementation details ==

This branch is entirely Gary's work. I am only shepherding it.

Basically, it's a very welcome refactoring of the entire mute handling for a single bug:

 - no use of JS Subscription class for muting/unmuting (it didn't really make sense since "mute" is not a subscription, but instead orthogonal to it)
 - no tests are provided because we plan to refactor this code further with our next bug (subscribers' list)

== Tests ==

Nada.

== Demo and Q/A ==

Go to https://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/3 and play with muting/unmuting while having a subscription or not having one.

= Launchpad lint =

Checking for conflicts and issues in changed files.

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

[Данило Шеган (danilo)]

review: Abstain

[Brad Crittenden (bac)]

Hi Gary/Danilo,

The branch looks good with the exception that if I am muted and there are no other subscribers then "No subscribers" is correctly shown. However when unmuting, the person's name is added to the subscriber list but "No subscribers" is not removed. Looks like that action should happen around line 119 of the diff.

review: Needs Fixing (code)

[Данило Шеган (danilo)]

FTR: superseded by https://code.launchpad.net/~gary/launchpad/bug-772763-remove-unmute-dialog/+merge/61806

=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-05-18 18:34:19 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-05-20 15:07:33 +0000
@@ -246,34 +246,6 @@
     }, '.unsub-icon');
 }
 
-/*
- * Set up and return a Subscription object for the mute link.
- * @method get_mute_subscription
- */
-function get_mute_subscription() {
-    setup_client_and_bug();
-    var mute_link = Y.one('.menu-link-mute_subscription');
-    if (Y.Lang.isNull(mute_link)) {
-        return null;
-    }
-    var mute_subscription = new Y.lp.bugs.subscriber.Subscription({
-        link: mute_link,
-        spinner: Y.one('#mute-unmute-spinner'),
-        subscriber: new Y.lp.bugs.subscriber.Subscriber({
-            uri: LP.links.me,
-            subscriber_ids: subscriber_ids
-        })
-    });
-    var parent_node = mute_link.get('parentNode');
-    mute_subscription.set(
-        'is_subscribed', !parent_node.hasClass('subscribed-false'));
-    mute_subscription.set(
-        'is_muted', parent_node.hasClass('muted-true'));
-    mute_subscription.set(
-        'person', mute_subscription.get('subscriber'));
-    return mute_subscription;
-}
-
 
 /**
  * We can have at most one advanced subscription overlay shown,
@@ -310,74 +282,118 @@
     if (LP.links.me === undefined) {
         return;
     }
+    var link = Y.one('.menu-link-mute_subscription');
+    if (Y.Lang.isNull(link)) {
+        return;
+    }
+    link.addClass('js-action');
+    setup_client_and_bug();
+    link.on('click', _get_toggle_mute(link));
+}
 
-    var mute_subscription = get_mute_subscription();
-    if (Y.Lang.isNull(mute_subscription)) {
-        return;
-    }
-    var mute_link = mute_subscription.get('link');
-    var parent_node = mute_link.get('parentNode');
-    mute_link.addClass('js-action');
-    mute_link.on('click', function(e) {
-        e.halt();
+// This is a helper, factored out for reusability by setup_mute_link_handlers
+// and toggle_mute.
+function _get_toggle_mute(link) {
+    var parent_node = link.get('parentNode');
+    var spinner = Y.one('#mute-unmute-spinner');
+    var hide_spinner = function() {
+        link.set('display', 'inline');
+        spinner.set('display', 'none');
+    };
+    var handler = new Y.lp.client.ErrorHandler();
+    handler.showError = function(error_msg) {
+        Y.lp.app.errors.display_error(link, error_msg);
+    };
+    handler.clearProgressUI = hide_spinner;
+    return function (e) {
+        if (!Y.Lang.isUndefined(e)) {
+            e.halt();
+        }
         var is_muted = parent_node.hasClass('muted-true');
+        var spinner_text, method_name;
         if (! is_muted) {
-            mute_subscription.enable_spinner('Muting...');
-            mute_current_user(mute_subscription);
+            spinner_text = 'Muting...';
+            method_name = 'mute';
         } else {
-            create_new_subscription_overlay(
-                mute_subscription, "Unmute subscription");
+            spinner_text = 'Unmuting...';
+            method_name = 'unmute';
         }
-    });
+        spinner.set('innerHTML', spinner_text);
+        link.set('display', 'none');
+        spinner.set('display', 'block');
+        var config = {
+            on: {
+                success: function(response) {
+                    is_muted = ! is_muted; // We successfully toggled.
+                    var subscription = get_subscribe_self_subscription();
+                    subscription.enable_spinner('Updating...');
+                    var subscription_link = subscription.get('link');
+                    var is_subscribed = false;
+                    var is_dupe_subscribed =
+                        subscription.has_duplicate_subscriptions();
+                    var label = subscription_labels.SUBSCRIBE;
+                    update_mute_after_subscription_change(is_muted);
+                    if (is_muted) {
+                        // Remove the subscriber's name from the subscriber
+                        // list, if it's there.
+                        Y.lp.bugs.subscribers_list.remove_user_link(
+                            subscription.get('subscriber'));
+                    } else {
+                        // When unmuting, the ``response`` is the previously
+                        // muted subscription, or null.
+                        is_subscribed = ! Y.Lang.isNull(response);
+                        if (is_subscribed) {
+                            subscription.set('is_direct', true);
+                            add_user_name_link(subscription);
+                            label = subscription_labels.EDIT;
+                        }
+                    }
+                    hide_spinner();
+                    Y.lazr.anim.green_flash({ node: parent_node }).run();
+                    set_subscription_link_parent_class(
+                        subscription_link, is_subscribed, is_dupe_subscribed);
+                    subscription.disable_spinner(label);
+                },
+                failure: handler.getFailureHandler()
+            },
+            parameters: {}
+        };
+        lp_client.named_post(bug_repr.self_link, method_name, config);
+    };
+}
+
+/*
+ * Toggle the mute state.
+ */
+
+function toggle_mute() {
+    if (LP.links.me === undefined) {
+        return;
+    }
+    var link = Y.one('.menu-link-mute_subscription');
+    if (Y.Lang.isNull(link)) {
+        return;
+    }
+    return _get_toggle_mute(link)();
 }
 
 /*
  * Update the Mute link after the user's subscriptions or mutes have
  * changed.
  */
-function update_mute_after_subscription_change(mute_subscription) {
-    var mute_link = mute_subscription.get('link');
-    var parent_node = mute_link.get('parentNode');
+function update_mute_after_subscription_change(is_muted) {
+    var link = Y.one('.menu-link-mute_subscription');
+    var parent_node = link.get('parentNode');
+    if (Y.Lang.isUndefined(is_muted)) {
+        is_muted = parent_node.hasClass('muted-true');
+    }
     parent_node.removeClass('hidden');
-    if (mute_subscription.get('is_muted')) {
-        parent_node.removeClass('muted-false');
-        parent_node.addClass('muted-true');
-        mute_link.setAttribute(
-            'href', mute_link.getAttribute('href').replace(
-                /\+mute$/, '+subscribe'));
-        mute_subscription.disable_spinner("Unmute bug mail");
-    } else {
-        parent_node.removeClass('muted-true');
-        parent_node.addClass('muted-false');
-        mute_link.setAttribute(
-            'href', mute_link.getAttribute('href').replace(
-                /\+subscribe$/, '+mute'));
-        mute_subscription.disable_spinner("Mute bug mail");
-    }
-}
-
-/*
- * Update the subscription links after the mute button has been clicked.
- *
- * @param mute_subscription {Object} A Y.lp.bugs.subscriber.Subscription
- *                                   object.
- */
-function update_subscription_after_mute_or_unmute(mute_subscription) {
-    var subscription = get_subscribe_self_subscription();
-    var subscription_link = subscription.get('link');
-
-    subscription.enable_spinner('Updating...');
-    if (mute_subscription.get('is_muted')) {
-        subscription.disable_spinner(subscription_labels.SUBSCRIBE);
-        if (subscription.has_duplicate_subscriptions()) {
-            set_subscription_link_parent_class(
-                subscription_link, false, true);
-        } else {
-            set_subscription_link_parent_class(
-                subscription_link, false, false);
-        }
-    } else {
-        subscription.disable_spinner(subscription_labels.SUBSCRIBE);
+    if (is_muted) {
+        parent_node.replaceClass('muted-false', 'muted-true');
+        link.set('innerHTML', "Unmute bug mail");
+    } else {
+        parent_node.replaceClass('muted-true', 'muted-false');
+        link.set('innerHTML', "Mute bug mail");
     }
 }
 
@@ -768,8 +784,7 @@
                 }
 
                 add_user_name_link(subscription);
-                var mute_subscription = get_mute_subscription();
-                update_mute_after_subscription_change(mute_subscription);
+                update_mute_after_subscription_change();
 
                 // Only when the link is added to the page, indicate success.
                 Y.on('contentready', function() {
@@ -884,95 +899,6 @@
 }
 
 /*
- * Mute the current user via the LP API.
- *
- * @method mute_current_user
- * @param subscription {Object} A Y.lp.bugs.subscribe.Subscription object.
- */
-function mute_current_user(subscription) {
-    subscription.enable_spinner('Muting...');
-    var subscription_link = subscription.get('link');
-    var subscriber = subscription.get('subscriber');
-    var bug_notification_level = subscription.get('bug_notification_level');
-
-    var error_handler = new Y.lp.client.ErrorHandler();
-    error_handler.clearProgressUI = function () {
-        subscription.disable_spinner();
-    };
-    error_handler.showError = function (error_msg) {
-        Y.lp.app.errors.display_error(subscription_link, error_msg);
-    };
-
-    var config = {
-        on: {
-            success: function(client) {
-                subscription.disable_spinner('Unmute bug mail');
-                var flash_node = subscription_link.get('parentNode');
-                var mute_anim = Y.lazr.anim.green_flash({ node: flash_node });
-                mute_anim.run();
-
-                // Remove the subscriber's name from the subscriber
-                // list, if it's there.
-                Y.lp.bugs.subscribers_list.remove_user_link(subscriber);
-                subscription.set('is_muted', true);
-                update_mute_after_subscription_change(subscription);
-                update_subscription_after_mute_or_unmute(subscription);
-            },
-
-            failure: error_handler.getFailureHandler()
-        },
-
-        parameters: {
-            person: Y.lp.client.get_absolute_uri(
-                subscriber.get('escaped_uri'))
-        }
-    };
-    lp_client.named_post(bug_repr.self_link, 'mute', config);
-}
-
-/*
- * Unmute the current user via the LP API.
- *
- * @method unmute_current_user
- * @param subscription {Object} A Y.lp.bugs.subscriber.Subscription object.
- */
-function unmute_current_user(subscription) {
-    subscription.enable_spinner('Unmuting...');
-    var subscription_link = subscription.get('link');
-    var subscriber = subscription.get('subscriber');
-
-    var error_handler = new Y.lp.client.ErrorHandler();
-    error_handler.clearProgressUI = function () {
-        subscription.disable_spinner();
-    };
-    error_handler.showError = function (error_msg) {
-        Y.lp.app.errors.display_error(subscription_link, error_msg);
-    };
-
-    var config = {
-        on: {
-            success: function(client) {
-                subscription.disable_spinner('Mute bug mail');
-                var flash_node = subscription_link.get('parentNode');
-                var anim = Y.lazr.anim.green_flash({ node: flash_node });
-                anim.run();
-                subscription.set('is_muted', false);
-                update_mute_after_subscription_change(subscription);
-                update_subscription_after_mute_or_unmute(subscription);
-            },
-
-            failure: error_handler.getFailureHandler()
-        },
-
-        parameters: {
-            person: Y.lp.client.get_absolute_uri(
-                subscriber.get('escaped_uri'))
-        }
-    };
-    lp_client.named_post(bug_repr.self_link, 'unmute', config);
-}
-
-/*
  * Initialize click handler for the subscribe someone else link.
  *
  * @method setup_subscribe_someone_else_handler
@@ -1219,9 +1145,9 @@
  */
 function handle_advanced_subscription_overlay(form_data) {
     var subscription;
-    var mute_subscription = get_mute_subscription();
-    var is_muted = (!Y.Lang.isNull(mute_subscription) &&
-                    mute_subscription.get('is_muted'));
+    var mute_link = Y.one('.menu-link-mute_subscription');
+    var parent_node = mute_link.get('parentNode');
+    var is_muted = parent_node.hasClass('muted-true');
     var requested_subscriber;
     var request_for_self = false;
     // XXX Danilo 20110422: this is a very lousy "special string"
@@ -1259,9 +1185,6 @@
             on: {
                 success: function(lp_subscription) {
                     subscription.enable_spinner('Updating subscription...');
-                    if (is_muted) {
-                        mute_subscription.enable_spinner('Unmuting...');
-                    }
                     lp_subscription.set(
                         'bug_notification_level',
                         form_data['field.bug_notification_level'][0]);
@@ -1277,12 +1200,10 @@
                                     });
                                 anim.run();
                                 if (is_muted) {
-                                    mute_subscription.set('is_muted', false);
-                                    mute_subscription.disable_spinner(
-                                        "Mute bug mail");
-                                    update_mute_after_subscription_change(
-                                        mute_subscription);
-                                    add_user_name_link(subscription);
+                                    // We're going to assume that the user
+                                    // wants to unmute also, since they
+                                    // bothered to change their subscription.
+                                    toggle_mute();
                                 }
                             },
                             failure: function(e) {
@@ -1315,7 +1236,7 @@
         // 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);
+        toggle_mute();
     } else {
         // Unsubscribe this person/team.
         unsubscribe_current_user(subscription);
@@ -1392,10 +1313,7 @@
                             if (team_member) {
                                 subscription.set('can_be_unsubscribed', true);
                                 add_temp_user_name(subscription);
-                                var mute_subscription;
-                                mute_subscription = get_mute_subscription();
-                                update_mute_after_subscription_change(
-                                    mute_subscription);
+                                update_mute_after_subscription_change();
                             } else {
                                 subscription.set(
                                    'can_be_unsubscribed', false);

=== modified file 'lib/lp/bugs/javascript/subscriber.js'
--- lib/lp/bugs/javascript/subscriber.js	2011-04-22 16:59:16 +0000
+++ lib/lp/bugs/javascript/subscriber.js	2011-05-20 15:07:33 +0000
@@ -57,7 +57,7 @@
     },
 
     'spinner': {
-        vallue: null
+        value: null
     },
 
     'bug_notification_level': {