Launchpad itself

cronscripts/send-bug-notifications.py (+8/-1)
database/schema/comments.sql (+1/-0)
database/schema/patch-2208-45-0.sql (+12/-0)
lib/lp/bugs/doc/bugnotification-sending.txt (+61/-19)
lib/lp/bugs/doc/bugnotification-threading.txt (+3/-3)
lib/lp/bugs/enum.py (+27/-0)
lib/lp/bugs/interfaces/bugnotification.py (+9/-0)
lib/lp/bugs/mail/tests/test_bug_task_assignment.py (+2/-2)
lib/lp/bugs/mail/tests/test_bug_task_modification.py (+1/-1)
lib/lp/bugs/model/bugnotification.py (+6/-0)
lib/lp/bugs/scripts/bugnotification.py (+4/-1)
lib/lp/bugs/scripts/tests/test_bugnotification.py (+4/-2)
lib/lp/bugs/tests/test_bugchanges.py (+2/-1)

To merge this branch:

bzr merge lp://staging/~gary/launchpad/bug164196-3

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Robert Collins (community)		2011-02-16	Approve on 2011-02-17
Stuart Bishop (community)	db	2011-02-16	Approve on 2011-02-17
Aaron Bentley (community)		2011-02-16	Approve on 2011-02-16
Review via email: mp+49980@code.staging.launchpad.net

Commit message

[r=abentley,stub][bug=164196] makes sure that the omitted notification objects left over from duplicate actions are marked as processed so that they are not perpetually considered for subsequent notification cronscript runs.

Description of the change

This is the last of three branches that address bug 164196, and one of two that have a database patch. 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 makes sure that the omitted notification objects left over from actions in the previous branch are marked as processed so that they are not perpetually considered for subsequent notification cronscript runs. It also marks these omitted notification objects with a flag in case we want to analyze them for debugging later, if we get a report of something gone wrong in this regard.

The database changes simply add the flag as described above.

The code in lib/lp/bugs/scripts/bugnotification.py now includes the omitted notifications so that the cronscript can mark them. This required small changes to a number of tests.

lib/lp/bugs/doc/bugnotification-sending.txt had the test for the cronscript and were a natural place for a smoketest of this behavior.

Thank you

Gary

Revision history for this message

Aaron Bentley (abentley) wrote on 2011-02-16:

Per our IRC discussion, I recommend providing a status enum, rather than just is_omitted. The values could include PENDING, SENT, SKIPPED.

I believe ResultSets support slicing, so it might be clearer in the doctest to get a ResultSet and then slice it.

However, the branch as it stands is a worthwhile improvement, so I'll approve it.

review: Approve

Revision history for this message

Gary Poster (gary) wrote on 2011-02-16:

I have followed both of Aaron's suggestions, because I agreed that they were nicer. If Stuart prefers a bool after all (presumably because of space reasons), I can revert that bit pretty easily.

Thank you

Revision history for this message

Stuart Bishop (stub) wrote on 2011-02-17:

patch-2208-45-0.sql

Please add the following statement after the ALTER TABLE (all rows are going to be rewritten, so we want to repack the table):

CLUSTER BugNotification USING bugnotification__date_emailed__idx

Bug notification is regularly trimmed, so there are not too many rows to deal with - should be fast enough for a db patch.

I prefer status rather than the boolean too.

review: Approve (db)

Revision history for this message

Robert Collins (lifeless) on 2011-02-17:

review: Approve