Merge lp:~gmb/launchpad/fix-advanced-bug-sub-ui-bug-673288 into lp:launchpad

[Graham Binns]

[r=allenap][ui=none][bug=673288] Users who are not directly subscribed to a bug will no longer be offered the option to update their (nonexistent) subscription.

This branch fixes bug 673288 by making the "Update my subscription"
option on the +subscribe page disappear if the user is not directly
subscribed to the bug.

I've also made the bug_notification_level field invisible if the user
isn't directly subscribed, since having it there makes no sense.

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

 - I've made the changes necessary to fix bug 673288.

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

 - I've added a regresession test for the bug as well as two other tests
   to ensure proper behaviour of the view.

[Graham Binns (gmb)]

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(
+ ...

[Gavin Panella (allenap)]

Looks good, but I have a suggestion about making the code a little
clearer and I have some concerns about the tests.

[1]

+ if (self._use_advanced_features and
+ self.current_user_subscription is not None):
...
+ if (self._use_advanced_features and
+ self.current_user_subscription is not None):
...
+ if (self.user_is_subscribed and
+ self.current_user_subscription is None):

user_is_subscribed is defined as:

    @cachedproperty
    def user_is_subscribed(self):
        """Is the user subscribed to this bug?"""
        return (
            self.context.bug.isSubscribed(self.user) or
            self.context.bug.isSubscribedToDupes(self.user))

I think it would probably be easier to understand some of this logic
if this were split up:

    @cachedproperty
    def user_is_subscribed_directly(self):
        """Is the user subscribed directly to this bug?"""
        return self.context.bug.isSubscribed(self.user)

    @cachedproperty
    def user_is_subscribed_to_dupes(self):
        """Is the user subscribed to dupes of this bug?"""
        return self.context.bug.isSubscribedToDupes(self.user)

    def user_is_subscribed(self):
        """Is the user subscribed to this bug?"""
        return (self.user_is_subscribed_directly or
                self.user_is_subscribed_to_dupes)

    def user_is_subscribed_to_dupes_only(self):
        """Is the user subscribed to this bug only via a dupe?"""
        return (self.user_is_subscribed_to_dupes and
                not self.user_is_subscribed_directly)

Then the three examples become:

                if (self._use_advanced_features and
                    self.user_is_subscribed_directly):
...
        if (self._use_advanced_features and
            self.user_is_subscribed_directly):
...
            if (self.user_is_subscribed_to_dupes_only):

I know it's more typing - hehe, done for you :) - but it reads
better. This view is complicated and difficult to follow, so I think
it could do with some help in this regard.

[2]

+ def test_update_subscription_fails_if_user_not_subscribed(self):
+ def test_update_subscription_fails_for_users_subscribed_via_teams(self):
+ def test_bug_673288(self):

The tests are fine, but I wonder if they're the right way to test
this bug.

From reading the bug, it seems that the view was already doing
something like the right thing: it was breaking when presented with a
nonsensical request. The real bug, as far as I can tell, is that the
form presented to the user is wrong.

I think the tests should be demonstrating what fields the view expects
in the various scenarios. I don't think it's even necessary to submit
forms to the view.

review: Needs Fixing

[Graham Binns (gmb)]

Hi Gavin,

On Thu, Nov 11, 2010 at 04:32:54PM -0000, Gavin Panella wrote:
> Review: Needs Fixing
> Looks good, but I have a suggestion about making the code a little
> clearer and I have some concerns about the tests.

Cool, thanks.

> [1]
>[...]
> I know it's more typing - hehe, done for you :) - but it reads
> better. This view is complicated and difficult to follow, so I think
> it could do with some help in this regard.

I agree. I've done as you suggested (thanks for doing the work for me
;)).

> [2]
>
> + def test_update_subscription_fails_if_user_not_subscribed(self):
> + def test_update_subscription_fails_for_users_subscribed_via_teams(self):
> + def test_bug_673288(self):
>
> The tests are fine, but I wonder if they're the right way to test
> this bug.
>
> >From reading the bug, it seems that the view was already doing
> something like the right thing: it was breaking when presented with a
> nonsensical request. The real bug, as far as I can tell, is that the
> form presented to the user is wrong.
>
> I think the tests should be demonstrating what fields the view expects
> in the various scenarios. I don't think it's even necessary to submit
> forms to the view.
>

Hmm. I see your point. I've updated the tests as you suggest (and in the
process discovered how to test the options in the fields, which is
cool).

[Gavin Panella (allenap)]

It's a beauty :)

review: Approve

=== 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-12 11:07:34 +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.user_is_subscribed_directly):
                     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.user_is_subscribed_directly):
             default_subscription_value = self._update_subscription_term.value
         else:
             default_subscription_value = (
@@ -284,12 +286,36 @@
                 # subscribe theirself or unsubscribe their team.
                 self.widgets['subscription'].visible = True
 
-    @cachedproperty
+            if (self.user_is_subscribed and
+                self.user_is_subscribed_to_dupes_only):
+                # 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_directly(self):
+        """Is the user subscribed directly to this bug?"""
+        return self.context.bug.isSubscribed(self.user)
+
+    @cachedproperty
+    def user_is_subscribed_to_dupes(self):
+        """Is the user subscribed to dupes of this bug?"""
+        return self.context.bug.isSubscribedToDupes(self.user)
+
+    @property
     def user_is_subscribed(self):
         """Is the user subscribed to this bug?"""
         return (
-            self.context.bug.isSubscribed(self.user) or
-            self.context.bug.isSubscribedToDupes(self.user))
+            self.user_is_subscribed_directly or
+            self.user_is_subscribed_to_dupes)
+
+    @property
+    def user_is_subscribed_to_dupes_only(self):
+        """Is the user subscribed to this bug only via a dupe?"""
+        return (
+            self.user_is_subscribed_to_dupes and
+            not self.user_is_subscribed_directly)
 
     def shouldShowUnsubscribeFromDupesWarning(self):
         """Should we warn the user about unsubscribing and duplicates?

=== 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-12 11:07:34 +0000
@@ -10,6 +10,7 @@
 
 from lp.bugs.browser.bugsubscription import (
     BugPortletSubcribersIds,
+    BugSubscriptionAddView,
     BugSubscriptionSubscribeSelfView,
     )
 from lp.registry.enum import BugNotificationLevel
@@ -170,6 +171,84 @@
                     "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)
+                subscription_field = (
+                    harness.view.form_fields['subscription'].field)
+                # The update-subscription option won't appear.
+                self.assertNotIn(
+                    'update-subscription',
+                    subscription_field.vocabulary.by_token)
+
+    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)
+                subscription_field = (
+                    harness.view.form_fields['subscription'].field)
+                # The update-subscription option won't appear.
+                self.assertNotIn(
+                    'update-subscription',
+                    subscription_field.vocabulary.by_token)
+
+    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)
+                subscription_field = (
+                    harness.view.form_fields['subscription'].field)
+                # The update-subscription option won't appear.
+                self.assertNotIn(
+                    'update-subscription',
+                    subscription_field.vocabulary.by_token)
+
+    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):