Merge lp:~bac/launchpad/nix-subscriber-list into lp:launchpad

[Brad Crittenden]

[r=gary][bug=768483] Do not show 'Also notified' in the bug subscriber portlet when advanced subscriptions are used.

= Summary =

With advanced bug subscriptions it is unclear whether a person will be
notified for any given change to a bug. Because of this new granularity
of notification, the list of subscribed people shown under 'Also
notified' is misleading. We feel the list is now of diminished
usefulness and are removing it.

== Proposed fix ==

Use a feature flag currently tied to the malone-alpha team to suppress
the generation of that portion of the portlet.

== Pre-implementation notes ==

Chat with Gary and Benji.

== Implementation details ==

Some changes were made to other parts of the test for clarity regarding
the use of feature flags.

== Tests ==

bin/test -vvm lp.bugs -t test_bugscription_views

== Demo and Q/A ==

= Launchpad lint =

I'll fix the lint.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/templates/bug-portlet-subscribers-content.pt
  lib/lp/bugs/browser/bug.py
  lib/lp/bugs/browser/tests/test_bugsubscription_views.py

./lib/lp/bugs/browser/tests/test_bugsubscription_views.py
     190: local variable 'level' is assigned to but never used
     210: local variable 'level' is assigned to but never used
     235: local variable 'level' is assigned to but never used
     269: local variable 'subscription' is assigned to but never used
     340: local variable 'subscribe_view' is assigned to but never used
     361: local variable 'subscribe_view' is assigned to but never used
     450: local variable 'sub' is assigned to but never used
     500: local variable 'mute_view' is assigned to but never used

[Gary Poster (gary)]

Great, thank you Brad!

Per our discussion on IRC:

 - We agreed that the comment on line 206 is wrong ("# Subscribe someone to the target.") Please move or delete it.

 - Calculating also notified subscribers can be expensive; if we don't render it, let's not calculate it.

Gary

review: Approve

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2011-04-05 17:55:44 +0000
+++ lib/lp/bugs/browser/bug.py	2011-04-21 18:31:32 +0000
@@ -275,7 +275,7 @@
 
     def editsubscriptions(self):
         """Return the 'Edit subscriptions' Link."""
-        text = 'Edit subscriptions'
+        text = 'Edit all of your subscriptions'
         return Link(
             '+subscriptions', text, icon='edit', summary=(
                 'View and change your subscriptions to this bug'))

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-04-15 11:02:24 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-04-21 18:31:32 +0000
@@ -15,26 +15,28 @@
     BugSubscriptionSubscribeSelfView,
     )
 from lp.bugs.enum import BugNotificationLevel
+from lp.services.features.testing import FeatureFixture
 from lp.testing import (
-    feature_flags,
     person_logged_in,
-    set_feature_flag,
     TestCaseWithFactory,
     )
 from lp.testing.views import create_initialized_view
 
 
+ON = 'on'
+OFF = None
+
+
 class BugSubscriptionAdvancedFeaturesTestCase(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
+    feature_flag = 'malone.advanced-subscriptions.enabled'
 
     def setUp(self):
         super(BugSubscriptionAdvancedFeaturesTestCase, self).setUp()
         self.bug = self.factory.makeBug()
         self.person = self.factory.makePerson()
         self.team = self.factory.makeTeam()
-        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
@@ -48,7 +50,7 @@
 
         # We don't display BugNotificationLevel.NOTHING as an option.
         # This is tested below.
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             displayed_levels = [
                 level for level in BugNotificationLevel.items
                 if level != BugNotificationLevel.NOTHING]
@@ -76,7 +78,7 @@
         # someone is trying to subscribe.
         bug = self.factory.makeBug()
         person = self.factory.makePerson()
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(person):
                 level = BugNotificationLevel.NOTHING
                 harness = LaunchpadFormHarness(
@@ -98,7 +100,7 @@
         # BugSubscriptionSubscribeSelfView.
         bug = self.factory.makeBug()
         person = self.factory.makePerson()
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(person):
                 bug.subscribe(person, person, BugNotificationLevel.COMMENTS)
                 # Now the person updates their subscription so they're
@@ -125,7 +127,7 @@
         # BugSubscriptionSubscribeSelfView.
         bug = self.factory.makeBug()
         person = self.factory.makePerson()
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(person):
                 bug.subscribe(person, person)
                 harness = LaunchpadFormHarness(
@@ -147,7 +149,7 @@
         # level.
         bug = self.factory.makeBug()
         person = self.factory.makePerson()
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(person):
                 # We subscribe using the harness rather than doing it
                 # directly so that we don't have to commit() between
@@ -183,9 +185,8 @@
         # subscription that doesn't exist).
         bug = self.factory.makeBug()
         person = self.factory.makePerson()
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(person):
-                level = BugNotificationLevel.METADATA
                 harness = LaunchpadFormHarness(
                     bug.default_bugtask, BugSubscriptionSubscribeSelfView)
                 subscription_field = (
@@ -202,10 +203,9 @@
         bug = self.factory.makeBug()
         person = self.factory.makePerson()
         team = self.factory.makeTeam(owner=person)
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(person):
                 bug.subscribe(team, person)
-                level = BugNotificationLevel.METADATA
                 harness = LaunchpadFormHarness(
                     bug.default_bugtask, BugSubscriptionSubscribeSelfView)
                 subscription_field = (
@@ -224,13 +224,12 @@
         duplicate = self.factory.makeBug()
         person = self.factory.makePerson()
         team = self.factory.makeTeam(owner=person)
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(person):
                 duplicate.markAsDuplicate(bug)
                 duplicate.subscribe(person, person)
                 bug.subscribe(team, person)
 
-                level = BugNotificationLevel.METADATA
                 harness = LaunchpadFormHarness(
                     bug.default_bugtask, BugSubscriptionSubscribeSelfView)
                 subscription_field = (
@@ -246,7 +245,7 @@
         bug = self.factory.makeBug()
         duplicate = self.factory.makeBug()
         person = self.factory.makePerson()
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(person):
                 duplicate.markAsDuplicate(bug)
                 duplicate.subscribe(person, person)
@@ -264,10 +263,10 @@
         bug = self.factory.makeBug()
         person = self.factory.makePerson()
         with person_logged_in(person):
-            subscription = bug.subscribe(
+            bug.subscribe(
                 person, person, level=BugNotificationLevel.NOTHING)
 
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(person):
                 subscribe_view = create_initialized_view(
                     bug.default_bugtask, name='+subscribe')
@@ -287,7 +286,7 @@
         with person_logged_in(self.person):
             self.bug.mute(self.person, self.person)
 
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(self.person):
                 subscribe_view = create_initialized_view(
                     self.bug.default_bugtask, name='+subscribe')
@@ -307,7 +306,7 @@
         with person_logged_in(self.person):
             self.bug.mute(self.person, self.person)
 
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(self.person):
                 subscribe_view = create_initialized_view(
                     self.bug.default_bugtask, name='+subscribe')
@@ -325,7 +324,7 @@
         with person_logged_in(self.person):
             self.bug.mute(self.person, self.person)
 
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(self.person):
                 level = BugNotificationLevel.METADATA
                 form_data = {
@@ -335,7 +334,7 @@
                     'field.bug_notification_level': level.title,
                     'field.actions.continue': 'Continue',
                     }
-                subscribe_view = create_initialized_view(
+                create_initialized_view(
                     self.bug.default_bugtask, form=form_data,
                     name='+subscribe')
                 self.assertFalse(self.bug.isMuted(self.person))
@@ -348,7 +347,7 @@
         with person_logged_in(self.person):
             muted_subscription = self.bug.mute(self.person, self.person)
 
-        with feature_flags():
+        with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(self.person):
                 level = BugNotificationLevel.COMMENTS
                 form_data = {
@@ -356,7 +355,7 @@
                     'field.bug_notification_level': level.title,
                     'field.actions.continue': 'Continue',
                     }
-                subscribe_view = create_initialized_view(
+                create_initialized_view(
                     self.bug.default_bugtask, form=form_data,
                     name='+subscribe')
                 self.assertFalse(self.bug.isMuted(self.person))
@@ -368,7 +367,7 @@
         # The bug_notification_level widget has a widget_class property
         # that can be used to manipulate it with JavaScript.
         with person_logged_in(self.person):
-            with feature_flags():
+            with FeatureFixture({self.feature_flag: ON}):
                 subscribe_view = create_initialized_view(
                     self.bug.default_bugtask, name='+subscribe')
             widget_class = (
@@ -377,6 +376,40 @@
                 'bug-notification-level-field', widget_class)
 
 
+class BugSubscriptionAdvancedFeaturesPortletTestCase(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+    feature_flag = 'malone.advanced-subscriptions.enabled'
+
+    def setUp(self):
+        super(BugSubscriptionAdvancedFeaturesPortletTestCase, self).setUp()
+        self.bug = self.factory.makeBug()
+        self.person = self.factory.makePerson()
+        self.target = self.bug.default_bugtask.target
+        subscriber = self.factory.makePerson()
+        with person_logged_in(self.person):
+            self.target.addBugSubscription(subscriber, subscriber)
+
+    def get_contents(self, flag):
+        with person_logged_in(self.person):
+            with FeatureFixture({self.feature_flag: flag}):
+                bug_view = create_initialized_view(
+                    self.bug, name="+bug-portlet-subscribers-content")
+                return bug_view.render()
+
+    def test_also_notified_suppressed(self):
+        # If the advanced-subscription.enabled feature flag is on then the
+        # "Also notified" portion of the portlet is suppressed.
+        contents = self.get_contents(ON)
+        self.assertFalse('Also notified' in contents)
+
+    def test_also_notified_not_suppressed(self):
+        # If the advanced-subscription.enabled feature flag is off then the
+        # "Also notified" portion of the portlet is shown.
+        contents = self.get_contents(OFF)
+        self.assertTrue('Also notified' in contents)
+
+
 class BugPortletSubcribersIdsTests(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
@@ -410,7 +443,7 @@
     def test_form_initializes(self):
         # It's a start.
         with person_logged_in(self.subscriber):
-            sub = self.product.addBugSubscription(
+            self.product.addBugSubscription(
                 self.subscriber, self.subscriber)
             harness = LaunchpadFormHarness(
                 self.bug.default_bugtask, BugSubscriptionListView)
@@ -460,7 +493,7 @@
         # its form is submitted.
         with person_logged_in(self.person):
             self.assertFalse(self.bug.isMuted(self.person))
-            mute_view = create_initialized_view(
+            create_initialized_view(
                 self.bug.default_bugtask, name="+mute",
                 form={'field.actions.mute': 'Mute bug mail'})
             self.assertTrue(self.bug.isMuted(self.person))

=== modified file 'lib/lp/bugs/templates/bug-portlet-subscribers-content.pt'
--- lib/lp/bugs/templates/bug-portlet-subscribers-content.pt	2011-02-03 05:14:54 +0000
+++ lib/lp/bugs/templates/bug-portlet-subscribers-content.pt	2011-04-21 18:31:32 +0000
@@ -6,7 +6,6 @@
   tal:define="
     bug context/bug|context;
     direct_subscriptions view/sorted_direct_subscriptions;
-    also_notified_subscribers bug/getAlsoNotifiedSubscribers
   ">
   <div class="section" id="subscribers-direct">
     <h2>Subscribers</h2>
@@ -57,15 +56,18 @@
       <img src="/@@/spinner" />
     </div>
   </div>
-  <div
-    tal:condition="also_notified_subscribers"
-    id="subscribers-indirect"
-    class="Section"
-  >
-    <h2>Also notified</h2>
+  <tal:also_notified condition="not: request/features/malone.advanced-subscriptions.enabled">
     <div
-      tal:repeat="subscriber also_notified_subscribers"
-      tal:content="structure subscriber/fmt:link"
-    />
-  </div>
+      tal:define="also_notified_subscribers bug/getAlsoNotifiedSubscribers"
+      tal:condition="also_notified_subscribers"
+      id="subscribers-indirect"
+      class="Section"
+    >
+      <h2>Also notified</h2>
+      <div
+        tal:repeat="subscriber also_notified_subscribers"
+        tal:content="structure subscriber/fmt:link"
+      />
+    </div>
+  </tal:also_notified>
 </div>