Merge lp:~gmb/launchpad/dont-leak-privacy-in-notifications-bug-373683 into lp:launchpad/db-devel

[Graham Binns]

Private bug summaries will no longer be included in bug duplication notifications.

This branch fixes bug 373683 by preventing bug notifications generated when a bug is marked a duplicate of a private bug from including the private bug's summary.

I've made the following changes:

 * lib/lp/bugs/adapters/bugchange.py
  * I've updated BugDuplicateChange to make it not include private bug summaries.
 * lib/lp/bugs/tests/test_bugchange.py
  * I've updated the tests to test the above change.

I did consider refactoring BugDuplicateChange.getBugNotification() to make it into something that wasn't just a bunch of nested if/elif/elses, but I couldn't think of an elegant solution (feel free to suggest one). That said, the method is pretty short so I'm not sure I'm not just bikeshedding.

[Leonard Richardson (leonardr)]

r=me with some minor changes.

Did you mean to merge this with devel rather than db-devel? I make that mistake all the time.

Rather than bikeshedding the logic of the message generator, I suggest you just define four constants so you don't write the same string multiple times.

75 # extra notificationse by mistake.
118 # extra notificationse by mistake.
(Should be 'notifications')

review: Approve (code)

=== modified file 'lib/lp/bugs/adapters/bugchange.py'
--- lib/lp/bugs/adapters/bugchange.py	2010-01-20 17:46:53 +0000
+++ lib/lp/bugs/adapters/bugchange.py	2010-06-09 16:28:30 +0000
@@ -366,17 +366,45 @@
 
     def getBugNotification(self):
         if self.old_value is not None and self.new_value is not None:
-            text = ("** This bug is no longer a duplicate of bug %d\n"
-                    "   %s\n"
+            if self.old_value.private:
+                old_value_text = (
+                    "** This bug is no longer a duplicate of private bug "
+                    "%d" % self.old_value.id)
+            else:
+                old_value_text = (
+                    "** This bug is no longer a duplicate of bug %d\n"
+                    "   %s" % (self.old_value.id, self.old_value.title))
+            if self.new_value.private:
+                new_value_text = (
+                    "** This bug has been marked a duplicate of private bug "
+                    "%d" % self.new_value.id)
+            else:
+                new_value_text = (
                     "** This bug has been marked a duplicate of bug %d\n"
-                    "   %s" % (self.old_value.id, self.old_value.title,
-                               self.new_value.id, self.new_value.title))
+                    "   %s" % (self.new_value.id, self.new_value.title))
+
+            text = "\n".join((old_value_text, new_value_text))
+
         elif self.old_value is None:
-            text = ("** This bug has been marked a duplicate of bug %d\n"
+            if self.new_value.private:
+                text = (
+                    "** This bug has been marked a duplicate of private bug "
+                    "%d" % self.new_value.id)
+            else:
+                text = (
+                    "** This bug has been marked a duplicate of bug %d\n"
                     "   %s" % (self.new_value.id, self.new_value.title))
+
         elif self.new_value is None:
-            text = ("** This bug is no longer a duplicate of bug %d\n"
+            if self.old_value.private:
+                text = (
+                    "** This bug is no longer a duplicate of private bug "
+                    "%d" % self.old_value.id)
+            else:
+                text = (
+                    "** This bug is no longer a duplicate of bug %d\n"
                     "   %s" % (self.old_value.id, self.old_value.title))
+
         else:
             raise AssertionError(
                 "There is no change: both the old bug and new bug are None.")

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2010-06-03 18:55:41 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2010-06-09 16:28:30 +0000
@@ -1265,7 +1265,7 @@
         self.saveOldChanges(self.bug, append=True)
         # Save the initial "bug created" notifications before
         # marking this bug a duplicate, so that we don't get
-        # extra notificationse by mistake.
+        # extra notifications by mistake.
         duplicate_bug_recipients = duplicate_bug.getBugNotificationRecipients(
             level=BugNotificationLevel.METADATA).getRecipients()
         self.changeAttribute(duplicate_bug, 'duplicateof', self.bug)
@@ -1360,6 +1360,119 @@
             expected_activity=expected_activity,
             expected_notification=expected_notification)
 
+    def test_duplicate_private_bug(self):
+        # When a bug is marked as the duplicate of a private bug the
+        # private bug's summary won't be included in the notification.
+        private_bug = self.factory.makeBug()
+        private_bug.setPrivate(True, self.user)
+        public_bug = self.factory.makeBug()
+        self.saveOldChanges(private_bug)
+        self.saveOldChanges(public_bug)
+
+        # Save the initial "bug created" notifications before
+        # marking this bug a duplicate, so that we don't get
+        # extra notifications by mistake.
+        public_bug_recipients = public_bug.getBugNotificationRecipients(
+            level=BugNotificationLevel.METADATA).getRecipients()
+        self.changeAttribute(public_bug, 'duplicateof', private_bug)
+
+        expected_activity = {
+            'person': self.user,
+            'whatchanged': 'marked as duplicate',
+            'oldvalue': None,
+            'newvalue': str(private_bug.id),
+            }
+
+        expected_notification = {
+            'person': self.user,
+            'text': (
+                "** This bug has been marked a duplicate of private bug %d"
+                % private_bug.id),
+            'recipients': public_bug_recipients,
+            }
+
+        self.assertRecordedChange(
+            expected_activity=expected_activity,
+            expected_notification=expected_notification,
+            bug=public_bug)
+
+    def test_unmarked_as_duplicate_of_private_bug(self):
+        # When a bug is unmarked as a duplicate of a private bug,
+        # the private bug's summary isn't sent in the notification.
+        private_bug = self.factory.makeBug()
+        private_bug.setPrivate(True, self.user)
+        public_bug = self.factory.makeBug()
+
+        # Save the initial "bug created" notifications before
+        # marking this bug a duplicate, so that we don't get
+        # extra notifications by mistake.
+        public_bug_recipients = public_bug.getBugNotificationRecipients(
+            level=BugNotificationLevel.METADATA).getRecipients()
+        self.changeAttribute(public_bug, 'duplicateof', private_bug)
+
+        self.saveOldChanges(private_bug)
+        self.saveOldChanges(public_bug)
+
+        self.changeAttribute(public_bug, 'duplicateof', None)
+
+        expected_activity = {
+            'person': self.user,
+            'whatchanged': 'removed duplicate marker',
+            'oldvalue': str(private_bug.id),
+            'newvalue': None,
+            }
+
+        expected_notification = {
+            'person': self.user,
+            'text': (
+                "** This bug is no longer a duplicate of private bug %d"
+                % private_bug.id),
+            'recipients': public_bug_recipients,
+            }
+
+        self.assertRecordedChange(
+            expected_activity=expected_activity,
+            expected_notification=expected_notification,
+            bug=public_bug)
+
+    def test_changed_private_duplicate(self):
+        # When a bug is change from being the duplicate of a private bug
+        # to being the duplicate of a public bug, the private bug's
+        # summary won't be sent in the notification.
+        private_bug = self.factory.makeBug()
+        private_bug.setPrivate(True, self.user)
+        duplicate_bug = self.factory.makeBug()
+        public_bug = self.factory.makeBug()
+        bug_recipients = duplicate_bug.getBugNotificationRecipients(
+            level=BugNotificationLevel.METADATA).getRecipients()
+
+        self.changeAttribute(duplicate_bug, 'duplicateof', private_bug)
+        self.saveOldChanges(duplicate_bug)
+
+        self.changeAttribute(duplicate_bug, 'duplicateof', public_bug)
+
+        expected_activity = {
+            'person': self.user,
+            'whatchanged': 'changed duplicate marker',
+            'oldvalue': str(private_bug.id),
+            'newvalue': str(public_bug.id),
+            }
+
+        expected_notification = {
+            'person': self.user,
+            'text': (
+                "** This bug is no longer a duplicate of private bug %d\n"
+                "** This bug has been marked a duplicate of bug %d\n"
+                "   %s" % (private_bug.id, public_bug.id,
+                           public_bug.title)),
+            'recipients': bug_recipients,
+            }
+
+        self.assertRecordedChange(
+            expected_activity=expected_activity,
+            expected_notification=expected_notification,
+            bug=duplicate_bug)
+
     def test_convert_to_question_no_comment(self):
         # When a bug task is converted to a question, its status is
         # first set to invalid, which causes the normal notifications for