launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

The proposal to merge lp:~gary/launchpad/bug164196-2 into lp:launchpad/db-devel has been updated.

Description changed to:

This is the second of three branches that address bug 164196. The other two are lp:~gary/launchpad/bug164196-1 and lp:~gary/launchpad/bug164196-3. My pre-implementation call for these changes was with Graham Binns.

This branch uses the activity attributes on bug notifications introduced in the first branch to implement the logic to silence notifications for actions that are done and then undone in rapid succession (that is, to do what the bug requests).  Undone actions are omitted from emails, and if the emails then have no content, they are not sent.

The only thing left to do is to make sure that the omitted notification objects are marked as processed so that they are not perpetually considered for subsequent notification cronscript runs.  This is tackled in the last branch of the series.

The implementation is all in lib/lp/bugs/scripts/bugnotification.py.  Within construct_email_notifications, there were already two loops through the notification objects, so I rearranged them so I could hook into one of them to determine what could be omitted.  The changes are a bit smaller than what a quick scan of the diff suggests, because I reordered the two loops.

`get_key` probably holds the subtlest code here: we normalize the bug activity's data to give us a key that will work for items that are linked/added and unlinked/removed.  For instance, if a change adds one branch but then removes another, both of these should be reported.  We can only squelch the notification if the same branch is added and removed.  A less subtle but important part of get_key is that it normalizes the "duplicate" names.  Unlike other changes, the activity text changes depending on what happens, so we need to normalize that here.

I renamed the local variable "person_causing_change" to "actor" as an easy solution to some long lines.

The bulk of the branch is the tests for these relatively small changes.  I changed the doctest enough for it to pass (and mention the behavior in passing), and updated some test infrastructure in lib/lp/bugs/scripts/tests/test_bugnotification.py (by adding the "activity" attribute to the mock bug notifications), but the main changes are brand new tests at the bottom of lib/lp/bugs/scripts/tests/test_bugnotification.py .  I don't test all change objects, just the ones that were unusual.  In this case, it means I omitted tests for non-collection bug attributes other than title (representative) and duplicate (exceptional), expecting the rest to work out fine; and I omitted tests for non-collection bugtask attributes other than status.  I can add them relatively easily--with fun test subclasses--if you like.

Speaking of subclasses, I don't love what I've done to assemble these tests from subclasses and mixins, but it certainly has precedence in our codebase, and the alternative, with lots of repetition, is even less attractive.

I removed the "def test_suite():" boilerplate because, AIUI, we have now 'fessed up to the fact that it didn't accomplish anything practical, and are actively encouraging removal.

Thank you

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug164196-2/+merge/49977
-- 
https://code.launchpad.net/~gary/launchpad/bug164196-2/+merge/49977
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug164196-2 into lp:launchpad/db-devel.