Merge lp://staging/~gary/launchpad/bug164196-2 into lp://staging/launchpad/db-devel
Status: | Merged |
---|---|
Approved by: | Данило Шеган |
Approved revision: | no longer in the source branch. |
Merged at revision: | 10215 |
Proposed branch: | lp://staging/~gary/launchpad/bug164196-2 |
Merge into: | lp://staging/launchpad/db-devel |
Prerequisite: | lp://staging/~gary/launchpad/bug164196-1 |
Diff against target: |
1212 lines (+691/-80) 8 files modified
lib/lp/bugs/adapters/bugchange.py (+38/-12) lib/lp/bugs/browser/bugtask.py (+13/-6) lib/lp/bugs/doc/bugactivity.txt (+8/-3) lib/lp/bugs/doc/bugnotification-sending.txt (+23/-21) lib/lp/bugs/model/bugactivity.py (+35/-3) lib/lp/bugs/scripts/bugnotification.py (+64/-18) lib/lp/bugs/scripts/tests/test_bugnotification.py (+426/-16) lib/lp/bugs/tests/test_bugchanges.py (+84/-1) |
To merge this branch: | bzr merge lp://staging/~gary/launchpad/bug164196-2 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Данило Шеган (community) | Approve | ||
Review via email:
|
Commit message
use the activity attributes on bug notifications to implement the logic to silence emails for actions that are done and then undone in rapid succession.
Description of the change
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/
`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_
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/
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
Conclusions from IRC.
To increase trust in what's going on in parsing whatchanged, we should:
1) add docstring and unittest for get_key
2) move duplicateof code to target computed attribute code, and add test
3) change get_key to use target/attribute
As for the rest, I really like the test coverage: great job there!
And a side note for s/person_ causing_ change/ actor/: wordy name was introduced instead of 'person' to help make code easier to understand/read. It's up to you to decide which you want to keep as long as you take code readability into account :)
Considering we have agreed on a few changes, I am marking this as 'approved', though I'd be happy to take a look at incremental diff if it's a significant change.