Merge lp:~gmb/launchpad/add-bnl-to-structsubs-bug-672507 into lp:launchpad

[Graham Binns]

The bug_notification_level field is now available for structural subscriptions when the advanced subscriptions feature flag is switched on.

this branch makes the bug_notification_level field available in the
structural subscription ui when the
malone.advanced-subscriptions.enabled flag is turned on.

== .bzrignore ==

 - i've added the generated testrunner configs to .bzrignore, since i
   have strict commits enabled and they were annoying the hell out of
   me.

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

 - i've updated the bugsubscription_views tests after noticing that a
   test that should have been carried out once per iteration of a loop
   was happening outside the loop (as it happens it still passes, but
   now the testing is more thorough).

== lib/lp/registry/browser/structuralsubscription.py ==

 - i've made structuralsubscriptionview descend from
   advancedsubscriptionmixin so that the bug_notification_level field
   becomes available.
 - i've added a current_user_subscription property, in accordance with
   the requirements of advancedsubscriptionmixin.
 - i've updated the callsites for
   istructuralsubscriptiontarget.addbugsubscription() to also pass the
   bug_notification_level where available.

== lib/lp/registry/browser/tests/test_structuralsubscription.py ==

 - i've added tests to cover the changes to structuralsubscriptionview,
   covering the subscription of people and teams to a
   structuralsubscriptiontarget.

== lib/lp/registry/interfaces/structuralsubscription.py ==

 - i've updated the declaration of
   istructuralsubscriptiontarget.addbugsubscription() to include the
   bug_notifcation_level field.

== lib/lp/registry/model/structuralsubscription.py ==

 - i've updated the implementation of
   istructuralsubscriptiontarget.addbugsubscription() to make use of the
   bug_notification_level field.

There's no lint.

[Jelmer Vernooij (jelmer)]

It seems like it would be useful to have some infrastructure for running tests against all things that support structural subscription. I reviewed another branch a while back that also did it manually using a seperate TestCase subclass for each.

review: Approve (code)

=== modified file '.bzrignore'
--- .bzrignore	2010-11-08 05:22:26 +0000
+++ .bzrignore	2010-11-11 10:39:57 +0000
@@ -74,3 +74,5 @@
 .project
 .pydevproject
 librarian.log
+configs/testrunner_*
+configs/testrunner-appserver_*

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2010-11-09 19:17:24 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2010-11-11 10:39:57 +0000
@@ -57,12 +57,12 @@
                         }
                     harness.submit('continue', form_data)
 
-        subscription = bug.getSubscriptionForPerson(person)
-        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))
+                subscription = bug.getSubscriptionForPerson(person)
+                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))
 
     def test_nothing_is_not_a_valid_level(self):
         # BugNotificationLevel.NOTHING isn't considered valid when

=== modified file 'lib/lp/registry/browser/structuralsubscription.py'
--- lib/lp/registry/browser/structuralsubscription.py	2010-09-14 19:16:47 +0000
+++ lib/lp/registry/browser/structuralsubscription.py	2010-11-11 10:39:57 +0000
@@ -32,6 +32,7 @@
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.menu import Link
 from canonical.widgets import LabeledMultiCheckBoxWidget
+from lp.bugs.browser.bugsubscription import AdvancedSubscriptionMixin
 from lp.registry.enum import BugNotificationLevel
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
@@ -45,7 +46,8 @@
 from lp.services.propertycache import cachedproperty
 
 
-class StructuralSubscriptionView(LaunchpadFormView):
+class StructuralSubscriptionView(LaunchpadFormView,
+                                 AdvancedSubscriptionMixin):
     """View class for structural subscriptions."""
 
     schema = IStructuralSubscriptionForm
@@ -55,6 +57,21 @@
 
     override_title_breadcrumbs = True
 
+    @cachedproperty
+    def _bug_notification_level_descriptions(self):
+        return {
+            BugNotificationLevel.LIFECYCLE: (
+                "A bug in %s is fixed or re-opened." %
+                self.context.displayname),
+            BugNotificationLevel.METADATA: (
+                "Any change is made to a bug in %s, other than a new "
+                "comment being added." %
+                self.context.displayname),
+            BugNotificationLevel.COMMENTS: (
+                "A change is made or a new comment is added to a bug in %s."
+                % self.context.displayname),
+            }
+
     @property
     def page_title(self):
         return 'Subscribe to Bugs in %s' % self.context.title
@@ -75,6 +92,7 @@
             remove_other = self._createRemoveOtherSubscriptionsField()
             if remove_other:
                 self.form_fields += form.Fields(remove_other)
+        self._setUpBugNotificationLevelField()
 
     def _createTeamSubscriptionsField(self):
         """Create field with a list of the teams the user is a member of.
@@ -172,6 +190,11 @@
         """Return True, if the current user is subscribed."""
         return self.isSubscribed(self.user)
 
+    @cachedproperty
+    def current_user_subscription(self):
+        """Return the subscription of the current user."""
+        return self.context.getSubscription(self.user)
+
     def userCanAlter(self):
         if self.context.userCanAlterBugSubscription(self.user, self.user):
             return True
@@ -194,7 +217,9 @@
         is_subscribed = self.isSubscribed(self.user)
         subscribe = data['subscribe_me']
         if (not is_subscribed) and subscribe:
-            target.addBugSubscription(self.user, self.user)
+            target.addBugSubscription(
+                self.user, self.user,
+                data.get('bug_notification_level', None))
             self.request.response.addNotification(
                 'You have subscribed to "%s". You will now receive an '
                 'e-mail each time someone reports or changes one of '
@@ -223,7 +248,8 @@
             team for team in teams if self.isSubscribed(team))
 
         for team in form_selected_teams - subscriptions:
-            target.addBugSubscription(team, self.user)
+            target.addBugSubscription(
+                team, self.user, data.get('bug_notification_level', None))
             self.request.response.addNotification(
                 'The %s team will now receive an e-mail each time '
                 'someone reports or changes a public bug in "%s".' % (

=== modified file 'lib/lp/registry/browser/tests/test_structuralsubscription.py'
--- lib/lp/registry/browser/tests/test_structuralsubscription.py	2010-10-04 19:50:45 +0000
+++ lib/lp/registry/browser/tests/test_structuralsubscription.py	2010-11-11 10:39:57 +0000
@@ -9,12 +9,16 @@
 from zope.publisher.interfaces import NotFound
 
 from canonical.launchpad.ftests import (
+    LaunchpadFormHarness,
     login,
     logout,
     )
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.launchpad.webapp.servers import StepsToGo
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.registry.browser.distribution import DistributionNavigation
 from lp.registry.browser.distributionsourcepackage import (
     DistributionSourcePackageNavigation,
@@ -24,10 +28,19 @@
 from lp.registry.browser.product import ProductNavigation
 from lp.registry.browser.productseries import ProductSeriesNavigation
 from lp.registry.browser.project import ProjectNavigation
-from lp.testing import TestCaseWithFactory
+from lp.registry.browser.structuralsubscription import (
+    StructuralSubscriptionView)
+from lp.registry.enum import BugNotificationLevel
+from lp.testing import (
+    feature_flags,
+    person_logged_in,
+    set_feature_flag,
+    TestCaseWithFactory,
+    )
 
 
 class FakeLaunchpadRequest(FakeRequest):
+
     @property
     def stepstogo(self):
         """See `IBasicLaunchpadRequest`."""
@@ -94,6 +107,7 @@
 class TestProductSeriesStructuralSubscriptionTraversal(
     StructuralSubscriptionTraversalTestBase):
     """Test IStructuralSubscription traversal from IProductSeries."""
+
     def setUpTarget(self):
         self.target = self.factory.makeProduct(name='fooix').newSeries(
             self.eric, '0.1', '0.1')
@@ -103,6 +117,7 @@
 class TestMilestoneStructuralSubscriptionTraversal(
     StructuralSubscriptionTraversalTestBase):
     """Test IStructuralSubscription traversal from IMilestone."""
+
     def setUpTarget(self):
         self.target = self.factory.makeProduct(name='fooix').newSeries(
             self.eric, '0.1', '0.1').newMilestone('0.1.0')
@@ -112,6 +127,7 @@
 class TestProjectStructuralSubscriptionTraversal(
     StructuralSubscriptionTraversalTestBase):
     """Test IStructuralSubscription traversal from IProjectGroup."""
+
     def setUpTarget(self):
         self.target = self.factory.makeProject(name='fooix-project')
         self.navigation = ProjectNavigation
@@ -120,6 +136,7 @@
 class TestDistributionStructuralSubscriptionTraversal(
     StructuralSubscriptionTraversalTestBase):
     """Test IStructuralSubscription traversal from IDistribution."""
+
     def setUpTarget(self):
         self.target = self.factory.makeDistribution(name='debuntu')
         self.navigation = DistributionNavigation
@@ -128,6 +145,7 @@
 class TestDistroSeriesStructuralSubscriptionTraversal(
     StructuralSubscriptionTraversalTestBase):
     """Test IStructuralSubscription traversal from IDistroSeries."""
+
     def setUpTarget(self):
         self.target = self.factory.makeDistribution(name='debuntu').newSeries(
             '5.0', '5.0', '5.0', '5.0', '5.0', '5.0', None, self.eric)
@@ -138,6 +156,7 @@
     StructuralSubscriptionTraversalTestBase):
     """Test IStructuralSubscription traversal from IDistributionSourcePackage.
     """
+
     def setUpTarget(self):
         debuntu = self.factory.makeDistribution(name='debuntu')
         fooix = self.factory.makeSourcePackageName('fooix')
@@ -145,5 +164,123 @@
         self.navigation = DistributionSourcePackageNavigation
 
 
+class TestStructuralSubscriptionAdvancedFeaturesBase(TestCaseWithFactory):
+    """A base class for testing advanced structural subscription features."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestStructuralSubscriptionAdvancedFeaturesBase, self).setUp()
+        self.setUpTarget()
+        with feature_flags():
+            set_feature_flag(u'malone.advanced-subscriptions.enabled', u'on')
+
+    def setUpTarget(self):
+        self.target = self.factory.makeProduct()
+
+    def test_subscribe_uses_bug_notification_level(self):
+        # When advanced features are turned on for subscriptions a user
+        # can specify a bug_notification_level on the +subscribe form.
+        with feature_flags():
+            # We don't display BugNotificationLevel.NOTHING as an option.
+            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(
+                        self.target, StructuralSubscriptionView)
+                    form_data = {
+                        'field.subscribe_me': 'on',
+                        'field.bug_notification_level': level.name,
+                        }
+                    harness.submit('save', form_data)
+                    self.assertFalse(harness.hasErrors())
+
+                subscription = self.target.getSubscription(person)
+                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))
+
+    def test_subscribe_uses_bug_notification_level_for_teams(self):
+        # The bug_notification_level field is also used when subscribing
+        # a team.
+        with feature_flags():
+            displayed_levels = [
+                level for level in BugNotificationLevel.items
+                if level != BugNotificationLevel.NOTHING]
+            for level in displayed_levels:
+                person = self.factory.makePerson()
+                team = self.factory.makeTeam(owner=person)
+                with person_logged_in(person):
+                    harness = LaunchpadFormHarness(
+                        self.target, StructuralSubscriptionView)
+                    form_data = {
+                        'field.subscribe_me': '',
+                        'field.subscriptions_team': team.name,
+                        'field.bug_notification_level': level.name,
+                        }
+                    harness.submit('save', form_data)
+                    self.assertFalse(harness.hasErrors())
+
+                subscription = self.target.getSubscription(team)
+                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))
+
+    def test_nothing_is_not_a_valid_level(self):
+        # BugNotificationLevel.NOTHING isn't considered valid when a
+        # user is subscribing via the web UI.
+        person = self.factory.makePerson()
+        with feature_flags():
+            with person_logged_in(person):
+                harness = LaunchpadFormHarness(
+                    self.target, StructuralSubscriptionView)
+                form_data = {
+                    'field.subscribe_me': 'on',
+                    'field.bug_notification_level': (
+                        BugNotificationLevel.NOTHING),
+                    }
+                harness.submit('save', form_data)
+                self.assertTrue(harness.hasErrors())
+
+
+class TestProductSeriesAdvancedSubscriptionFeatures(
+    TestStructuralSubscriptionAdvancedFeaturesBase):
+    """Test advanced subscription features for ProductSeries."""
+
+    def setUpTarget(self):
+        self.target = self.factory.makeProductSeries()
+
+
+class TestDistributionAdvancedSubscriptionFeatures(
+    TestStructuralSubscriptionAdvancedFeaturesBase):
+    """Test advanced subscription features for distributions."""
+
+    def setUpTarget(self):
+        self.target = self.factory.makeDistribution()
+
+
+class TestDistroSeriesAdvancedSubscriptionFeatures(
+    TestStructuralSubscriptionAdvancedFeaturesBase):
+    """Test advanced subscription features for DistroSeries."""
+
+    def setUpTarget(self):
+        self.target = self.factory.makeDistroSeries()
+
+
+class TestMilestoneAdvancedSubscriptionFeatures(
+    TestStructuralSubscriptionAdvancedFeaturesBase):
+    """Test advanced subscription features for Milestones."""
+
+    def setUpTarget(self):
+        self.target = self.factory.makeMilestone()
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/interfaces/structuralsubscription.py'
--- lib/lp/registry/interfaces/structuralsubscription.py	2010-10-25 13:24:39 +0000
+++ lib/lp/registry/interfaces/structuralsubscription.py	2010-11-11 10:39:57 +0000
@@ -217,7 +217,8 @@
             required=False))
     @call_with(subscribed_by=REQUEST_USER)
     @export_factory_operation(IStructuralSubscription, [])
-    def addBugSubscription(subscriber, subscribed_by):
+    def addBugSubscription(subscriber, subscribed_by,
+                           bug_notification_level=None):
         """Add a bug subscription for this structure.
 
         This method is used to create a new `IStructuralSubscription`
@@ -227,6 +228,9 @@
         :subscriber: The IPerson who will be subscribed. If omitted,
             subscribed_by will be used.
         :subscribed_by: The IPerson creating the subscription.
+        :bug_notification_level: The BugNotificationLevel for the
+            subscription. If omitted, BugNotificationLevel.COMMENTS will be
+            used.
         :return: The new bug subscription.
         """
 

=== modified file 'lib/lp/registry/model/structuralsubscription.py'
--- lib/lp/registry/model/structuralsubscription.py	2010-10-25 13:24:39 +0000
+++ lib/lp/registry/model/structuralsubscription.py	2010-11-11 10:39:57 +0000
@@ -368,7 +368,8 @@
                 return False
         return True
 
-    def addBugSubscription(self, subscriber, subscribed_by):
+    def addBugSubscription(self, subscriber, subscribed_by,
+                           bug_notification_level=None):
         """See `IStructuralSubscriptionTarget`."""
         # This is a helper method for creating a structural
         # subscription and immediately giving it a full
@@ -381,7 +382,9 @@
                     subscribed_by.name, subscriber.name))
 
         sub = self.addSubscription(subscriber, subscribed_by)
-        sub.bug_notification_level = BugNotificationLevel.COMMENTS
+        if bug_notification_level is None:
+            bug_notification_level = BugNotificationLevel.COMMENTS
+        sub.bug_notification_level = bug_notification_level
         return sub
 
     def removeBugSubscription(self, subscriber, unsubscribed_by):