launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02501
[Merge] lp:~gmb/launchpad/make-subscriptions-editable-bug-710603 into lp:launchpad
Graham Binns has proposed merging lp:~gmb/launchpad/make-subscriptions-editable-bug-710603 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#710603 It should be possible to edit an existing subscription using the new JS overlay
https://bugs.launchpad.net/bugs/710603
For more details, see:
https://code.launchpad.net/~gmb/launchpad/make-subscriptions-editable-bug-710603/+merge/48359
This branch makes the new JS overlay for subscriptions work for both updating a subscription and for unsubscribing from a bug.
In order to make this work, I've made the following changes:
- I've updated the edit_description for the BugContextMenu to return
"Edit description" when someone's subscribed, but only if they're
able to see the new advanced subscriptions work.
- I've added two functions to bugtask_index.js:
- handle_advanced_subscription_overlay() handles the form submissions
from the new overlay. It handles the following cases:
- User is not subscribed (it subscribes them).
- User is subscribed and wants to update their subscription (it
updates their subscription).
- User is subscribed and wants to unsubscribe (it unsubscribes
them).
- setup_advanced_subscrition_overlay() creates and returns an
overlay for the advanced subscription UI.
- I've updated setup_subscription_link_handlers() so that it now uses
setup_subscription_link_handlers() rather than doing all the work of
creating the overlay in-line.
- I've updated the subscribe_current_user() function to make sure it
sets the link title correctly when it completes.
- I've updated bugtask-index.pt so that it uses the
malone.advanced-subscriptions-enabled feature flag to determine
whether or not to use the new JS.
--
https://code.launchpad.net/~gmb/launchpad/make-subscriptions-editable-bug-710603/+merge/48359
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/make-subscriptions-editable-bug-710603 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2011-02-03 02:45:33 +0000
+++ lib/lp/bugs/browser/bug.py 2011-02-03 11:11:44 +0000
@@ -103,6 +103,7 @@
from lp.bugs.interfaces.bugwatch import IBugWatchSet
from lp.bugs.interfaces.cve import ICveSet
from lp.bugs.mail.bugnotificationbuilder import format_rfc2822_date
+from lp.services import features
from lp.services.fields import DuplicateBug
from lp.services.propertycache import cachedproperty
@@ -207,6 +208,12 @@
# have to duplicate menu code.
ContextMenu.__init__(self, getUtility(ILaunchBag).bugtask)
+ @cachedproperty
+ def _use_advanced_features(self):
+ """Return True if advanced subscriptions features are enabled."""
+ return features.getFeatureFlag(
+ 'malone.advanced-subscriptions.enabled')
+
def editdescription(self):
"""Return the 'Edit description/tags' Link."""
text = 'Update description / tags'
@@ -240,8 +247,12 @@
elif user is not None and (
self.context.bug.isSubscribed(user) or
self.context.bug.isSubscribedToDupes(user)):
- text = 'Unsubscribe'
- icon = 'remove'
+ if self._use_advanced_features:
+ text = 'Edit subscription'
+ icon = 'edit'
+ else:
+ text = 'Unsubscribe'
+ icon = 'remove'
else:
text = 'Subscribe'
icon = 'add'
=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py 2011-01-21 20:23:43 +0000
+++ lib/lp/bugs/browser/bugsubscription.py 2011-02-03 11:11:44 +0000
@@ -109,7 +109,7 @@
# form. The BugNotificationLevel descriptions are too generic.
bug_notification_level_terms = [
SimpleTerm(
- level, level.name,
+ level, level.title,
self._bug_notification_level_descriptions[level])
# We reorder the items so that COMMENTS comes first. We also
# drop the NOTHING option since it just makes the UI
=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2010-11-12 11:02:58 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2011-02-03 11:11:44 +0000
@@ -54,7 +54,7 @@
bug.default_bugtask, BugSubscriptionSubscribeSelfView)
form_data = {
'field.subscription': person.name,
- 'field.bug_notification_level': level.name,
+ 'field.bug_notification_level': level.title,
}
harness.submit('continue', form_data)
@@ -63,7 +63,7 @@
level, subscription.bug_notification_level,
"Bug notification level of subscription should be %s, is "
"actually %s." % (
- level.name, subscription.bug_notification_level.name))
+ level.title, subscription.bug_notification_level.title))
def test_nothing_is_not_a_valid_level(self):
# BugNotificationLevel.NOTHING isn't considered valid when
@@ -77,7 +77,7 @@
bug.default_bugtask, BugSubscriptionSubscribeSelfView)
form_data = {
'field.subscription': person.name,
- 'field.bug_notification_level': level.name,
+ 'field.bug_notification_level': level.title,
}
harness.submit('continue', form_data)
self.assertTrue(harness.hasErrors())
@@ -102,7 +102,7 @@
bug.default_bugtask, BugSubscriptionSubscribeSelfView)
form_data = {
'field.subscription': 'update-subscription',
- 'field.bug_notification_level': level.name,
+ 'field.bug_notification_level': level.title,
}
harness.submit('continue', form_data)
self.assertFalse(harness.hasErrors())
@@ -112,7 +112,7 @@
BugNotificationLevel.METADATA,
subscription.bug_notification_level,
"Bug notification level of subscription should be METADATA, is "
- "actually %s." % subscription.bug_notification_level.name)
+ "actually %s." % subscription.bug_notification_level.title)
def test_user_can_unsubscribe(self):
# A user can unsubscribe from a bug using the
@@ -151,7 +151,7 @@
bug.default_bugtask, BugSubscriptionSubscribeSelfView)
form_data = {
'field.subscription': person.name,
- 'field.bug_notification_level': level.name,
+ 'field.bug_notification_level': level.title,
}
harness.submit('continue', form_data)
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2011-01-21 05:10:55 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2011-02-03 11:11:44 +0000
@@ -292,6 +292,104 @@
var picker = Y.lp.app.picker.create('ValidPersonOrTeam', config);
}
+
+/*
+ * Handle the advanced_subscription_overlay's form submissions.
+ *
+ * @method handle_advanced_subscription_overlay
+ * @param subscription {Object} A Y.lp.bugs.subscriber.Subscription object.
+ * @param form_data {Object} The data from the submitted form.
+ */
+function handle_advanced_subscription_overlay(subscription, form_data) {
+ var link = subscription.get('link');
+ var link_parent = link.get('parentNode');
+ if (link_parent.hasClass('subscribed-false') &&
+ link_parent.hasClass('dup-subscribed-false')) {
+ // The user isn't subscribed, 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') {
+ // The user is already subscribed and wants to update their
+ // subscription.
+ setup_client_and_bug();
+ var person_name = subscription.get('person').get('name');
+ var subscription_url =
+ lp_bug_entry.get('self_link') + '/+subscription/' +
+ person_name;
+ config = {
+ on: {
+ success: function(lp_subscription) {
+ subscription.enable_spinner('Updating subscription...');
+ lp_subscription.set(
+ 'bug_notification_level',
+ form_data['field.bug_notification_level'][0])
+ save_config = {
+ on: {
+ success: function(e) {
+ subscription.disable_spinner(
+ 'Edit subscription');
+ var anim = Y.lazr.anim.green_flash({
+ node: link_parent
+ });
+ anim.run();
+ },
+ failure: function(e) {
+ subscription.disable_spinner(
+ 'Edit subscription');
+ var anim = Y.lazr.anim.red_flash({
+ node: link_parent
+ });
+ anim.run();
+ }
+ }
+ }
+ lp_subscription.lp_save(save_config);
+ }
+ }
+ }
+ lp_client.get(subscription_url, config);
+ } else {
+ // The user is already subscribed and wants to unsubscribe.
+ unsubscribe_current_user(subscription);
+ }
+}
+
+
+/*
+ * Create and return a FormOverlay for advanced subscription
+ * interactions.
+ *
+ * @method setup_advanced_subscription_overlay
+ * @param subscription {Object} A Y.lp.bugs.subscriber.Subscription object.
+ */
+function setup_advanced_subscription_overlay(subscription) {
+ var subscription_overlay = new Y.lazr.FormOverlay({
+ headerContent: '<h2>Subscribe to bug</h2>',
+ form_submit_button:
+ Y.Node.create(submit_button_html),
+ form_cancel_button:
+ Y.Node.create(cancel_button_html),
+ centered: true,
+ visible: false
+ });
+ subscription_overlay.set(
+ 'form_submit_callback', function(form_data) {
+ handle_advanced_subscription_overlay(subscription, form_data);
+ subscription_overlay.hide();
+ });
+
+ var subscription_link_url = subscription.get(
+ 'link').get('href') + '/++form++';
+ subscription_overlay.loadFormContentAndRender(
+ subscription_link_url);
+ subscription_overlay.render('#privacy-form-container');
+ return subscription_overlay
+}
+
+
/*
* Initialize callbacks for subscribe/unsubscribe links.
*
@@ -326,16 +424,22 @@
subscription.set('person', subscription.get('subscriber'));
subscription.set('is_team', false);
var parent = e.target.get('parentNode');
- // Look for the false conditions of subscription, which
- // is_direct_subscription, etc. don't report correctly,
- // to make sure we only use subscribe_current_user for
- // the current user.
- if (parent.hasClass('subscribed-false') &&
- parent.hasClass('dup-subscribed-false')) {
- subscribe_current_user(subscription);
- }
- else {
- unsubscribe_current_user(subscription);
+ if (namespace.use_advanced_subscriptions) {
+ var subscription_overlay =
+ setup_advanced_subscription_overlay(subscription);
+ subscription_overlay.show();
+ } else {
+ // Look for the false conditions of subscription, which
+ // is_direct_subscription, etc. don't report correctly,
+ // to make sure we only use subscribe_current_user for
+ // the current user.
+ if (parent.hasClass('subscribed-false') &&
+ parent.hasClass('dup-subscribed-false')) {
+ subscribe_current_user(subscription);
+ }
+ else {
+ unsubscribe_current_user(subscription);
+ }
}
});
subscription.get('link').addClass('js-action');
@@ -1209,6 +1313,7 @@
subscription.enable_spinner('Subscribing...');
var subscription_link = subscription.get('link');
var subscriber = subscription.get('subscriber');
+ var bug_notification_level = subscription.get('bug_notification_level');
var error_handler = new LP.client.ErrorHandler();
error_handler.clearProgressUI = function () {
@@ -1221,7 +1326,11 @@
var config = {
on: {
success: function(client) {
- subscription.disable_spinner('Unsubscribe');
+ if (namespace.use_advanced_subscriptions) {
+ subscription.disable_spinner('Edit subscription');
+ } else {
+ subscription.disable_spinner('Unsubscribe');
+ }
if (subscription.has_duplicate_subscriptions()) {
set_subscription_link_parent_class(
@@ -1251,7 +1360,8 @@
parameters: {
person: LP.client.get_absolute_uri(subscriber.get('escaped_uri')),
- suppress_notify: false
+ suppress_notify: false,
+ level: bug_notification_level
}
};
lp_client.named_post(bug_repr.self_link, 'subscribe', config);
@@ -1929,4 +2039,4 @@
"lazr.formoverlay", "lazr.anim", "lazr.base",
"lazr.overlay", "lazr.choiceedit", "lp.app.picker",
"lp.client.plugins", "lp.bugs.subscriber",
- "lp.app.errors"]});
+ "lp.app.errors", "log"]});
=== modified file 'lib/lp/bugs/javascript/subscriber.js'
--- lib/lp/bugs/javascript/subscriber.js 2010-07-11 00:32:53 +0000
+++ lib/lp/bugs/javascript/subscriber.js 2011-02-03 11:11:44 +0000
@@ -52,6 +52,10 @@
'spinner': {
vallue: null
+ },
+
+ 'bug_notification_level': {
+ value: 'Discussion'
}
};
@@ -167,6 +171,9 @@
if (text == 'Subscribe') {
link.removeClass('modify remove');
link.addClass('add');
+ } else if (text == 'Edit subscription') {
+ link.removeClass('add');
+ link.addClass('modify edit');
} else {
link.removeClass('add');
link.addClass('modify remove');
=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions-advanced-features.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions-advanced-features.txt 2010-10-22 15:06:30 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions-advanced-features.txt 2011-02-03 11:11:44 +0000
@@ -26,9 +26,9 @@
... name='field.bug_notification_level')
>>> for control in bug_notification_level_control.controls:
... print control.optionValue
- COMMENTS
- METADATA
- LIFECYCLE
+ Discussion
+ Details
+ Lifecycle
The user can now subscribe to the bug at any of the given notification
levels. In this case, they want to subscribe to just metadata updates:
=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt 2011-02-03 05:57:10 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt 2011-02-03 11:11:44 +0000
@@ -10,12 +10,25 @@
<script type='text/javascript' tal:content="string:var yui_base='${yui}';" />
<script type='text/javascript' id='available-official-tags-js'
tal:content="view/available_official_tags_js" />
+ <tal:subscriptions-js
+ define="enabled features/malone.advanced-subscriptions.enabled">
+ <script type="text/javascript" id="use-advanced-subscriptions"
+ tal:condition="enabled">
+ use_advanced_subscriptions = true;
+ </script>
+ <script type="text/javascript" id="use-advanced-subscriptions"
+ tal:condition="not:enabled">
+ use_advanced_subscriptions = false;
+ </script>
+ </tal:subscriptions-js>
<script type="text/javascript">
LPS.use('base', 'node', 'oop', 'event', 'lp.bugs.bugtask_index',
'lp.code.branchmergeproposal.diff', function(Y) {
+ Y.lp.bugs.bugtask_index.use_advanced_subscriptions =
+ use_advanced_subscriptions;
Y.lp.bugs.bugtask_index.setup_bugtask_index();
Y.on('load', function(e) {
- Y.lp.code.branchmergeproposal.diff.connect_diff_links();
+ Y.lp.code.branchmergeproposal.diff.connect_diff_links();
}, window);
});
</script>
=== modified file 'lib/lp/registry/browser/tests/test_structuralsubscription.py'
--- lib/lp/registry/browser/tests/test_structuralsubscription.py 2010-12-21 09:42:47 +0000
+++ lib/lp/registry/browser/tests/test_structuralsubscription.py 2011-02-03 11:11:44 +0000
@@ -198,7 +198,7 @@
self.target, StructuralSubscriptionView)
form_data = {
'field.subscribe_me': 'on',
- 'field.bug_notification_level': level.name,
+ 'field.bug_notification_level': level.title,
}
harness.submit('save', form_data)
self.assertFalse(harness.hasErrors())
@@ -226,7 +226,7 @@
form_data = {
'field.subscribe_me': '',
'field.subscriptions_team': team.name,
- 'field.bug_notification_level': level.name,
+ 'field.bug_notification_level': level.title,
}
harness.submit('save', form_data)
self.assertFalse(harness.hasErrors())
