launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/bug-61531 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #61531 in Launchpad itself: "Forbidden error when trying to mark a bug as private"
  https://bugs.launchpad.net/launchpad/+bug/61531

For more details, see:
https://code.launchpad.net/~gmb/launchpad/bug-61531/+merge/63672

This branch fixes bug 61531 by subscribing the user who makes a bug private to that bug (unless they're already subscribed). This approach was arrived at after discussion with Gary.

This branch handles the non-JS workflow; the JS stuff will be done in a second branch.
-- 
https://code.launchpad.net/~gmb/launchpad/bug-61531/+merge/63672
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/bug-61531 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2011-06-07 06:24:04 +0000
+++ lib/lp/bugs/browser/bug.py	2011-06-07 10:14:36 +0000
@@ -848,6 +848,10 @@
         bug_before_modification = Snapshot(
             bug, providing=providedBy(bug))
         private = data.pop('private')
+        user_will_be_subscribed = False
+        if private:
+            user_will_be_subscribed = (
+                not bug.personIsDirectSubscriber(self.user))
         security_related = data.pop('security_related')
         private_changed = bug.setPrivate(
             private, getUtility(ILaunchBag).user)
@@ -856,6 +860,21 @@
             changed_fields = []
             if private_changed:
                 changed_fields.append('private')
+                if user_will_be_subscribed:
+                    notification_text = (
+                        "Since you marked this bug as private you have "
+                        "automatically been subscribed to it.")
+                    if features.getFeatureFlag(
+                        'malone.advanced-subscriptions.enabled'):
+                        notification_text = (
+                            "%s If you don't want to receive email about "
+                            "this bug you can <a href=\"%s\">mute your "
+                            "subscription or unsubscribe</a>." % (
+                            notification_text,
+                            canonical_url(
+                                self.context, view_name='+subscribe')))
+                    self.request.response.addInfoNotification(
+                        structured(notification_text))
             if security_related_changed:
                 changed_fields.append('security_related')
             notify(ObjectModifiedEvent(

=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py	2011-06-07 06:24:04 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py	2011-06-07 10:14:36 +0000
@@ -202,6 +202,7 @@
                 self.assertTrue(
                     self._hasCSSClass(html, 'mute-link-container', 'hidden'),
                     'No "hidden" CSS class in mute-link-container.')
+<<<<<<< TREE
 
     def test_mute_subscription_link_not_rendered_for_anonymous(self):
         # If a person is not already subscribed to a bug in some way,
@@ -248,3 +249,72 @@
         self.assertEqual(
             'muted-false hidden subscribed-false dup-subscribed-false',
             view.current_user_mute_class)
+=======
+
+
+class TestBugSecrecyViews(TestCaseWithFactory):
+    """Tests for the Bug secrecy views."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_notification_shown_if_marking_private_and_not_subscribed(self):
+        # If a user who is not subscribed to a bug marks that bug as
+        # private, the user will be subscribed to the bug. This allows
+        # them to un-mark the bug if they choose to, rather than being
+        # blocked from doing so.
+        person = self.factory.makePerson()
+        bug = self.factory.makeBug()
+        with person_logged_in(person):
+            view = create_initialized_view(
+                bug.default_bugtask, name='+secrecy', form={
+                    'field.private': 'on',
+                    'field.security_related': '',
+                    'field.actions.change': 'Change',
+                    })
+            self.assertEqual(1, len(view.request.response.notifications))
+            message_text = (
+                "Since you marked this bug as private you have "
+                "automatically been subscribed to it.")
+            self.assertEqual(
+                message_text, view.request.response.notifications[0].message)
+
+    def test_no_notification_shown_if_marking_private_and_subscribed(self):
+        # If a user who is subscribed to a bug marks that bug as
+        # private, the user will see not notification.
+        person = self.factory.makePerson()
+        bug = self.factory.makeBug()
+        with person_logged_in(person):
+            bug.subscribe(person, person)
+            view = create_initialized_view(
+                bug.default_bugtask, name='+secrecy', form={
+                    'field.private': 'on',
+                    'field.security_related': '',
+                    'field.actions.change': 'Change',
+                    })
+            self.assertEqual(0, len(view.request.response.notifications))
+
+    def test_notification_includes_link_for_advanced_subscriptions(self):
+        # If the advanced subscriptions feature flag is turned on, the
+        # notification will include a link to the edit subscription
+        # page.
+        person = self.factory.makePerson()
+        bug = self.factory.makeBug()
+        with FeatureFixture({'malone.advanced-subscriptions.enabled': 'on'}):
+            with person_logged_in(person):
+                view = create_initialized_view(
+                    bug.default_bugtask, name='+secrecy', form={
+                        'field.private': 'on',
+                        'field.security_related': '',
+                        'field.actions.change': 'Change',
+                        })
+                self.assertEqual(1, len(view.request.response.notifications))
+                message_text = (
+                    "Since you marked this bug as private you have "
+                    "automatically been subscribed to it. If you don't want "
+                    "to receive email about this bug you can <a href=\""
+                    "%s\">mute your subscription or unsubscribe</a>."
+                    % canonical_url(
+                        bug.default_bugtask, view_name='+subscribe'))
+                self.assertEqual(
+                    message_text, view.request.response.notifications[0].message)
+>>>>>>> MERGE-SOURCE

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-06-07 06:24:04 +0000
+++ lib/lp/bugs/model/bug.py	2011-06-07 10:14:36 +0000
@@ -1645,6 +1645,10 @@
                 # the bug is private.
                 for person in self.getIndirectSubscribers():
                     self.subscribe(person, who)
+                # We also add `who` as a subscriber so that they can
+                # see the bug they've just marked private. If they're
+                # already subscribed this will be a no-op.
+                self.subscribe(who, who)
 
             self.private = private
 

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2011-06-07 06:24:04 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2011-06-07 10:14:36 +0000
@@ -261,3 +261,12 @@
         with person_logged_in(bug.owner):
             info = bug.getSubscriptionInfo(BugNotificationLevel.METADATA)
         self.assertEqual(BugNotificationLevel.METADATA, info.level)
+
+    def test_setPrivate_subscribes_person_who_makes_bug_private(self):
+        # When setPrivate(True) is called on a bug, the person who is
+        # marking the bug private is subscribed to the bug.
+        bug = self.factory.makeBug()
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            bug.setPrivate(True, person)
+            self.assertTrue(bug.personIsDirectSubscriber(person))