Merge lp:~gary/launchpad/bug164196-3 into lp:launchpad/db-devel

[Gary Poster]

[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.

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

[Aaron Bentley (abentley)]

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

[Gary Poster (gary)]

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

[Stuart Bishop (stub)]

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)

[Robert Collins (lifeless)]

review: Approve

=== 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-17 17:43:44 +0000
@@ -20,6 +20,7 @@
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
 from canonical.launchpad.mail import sendmail
+from lp.bugs.enum import BugNotificationStatus
 from lp.bugs.interfaces.bugnotification import IBugNotificationSet
 from lp.bugs.scripts.bugnotification import get_email_notifications
 from lp.services.scripts.base import LaunchpadCronScript
@@ -30,7 +31,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 +41,10 @@
                 self.logger.debug(message.as_string())
             for notification in bug_notifications:
                 notification.date_emailed = UTC_NOW
+                notification.status = BugNotificationStatus.SENT
+            for notification in omitted_notifications:
+                notification.date_emailed = UTC_NOW
+                notification.status = BugNotificationStatus.OMITTED
             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-17 17:43:43 +0000
+++ database/schema/comments.sql	2011-02-17 17:43:44 +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.status IS 'The status of this bug notification: pending, omitted, or sent.';
 
 
 -- BugNotificationAttachment
 
=== added file 'database/schema/patch-2208-45-0.sql'
--- database/schema/patch-2208-45-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-45-0.sql	2011-02-17 17:43:44 +0000
@@ -0,0 +1,12 @@
+-- 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;
+
+-- The default value for status can be found in the
+-- BugNotificationStatus DBEnum in lib/lp/bugs/enum.py.
+ALTER TABLE BugNotification
+    ADD COLUMN status INTEGER NOT NULL DEFAULT 10;
+
+CLUSTER BugNotification USING bugnotification__date_emailed__idx;
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 45, 0);
 
=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt	2011-02-17 17:43:43 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt	2011-02-17 17:43:44 +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: [e-mail]
@@ -176,7 +176,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: [e-mail]
@@ -222,7 +222,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: [e-mail]
@@ -275,7 +275,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: [e-mail]
@@ -371,7 +371,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']
     [e-mail]
@@ -414,7 +414,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)
@@ -485,7 +485,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: [e-mail]
@@ -509,7 +509,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: [e-mail]
@@ -556,11 +556,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.
@@ -648,7 +656,41 @@
     INFO    Notifying [e-mail] 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 status,
+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 status has
+been set properly.
+
+    >>> from lp.bugs.model.bugnotification import BugNotification
+    >>> from lp.bugs.enum import BugNotificationStatus
+    >>> for notification in BugNotification.select(orderBy='id')[-8:]:
+    ...     if notification.is_comment:
+    ...         identifier = 'comment'
+    ...     else:
+    ...         identifier = notification.activity.whatchanged
+    ...     print identifier, notification.status.title
+    comment Sent
+    summary Sent
+    comment Sent
+    summary Sent
+    comment Sent
+    summary Sent
+    visibility Omitted
+    visibility Omitted
 
 
 The X-Launchpad-Bug header
@@ -673,7 +715,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;...',
@@ -707,7 +749,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'])
@@ -963,7 +1005,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:
@@ -1151,7 +1193,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: [e-mail]
@@ -1212,7 +1254,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: [e-mail]
@@ -1271,7 +1313,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: [e-mail]
@@ -1322,7 +1364,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: [e-mail]
@@ -1386,7 +1428,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: [e-mail]
@@ -1448,7 +1490,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: [e-mail]
 
=== 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-17 17:43:44 +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/enum.py'
--- lib/lp/bugs/enum.py	2011-02-01 17:52:45 +0000
+++ lib/lp/bugs/enum.py	2011-02-17 17:43:44 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'BugNotificationLevel',
+    'BugNotificationStatus',
     ]
 
 from lazr.enum import (
@@ -47,3 +48,29 @@
         notifications about new events in the bugs's discussion, like new
         comments.
         """)
+
+
+class BugNotificationStatus(DBEnumeratedType):
+    """The status of a bug notification.
+    
+    A notification may be pending, sent, or omitted."""
+
+    PENDING = DBItem(10, """
+        Pending
+        
+        The notification has not yet been sent.
+        """)
+
+    OMITTED = DBItem(20, """
+        Omitted
+        
+        The system considered sending the notification, but omitted it.
+        This is generally because the action reported by the notification
+        was immediately undone.
+        """)
+
+    SENT = DBItem(30, """
+        Sent
+        
+        The notification has been sent.
+        """)
 
=== modified file 'lib/lp/bugs/interfaces/bugnotification.py'
--- lib/lp/bugs/interfaces/bugnotification.py	2011-02-17 17:43:43 +0000
+++ lib/lp/bugs/interfaces/bugnotification.py	2011-02-17 17:43:44 +0000
@@ -18,11 +18,13 @@
     )
 from zope.schema import (
     Bool,
+    Choice,
     Datetime,
     TextLine,
     )
 
 from canonical.launchpad import _
+from lp.bugs.enum import BugNotificationStatus
 from lp.registry.interfaces.role import IHasOwner
 from lp.services.fields import BugField
 
@@ -51,6 +53,13 @@
         required=False)
     recipients = Attribute(
         "The people to which this notification should be sent.")
+    status = Choice(
+            title=_("Status"), required=True,
+            vocabulary=BugNotificationStatus,
+            default=BugNotificationStatus.PENDING,
+            description=_(
+                "The status of this bug notification."),
+            )
 
 
 class IBugNotificationSet(Interface):
 
=== modified file 'lib/lp/bugs/mail/tests/test_bug_task_assignment.py'
--- lib/lp/bugs/mail/tests/test_bug_task_assignment.py	2010-12-21 19:11:49 +0000
+++ lib/lp/bugs/mail/tests/test_bug_task_assignment.py	2011-02-17 17:43:44 +0000
@@ -90,7 +90,7 @@
             self.bug_task, self.bug_task_before_modification,
             ['assignee'], user=self.user))
         latest_notification = BugNotification.selectFirst(orderBy='-id')
-        notifications, messages = construct_email_notifications(
+        notifications, omitted, messages = construct_email_notifications(
             [latest_notification])
         self.assertEqual(len(notifications), 1,
                          'email notication not created')
@@ -107,7 +107,7 @@
             self.bug_task, self.bug_task_before_modification,
             ['assignee'], user=self.user))
         latest_notification = BugNotification.selectFirst(orderBy='-id')
-        notifications, messages = construct_email_notifications(
+        notifications, omitted, messages = construct_email_notifications(
             [latest_notification])
         self.assertEqual(len(notifications), 1,
                          'email notification not created')
 
=== modified file 'lib/lp/bugs/mail/tests/test_bug_task_modification.py'
--- lib/lp/bugs/mail/tests/test_bug_task_modification.py	2010-10-04 19:50:45 +0000
+++ lib/lp/bugs/mail/tests/test_bug_task_modification.py	2011-02-17 17:43:44 +0000
@@ -44,7 +44,7 @@
             ['status'], user=self.user))
         transaction.commit()
         latest_notification = BugNotification.selectFirst(orderBy='-id')
-        notifications, messages = construct_email_notifications(
+        notifications, omitted, messages = construct_email_notifications(
             [latest_notification])
         self.assertEqual(len(notifications), 1,
                          'email notification not created')
 
=== modified file 'lib/lp/bugs/model/bugnotification.py'
--- lib/lp/bugs/model/bugnotification.py	2011-02-17 17:43:43 +0000
+++ lib/lp/bugs/model/bugnotification.py	2011-02-17 17:43:44 +0000
@@ -28,10 +28,12 @@
 
 from canonical.config import config
 from canonical.database.datetimecol import UtcDateTimeCol
+from canonical.database.enumcol import EnumCol
 from canonical.database.sqlbase import (
     SQLBase,
     sqlvalues,
     )
+from lp.bugs.enum import BugNotificationStatus
 from lp.bugs.interfaces.bugnotification import (
     IBugNotification,
     IBugNotificationRecipient,
@@ -49,6 +51,10 @@
     bug = ForeignKey(dbName='bug', notNull=True, foreignKey='Bug')
     is_comment = BoolCol(notNull=True)
     date_emailed = UtcDateTimeCol(notNull=False)
+    status = EnumCol(
+        dbName='status',
+        schema=BugNotificationStatus, default=BugNotificationStatus.PENDING,
+        notNull=True)
 
     @property
     def recipients(self):
 
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2011-02-17 17:43:43 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2011-02-17 17:43:44 +0000
@@ -97,6 +97,7 @@
 
     recipients = {}
     filtered_notifications = []
+    omitted_notifications = []
     for notification in bug_notifications:
         key = get_activity_key(notification)
         if (notification.is_comment or
@@ -111,6 +112,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(
@@ -203,7 +206,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-17 17:43:43 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2011-02-17 17:43:44 +0000
@@ -248,7 +248,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 = {}
@@ -572,7 +572,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)
 
 
=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2011-02-17 17:43:43 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2011-02-17 17:43:44 +0000
@@ -183,7 +183,8 @@
 
     def assertRecipients(self, expected_recipients):
         notifications = self.getNewNotifications()
-        notifications, messages = construct_email_notifications(notifications)
+        notifications, omitted, messages = construct_email_notifications(
+            notifications)
         recipients = set(message['to'] for message in messages)
 
         self.assertEqual(