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

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://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
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_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
The diff is not available at this time. You can reload the page or download it.