launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Here's a conflict-free diff.

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2010-11-09 19:41:24 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2010-11-11 14:46:00 +0000
@@ -225,10 +225,11 @@
         self_subscribed = False
         for person in self._subscribers_for_current_user:
             if person.id == self.user.id:
-                if self._use_advanced_features:
+                if (self._use_advanced_features and
+                    self.current_user_subscription is not None):
                     subscription_terms.append(self._update_subscription_term)
-                subscription_terms.append(
-                    SimpleTerm(
+                subscription_terms.insert(
+                    0, SimpleTerm(
                         person, person.name,
                         'Unsubscribe me from this bug'))
                 self_subscribed = True
@@ -244,7 +245,8 @@
                 SimpleTerm(
                     self.user, self.user.name, 'Subscribe me to this bug'))
         subscription_vocabulary = SimpleVocabulary(subscription_terms)
-        if self.user_is_subscribed and self._use_advanced_features:
+        if (self._use_advanced_features and
+            self.current_user_subscription is not None):
             default_subscription_value = self._update_subscription_term.value
         else:
             default_subscription_value = (
@@ -284,6 +286,13 @@
                 # subscribe theirself or unsubscribe their team.
                 self.widgets['subscription'].visible = True
 
+            if (self.user_is_subscribed and
+                self.current_user_subscription is None):
+                # If the user is subscribed via a duplicate but is not
+                # directly subscribed, we hide the
+                # bug_notification_level field, since it's not used.
+                self.widgets['bug_notification_level'].visible = False
+
     @cachedproperty
     def user_is_subscribed(self):
         """Is the user subscribed to this bug?"""

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2010-11-11 10:37:48 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2010-11-11 14:46:42 +0000
@@ -10,6 +10,7 @@
 
 from lp.bugs.browser.bugsubscription import (
     BugPortletSubcribersIds,
+    BugSubscriptionAddView,
     BugSubscriptionSubscribeSelfView,
     )
 from lp.registry.enum import BugNotificationLevel
@@ -170,6 +171,99 @@
                     "METADATA, is actually %s"
                     % default_notification_level_value)
 
+    def test_update_subscription_fails_if_user_not_subscribed(self):
+        # If the user is not directly subscribed to the bug, trying to
+        # update the subscription will fail (since you can't update a
+        # subscription that doesn't exist).
+        bug = self.factory.makeBug()
+        person = self.factory.makePerson()
+        with feature_flags():
+            with person_logged_in(person):
+                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.assertTrue(harness.hasErrors())
+                self.assertEqual(
+                    'Invalid value',
+                    harness.getFieldError('subscription'),
+                    "User should not be able to update a subscription that "
+                    "doesn't exist.")
+
+    def test_update_subscription_fails_for_users_subscribed_via_teams(self):
+        # If the user is not directly subscribed, but is subscribed via
+        # a team, they will not be able to use the "Update my
+        # subscription" option.
+        bug = self.factory.makeBug()
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=person)
+        with feature_flags():
+            with person_logged_in(person):
+                bug.subscribe(team, person)
+                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.assertTrue(harness.hasErrors())
+                self.assertEqual(
+                    'Invalid value',
+                    harness.getFieldError('subscription'),
+                    "User should not be able to update a subscription that "
+                    "doesn't exist.")
+
+    def test_bug_673288(self):
+        # If the user is not directly subscribed, but is subscribed via
+        # a team and via a duplicate, they will not be able to use the
+        # "Update my subscription" option.
+        # This is a regression test for bug 673288.
+        bug = self.factory.makeBug()
+        duplicate = self.factory.makeBug()
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=person)
+        with feature_flags():
+            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)
+                form_data = {
+                    'field.subscription': 'update-subscription',
+                    'field.bug_notification_level': level.name,
+                    }
+                harness.submit('continue', form_data)
+                self.assertTrue(harness.hasErrors())
+                self.assertEqual(
+                    'Invalid value',
+                    harness.getFieldError('subscription'),
+                    "User should not be able to update a subscription that "
+                    "doesn't exist.")
+
+    def test_bug_notification_level_field_hidden_for_dupe_subs(self):
+        # If the user is subscribed to the bug via a duplicate, the
+        # bug_notification_level field won't be visible on the form.
+        bug = self.factory.makeBug()
+        duplicate = self.factory.makeBug()
+        person = self.factory.makePerson()
+        with feature_flags():
+            with person_logged_in(person):
+                duplicate.markAsDuplicate(bug)
+                duplicate.subscribe(person, person)
+                harness = LaunchpadFormHarness(
+                    bug.default_bugtask, BugSubscriptionSubscribeSelfView)
+                self.assertFalse(
+                    harness.view.widgets['bug_notification_level'].visible)
+
 
 class BugPortletSubcribersIdsTests(TestCaseWithFactory):
 

-- 
https://code.launchpad.net/~gmb/launchpad/fix-advanced-bug-sub-ui-bug-673288/+merge/40631
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/fix-advanced-bug-sub-ui-bug-673288 into lp:launchpad/devel.