Launchpad itself

Merge lp:~deryck/launchpad/too-much-dupe-email-noise-418659 into lp:launchpad

  1. too-much-dupe-email-noise-418659
  2. Merge into devel
Proposed by Deryck Hodge
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: no longer in the source branch.
Merged at revision: 10939
Proposed branch: lp:~deryck/launchpad/too-much-dupe-email-noise-418659
Merge into: lp:launchpad
Diff against target: 258 lines (+93/-70)
5 files modified
lib/lp/bugs/doc/bugnotification-sending.txt (+0/-54)
lib/lp/bugs/interfaces/bug.py (+2/-2)
lib/lp/bugs/model/bug.py (+1/-1)
lib/lp/bugs/tests/test_bugchanges.py (+33/-12)
lib/lp/bugs/tests/test_bugnotification.py (+57/-1)
To merge this branch: bzr merge lp:~deryck/launchpad/too-much-dupe-email-noise-418659
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) code Approve
Review via email: mp+26716@code.launchpad.net

Commit message

Do not send notifications for actions or comments on duplicate bugs to subscribers of the master bug.

Description of the change

This branch fixes bug 418659, which notes that reporting a bug that gets
marked as a duplicate can lead to lots of email notifications for other
duplicates. The bug report describes getting notifications for "this bug
has been marked a duplicate of bug XXX," but there was already some work
done to ensure this didn't happen. So working out what was going on proved
a bit of a challenge.

== Pre implementation chats ==

I had several discussions, both in the bug and in IRC, with BjornT about
the work that was done prior. There is a test that ensured duplicate
notifications were not sent out, and BjornT pointed me at the work he had
done to add the include_master_dupe_subscribers parameter. We agreed test
coverage could be extended to determine what was going on, and also that we
shouldn't be sending emails to master bug subscribers for any activity done
on a duplicate.

== The Problem ==

The test in test_bugchanges for test_marked_as_duplicate was a mostly good
test to ensure that duplicate subscribers received the "your bug has been
marked a duplicate" notifications. I cannot find any of these notices
being sent to a subscriber of the master bug. What is happening is that
Ubuntu developers add a comment when they mark a bug as a duplicate and
with notification batching the comment and the dup marking notice would be
sent to master bug subscribers.

So it turns out, we are sending notifications to the subscribers of the
master bug for any activity done on the duplicate bug after it has been
marked a duplicate.

== The Fix ==

The right thing to do here is to prevent notifications about activity on
the duplicate bug being sent to subscribers of the master bug. To do this,
I have changed the default for the include_master_dupe_subscribers to be
False.

So from here on out, if I had a comment to a duplicate bug, only direct and
indirect subscribers of the duplicate will receive an email about that
comment. Subscribers of the master bug will not know.

== Files changed ==

lib/lp/bugs/doc/bugnotification-sending.txt
    * After changing the default behavior this test had to be
      updated to not print mails that we no longer would send.

lib/lp/bugs/interfaces/bug.py
lib/lp/bugs/model/bug.py
    * The interface and the model for Bug were updated to set
      the include_master_dupe_subscribers parameter of
      Bug.getBugNotificationRecipients to False.

      This means that if the bug is a duplicate, we will no
      longer lookup the master subscribers by default.

lib/lp/bugs/tests/test_bugchanges.py
    * The original test for ensuring "this bug has been marked
      a duplicate" messages were sent to duplicate subscribers
      has been updated to ensure master subscribers are *not*
      notified.

      To do this, I had to also extend the test to allow me
      to pop "bug created" notifications from more than one
      bug per test, so saveOldChanges has been updated to
      enable this.

      There are also lots of comment updates in this test,
      since it was hard for me to work out what was actually
      tested and going on. I did not want to have to learn
      this again, or force this same pain on another dev.

lib/lp/bugs/tests/test_bugnotification.py
    * A test was added here to ensure activity on duplicates
      does not send notifications to the master bug's
      subscribers.

      I have added tests to cover adding comments, notices
      from the bug object being modified (the edit test),
      and notifices from Bug.addChange (the linked branch
      test). This should cover all cases for duplicate
      bug change notifications.

I have tested pretty thoroughly locally and couldn't find any other
affected tests, but I find it hard to believe this significant a change as
this small a test fallout. I expect to run into some minor test updates
when running this through ec2.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Hi Deryck,

One minor thing; it'd be nice if the append argument to saveOldChanges() was explicitly documented in the docstring. Other than that, great to see fewer doctests and more comments and unit tests.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt 2010-05-17 14:56:15 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt 2010-06-03 18:57:25 +0000
@@ -778,57 +778,6 @@
778 >>> for bug_notifications, messages in get_email_notifications(notifications):778 >>> for bug_notifications, messages in get_email_notifications(notifications):
779 ... for message in messages:779 ... for message in messages:
780 ... print_notification(message)780 ... print_notification(message)
781 To: foo.bar@canonical.com
782 From: Sample Person <test@canonical.com>
783 Subject: [Bug 16] subject
784 X-Launchpad-Message-Rationale: Subscriber (mozilla-firefox in ubuntu) via Bug 1
785 <BLANKLINE>
786 *** This bug is a duplicate of bug 1 ***
787 http://bugs.launchpad.dev/bugs/1
788 <BLANKLINE>
789 a comment.
790 <BLANKLINE>
791 --
792 new bug
793 http://bugs.launchpad.dev/bugs/16
794 You received this bug notification because you are subscribed to
795 mozilla-firefox in ubuntu (via bug 1).
796 <BLANKLINE>
797 ----------------------------------------------------------------------
798 To: marilize@hbd.com
799 From: Sample Person <test@canonical.com>
800 Subject: [Bug 16] subject
801 X-Launchpad-Message-Rationale: Subscriber @shipit-admins via Bug 1
802 <BLANKLINE>
803 *** This bug is a duplicate of bug 1 ***
804 http://bugs.launchpad.dev/bugs/1
805 <BLANKLINE>
806 a comment.
807 <BLANKLINE>
808 --
809 new bug
810 http://bugs.launchpad.dev/bugs/16
811 You received this bug notification because you are a member of ShipIt
812 Administrators, which is a direct subscriber (via bug 1).
813 <BLANKLINE>
814 ----------------------------------------------------------------------
815 To: mark@example.com
816 From: Sample Person <test@canonical.com>
817 Subject: [Bug 16] subject
818 X-Launchpad-Message-Rationale: Assignee via Bug 1
819 <BLANKLINE>
820 *** This bug is a duplicate of bug 1 ***
821 http://bugs.launchpad.dev/bugs/1
822 <BLANKLINE>
823 a comment.
824 <BLANKLINE>
825 --
826 new bug
827 http://bugs.launchpad.dev/bugs/16
828 You received this bug notification because you are a bug assignee (via
829 bug 1).
830 <BLANKLINE>
831 ----------------------------------------------------------------------
832 To: support@ubuntu.com781 To: support@ubuntu.com
833 From: Sample Person <test@canonical.com>782 From: Sample Person <test@canonical.com>
834 Subject: [Bug 16] subject783 Subject: [Bug 16] subject
@@ -864,9 +813,6 @@
864 <BLANKLINE>813 <BLANKLINE>
865 ----------------------------------------------------------------------814 ----------------------------------------------------------------------
866815
867Also note that notification was sent to Mark as well, since he's a
868subscriber of bug one.
869
870 >>> flush_notifications()816 >>> flush_notifications()
871817
872818
873819
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2010-04-29 17:49:19 +0000
+++ lib/lp/bugs/interfaces/bug.py 2010-06-03 18:57:25 +0000
@@ -444,7 +444,7 @@
444 """Return IPersons subscribed from dupes of this bug."""444 """Return IPersons subscribed from dupes of this bug."""
445445
446 def getBugNotificationRecipients(duplicateof=None, old_bug=None,446 def getBugNotificationRecipients(duplicateof=None, old_bug=None,
447 include_master_dupe_subscribers=True):447 include_master_dupe_subscribers=False):
448 """Return a complete INotificationRecipientSet instance.448 """Return a complete INotificationRecipientSet instance.
449449
450 The INotificationRecipientSet instance will contain details of450 The INotificationRecipientSet instance will contain details of
@@ -454,7 +454,7 @@
454 canonical.launchpad.interfaces.BugNotificationRecipients for454 canonical.launchpad.interfaces.BugNotificationRecipients for
455 details of this implementation.455 details of this implementation.
456 If this bug is a dupe, set include_master_dupe_subscribers to456 If this bug is a dupe, set include_master_dupe_subscribers to
457 False to not include the master bug's subscribers.457 True to include the master bug's subscribers as recipients.
458 """458 """
459459
460 def addChangeNotification(text, person, recipients=None, when=None):460 def addChangeNotification(text, person, recipients=None, when=None):
461461
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-05-25 15:48:47 +0000
+++ lib/lp/bugs/model/bug.py 2010-06-03 18:57:25 +0000
@@ -727,7 +727,7 @@
727727
728 def getBugNotificationRecipients(self, duplicateof=None, old_bug=None,728 def getBugNotificationRecipients(self, duplicateof=None, old_bug=None,
729 level=None,729 level=None,
730 include_master_dupe_subscribers=True):730 include_master_dupe_subscribers=False):
731 """See `IBug`."""731 """See `IBug`."""
732 recipients = BugNotificationRecipients(duplicateof=duplicateof)732 recipients = BugNotificationRecipients(duplicateof=duplicateof)
733 self.getDirectSubscribers(recipients)733 self.getDirectSubscribers(recipients)
734734
=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py 2010-04-14 12:55:44 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py 2010-06-03 18:57:25 +0000
@@ -58,18 +58,32 @@
58 subscription.bug_notification_level = level58 subscription.bug_notification_level = level
59 return subscriber59 return subscriber
6060
61 def saveOldChanges(self, bug=None):61 def saveOldChanges(self, bug=None, append=False):
62 """Save the old changes to a bug.62 """Save old activity and notifications for a test.
6363
64 This method should be called after all the setup is done.64 This method should be called after setup. Removing the
65 initial bug-created activity and notification messages
66 allows for a more accurate check of new activity and
67 notifications.
68
69 The append parameter can be used to save activity/notifications
70 for more than one bug in a single test, as when dealing
71 with duplicates.
65 """72 """
66 if bug is None:73 if bug is None:
67 bug = self.bug74 bug = self.bug
68 self.old_activities = set(bug.activity)75 old_activities = set(bug.activity)
69 self.old_notification_ids = set(76 old_notification_ids = set(
70 notification.id for notification in (77 notification.id for notification in (
71 BugNotification.selectBy(bug=bug)))78 BugNotification.selectBy(bug=bug)))
7279
80 if append:
81 self.old_activities.update(old_activities)
82 self.old_notification_ids.update(old_notification_ids)
83 else:
84 self.old_activities = old_activities
85 self.old_notification_ids = old_notification_ids
86
73 def changeAttribute(self, obj, attribute, new_value):87 def changeAttribute(self, obj, attribute, new_value):
74 """Set the value of `attribute` on `obj` to `new_value`.88 """Set the value of `attribute` on `obj` to `new_value`.
7589
@@ -1248,12 +1262,10 @@
1248 # and a notification is sent.1262 # and a notification is sent.
1249 duplicate_bug = self.factory.makeBug()1263 duplicate_bug = self.factory.makeBug()
1250 self.saveOldChanges(duplicate_bug)1264 self.saveOldChanges(duplicate_bug)
1251 # Save the people that are notified about the bug before it's1265 self.saveOldChanges(self.bug, append=True)
1252 # made a duplicate, so that we don't get extra people by1266 # Save the initial "bug created" notifications before
1253 # mistake. Only the people subscribed to the bug that gets marked1267 # marking this bug a duplicate, so that we don't get
1254 # as a duplicate are notified. People who are subscribed to the1268 # extra notificationse by mistake.
1255 # master bug aren't, to avoid them getting spammed with
1256 # unimportant bug notifications.
1257 duplicate_bug_recipients = duplicate_bug.getBugNotificationRecipients(1269 duplicate_bug_recipients = duplicate_bug.getBugNotificationRecipients(
1258 level=BugNotificationLevel.METADATA).getRecipients()1270 level=BugNotificationLevel.METADATA).getRecipients()
1259 self.changeAttribute(duplicate_bug, 'duplicateof', self.bug)1271 self.changeAttribute(duplicate_bug, 'duplicateof', self.bug)
@@ -1277,6 +1289,15 @@
1277 expected_notification=expected_notification,1289 expected_notification=expected_notification,
1278 bug=duplicate_bug)1290 bug=duplicate_bug)
12791291
1292 # Ensure that only the people subscribed to the bug that
1293 # gets marked as a duplicate are notified.
1294 master_notifications = BugNotification.selectBy(
1295 bug=self.bug, orderBy='id')
1296 new_notifications = [
1297 notification for notification in master_notifications
1298 if notification.id not in self.old_notification_ids]
1299 self.assertEqual(len(list(new_notifications)), 0)
1300
1280 def test_unmarked_as_duplicate(self):1301 def test_unmarked_as_duplicate(self):
1281 # When a bug is unmarked as a duplicate, activity is recorded1302 # When a bug is unmarked as a duplicate, activity is recorded
1282 # and a notification is sent.1303 # and a notification is sent.
12831304
=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py 2010-04-27 14:59:57 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py 2010-06-03 18:57:25 +0000
@@ -21,7 +21,8 @@
21from lp.testing import TestCaseWithFactory21from lp.testing import TestCaseWithFactory
22from lp.testing.factory import LaunchpadObjectFactory22from lp.testing.factory import LaunchpadObjectFactory
23from lp.testing.mail_helpers import pop_notifications23from lp.testing.mail_helpers import pop_notifications
24from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer24from canonical.testing import (
25 DatabaseFunctionalLayer, LaunchpadFunctionalLayer, LaunchpadZopelessLayer)
2526
2627
27class TestNotificationRecipientsOfPrivateBugs(unittest.TestCase):28class TestNotificationRecipientsOfPrivateBugs(unittest.TestCase):
@@ -144,6 +145,61 @@
144 bug=bug, is_comment=False, message=message, recipients=[])145 bug=bug, is_comment=False, message=message, recipients=[])
145146
146147
148class TestNotificationsForDuplicates(TestCaseWithFactory):
149 """Test who gets notified about actions on duplicate bugs."""
150
151 layer = DatabaseFunctionalLayer
152
153 def setUp(self):
154 super(TestNotificationsForDuplicates, self).setUp(
155 user='test@canonical.com')
156 self.bug = self.factory.makeBug()
157 self.dupe_bug = self.factory.makeBug()
158 self.dupe_bug.duplicateof = self.bug
159 self.dupe_subscribers = set(
160 self.dupe_bug.getDirectSubscribers() +
161 self.dupe_bug.getIndirectSubscribers())
162
163 def test_comment_notifications(self):
164 # New comments are only sent to subscribers of the duplicate
165 # bug, not to subscribers of the master bug.
166 self.dupe_bug.newMessage(
167 self.dupe_bug.owner, subject='subject', content='content')
168 latest_notification = BugNotification.selectFirst(orderBy='-id')
169 recipients = set(
170 recipient.person
171 for recipient in latest_notification.recipients)
172 self.assertEqual(self.dupe_subscribers, recipients)
173
174 def test_duplicate_edit_notifications(self):
175 # Bug edits for a duplicate are sent to duplicate subscribers only.
176 bug_before_modification = Snapshot(
177 self.dupe_bug, providing=providedBy(self.dupe_bug))
178 self.dupe_bug.description = 'A changed description'
179 notify(ObjectModifiedEvent(
180 self.dupe_bug, bug_before_modification, ['description'],
181 user=self.dupe_bug.owner))
182 latest_notification = BugNotification.selectFirst(orderBy='-id')
183 recipients = set(
184 recipient.person
185 for recipient in latest_notification.recipients)
186 self.assertEqual(self.dupe_subscribers, recipients)
187
188 def test_branch_linked_notification(self):
189 # Notices for branches linked to a duplicate are sent only
190 # to subscribers of the duplicate.
191 #
192 # No one should really do this, but this case covers notices
193 # provided by the Bug.addChange mechanism.
194 branch = self.factory.makeBranch(owner=self.dupe_bug.owner)
195 self.dupe_bug.linkBranch(branch, self.dupe_bug.owner)
196 latest_notification = BugNotification.selectFirst(orderBy='-id')
197 recipients = set(
198 recipient.person
199 for recipient in latest_notification.recipients)
200 self.assertEqual(self.dupe_subscribers, recipients)
201
202
147def test_suite():203def test_suite():
148 """Return the test suite for the tests in this module."""204 """Return the test suite for the tests in this module."""
149 return unittest.TestLoader().loadTestsFromName(__name__)205 return unittest.TestLoader().loadTestsFromName(__name__)