launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03695
[Merge] lp:~danilo/launchpad/bug-772763-remove-unmute-dialog into lp:launchpad/db-devel
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-772763-remove-unmute-dialog into lp:launchpad/db-devel with lp:~danilo/launchpad/bug-772763-remove-unmute-dialog-part1 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
Stuart Bishop (stub): db
Related bugs:
Bug #772763 in Launchpad itself: "Unmuting a bug's notifications should restore your previous direct subscription"
https://bugs.launchpad.net/launchpad/+bug/772763
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-772763-remove-unmute-dialog/+merge/61780
= 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
--
https://code.launchpad.net/~danilo/launchpad/bug-772763-remove-unmute-dialog/+merge/61780
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-772763-remove-unmute-dialog into lp:launchpad/db-devel.
=== 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': {
Follow ups
