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
1=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
2--- lib/lp/bugs/doc/bugnotification-sending.txt 2010-05-17 14:56:15 +0000
3+++ lib/lp/bugs/doc/bugnotification-sending.txt 2010-06-03 18:57:25 +0000
4@@ -778,57 +778,6 @@
5 >>> for bug_notifications, messages in get_email_notifications(notifications):
6 ... for message in messages:
7 ... print_notification(message)
8- To: foo.bar@canonical.com
9- From: Sample Person <test@canonical.com>
10- Subject: [Bug 16] subject
11- X-Launchpad-Message-Rationale: Subscriber (mozilla-firefox in ubuntu) via Bug 1
12- <BLANKLINE>
13- *** This bug is a duplicate of bug 1 ***
14- http://bugs.launchpad.dev/bugs/1
15- <BLANKLINE>
16- a comment.
17- <BLANKLINE>
18- --
19- new bug
20- http://bugs.launchpad.dev/bugs/16
21- You received this bug notification because you are subscribed to
22- mozilla-firefox in ubuntu (via bug 1).
23- <BLANKLINE>
24- ----------------------------------------------------------------------
25- To: marilize@hbd.com
26- From: Sample Person <test@canonical.com>
27- Subject: [Bug 16] subject
28- X-Launchpad-Message-Rationale: Subscriber @shipit-admins via Bug 1
29- <BLANKLINE>
30- *** This bug is a duplicate of bug 1 ***
31- http://bugs.launchpad.dev/bugs/1
32- <BLANKLINE>
33- a comment.
34- <BLANKLINE>
35- --
36- new bug
37- http://bugs.launchpad.dev/bugs/16
38- You received this bug notification because you are a member of ShipIt
39- Administrators, which is a direct subscriber (via bug 1).
40- <BLANKLINE>
41- ----------------------------------------------------------------------
42- To: mark@example.com
43- From: Sample Person <test@canonical.com>
44- Subject: [Bug 16] subject
45- X-Launchpad-Message-Rationale: Assignee via Bug 1
46- <BLANKLINE>
47- *** This bug is a duplicate of bug 1 ***
48- http://bugs.launchpad.dev/bugs/1
49- <BLANKLINE>
50- a comment.
51- <BLANKLINE>
52- --
53- new bug
54- http://bugs.launchpad.dev/bugs/16
55- You received this bug notification because you are a bug assignee (via
56- bug 1).
57- <BLANKLINE>
58- ----------------------------------------------------------------------
59 To: support@ubuntu.com
60 From: Sample Person <test@canonical.com>
61 Subject: [Bug 16] subject
62@@ -864,9 +813,6 @@
63 <BLANKLINE>
64 ----------------------------------------------------------------------
65
66-Also note that notification was sent to Mark as well, since he's a
67-subscriber of bug one.
68-
69 >>> flush_notifications()
70
71
72
73=== modified file 'lib/lp/bugs/interfaces/bug.py'
74--- lib/lp/bugs/interfaces/bug.py 2010-04-29 17:49:19 +0000
75+++ lib/lp/bugs/interfaces/bug.py 2010-06-03 18:57:25 +0000
76@@ -444,7 +444,7 @@
77 """Return IPersons subscribed from dupes of this bug."""
78
79 def getBugNotificationRecipients(duplicateof=None, old_bug=None,
80- include_master_dupe_subscribers=True):
81+ include_master_dupe_subscribers=False):
82 """Return a complete INotificationRecipientSet instance.
83
84 The INotificationRecipientSet instance will contain details of
85@@ -454,7 +454,7 @@
86 canonical.launchpad.interfaces.BugNotificationRecipients for
87 details of this implementation.
88 If this bug is a dupe, set include_master_dupe_subscribers to
89- False to not include the master bug's subscribers.
90+ True to include the master bug's subscribers as recipients.
91 """
92
93 def addChangeNotification(text, person, recipients=None, when=None):
94
95=== modified file 'lib/lp/bugs/model/bug.py'
96--- lib/lp/bugs/model/bug.py 2010-05-25 15:48:47 +0000
97+++ lib/lp/bugs/model/bug.py 2010-06-03 18:57:25 +0000
98@@ -727,7 +727,7 @@
99
100 def getBugNotificationRecipients(self, duplicateof=None, old_bug=None,
101 level=None,
102- include_master_dupe_subscribers=True):
103+ include_master_dupe_subscribers=False):
104 """See `IBug`."""
105 recipients = BugNotificationRecipients(duplicateof=duplicateof)
106 self.getDirectSubscribers(recipients)
107
108=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
109--- lib/lp/bugs/tests/test_bugchanges.py 2010-04-14 12:55:44 +0000
110+++ lib/lp/bugs/tests/test_bugchanges.py 2010-06-03 18:57:25 +0000
111@@ -58,18 +58,32 @@
112 subscription.bug_notification_level = level
113 return subscriber
114
115- def saveOldChanges(self, bug=None):
116- """Save the old changes to a bug.
117-
118- This method should be called after all the setup is done.
119+ def saveOldChanges(self, bug=None, append=False):
120+ """Save old activity and notifications for a test.
121+
122+ This method should be called after setup. Removing the
123+ initial bug-created activity and notification messages
124+ allows for a more accurate check of new activity and
125+ notifications.
126+
127+ The append parameter can be used to save activity/notifications
128+ for more than one bug in a single test, as when dealing
129+ with duplicates.
130 """
131 if bug is None:
132 bug = self.bug
133- self.old_activities = set(bug.activity)
134- self.old_notification_ids = set(
135+ old_activities = set(bug.activity)
136+ old_notification_ids = set(
137 notification.id for notification in (
138 BugNotification.selectBy(bug=bug)))
139
140+ if append:
141+ self.old_activities.update(old_activities)
142+ self.old_notification_ids.update(old_notification_ids)
143+ else:
144+ self.old_activities = old_activities
145+ self.old_notification_ids = old_notification_ids
146+
147 def changeAttribute(self, obj, attribute, new_value):
148 """Set the value of `attribute` on `obj` to `new_value`.
149
150@@ -1248,12 +1262,10 @@
151 # and a notification is sent.
152 duplicate_bug = self.factory.makeBug()
153 self.saveOldChanges(duplicate_bug)
154- # Save the people that are notified about the bug before it's
155- # made a duplicate, so that we don't get extra people by
156- # mistake. Only the people subscribed to the bug that gets marked
157- # as a duplicate are notified. People who are subscribed to the
158- # master bug aren't, to avoid them getting spammed with
159- # unimportant bug notifications.
160+ self.saveOldChanges(self.bug, append=True)
161+ # Save the initial "bug created" notifications before
162+ # marking this bug a duplicate, so that we don't get
163+ # extra notificationse by mistake.
164 duplicate_bug_recipients = duplicate_bug.getBugNotificationRecipients(
165 level=BugNotificationLevel.METADATA).getRecipients()
166 self.changeAttribute(duplicate_bug, 'duplicateof', self.bug)
167@@ -1277,6 +1289,15 @@
168 expected_notification=expected_notification,
169 bug=duplicate_bug)
170
171+ # Ensure that only the people subscribed to the bug that
172+ # gets marked as a duplicate are notified.
173+ master_notifications = BugNotification.selectBy(
174+ bug=self.bug, orderBy='id')
175+ new_notifications = [
176+ notification for notification in master_notifications
177+ if notification.id not in self.old_notification_ids]
178+ self.assertEqual(len(list(new_notifications)), 0)
179+
180 def test_unmarked_as_duplicate(self):
181 # When a bug is unmarked as a duplicate, activity is recorded
182 # and a notification is sent.
183
184=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
185--- lib/lp/bugs/tests/test_bugnotification.py 2010-04-27 14:59:57 +0000
186+++ lib/lp/bugs/tests/test_bugnotification.py 2010-06-03 18:57:25 +0000
187@@ -21,7 +21,8 @@
188 from lp.testing import TestCaseWithFactory
189 from lp.testing.factory import LaunchpadObjectFactory
190 from lp.testing.mail_helpers import pop_notifications
191-from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer
192+from canonical.testing import (
193+ DatabaseFunctionalLayer, LaunchpadFunctionalLayer, LaunchpadZopelessLayer)
194
195
196 class TestNotificationRecipientsOfPrivateBugs(unittest.TestCase):
197@@ -144,6 +145,61 @@
198 bug=bug, is_comment=False, message=message, recipients=[])
199
200
201+class TestNotificationsForDuplicates(TestCaseWithFactory):
202+ """Test who gets notified about actions on duplicate bugs."""
203+
204+ layer = DatabaseFunctionalLayer
205+
206+ def setUp(self):
207+ super(TestNotificationsForDuplicates, self).setUp(
208+ user='test@canonical.com')
209+ self.bug = self.factory.makeBug()
210+ self.dupe_bug = self.factory.makeBug()
211+ self.dupe_bug.duplicateof = self.bug
212+ self.dupe_subscribers = set(
213+ self.dupe_bug.getDirectSubscribers() +
214+ self.dupe_bug.getIndirectSubscribers())
215+
216+ def test_comment_notifications(self):
217+ # New comments are only sent to subscribers of the duplicate
218+ # bug, not to subscribers of the master bug.
219+ self.dupe_bug.newMessage(
220+ self.dupe_bug.owner, subject='subject', content='content')
221+ latest_notification = BugNotification.selectFirst(orderBy='-id')
222+ recipients = set(
223+ recipient.person
224+ for recipient in latest_notification.recipients)
225+ self.assertEqual(self.dupe_subscribers, recipients)
226+
227+ def test_duplicate_edit_notifications(self):
228+ # Bug edits for a duplicate are sent to duplicate subscribers only.
229+ bug_before_modification = Snapshot(
230+ self.dupe_bug, providing=providedBy(self.dupe_bug))
231+ self.dupe_bug.description = 'A changed description'
232+ notify(ObjectModifiedEvent(
233+ self.dupe_bug, bug_before_modification, ['description'],
234+ user=self.dupe_bug.owner))
235+ latest_notification = BugNotification.selectFirst(orderBy='-id')
236+ recipients = set(
237+ recipient.person
238+ for recipient in latest_notification.recipients)
239+ self.assertEqual(self.dupe_subscribers, recipients)
240+
241+ def test_branch_linked_notification(self):
242+ # Notices for branches linked to a duplicate are sent only
243+ # to subscribers of the duplicate.
244+ #
245+ # No one should really do this, but this case covers notices
246+ # provided by the Bug.addChange mechanism.
247+ branch = self.factory.makeBranch(owner=self.dupe_bug.owner)
248+ self.dupe_bug.linkBranch(branch, self.dupe_bug.owner)
249+ latest_notification = BugNotification.selectFirst(orderBy='-id')
250+ recipients = set(
251+ recipient.person
252+ for recipient in latest_notification.recipients)
253+ self.assertEqual(self.dupe_subscribers, recipients)
254+
255+
256 def test_suite():
257 """Return the test suite for the tests in this module."""
258 return unittest.TestLoader().loadTestsFromName(__name__)