launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03421
[Merge] lp:~danilo/launchpad/bug-761257 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-761257 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #761257 in Launchpad itself: "Advanced subscription overlay for direct subscriptions does not handle unsubscribing teams"
https://bugs.launchpad.net/launchpad/+bug/761257
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-761257/+merge/58801
= 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
--
https://code.launchpad.net/~danilo/launchpad/bug-761257/+merge/58801
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-761257 into lp:launchpad.
=== 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);
}
}
