Merge lp:~gmb/launchpad/make-bnl-descriptions-readable-bug-664566 into lp:launchpad

[Graham Binns]

BugNotificationLevel now has a more readable set of descriptions for Bug:+subscribe. BugNotificationLevel.NOTHING is no longer accepted when one is attempting to subscribe to a bug through the web UI.

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.

[Abel Deuring (adeuring)]

nice work! just one nitpick:

(17:58:02) adeuring: gmb: I think @cachedproperty would be better for _bug_notification_level_field. I understand that it is used just once, but just in case it is used later somewhere else, we can/should use the same Choice instance
(17:58:32) gmb: adeuring: Okay, sure.

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2010-10-19 10:29:00 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2010-11-01 13:08:34 +0000
@@ -18,8 +18,6 @@
 from zope import formlib
 from zope.app.form import CustomWidgetFactory
 from zope.app.form.browser.itemswidgets import RadioWidget
-from zope.app.form.interfaces import IInputWidget
-from zope.app.form.utility import setUpWidget
 from zope.schema import Choice
 from zope.schema.vocabulary import (
     SimpleTerm,
@@ -30,7 +28,6 @@
 from canonical.launchpad.webapp import (
     action,
     canonical_url,
-    custom_widget,
     LaunchpadFormView,
     LaunchpadView,
     )
@@ -85,10 +82,25 @@
     """A view to handle the +subscribe page for a bug."""
 
     schema = IBugSubscription
-    # XXX 2010-10-18 gmb bug=651108:
-    #     field_names is currently empty because we don't use any of
-    #     them.  This will be amended in a subsequent branch.
-    field_names = []
+
+    # 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:
+            return ['bug_notification_level']
+        else:
+            return []
 
     @property
     def next_url(self):
@@ -102,7 +114,7 @@
         # far as referrers are concerned so that we can handle privacy
         # issues properly.
         ignored_referrer_urls = (
-            'localhost', self.request.getURL(),  context_url)
+            'localhost', self.request.getURL(), context_url)
         if referer and referer not in ignored_referrer_urls:
             next_url = referer
         elif self._redirecting_to_bug_list:
@@ -112,7 +124,6 @@
         return next_url
 
     cancel_url = next_url
-    _redirecting_to_bug_list = False
 
     @cachedproperty
     def _subscribers_for_current_user(self):
@@ -128,10 +139,45 @@
         self._subscriber_count_for_current_user = person_count
         return persons_for_user.values()
 
+    @cachedproperty
+    def _use_advanced_features(self):
+        """Return True if advanced subscriptions features are enabled."""
+        return features.getFeatureFlag(
+            'malone.advanced-subscriptions.enabled')
+
+    def initialize(self):
+        """See `LaunchpadFormView`."""
+        self._subscriber_count_for_current_user = 0
+        self._redirecting_to_bug_list = False
+        super(BugSubscriptionSubscribeSelfView, self).initialize()
+
+    @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
+        # 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()
-        bug = self.context.bug
         if self.user is None:
             return
 
@@ -162,10 +208,37 @@
             __name__='subscription', title=_("Subscription options"),
             vocabulary=subscription_vocabulary, required=True,
             default=default_subscription_value)
-        self.form_fields += formlib.form.Fields(subscription_field)
+
+        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._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['subscription'].custom_widget = CustomWidgetFactory(
             RadioWidget)
 
+    def setUpWidgets(self):
+        """See `LaunchpadFormView`."""
+        super(BugSubscriptionSubscribeSelfView, self).setUpWidgets()
+        if self._use_advanced_features:
+            if self.user_is_subscribed:
+                self.widgets['bug_notification_level'].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
+
     @cachedproperty
     def user_is_subscribed(self):
         """Is the user subscribed to this bug?"""
@@ -195,14 +268,18 @@
         subscription_person = self.widgets['subscription'].getInputValue()
         if (not self.user_is_subscribed and
             (subscription_person == self.user)):
-            self._handleSubscribe()
+            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)
         self.request.response.redirect(self.next_url)
 
-    def _handleSubscribe(self):
+    def _handleSubscribe(self, level=None):
         """Handle a subscribe request."""
-        self.context.bug.subscribe(self.user, self.user)
+        self.context.bug.subscribe(self.user, self.user, level=level)
         self.request.response.addNotification(
             "You have been subscribed to this bug.")
 

=== added file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2010-11-01 13:08:34 +0000
@@ -0,0 +1,94 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for BugSubscription views."""
+
+__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,
+    person_logged_in,
+    set_feature_flag,
+    TestCaseWithFactory,
+    )
+
+
+class BugSubscriptionAdvancedFeaturesTestCase(TestCaseWithFactory):
+
+    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 test_subscribe_uses_bug_notification_level(self):
+        # When a user subscribes to a bug using the advanced features on
+        # the Bug +subscribe page, the bug notification level they
+        # choose is taken into account.
+        bug = self.factory.makeBug()
+        # We unsubscribe the bug's owner because if we don't there will
+        # be two COMMENTS-level subscribers.
+        with person_logged_in(bug.owner):
+            bug.unsubscribe(bug.owner, bug.owner)
+
+        # 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]
+            for level in displayed_levels:
+                person = self.factory.makePerson()
+                with person_logged_in(person):
+                    harness = LaunchpadFormHarness(
+                        bug.default_bugtask, BugSubscriptionSubscribeSelfView)
+                    form_data = {
+                        'field.subscription': person.name,
+                        'field.bug_notification_level': level.name,
+                        }
+                    harness.submit('continue', form_data)
+
+        subscription = self._getBugSubscriptionForUserAndBug(
+            person, bug)
+        self.assertEqual(
+            level, subscription.bug_notification_level,
+            "Bug notification level of subscription should be %s, is "
+            "actually %s." % (
+                level.name, subscription.bug_notification_level.name))
+
+        # XXX 2010-11-01 gmb bug=668334:
+        #     This should be in its own test but at the moment the above
+        #     bug causes it to fail because of feature flag
+        #     inconsistencies. It should be moved to its own test when
+        #     the above bug is resolved.
+        # BugNotificationLevel.NOTHING isn't considered valid when
+        # someone is trying to subscribe.
+        with feature_flags():
+            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.")

=== added 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	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions-advanced-features.txt	2010-11-01 13:08:34 +0000
@@ -0,0 +1,43 @@
+Advanced personal subscriptions
+-------------------------------
+
+There are advanced features for bug subscriptions. These require a
+feature flag to be turned on for them to be accessible.
+
+    >>> from lp.services.features.model import FeatureFlag, getFeatureStore
+    >>> feature_store = getFeatureStore()
+    >>> ignore = feature_store.add(FeatureFlag(
+    ...     scope=u'default',
+    ...     flag=u'malone.advanced-subscriptions.enabled',
+    ...     value=u'on', priority=1))
+    >>> feature_store.flush()
+
+Now when a user visits the +subscribe page of a bug they are given the
+option to subscribe to the bug at a given BugNotificationLevel.
+
+    >>> from canonical.launchpad.webapp import canonical_url
+    >>> from lp.testing.sampledata import USER_EMAIL
+    >>> login(USER_EMAIL)
+    >>> bug = factory.makeBug()
+    >>> logout()
+    >>> user_browser.open(
+    ...     canonical_url(bug.default_bugtask, view_name='+subscribe'))
+    >>> bug_notification_level_control = user_browser.getControl(
+    ...     name='field.bug_notification_level')
+    >>> for control in bug_notification_level_control.controls:
+    ...     print control.optionValue
+    COMMENTS
+    METADATA
+    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:
+
+    >>> 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'):
+    ...     print extract_text(message)
+    You have been subscribed to this bug.

=== modified file 'lib/lp/services/features/rulesource.py'
--- lib/lp/services/features/rulesource.py	2010-09-29 07:13:11 +0000
+++ lib/lp/services/features/rulesource.py	2010-11-01 13:08:34 +0000
@@ -12,6 +12,7 @@
 __metaclass__ = type
 
 import re
+from collections import namedtuple
 
 from storm.locals import Desc
 
@@ -21,14 +22,18 @@
     )
 
 
+# A convenient mapping for a feature flag rule in the database.
+Rule = namedtuple("Rule", "flag scope priority value")
+
+
 class FeatureRuleSource(object):
     """Access feature rule sources from the database or elsewhere."""
 
     def getAllRulesAsDict(self):
         """Return all rule definitions.
 
-        :returns: dict from flag name to a list of 
-            (scope, priority, value) 
+        :returns: dict from flag name to a list of
+            (scope, priority, value)
             in descending order by priority.
         """
         d = {}
@@ -67,7 +72,7 @@
         """Return a list of tuples for the parsed form of the text input.
 
         For each non-blank line gives back a tuple of (flag, scope, priority, value).
-        
+
         Returns a list rather than a generator so that you see any syntax
         errors immediately.
         """
@@ -90,7 +95,7 @@
                 .find(FeatureFlag)
                 .order_by(FeatureFlag.flag, Desc(FeatureFlag.priority)))
         for r in rs:
-            yield str(r.flag), str(r.scope), r.priority, r.value
+            yield Rule(str(r.flag), str(r.scope), r.priority, r.value)
 
     def setAllRules(self, new_rules):
         """Replace all existing rules with a new set.

=== added file 'lib/lp/services/features/testing.py'
--- lib/lp/services/features/testing.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/testing.py	2010-11-01 13:08:34 +0000
@@ -0,0 +1,68 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Helpers for writing tests that use feature flags."""
+
+__metaclass__ = type
+__all__ = ['active_features']
+
+
+from fixtures import Fixture
+from lp.services.features import per_thread
+from lp.services.features.flags import FeatureController
+from lp.services.features.rulesource import Rule, StormFeatureRuleSource
+
+
+class FeatureFixture(Fixture):
+    """A fixture that sets a feature.
+
+    The fixture takes a dictonary as its constructor argument. The keys of the
+    dictionary are features to be set.
+
+    Call the fixture's `setUp()' method to install the features with the
+    desired values.  Calling `cleanUp()' will restore the original values.
+    You can also install this fixture by inheriting from
+    `fixtures.TestWithFixtures' and then calling the TestCase's
+    `self.useFixture()' method.
+
+    The fixture can also be used as a context manager. The value of the
+    feature within the context block is set to the dictonary's key's value.
+    The values are restored when the block exits.
+    """
+
+    def __init__(self, features_dict):
+        """Constructor.
+
+        :param features_dict: A dictionary-like object with keys and values
+            that are flag names and those flags' settings.
+        """
+        self.desired_features = features_dict
+
+    def setUp(self):
+        """Set the feature flags that this fixture is responsible for."""
+        super(FeatureFixture, self).setUp()
+
+        rule_source = StormFeatureRuleSource()
+        self.addCleanup(
+            rule_source.setAllRules, rule_source.getAllRulesAsTuples())
+
+        # Create a list of the new rules. Note that rules with a None
+        # value are quietly dropped, since you can't assign None as a
+        # feature flag value (it would come out as u'None') and setting
+        # a flag to None essentially means turning it off anyway.
+        new_rules = [
+            Rule(
+                flag=flag_name,
+                scope='default',
+                priority=999,
+                value=unicode(value))
+            for flag_name, value in self.desired_features.iteritems()
+                if value is not None]
+
+        rule_source.setAllRules(new_rules)
+
+
+        original_controller = getattr(per_thread, 'features', None)
+        controller = FeatureController(lambda _: True, rule_source)
+        per_thread.features = controller
+        self.addCleanup(setattr, per_thread, 'features', original_controller)

=== added file 'lib/lp/services/features/tests/test_helpers.py'
--- lib/lp/services/features/tests/test_helpers.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/tests/test_helpers.py	2010-11-01 13:08:34 +0000
@@ -0,0 +1,67 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the feature flags test helpers."""
+
+from __future__ import with_statement
+
+
+__metaclass__ = type
+__all__ = []
+
+from canonical.testing import layers
+from lp.testing import TestCase
+from lp.services.features import getFeatureFlag
+from lp.services.features.testing import FeatureFixture
+
+
+class TestFeaturesContextManager(TestCase):
+    """Tests for the feature flags context manager test helper."""
+
+    layer = layers.DatabaseFunctionalLayer
+
+    def test_setting_one_flag_with_manager(self):
+        flag = self.getUniqueString()
+        value_outside_manager = getFeatureFlag(flag)
+        value_in_manager = None
+
+        with FeatureFixture({flag: u'on'}):
+            value_in_manager = getFeatureFlag(flag)
+
+        self.assertEqual(value_in_manager, u'on')
+        self.assertEqual(value_outside_manager, getFeatureFlag(flag))
+        self.assertNotEqual(value_outside_manager, value_in_manager)
+
+
+class TestFeaturesFixture(TestCase):
+    """Tests for the feature flags test fixture."""
+
+    layer = layers.DatabaseFunctionalLayer
+
+    def test_fixture_sets_one_flag_and_cleans_up_again(self):
+        flag = self.getUniqueString()
+        value_before_fixture_setup = getFeatureFlag(flag)
+        value_after_fixture_setup = None
+
+        fixture = FeatureFixture({flag: 'on'})
+        fixture.setUp()
+        value_after_fixture_setup = getFeatureFlag(flag)
+        fixture.cleanUp()
+
+        self.assertEqual(value_after_fixture_setup, 'on')
+        self.assertEqual(value_before_fixture_setup, getFeatureFlag(flag))
+        self.assertNotEqual(
+            value_before_fixture_setup, value_after_fixture_setup)
+
+    def test_fixture_deletes_existing_values(self):
+        self.useFixture(FeatureFixture({'one': '1'}))
+        self.useFixture(FeatureFixture({'two': '2'}))
+
+        self.assertEqual(getFeatureFlag('one'), None)
+        self.assertEqual(getFeatureFlag('two'), u'2')
+
+    def test_fixture_overrides_previously_set_flags(self):
+        self.useFixture(FeatureFixture({'one': '1'}))
+        self.useFixture(FeatureFixture({'one': '5'}))
+
+        self.assertEqual(getFeatureFlag('one'), u'5')

=== modified file 'lib/lp/services/memcache/doc/tales-cache.txt'
--- lib/lp/services/memcache/doc/tales-cache.txt	2010-10-25 13:16:10 +0000
+++ lib/lp/services/memcache/doc/tales-cache.txt	2010-11-01 13:08:34 +0000
@@ -350,9 +350,9 @@
 
     >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
     >>> from lp.services.features.model import FeatureFlag, getFeatureStore
+    >>> from lp.services.features.webapp import ScopesFromRequest
+    >>> from lp.services.features.flags import FeatureController
     >>> from lp.services.features import per_thread
-    >>> from lp.services.features.flags import FeatureController
-    >>> from lp.services.features.webapp import ScopesFromRequest
     >>> ignore = getFeatureStore().add(FeatureFlag(
     ...     scope=u'default', flag=u'memcache', value=u'disabled',
     ...     priority=1))