launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02624
[Merge] lp:~gary/launchpad/bug164196-3 into lp:launchpad/db-devel
Gary Poster has proposed merging lp:~gary/launchpad/bug164196-3 into lp:launchpad/db-devel with lp:~gary/launchpad/bug164196-2 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#164196 Quickly-undone actions shouldn't send mail notifications
https://bugs.launchpad.net/bugs/164196
For more details, see:
https://code.launchpad.net/~gary/launchpad/bug164196-3/+merge/49980
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
--
https://code.launchpad.net/~gary/launchpad/bug164196-3/+merge/49980
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug164196-3 into lp:launchpad/db-devel.
=== modified file 'cronscripts/send-bug-notifications.py'
--- cronscripts/send-bug-notifications.py 2010-11-08 12:52:43 +0000
+++ cronscripts/send-bug-notifications.py 2011-02-16 14:58:08 +0000
@@ -30,7 +30,9 @@
notifications_sent = False
pending_notifications = get_email_notifications(getUtility(
IBugNotificationSet).getNotificationsToSend())
- for bug_notifications, messages in pending_notifications:
+ for (bug_notifications,
+ omitted_notifications,
+ messages) in pending_notifications:
for message in messages:
self.logger.info("Notifying %s about bug %d." % (
message['To'], bug_notifications[0].bug.id))
@@ -38,6 +40,9 @@
self.logger.debug(message.as_string())
for notification in bug_notifications:
notification.date_emailed = UTC_NOW
+ for notification in omitted_notifications:
+ notification.date_emailed = UTC_NOW
+ notification.is_omitted = True
notifications_sent = True
# Commit after each batch of email sent, so that we won't
# re-mail the notifications in case of something going wrong
=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql 2011-02-16 14:58:06 +0000
+++ database/schema/comments.sql 2011-02-16 14:58:08 +0000
@@ -281,6 +281,7 @@
COMMENT ON COLUMN BugNotification.is_comment IS 'Is the change a comment addition.';
COMMENT ON COLUMN BugNotification.date_emailed IS 'When this notification was emailed to the bug subscribers.';
COMMENT ON COLUMN BugNotification.activity IS 'The BugActivity record corresponding to this notification, if any.';
+COMMENT ON COLUMN BugNotification.is_omitted IS 'Was this notification omitted when emails were sent? Ignore if date_emailed is not yet set. This is only intended to be useful for debugging purposes.';
-- BugNotificationAttachment
=== added file 'database/schema/patch-2208-99-1.sql'
--- database/schema/patch-2208-99-1.sql 1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-99-1.sql 2011-02-16 14:58:08 +0000
@@ -0,0 +1,8 @@
+-- Copyright 2011 Canonical Ltd. This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+SET client_min_messages=ERROR;
+
+ALTER TABLE BugNotification
+ ADD COLUMN is_omitted BOOLEAN NOT NULL DEFAULT False;
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 99, 1);
=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt 2011-02-16 14:58:06 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt 2011-02-16 14:58:08 +0000
@@ -63,7 +63,7 @@
>>> from lp.bugs.scripts.bugnotification import (
... get_email_notifications)
>>> email_notifications = get_email_notifications(notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
@@ -178,7 +178,7 @@
>>> pending_notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
@@ -224,7 +224,7 @@
2
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
@@ -278,7 +278,7 @@
in the order they were added:
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
@@ -374,7 +374,7 @@
1
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print message['To']
foo.bar@xxxxxxxxxxxxx
@@ -417,7 +417,7 @@
>>> len(notifications)
1
- >>> for bug_notifications, messages in (
+ >>> for bug_notifications, omitted, messages in (
... get_email_notifications(notifications)):
... for message in messages:
... print_notification(message)
@@ -487,7 +487,7 @@
>>> notifications = (
... getUtility(IBugNotificationSet).getNotificationsToSend())
>>> email_notifications = get_email_notifications(notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: support@xxxxxxxxxx
@@ -511,7 +511,7 @@
>>> notifications = (
... getUtility(IBugNotificationSet).getNotificationsToSend())
>>> email_notifications = get_email_notifications(notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: support@xxxxxxxxxx
@@ -558,11 +558,19 @@
... BugTitleChange(
... ten_minutes_ago, sample_person, "title",
... "Old summary", "New summary"))
+ >>> bug_two.addChange(
+ ... BugVisibilityChange(
+ ... ten_minutes_ago, sample_person, "title",
+ ... False, True))
+ >>> bug_two.addChange(
+ ... BugVisibilityChange(
+ ... ten_minutes_ago, sample_person, "title",
+ ... True, False))
>>> notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> len(notifications)
- 6
+ 8
We need to commit the transaction so that the cronscript will see the
notifications.
@@ -650,7 +658,42 @@
INFO Notifying test@xxxxxxxxxxxxx about bug 1.
...
- >>> flush_notifications()
+Note that the message omitted the undone visibility change.
+
+The cronscript has to be sure to mark all notifications, omitted and
+otherwise, as sent. It also marks the omitted notifications with a flag,
+so if there are any problems we can identify which notifications were omitted
+during analysis. We'll commit a transaction to synchronize the database,
+and then look at the notifications available.
+
+ >>> transaction.commit()
+
+ >>> notifications = getUtility(
+ ... IBugNotificationSet).getNotificationsToSend()
+ >>> len(notifications)
+ 0
+
+They have all been marked as sent, including the omitted ones. Let's look
+more carefully at the notifications just to see that the is_omitted flag has
+been set properly.
+
+ >>> from lp.bugs.model.bugnotification import BugNotification
+ >>> notifications = list(BugNotification.select(orderBy='-id', limit=8))
+ >>> notifications.reverse()
+ >>> for notification in notifications:
+ ... if notification.is_comment:
+ ... identifier = 'comment'
+ ... else:
+ ... identifier = notification.activity.whatchanged
+ ... print identifier, notification.is_omitted
+ comment False
+ summary False
+ comment False
+ summary False
+ comment False
+ summary False
+ visibility True
+ visibility True
The X-Launchpad-Bug header
@@ -675,7 +718,7 @@
X-Launchpad-Bug headers were added:
>>> email_notifications = get_email_notifications(notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... sorted(message.get_all('X-Launchpad-Bug'))
[u'distribution=debian; distroseries=sarge;... milestone=3.1;...',
@@ -709,7 +752,7 @@
>>> def get_email_messages(notifications):
... messages = (message
- ... for bug_notifications, messages in
+ ... for bug_notifications, omitted, messages in
... get_email_notifications(notifications)
... for message in messages)
... return sorted(messages, key=lambda message: message['To'])
@@ -964,7 +1007,7 @@
>>> from itertools import chain
>>> collated_messages = collate_messages_by_recipient(
- ... chain(*(messages for bug_notifications, messages in
+ ... chain(*(messages for bug_notifications, omitted, messages in
... get_email_notifications(notifications))))
We can see that Concise Person doesn't receive verbose notifications:
@@ -1147,7 +1190,7 @@
>>> pending_notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
@@ -1208,7 +1251,7 @@
>>> pending_notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
@@ -1267,7 +1310,7 @@
>>> pending_notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
@@ -1318,7 +1361,7 @@
>>> pending_notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
@@ -1382,7 +1425,7 @@
>>> pending_notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
@@ -1446,7 +1489,7 @@
>>> pending_notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> email_notifications = get_email_notifications(pending_notifications)
- >>> for bug_notifications, messages in email_notifications:
+ >>> for bug_notifications, omitted, messages in email_notifications:
... for message in messages:
... print_notification(message)
To: foo.bar@xxxxxxxxxxxxx
=== modified file 'lib/lp/bugs/doc/bugnotification-threading.txt'
--- lib/lp/bugs/doc/bugnotification-threading.txt 2011-02-02 17:33:21 +0000
+++ lib/lp/bugs/doc/bugnotification-threading.txt 2011-02-16 14:58:08 +0000
@@ -35,7 +35,7 @@
>>> notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
- >>> messages = [emails for dummy, emails in
+ >>> messages = [emails for notifications, omitted, emails in
... get_email_notifications(notifications)]
>>> len(messages)
1
@@ -73,7 +73,7 @@
... True, False))
>>> notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
- >>> messages = [emails for dummy, emails in
+ >>> messages = [emails for notifications, omitted, emails in
... get_email_notifications(notifications)]
>>> len(messages)
1
@@ -106,7 +106,7 @@
>>> notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
- >>> messages = [emails for dummy, emails in
+ >>> messages = [emails for notifications, omitted, emails in
... get_email_notifications(notifications)]
>>> len(messages)
1
=== modified file 'lib/lp/bugs/interfaces/bugnotification.py'
--- lib/lp/bugs/interfaces/bugnotification.py 2011-02-16 14:58:06 +0000
+++ lib/lp/bugs/interfaces/bugnotification.py 2011-02-16 14:58:08 +0000
@@ -51,6 +51,12 @@
required=False)
recipients = Attribute(
"The people to which this notification should be sent.")
+ is_omitted = Bool(
+ title=u"Omitted", description=(
+ u"Was this notification omitted when emails were sent? Ignore "
+ "if date_emailed is not yet set. Only intended to be useful "
+ "for debugging purposes."),
+ required=True)
class IBugNotificationSet(Interface):
=== modified file 'lib/lp/bugs/model/bugnotification.py'
--- lib/lp/bugs/model/bugnotification.py 2011-02-16 14:58:06 +0000
+++ lib/lp/bugs/model/bugnotification.py 2011-02-16 14:58:08 +0000
@@ -49,6 +49,7 @@
bug = ForeignKey(dbName='bug', notNull=True, foreignKey='Bug')
is_comment = BoolCol(notNull=True)
date_emailed = UtcDateTimeCol(notNull=False)
+ is_omitted = BoolCol(notNull=True)
@property
def recipients(self):
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py 2011-02-16 14:58:06 +0000
+++ lib/lp/bugs/scripts/bugnotification.py 2011-02-16 14:58:08 +0000
@@ -83,6 +83,7 @@
recipients = {}
filtered_notifications = []
+ omitted_notifications = []
for notification in bug_notifications:
key = get_key(notification)
if (notification.is_comment or
@@ -97,6 +98,8 @@
email_people.remove(actor)
for email_person in email_people:
recipients[email_person] = recipient
+ else:
+ omitted_notifications.append(notification)
if bug.duplicateof is not None:
text_notifications.append(
@@ -189,7 +192,7 @@
rationale, references, msgid)
messages.append(msg)
- return filtered_notifications, messages
+ return filtered_notifications, omitted_notifications, messages
def notification_comment_batches(notifications):
=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-02-16 14:58:06 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-02-16 14:58:08 +0000
@@ -185,7 +185,7 @@
email_notifications = get_email_notifications(notifications_to_send)
to_addresses = set()
sent_notifications = []
- for notifications, messages in email_notifications:
+ for notifications, omitted, messages in email_notifications:
for message in messages:
to_addresses.add(message['to'])
recipients = {}
@@ -523,7 +523,9 @@
def get_messages(self):
notifications = self.notification_set.getNotificationsToSend()
email_notifications = get_email_notifications(notifications)
- for bug_notifications, messages in email_notifications:
+ for (bug_notifications,
+ omitted_notifications,
+ messages) in email_notifications:
for message in messages:
yield message, message.get_payload(decode=True)
