launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug-772763-edit-subscription into lp:launchpad/db-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug-772763-edit-subscription/+merge/61782

This branch, primarily from Danilo, builds on the other mute work we have done recently to make unmuting restore previous direct bug subscriptions.

The only goal of this branch is to make the +subscribe page, shown as a form overlay when you subscribe or edit a subscription, have options that are pertinent to the new functionality.  These changes are made, and tests are adjusted accordingly.

We no longer offer the option to unmute and edit your subscription simultaneously (and in fact unmuting will typically be done with a simple API-backed click in the UI rather than this form) so I removed the test for that case.

The UI is a bit odd after this branch in two ways.  First, it relies on the JS branch that I mentioned before to be completely resolved.  Second, in some cases it will show a single (pre-selected) radio button option.  I would worry more about this if we were not about to readdress the entire UI for bug 772754, and if this were not a db-devel branch.

Thank you!

Gary
-- 
https://code.launchpad.net/~gary/launchpad/bug-772763-edit-subscription/+merge/61782
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug-772763-edit-subscription into lp:launchpad/db-devel.

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2011-05-18 08:17:35 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2011-05-20 15:17:55 +0000
@@ -211,14 +211,6 @@
                 persons_for_user[person.id] = person
                 person_count += 1
 
-        # The view code previously expected a 'mute' to be a subscription
-        # as well.  Since it is not anymore, we add the user to the
-        # subscribers list as needed.
-        if self.user_is_muted:
-            if self.user.id not in persons_for_user:
-                persons_for_user[self.user.id] = self.user
-                person_count += 1
-
         self._subscriber_count_for_current_user = person_count
         return persons_for_user.values()
 
@@ -234,10 +226,7 @@
 
     @cachedproperty
     def _update_subscription_term(self):
-        if self.user_is_muted:
-            label = "unmute bug mail from this bug and subscribe me to it"
-        else:
-            label = "update my current subscription"
+        label = "update my current subscription"
         return SimpleTerm(
             'update-subscription', 'update-subscription', label)
 
@@ -250,19 +239,35 @@
         return SimpleTerm(self.user, self.user.name, label)
 
     @cachedproperty
+    def _unmute_user_term(self):
+        if self.user_is_subscribed_directly:
+            return SimpleTerm(
+                'update-subscription', 'update-subscription',
+                "unmute bug mail from this bug and restore my subscription")
+        else:
+            return SimpleTerm(self.user, self.user.name,
+                              "unmute bug mail from this bug")
+
+    @cachedproperty
     def _subscription_field(self):
         subscription_terms = []
         self_subscribed = False
+        is_really_muted = self._use_advanced_features and self.user_is_muted
+        if is_really_muted:
+            subscription_terms.insert(0, self._unmute_user_term)
         for person in self._subscribers_for_current_user:
             if person.id == self.user.id:
-                if (self._use_advanced_features and
-                    (self.user_is_subscribed_directly or
-                     self.user_is_muted)):
+                if is_really_muted:
+                    # We've already added the unmute option.
+                    continue
+                else:
+                    if (self._use_advanced_features and
+                        self.user_is_subscribed_directly):
                         subscription_terms.append(
                             self._update_subscription_term)
-                subscription_terms.insert(
-                    0, self._unsubscribe_current_user_term)
-                self_subscribed = True
+                    subscription_terms.insert(
+                        0, self._unsubscribe_current_user_term)
+                    self_subscribed = True
             else:
                 subscription_terms.append(
                     SimpleTerm(
@@ -270,7 +275,7 @@
                         'unsubscribe <a href="%s">%s</a> from this bug' % (
                             canonical_url(person),
                             cgi.escape(person.displayname))))
-        if not self_subscribed:
+        if not self_subscribed and not(is_really_muted):
             subscription_terms.insert(0,
                 SimpleTerm(
                     self.user, self.user.name, 'subscribe me to this bug'))
@@ -325,8 +330,9 @@
                 # subscribe theirself or unsubscribe their team.
                 self.widgets['subscription'].visible = True
 
-            if (self.user_is_subscribed and
-                self.user_is_subscribed_to_dupes_only):
+            if (self.user_is_subscribed_to_dupes_only or
+                (self._use_advanced_features and self.user_is_muted and
+                 not self.user_is_subscribed)):
                 # 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.
@@ -339,16 +345,12 @@
     @cachedproperty
     def user_is_subscribed_directly(self):
         """Is the user subscribed directly to this bug?"""
-        return (
-            self.context.bug.isSubscribed(self.user) and not
-            self.user_is_muted)
+        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) and not
-            self.user_is_muted)
+        return self.context.bug.isSubscribedToDupes(self.user)
 
     @property
     def user_is_subscribed(self):

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-05-17 11:44:33 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-05-20 15:17:55 +0000
@@ -231,15 +231,14 @@
                     harness.view.widgets['bug_notification_level'].visible)
 
     def test_muted_subs_have_unmute_option(self):
-        # If a user has a muted subscription, the
-        # BugSubscriptionSubscribeSelfView's subscription field will
-        # show an "Unmute" option.
+        # If a user has a muted subscription, but no previous
+        # direct bug subscription, the BugSubscriptionSubscribeSelfView's
+        # subscription field will show an "Unmute" option.
         with person_logged_in(self.person):
             self.bug.mute(self.person, self.person)
 
         with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(self.person):
-                self.bug.mute(self.person, self.person)
                 subscribe_view = create_initialized_view(
                     self.bug.default_bugtask, name='+subscribe')
                 subscription_widget = (
@@ -247,15 +246,17 @@
                 # The Unmute option is actually treated the same way as
                 # the unsubscribe option.
                 self.assertEqual(
-                    "unmute bug mail from this bug, or",
+                    "unmute bug mail from this bug",
                     subscription_widget.vocabulary.getTerm(self.person).title)
 
-    def test_muted_subs_have_unmute_and_update_option(self):
+    def test_muted_subs_have_unmute_and_restore_option(self):
         # If a user has a muted subscription, the
         # BugSubscriptionSubscribeSelfView's subscription field will
-        # show an option to unmute the subscription and update it to a
-        # new BugNotificationLevel.
+        # show an option to unmute the subscription and restore it to a
+        # previous or new BugNotificationLevel.
         with person_logged_in(self.person):
+            self.bug.subscribe(self.person, self.person,
+                               level=BugNotificationLevel.COMMENTS)
             self.bug.mute(self.person, self.person)
 
         with FeatureFixture({self.feature_flag: ON}):
@@ -267,12 +268,12 @@
                 update_term = subscription_widget.vocabulary.getTermByToken(
                     'update-subscription')
                 self.assertEqual(
-                    "unmute bug mail from this bug and subscribe me to it.",
+                    "unmute bug mail from this bug and restore my subscription",
                     update_term.title)
 
     def test_unmute_unmutes(self):
         # Using the "Unmute bug mail" option when the user has a muted
-        # subscription will remove the muted subscription.
+        # subscription will unmute.
         with person_logged_in(self.person):
             self.bug.mute(self.person, self.person)
 
@@ -290,28 +291,6 @@
                 self.assertFalse(self.bug.isMuted(self.person))
                 self.assertFalse(self.bug.isSubscribed(self.person))
 
-    def test_update_when_muted_updates(self):
-        # Using the "Unmute and subscribe me" option when the user has a
-        # muted subscription will update the existing subscription to a
-        # new BugNotificationLevel.
-        with person_logged_in(self.person):
-            self.bug.mute(self.person, self.person)
-
-        with FeatureFixture({self.feature_flag: ON}):
-            with person_logged_in(self.person):
-                level = BugNotificationLevel.COMMENTS
-                form_data = {
-                    'field.subscription': 'update-subscription',
-                    'field.bug_notification_level': level.title,
-                    'field.actions.continue': 'Continue',
-                    }
-                create_initialized_view(
-                    self.bug.default_bugtask, form=form_data,
-                    name='+subscribe')
-                transaction.commit()
-                self.assertFalse(self.bug.isMuted(self.person))
-                self.assertTrue(self.bug.isSubscribed(self.person))
-
     def test_bug_notification_level_field_has_widget_class(self):
         # The bug_notification_level widget has a widget_class property
         # that can be used to manipulate it with JavaScript.