launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/make-bnl-descriptions-readable-bug-664566 into lp:launchpad/devel with lp:~gmb/launchpad/include-bnl-bug-651108 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
  #664566 Bug:+subscribe bug_notification_level options should be more readable
  https://bugs.launchpad.net/bugs/664566
  #664569 BugNotificationLevel.NOTHING shouldn't show up as an option on Bug:+subscribe
  https://bugs.launchpad.net/bugs/664569


This branch fixes bug 664566 and bug 664569 by altering the way that BugNotificationLevel is displayed as an option on the bug +subscribe page.

== lib/lp/bugs/browser/bugsubscription.py ==

 - I've added a mapping of BugNotificationLevel values to human-readable descriptions. This is now used to build a custom field for bug_notification_level. Note that I didn't just change the descriptions in BugNotificationLevel directly because different contexts will mean that the descriptions will need to be worded differently (e.g. subscriptions to searches vs. direct sbscriptions vs. structural subscriptions).
 - I've dropped BugNotificationLevel.NOTHING from the set of options displayed.

== lib/lp/bugs/browser/tests/test_bugsubscription_views.py ==

 - I've added a test to ensure that trying to subscribe with BugNotificationLevel.NOTHING raises an error.

== lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions-advanced-features.txt ==

 - I've updated the existing pagetest to reflect the changes in this branch.
-- 
https://code.launchpad.net/~gmb/launchpad/make-bnl-descriptions-readable-bug-664566/+merge/39158
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/make-bnl-descriptions-readable-bug-664566 into lp:launchpad/devel.

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2010-10-22 15:43:02 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2010-10-22 15:43:04 +0000
@@ -36,6 +36,7 @@
 from canonical.launchpad.webapp.menu import structured
 from lp.bugs.browser.bug import BugViewMixin
 from lp.bugs.interfaces.bugsubscription import IBugSubscription
+from lp.registry.enum import BugNotificationLevel
 from lp.services import features
 from lp.services.propertycache import cachedproperty
 
@@ -82,6 +83,18 @@
 
     schema = IBugSubscription
 
+    # A mapping of BugNotificationLevel values to descriptions to be
+    # shown on the +subscribe page.
+    _bug_notification_level_descriptions = {
+        BugNotificationLevel.LIFECYCLE: (
+            "The bug is fixed or re-opened."),
+        BugNotificationLevel.METADATA: (
+            "Any change is made to this bug, other than a new comment "
+            "being added."),
+        BugNotificationLevel.COMMENTS: (
+            "A change is made to this bug or a new comment is added."),
+        }
+
     @property
     def field_names(self):
         if self._use_advanced_features:
@@ -138,6 +151,30 @@
         self._redirecting_to_bug_list = False
         super(BugSubscriptionSubscribeSelfView, self).initialize()
 
+    @property
+    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
+        # labels shown are human readable and specific to the +subscribe
+        # form. The BugNotificationLevel descriptions are too generic.
+        bug_notification_level_terms = [
+            SimpleTerm(
+                level, level.name,
+                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
+            # confusing.
+            for level in sorted(BugNotificationLevel.items, reverse=True)
+                if level != BugNotificationLevel.NOTHING
+            ]
+        bug_notification_vocabulary = SimpleVocabulary(
+            bug_notification_level_terms)
+        bug_notification_level_field = Choice(
+            __name__='bug_notification_level', title=_("Tell me when"),
+            vocabulary=bug_notification_vocabulary, required=True,
+            default=BugNotificationLevel.COMMENTS)
+        return bug_notification_level_field
+
     def setUpFields(self):
         """See `LaunchpadFormView`."""
         super(BugSubscriptionSubscribeSelfView, self).setUpFields()
@@ -173,14 +210,10 @@
             default=default_subscription_value)
 
         if self._use_advanced_features:
-            # We need to pop the bug_notification_level field out of
-            # form_fields so that we can put the subscription_field first in
-            # the list. formlib.form.Fields doesn't have an insert() method.
-            bug_notification_level_field = self.form_fields.select(
-                'bug_notification_level')
             self.form_fields = self.form_fields.omit('bug_notification_level')
             self.form_fields += formlib.form.Fields(subscription_field)
-            self.form_fields += bug_notification_level_field
+            self.form_fields += formlib.form.Fields(
+                self._bug_notification_level_field)
             self.form_fields['bug_notification_level'].custom_widget = (
                 CustomWidgetFactory(RadioWidget))
         else:

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2010-10-22 15:43:02 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2010-10-22 15:43:04 +0000
@@ -45,7 +45,12 @@
         with person_logged_in(bug.owner):
             bug.unsubscribe(bug.owner, bug.owner)
 
-        for level in BugNotificationLevel.items:
+        # We don't display BugNotificationLevel.NOTHING as an option.
+        # This is tested below.
+        displayed_levels = [
+            level for level in BugNotificationLevel.items
+            if level != BugNotificationLevel.NOTHING]
+        for level in displayed_levels:
             person = self.factory.makePerson()
             with person_logged_in(person):
                 harness = LaunchpadFormHarness(
@@ -63,3 +68,24 @@
                 "Bug notification level of subscription should be %s, is "
                 "actually %s." % (
                     level.name, subscription.bug_notification_level.name))
+
+    def test_bug_notification_level_nothing_is_invalid(self):
+        # BugNotificationLevel.NOTHING isn't considered valid when
+        # someone is trying to subscribe.
+        bug = self.factory.makeBug()
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            level = BugNotificationLevel.NOTHING
+            harness = LaunchpadFormHarness(
+                bug.default_bugtask, BugSubscriptionSubscribeSelfView)
+            form_data = {
+                'field.subscription': person.name,
+                'field.bug_notification_level': level.name,
+                }
+            harness.submit('continue', form_data)
+            self.assertTrue(harness.hasErrors())
+            self.assertEqual(
+                'Invalid value',
+                harness.getFieldError('bug_notification_level'),
+                "The view should treat BugNotificationLevel.NOTHING as an "
+                "invalid value.")

=== 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:43:02 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions-advanced-features.txt	2010-10-22 15:43:04 +0000
@@ -26,15 +26,16 @@
     ...     name='field.bug_notification_level')
     >>> for control in bug_notification_level_control.controls:
     ...     print control.optionValue
-    NOTHING
+    COMMENTS
+    METADATA
     LIFECYCLE
-    METADATA
-    COMMENTS
 
 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:
 
-    >>> bug_notification_level_control.getControl('Details').click()
+    >>> bug_notification_level_control.getControl(
+    ...     'Any change is made to this bug, other than a new comment '
+    ...     'being added.').click()
     >>> user_browser.getControl('Continue').click()
 
     >>> for message in find_tags_by_class(user_browser.contents, 'message'):