Merge lp://staging/~deryck/launchpad/too-much-dupe-email-noise-418659 into lp://staging/launchpad
Status: | Merged |
---|---|
Approved by: | Jelmer Vernooij |
Approved revision: | no longer in the source branch. |
Merged at revision: | 10939 |
Proposed branch: | lp://staging/~deryck/launchpad/too-much-dupe-email-noise-418659 |
Merge into: | lp://staging/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://staging/~deryck/launchpad/too-much-dupe-email-noise-418659 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jelmer Vernooij (community) | code | Approve | |
Review via email: mp+26716@code.staging.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_
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_
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_
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/
* After changing the default behavior this test had to be
updated to not print mails that we no longer would send.
lib/lp/
lib/lp/
* The interface and the model for Bug were updated to set
the include_
Bug.
This means that if the bug is a duplicate, we will no
longer lookup the master subscribers by default.
lib/lp/
* 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/
* 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.
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.