launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05041
[Merge] lp:~stevenk/launchpad/private-bug-unsubscribe-confirm into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/private-bug-unsubscribe-confirm into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #485126 in Launchpad itself: "Accidentally unsubscribing myself from a private bug makes the bug inaccessible"
https://bugs.launchpad.net/launchpad/+bug/485126
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/private-bug-unsubscribe-confirm/+merge/76326
This is my first JS branch, so please be gentle.
This bug now brings up a confirmation overlay when a user using the +subscriptions page on a private bug removes their subscription. I have added tests as well as checking that the confirmationoverlay is not present for a public bug.
This branch does not fully fix the bug, however, so I've added an XXX to the other place that needs fixing, and I will address it in a future branch.
--
https://code.launchpad.net/~stevenk/launchpad/private-bug-unsubscribe-confirm/+merge/76326
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/private-bug-unsubscribe-confirm into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py 2011-07-21 18:07:26 +0000
+++ lib/lp/bugs/browser/bugsubscription.py 2011-09-21 04:22:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Views for BugSubscription."""
@@ -416,15 +416,10 @@
self.context.bug.unmute(self.user, self.user)
def _handleUnsubscribeCurrentUser(self):
- """Handle the special cases for unsubscribing the current user.
-
- when the bug is private. The user must be unsubscribed from all dupes
- too, or they would keep getting mail about this bug!
- """
- # ** Important ** We call unsubscribeFromDupes() before
- # unsubscribe(), because if the bug is private, the current user
- # will be prevented from calling methods on the main bug after
- # they unsubscribe from it!
+ """Handle the special cases for unsubscribing the current user."""
+ # We call unsubscribeFromDupes() before unsubscribe(), because
+ # if the bug is private, the current user will be prevented from
+ # calling methods on the main bug after they unsubscribe from it.
unsubed_dupes = self.context.bug.unsubscribeFromDupes(
self.user, self.user)
self.context.bug.unsubscribe(self.user, self.user)
@@ -623,6 +618,7 @@
cache = IJSONRequestCache(self.request).objects
cache.update(references)
cache['bug_subscription_info'] = subdata
+ cache['bug_is_private'] = self.context.bug.private
@property
def label(self):
=== modified file 'lib/lp/bugs/javascript/bug_subscription_portlet.js'
--- lib/lp/bugs/javascript/bug_subscription_portlet.js 2011-08-31 10:40:04 +0000
+++ lib/lp/bugs/javascript/bug_subscription_portlet.js 2011-09-21 04:22:25 +0000
@@ -206,6 +206,8 @@
// the `method_name` to call on the server, and the `parameters` to
// send across, it returns a link node with the click handler properly
// set.
+ // XXX StevenK 2011-09-26 bug=485126 We should expand the overlay here
+ // and ask for confirmation if it's a private bug
var make_action = function(text, css_class, method_name, parameters) {
result = make_link(text, css_class)
.addClass('js-action');
=== modified file 'lib/lp/bugs/javascript/subscription.js'
--- lib/lp/bugs/javascript/subscription.js 2011-06-02 18:23:05 +0000
+++ lib/lp/bugs/javascript/subscription.js 2011-09-21 04:22:25 +0000
@@ -997,6 +997,61 @@
);
}
+function make_action_link_function (
+ node, sprite_class, method_name, handler, parameters, client) {
+ var config = {
+ on: {success:
+ function (maybe_sub) {
+ node.replaceClass('spinner', sprite_class);
+ var info = LP.cache.bug_subscription_info;
+ var old = info.direct.personal[0];
+ if (Y.Lang.isValue(maybe_sub)) {
+ // Set the subscription in info and in cache.
+ var sub = maybe_sub.getAttrs();
+ if (Y.Lang.isValue(old)) {
+ info.direct.personal[0].subscription = sub;
+ } else {
+ // We don't have enough information to calculate
+ // everything on the fly. Luckily, we don't need
+ // most of it, and we think it is alright to not
+ // include the extra information about
+ // principal_is_reporter, security_contact_pillars,
+ // and bug_supervisor_pillars.
+ info.direct.personal.push(
+ {principal: {},
+ bug: {},
+ subscription: sub,
+ principal_is_reporter: false,
+ security_contact_pillars: [],
+ bug_supervisor_pillars: []
+ });
+ info.direct.count += 1;
+ info.count += 1;
+ }
+ } else {
+ if (Y.Lang.isValue(old)) {
+ info.direct.personal.pop();
+ info.direct.count -= 1;
+ info.count -= 1;
+ }
+ }
+ if (method_name === 'mute') {
+ info.muted = true;
+ } else if (method_name === 'unmute') {
+ info.muted = false;
+ }
+ reveal_direct_description_actions(
+ Y.one('#direct-subscription'),
+ get_direct_subscription_information(info));
+ },
+ failure: handler.getFailureHandler()},
+ parameters: parameters
+ };
+ node.replaceClass(sprite_class, 'spinner');
+ client.named_post(
+ LP.cache.context.bug_link, method_name, config);
+}
+
/**
* Makes links for all kinds of actions.
*
@@ -1030,65 +1085,31 @@
'click',
function (e) {
e.halt();
- var config = {
- on: {success:
- function (maybe_sub) {
- node.replaceClass('spinner', sprite_class);
- var info = LP.cache.bug_subscription_info;
- var old = info.direct.personal[0];
- if (Y.Lang.isValue(maybe_sub)) {
- // Set the subscription in info and in cache.
- var sub = maybe_sub.getAttrs();
- if (Y.Lang.isValue(old)) {
- info.direct.personal[0].subscription =
- sub;
- } else {
- // We don't have enough information to
- // calculate everything on the fly.
- // Luckily, we don't need most of it,
- // and we think it is alright to not
- // include the extra information about
- // principal_is_reporter,
- // security_contact_pillars, and
- // bug_supervisor_pillars.
- info.direct.personal.push(
- {principal: {},
- bug: {},
- subscription: sub,
- principal_is_reporter: false,
- security_contact_pillars: [],
- bug_supervisor_pillars: []
- });
- info.direct.count += 1;
- info.count += 1;
- }
- } else {
- if (Y.Lang.isValue(old)) {
- info.direct.personal.pop();
- info.direct.count -= 1;
- info.count -= 1;
- }
- }
- if (method_name === 'mute') {
- info.muted = true;
- } else if (method_name === 'unmute') {
- info.muted = false;
- }
- reveal_direct_description_actions(
- Y.one('#direct-subscription'),
- get_direct_subscription_information(info));
- },
- failure: handler.getFailureHandler()
- },
- parameters: parameters
+ var method_call = function() {
+ make_action_link_function(
+ node, sprite_class, method_name, handler, parameters,
+ client);
};
- node.replaceClass(sprite_class, 'spinner');
- client.named_post(
- LP.cache.context.bug_link,
- method_name,
- config);
- }
- );
+ // If this a private bug, and the user is unsubscribing
+ // themselves, give them a warning.
+ if (method_name === 'unsubscribe' && LP.cache.bug_is_private) {
+ var content = [
+ '<p>You will not have access to this bug or any of ',
+ 'its pages if you unsubscribe. If you want to stop ',
+ 'emails, choose the "Mute bug mail" option.</p><p>Do ',
+ 'you really want to unsubscribe from this bug?</p>'
+ ].join('');
+ var ns = Y.lp.app.confirmationoverlay;
+ var co = new ns.ConfirmationOverlay({
+ submit_fn: method_call,
+ form_content: content,
+ headerContent: 'Unsubscribe from this bug'
+ });
+ co.show();
+ } else {
+ method_call();
+ }
+ });
return node;
}
namespace._make_action_link = make_action_link;
@@ -1347,5 +1368,5 @@
}, '0.1', {requires: [
'dom', 'event', 'node', 'substitute', 'lazr.effects', 'lp.app.errors',
- 'lp.client'
+ 'lp.app.confirmationoverlay', 'lp.client'
]});
=== modified file 'lib/lp/bugs/javascript/tests/test_subscription.html'
--- lib/lp/bugs/javascript/tests/test_subscription.html 2011-08-10 08:43:17 +0000
+++ lib/lp/bugs/javascript/tests/test_subscription.html 2011-09-21 04:22:25 +0000
@@ -25,6 +25,8 @@
src="../../../app/javascript/overlay/overlay.js"></script>
<script type="text/javascript"
src="../../../app/javascript/extras/extras.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/confirmationoverlay/confirmationoverlay.js"></script>
<!-- The module under test -->
<script type="text/javascript"
=== modified file 'lib/lp/bugs/javascript/tests/test_subscription.js'
--- lib/lp/bugs/javascript/tests/test_subscription.js 2011-08-09 14:18:02 +0000
+++ lib/lp/bugs/javascript/tests/test_subscription.js 2011-09-21 04:22:25 +0000
@@ -1681,13 +1681,13 @@
assert_action_matches_expectations: function (
node_function, expected_id, expected_text, expected_class,
- expected_method, expected_args,
- begin_with_subscription, send_subscription) {
+ expected_method, expected_args, begin_with_subscription,
+ send_subscription, private_bug, click_ok) {
// We begin with set up.
module._lp_client = new LPClient();
var sub = {
bug: {
- 'private': false,
+ 'private': private_bug,
security_related: false
},
principal_is_reporter: false,
@@ -1708,6 +1708,7 @@
var bug_link = 'http://example.net/firefox/bug/1';
window.LP = { links: {me: '~tweedledee'},
cache: {context: {bug_link: bug_link},
+ bug_is_private: private_bug,
bug_subscription_info: {
direct: _constructCategory(
initial_subscriptions),
@@ -1730,6 +1731,26 @@
// same id.
node = display.one('#'+expected_id);
node.one('a').simulate('click');
+ var co = Y.one('.yui3-overlay.yui3-lp-app-confirmationoverlay');
+ if (private_bug === false) {
+ // If this a public bug, check the confirmation overlay is
+ // nowhere to be found
+ Y.Assert.isNull(co);
+ } else {
+ // Otherwise (private bug), click true
+ var div = Y.one('.yui3-lazr-formoverlay-actions');
+ if (click_ok === true) {
+ var ok = div.one('.ok-btn');
+ ok.simulate('click');
+ } else {
+ var cancel = div.one('.cancel-btn');
+ cancel.simulate('click');
+ Y.Assert.areEqual(
+ initial_subscriptions.length,
+ window.LP.cache.bug_subscription_info.count);
+ return;
+ }
+ }
// We should have had a named_post to the bug_link, calling the
// expected_method with the expected_args.
Y.Assert.areEqual(1, module._lp_client.received.length);
@@ -1753,14 +1774,14 @@
this.assert_action_matches_expectations(
module._mute_action, module._action_ids.mute,
'mute all emails from this bug',
- 'mute', 'mute', {}, true, true);
+ 'mute', 'mute', {}, true, true, false, true);
},
test_unmute_action: function() {
this.assert_action_matches_expectations(
module._unmute_action, module._action_ids.unmute,
'unmute emails from this bug',
- 'unmute', 'unmute', {}, true, false);
+ 'unmute', 'unmute', {}, true, false, false, true);
},
test_subscribe_all_action: function() {
@@ -1769,7 +1790,7 @@
'receive all emails about this bug',
'edit', 'subscribe',
{person: '~tweedledee', level: 'Discussion'},
- false, true);
+ false, true, false, true);
},
test_subscribe_metadata_action: function() {
@@ -1779,7 +1800,7 @@
'receive all emails about this bug except comments',
'edit', 'subscribe',
{person: '~tweedledee', level: 'Details'},
- false, true);
+ false, true, false, true);
},
test_subscribe_closed_action: function() {
@@ -1789,7 +1810,7 @@
'only receive email when this bug is closed',
'edit', 'subscribe',
{person: '~tweedledee', level: 'Lifecycle'},
- false, true);
+ false, true, false, true);
},
test_subscribe_only_metadata_action: function() {
@@ -1799,7 +1820,7 @@
'stop receiving comments from this bug',
'edit', 'subscribe',
{person: '~tweedledee', level: 'Details'},
- false, true);
+ false, true, false, true);
},
test_subscribe_only_closed_action: function() {
@@ -1809,14 +1830,14 @@
'only receive email when this bug is closed',
'edit', 'subscribe',
{person: '~tweedledee', level: 'Lifecycle'},
- false, true);
+ false, true, false, true);
},
test_unsubscribe_action: function() {
this.assert_action_matches_expectations(
module._unsubscribe_action, module._action_ids.unsubscribe,
'unsubscribe from this bug',
- 'remove', 'unsubscribe', {}, true, false);
+ 'remove', 'unsubscribe', {}, true, false, false, true);
},
test_unsubscribe_with_warning_action: function() {
@@ -1826,7 +1847,21 @@
'You can also unsubscribe from this bug. However, you also '+
'have other subscriptions to this bug that may send you email '+
'once you have unsubscribed.',
- 'remove', 'unsubscribe', {}, true, false);
+ 'remove', 'unsubscribe', {}, true, false, false, true);
+ },
+
+ test_unsubscribe_action_private_bug_cancel: function() {
+ this.assert_action_matches_expectations(
+ module._unsubscribe_action, module._action_ids.unsubscribe,
+ 'unsubscribe from this bug',
+ 'remove', null, {}, true, false, true, false);
+ },
+
+ test_unsubscribe_action_private_bug: function() {
+ this.assert_action_matches_expectations(
+ module._unsubscribe_action, module._action_ids.unsubscribe,
+ 'unsubscribe from this bug',
+ 'remove', 'unsubscribe', {}, true, false, true, true);
}
}));
Follow ups
