launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01793
[Merge] lp:~gmb/launchpad/add-update-subscription-workflow-bug-664571 into lp:launchpad/devel
Graham Binns has proposed merging lp:~gmb/launchpad/add-update-subscription-workflow-bug-664571 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
#664571 Users should have the option to alter a subscription's BugNotificationLevel
https://bugs.launchpad.net/bugs/664571
This branch fixes bug 664571 by adding a workflow to the bug +subscribe
view that allows users to update the BugNotificationLevel of an existing
bug subscription.
Changes made:
== lib/canonical/launchpad/webapp/servers.py ==
- I've added a clearSecurityPolicyCache() stub to LaunchpadTestRequest
so that the +subscribe view doesn't choke on its non-existance.
== lib/lp/bugs/browser/bugsubscription.py ==
- I've added a workflow that allows a subscriber to update their
current bug subscription to a new BugNotificationLevel.
- I've done a little refactoring to make the class a bit tidier.
== lib/lp/bugs/browser/tests/test_bugsubscription_views.py ==
- I've added a couple of tests to cover the update-subscription
workflow.
- I've added a unit test to cover unsubscribing from a bug using the
+subscribe form (just in case we do deprecate pagetests after all).
== lib/lp/bugs/configure.zcml ==
- I've updated this to reflect the changes to IBug (below).
== lib/lp/bugs/interfaces/bug.py ==
- I've added a method declaration for getSubscriptionForPerson() to
IBug. This will allow us to get the BugSubscription object for a
given person on a bug so that we can update it.
== lib/lp/bugs/model/bug.py ==
- I've added the implementation for getSubscriptionForPerson() to Bug.
--
https://code.launchpad.net/~gmb/launchpad/add-update-subscription-workflow-bug-664571/+merge/39966
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/add-update-subscription-workflow-bug-664571 into lp:launchpad/devel.
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py 2010-10-25 13:16:10 +0000
+++ lib/canonical/launchpad/webapp/servers.py 2010-11-03 14:03:50 +0000
@@ -912,6 +912,10 @@
"""See `IPublicationRequest`."""
self.principal = principal
+ def clearSecurityPolicyCache(self):
+ """See ILaunchpadBrowserApplicationRequest."""
+ return
+
class LaunchpadTestResponse(LaunchpadBrowserResponse):
"""Mock response for use in unit and functional tests.
=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py 2010-10-22 16:01:06 +0000
+++ lib/lp/bugs/browser/bugsubscription.py 2010-11-03 14:03:50 +0000
@@ -152,6 +152,10 @@
super(BugSubscriptionSubscribeSelfView, self).initialize()
@cachedproperty
+ def current_user_subscription(self):
+ return self.context.bug.getSubscriptionForPerson(self.user)
+
+ @cachedproperty
def _bug_notification_level_field(self):
"""Return a custom form field for bug_notification_level."""
# We rebuild the items that we show in the field so that the
@@ -169,22 +173,33 @@
]
bug_notification_vocabulary = SimpleVocabulary(
bug_notification_level_terms)
+
+ if self.current_user_subscription is not None:
+ default_value = (
+ self.current_user_subscription.bug_notification_level)
+ else:
+ default_value = BugNotificationLevel.COMMENTS
+
bug_notification_level_field = Choice(
__name__='bug_notification_level', title=_("Tell me when"),
vocabulary=bug_notification_vocabulary, required=True,
- default=BugNotificationLevel.COMMENTS)
+ default=default_value)
return bug_notification_level_field
- def setUpFields(self):
- """See `LaunchpadFormView`."""
- super(BugSubscriptionSubscribeSelfView, self).setUpFields()
- if self.user is None:
- return
+ @cachedproperty
+ def _update_subscription_term(self):
+ return SimpleTerm(
+ 'update-subscription', 'update-subscription',
+ 'Update my current subscription')
+ @cachedproperty
+ def _subscription_field(self):
subscription_terms = []
self_subscribed = False
for person in self._subscribers_for_current_user:
if person.id == self.user.id:
+ if self._use_advanced_features:
+ subscription_terms.append(self._update_subscription_term)
subscription_terms.append(
SimpleTerm(
person, person.name,
@@ -202,22 +217,32 @@
SimpleTerm(
self.user, self.user.name, 'Subscribe me to this bug'))
subscription_vocabulary = SimpleVocabulary(subscription_terms)
- default_subscription_value = subscription_vocabulary.getTermByToken(
- self.user.name).value
+ if self.user_is_subscribed and self._use_advanced_features:
+ default_subscription_value = self._update_subscription_term.value
+ else:
+ default_subscription_value = (
+ subscription_vocabulary.getTermByToken(self.user.name).value)
subscription_field = Choice(
__name__='subscription', title=_("Subscription options"),
vocabulary=subscription_vocabulary, required=True,
default=default_subscription_value)
+ return subscription_field
+
+ def setUpFields(self):
+ """See `LaunchpadFormView`."""
+ super(BugSubscriptionSubscribeSelfView, self).setUpFields()
+ if self.user is None:
+ return
if self._use_advanced_features:
self.form_fields = self.form_fields.omit('bug_notification_level')
- self.form_fields += formlib.form.Fields(subscription_field)
+ self.form_fields += formlib.form.Fields(self._subscription_field)
self.form_fields += formlib.form.Fields(
self._bug_notification_level_field)
self.form_fields['bug_notification_level'].custom_widget = (
CustomWidgetFactory(RadioWidget))
else:
- self.form_fields += formlib.form.Fields(subscription_field)
+ self.form_fields += formlib.form.Fields(self._subscription_field)
self.form_fields['subscription'].custom_widget = CustomWidgetFactory(
RadioWidget)
@@ -225,19 +250,16 @@
"""See `LaunchpadFormView`."""
super(BugSubscriptionSubscribeSelfView, self).setUpWidgets()
if self._use_advanced_features:
- if self.user_is_subscribed:
- self.widgets['bug_notification_level'].visible = False
+ if self._subscriber_count_for_current_user == 0:
+ # We hide the subscription widget if the user isn't
+ # subscribed, since we know who the subscriber is and we
+ # don't need to present them with a single radio button.
+ self.widgets['subscription'].visible = False
else:
- if self._subscriber_count_for_current_user == 0:
- # We hide the subscription widget if the user isn't
- # subscribed, since we know who the subscriber is and we
- # don't need to present them with a single radio button.
- self.widgets['subscription'].visible = False
- else:
- # We show the subscription widget when the user is
- # subscribed via a team, because they can either
- # subscribe theirself or unsubscribe their team.
- self.widgets['subscription'].visible = True
+ # We show the subscription widget when the user is
+ # subscribed via a team, because they can either
+ # subscribe theirself or unsubscribe their team.
+ self.widgets['subscription'].visible = True
@cachedproperty
def user_is_subscribed(self):
@@ -266,12 +288,16 @@
def subscribe_action(self, action, data):
"""Handle subscription requests."""
subscription_person = self.widgets['subscription'].getInputValue()
- if (not self.user_is_subscribed and
+ if self._use_advanced_features:
+ bug_notification_level = data.get('bug_notification_level', None)
+ else:
+ bug_notification_level = None
+
+ if (subscription_person == self._update_subscription_term.value and
+ self.user_is_subscribed):
+ self._handleUpdateSubscription(level=bug_notification_level)
+ elif (not self.user_is_subscribed and
(subscription_person == self.user)):
- if self._use_advanced_features:
- bug_notification_level = data['bug_notification_level']
- else:
- bug_notification_level = None
self._handleSubscribe(bug_notification_level)
else:
self._handleUnsubscribe(subscription_person)
@@ -296,7 +322,6 @@
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
@@ -331,6 +356,13 @@
structured(
self._getUnsubscribeNotification(user, unsubed_dupes)))
+ def _handleUpdateSubscription(self, level):
+ """Handle updating a user's subscription."""
+ subscription = self.current_user_subscription
+ subscription.bug_notification_level = level
+ self.request.response.addNotification(
+ "Your subscription to this bug has been updated.")
+
def _getUnsubscribeNotification(self, user, unsubed_dupes):
"""Construct and return the unsubscribe-from-bug feedback message.
=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2010-11-02 12:00:57 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2010-11-03 14:03:50 +0000
@@ -5,13 +5,10 @@
__metaclass__ = type
-from storm.store import Store
-
from canonical.launchpad.ftests import LaunchpadFormHarness
from canonical.testing.layers import LaunchpadFunctionalLayer
from lp.bugs.browser.bugsubscription import BugSubscriptionSubscribeSelfView
-from lp.bugs.model.bugsubscription import BugSubscription
from lp.registry.enum import BugNotificationLevel
from lp.testing import (
feature_flags,
@@ -25,13 +22,10 @@
layer = LaunchpadFunctionalLayer
- def _getBugSubscriptionForUserAndBug(self, user, bug):
- """Return the BugSubscription for a given user, bug combination."""
- store = Store.of(bug)
- return store.find(
- BugSubscription,
- BugSubscription.person == user,
- BugSubscription.bug == bug).one()
+ def setUp(self):
+ super(BugSubscriptionAdvancedFeaturesTestCase, self).setUp()
+ with feature_flags():
+ set_feature_flag(u'malone.advanced-subscriptions.enabled', u'on')
def test_subscribe_uses_bug_notification_level(self):
# When a user subscribes to a bug using the advanced features on
@@ -46,7 +40,6 @@
# We don't display BugNotificationLevel.NOTHING as an option.
# This is tested below.
with feature_flags():
- set_feature_flag(u'malone.advanced-subscriptions.enabled', u'on')
displayed_levels = [
level for level in BugNotificationLevel.items
if level != BugNotificationLevel.NOTHING]
@@ -61,8 +54,7 @@
}
harness.submit('continue', form_data)
- subscription = self._getBugSubscriptionForUserAndBug(
- person, bug)
+ subscription = bug.getSubscriptionForPerson(person)
self.assertEqual(
level, subscription.bug_notification_level,
"Bug notification level of subscription should be %s, is "
@@ -75,7 +67,6 @@
bug = self.factory.makeBug()
person = self.factory.makePerson()
with feature_flags():
- set_feature_flag(u'malone.advanced-subscriptions.enabled', u'on')
with person_logged_in(person):
level = BugNotificationLevel.NOTHING
harness = LaunchpadFormHarness(
@@ -91,3 +82,86 @@
harness.getFieldError('bug_notification_level'),
"The view should treat BugNotificationLevel.NOTHING "
"as an invalid value.")
+
+ def test_user_can_update_subscription(self):
+ # A user can update their bug subscription using the
+ # BugSubscriptionSubscribeSelfView.
+ bug = self.factory.makeBug()
+ person = self.factory.makePerson()
+ with feature_flags():
+ with person_logged_in(person):
+ bug.subscribe(person, person, BugNotificationLevel.COMMENTS)
+ # Now the person updates their subscription so they're
+ # subscribed at the METADATA level.
+ level = BugNotificationLevel.METADATA
+ harness = LaunchpadFormHarness(
+ bug.default_bugtask, BugSubscriptionSubscribeSelfView)
+ form_data = {
+ 'field.subscription': 'update-subscription',
+ 'field.bug_notification_level': level.name,
+ }
+ harness.submit('continue', form_data)
+ self.assertFalse(harness.hasErrors())
+
+ subscription = bug.getSubscriptionForPerson(person)
+ self.assertEqual(
+ BugNotificationLevel.METADATA,
+ subscription.bug_notification_level,
+ "Bug notification level of subscription should be METADATA, is "
+ "actually %s." % subscription.bug_notification_level.name)
+
+ def test_user_can_unsubscribe(self):
+ # A user can unsubscribe from a bug using the
+ # BugSubscriptionSubscribeSelfView.
+ bug = self.factory.makeBug()
+ person = self.factory.makePerson()
+ with feature_flags():
+ with person_logged_in(person):
+ bug.subscribe(person, person)
+ harness = LaunchpadFormHarness(
+ bug.default_bugtask, BugSubscriptionSubscribeSelfView)
+ form_data = {
+ 'field.subscription': person.name,
+ }
+ harness.submit('continue', form_data)
+
+ subscription = bug.getSubscriptionForPerson(person)
+ self.assertIs(
+ None, subscription,
+ "There should be no BugSubscription for this person.")
+
+ def test_field_values_set_correctly_for_existing_subscriptions(self):
+ # When a user who is already subscribed to a bug visits the
+ # BugSubscriptionSubscribeSelfView, its bug_notification_level
+ # field will be set according to their current susbscription
+ # level.
+ bug = self.factory.makeBug()
+ person = self.factory.makePerson()
+ with feature_flags():
+ with person_logged_in(person):
+ # We subscribe using the harness rather than doing it
+ # directly so that we don't have to commit() between
+ # subscribing and checking the default value.
+ level = BugNotificationLevel.METADATA
+ harness = LaunchpadFormHarness(
+ bug.default_bugtask, BugSubscriptionSubscribeSelfView)
+ form_data = {
+ 'field.subscription': person.name,
+ 'field.bug_notification_level': level.name,
+ }
+ harness.submit('continue', form_data)
+
+ # The default value for the bug_notification_level field
+ # should now be the same as the level used to subscribe
+ # above.
+ harness = LaunchpadFormHarness(
+ bug.default_bugtask, BugSubscriptionSubscribeSelfView)
+ bug_notification_level_widget = (
+ harness.view.widgets['bug_notification_level'])
+ default_notification_level_value = (
+ bug_notification_level_widget._getDefault())
+ self.assertEqual(
+ BugNotificationLevel.METADATA,
+ default_notification_level_value,
+ "Default value for bug_notification_level should be "
+ "METADATA, is actually %s" % default_notification_level_value)
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2010-10-31 20:18:45 +0000
+++ lib/lp/bugs/configure.zcml 2010-11-03 14:03:50 +0000
@@ -694,6 +694,7 @@
getSubscribersFromDuplicates
getSubscriptionsFromDuplicates
getSubscribersForPerson
+ getSubscriptionForPerson
indexed_messages
getAlsoNotifiedSubscribers
getBugWatch
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2010-10-31 20:18:45 +0000
+++ lib/lp/bugs/interfaces/bug.py 2010-11-03 14:03:50 +0000
@@ -505,6 +505,12 @@
:return: An IResultSet.
"""
+ def getSubscriptionForPerson(person):
+ """Return the `BugSubscription` for a `Person` to this `Bug`.
+
+ If no such `BugSubscription` exists, return None.
+ """
+
def getBugNotificationRecipients(duplicateof=None, old_bug=None,
include_master_dupe_subscribers=False):
"""Return a complete INotificationRecipientSet instance.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-10-31 20:18:45 +0000
+++ lib/lp/bugs/model/bug.py 2010-11-03 14:03:50 +0000
@@ -954,6 +954,14 @@
).order_by(Person.name),
cache_subscriber, pre_iter_hook=cache_unsubscribed)
+ def getSubscriptionForPerson(self, person):
+ """See `IBug`."""
+ store = Store.of(self)
+ return store.find(
+ BugSubscription,
+ BugSubscription.person == person,
+ BugSubscription.bug == self).one()
+
def getAlsoNotifiedSubscribers(self, recipients=None, level=None):
"""See `IBug`.
Follow ups
