← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~allenap/launchpad/refactor-get-email-notifications into lp:launchpad/devel

 

Gavin Panella has proposed merging lp:~allenap/launchpad/refactor-get-email-notifications into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This refactors the get_email_notifications() to make sense :) I was working in the same file and got a little carried away with fixing it, so split it off into a separate branch.
-- 
https://code.launchpad.net/~allenap/launchpad/refactor-get-email-notifications/+merge/32922
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/refactor-get-email-notifications into lp:launchpad/devel.
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2010-06-23 09:36:02 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2010-08-17 20:01:44 +0000
@@ -7,20 +7,30 @@
 
 __metaclass__ = type
 
+__all__ = [
+    "construct_email_notifications",
+    "get_email_notifications",
+    ]
+
+from itertools import groupby
+from operator import attrgetter, itemgetter
+
+import transaction
+
 from zope.component import getUtility
 
 from canonical.config import config
-from canonical.database.sqlbase import rollback, begin
 from canonical.launchpad.helpers import emailPeople, get_email_template
-from lp.bugs.interfaces.bugmessage import IBugMessageSet
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
-from lp.registry.interfaces.person import IPersonSet
 from canonical.launchpad.mailnotification import (
-    generate_bug_add_email, MailWrapper)
+    MailWrapper, generate_bug_add_email)
+from canonical.launchpad.scripts.logger import log
+from canonical.launchpad.webapp import canonical_url
+
+from lp.bugs.interfaces.bugmessage import IBugMessageSet
 from lp.bugs.mail.bugnotificationbuilder import (
     BugNotificationBuilder, get_bugmail_from_address)
-from canonical.launchpad.scripts.logger import log
-from canonical.launchpad.webapp import canonical_url
+from lp.registry.interfaces.person import IPersonSet
 
 
 def construct_email_notifications(bug_notifications):
@@ -162,19 +172,35 @@
 
     return bug_notifications, messages
 
-def _log_exception_and_restart_transaction():
-    """Log an exception and restart the current transaction.
-
-    It's important to restart the transaction if an exception occurs,
-    since if it's a DB exception, the transaction isn't usable anymore.
+
+def notification_comment_batches(notifications):
+    """Search `notification` for continuous spans with only one comment.
+
+    Generates `comment_group, notification` tuples.
+
+    The notifications are searched in order for continuous spans containing
+    only one comment. Each continous span is given a unique number. Each
+    notification is yielded along with its span number.
     """
-    log.exception(
-        "An exception was raised while building the email notification.")
-    rollback()
-    begin()
-
-
-def get_email_notifications(bug_notifications, date_emailed=None):
+    comment_count = 0
+    for notification in notifications:
+        if notification.is_comment:
+            comment_count += 1
+        # Everything before the 2nd comment is in the first comment group.
+        yield comment_count or 1, notification
+
+
+def notification_batches(notifications):
+    """Batch notifications for `get_email_notifications`."""
+    notifications_grouped = groupby(
+        notifications, attrgetter("bug", "message.owner"))
+    for (bug, person), notification_group in notifications_grouped:
+        batches = notification_comment_batches(notification_group)
+        for comment_group, batch in groupby(batches, itemgetter(0)):
+            yield [notification for (comment_group, notification) in batch]
+
+
+def get_email_notifications(bug_notifications):
     """Return the email notifications pending to be sent.
 
     The intention of this code is to ensure that as many notifications
@@ -184,44 +210,14 @@
         - Must be related to the same bug.
         - Must contain at most one comment.
     """
-    # Avoid spurious lint about possibly undefined loop variables.
-    notification = None
-    # Copy bug_notifications because we will modify it as we go.
-    bug_notifications = list(bug_notifications)
-    while bug_notifications:
-        found_comment = False
-        notification_batch = []
-        bug = bug_notifications[0].bug
-        person = bug_notifications[0].message.owner
-        # What the loop below does is find the largest contiguous set of
-        # bug notifications as specified above.
-        #
-        # Note that we iterate over a copy of the notifications here
-        # because we are modifying bug_modifications as we go.
-        for notification in list(bug_notifications):
-            if notification.is_comment and found_comment:
-                # Oops, found a second comment, stop batching.
-                break
-            if notification.bug != bug:
-                # Found a different change, stop batching.
-                break
-            if notification.message.owner != person:
-                # Ah, we've found a change made by somebody else; time
-                # to stop batching too.
-                break
-            notification_batch.append(notification)
-            bug_notifications.remove(notification)
-            if notification.is_comment:
-                found_comment = True
-
-        if date_emailed is not None:
-            notification.date_emailed = date_emailed
+    for batch in notification_batches(bug_notifications):
         # We don't want bugs preventing all bug notifications from
         # being sent, so catch and log all exceptions.
         try:
-            yield construct_email_notifications(notification_batch)
+            yield construct_email_notifications(batch)
         except (KeyboardInterrupt, SystemExit):
             raise
         except:
-            _log_exception_and_restart_transaction()
-
+            log.exception("Error while building email notifications.")
+            transaction.abort()
+            transaction.begin()

=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py	2010-06-16 08:20:23 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2010-08-17 20:01:44 +0000
@@ -18,13 +18,15 @@
 from canonical.launchpad.database import BugTask
 from canonical.launchpad.helpers import get_contact_email_addresses
 from canonical.launchpad.interfaces.message import IMessageSet
+from canonical.testing import LaunchpadZopelessLayer
+
 from lp.bugs.interfaces.bug import IBug, IBugSet
+from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
+from lp.bugs.scripts.bugnotification import (
+    get_email_notifications, notification_batches,
+    notification_comment_batches)
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
-from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
-from lp.bugs.scripts.bugnotification import (
-    get_email_notifications)
-from canonical.testing import LaunchpadZopelessLayer
 
 
 class MockBug:
@@ -94,6 +96,7 @@
     Using a real BugNotification won't allow us to set the bug to a mock
     object.
     """
+
     def __init__(self, message, bug, is_comment, date_emailed):
         self.message = message
         self.bug = bug
@@ -164,8 +167,7 @@
         also checks that the notifications got sent to the correct
         addresses.
         """
-        email_notifications = get_email_notifications(
-            notifications_to_send, date_emailed=self.now)
+        email_notifications = get_email_notifications(notifications_to_send)
         to_addresses = set()
         sent_notifications = []
         for notifications, messages in email_notifications:
@@ -194,7 +196,6 @@
             notifications_to_send)
         self.assertEqual(sent_notifications, notifications_to_send)
 
-
     def test_catch_simple_exception_in_the_middle(self):
         # Make sure that the first and last notifications are sent even
         # if the middle one causes an exception to be raised.
@@ -248,5 +249,222 @@
         self.assertEqual(bug_four.id, 4)
 
 
+class FakeNotification:
+    """Used by TestNotificationCommentBatches and TestNotificationBatches."""
+
+    class Message(object):
+        pass
+
+    def __init__(self, is_comment=False, bug=None, owner=None):
+        self.is_comment = is_comment
+        self.bug = bug
+        self.message = self.Message()
+        self.message.owner = owner
+
+
+class TestNotificationCommentBatches(unittest.TestCase):
+    """Tests of `notification_comment_batches`."""
+
+    def test_with_nothing(self):
+        # Nothing is generated if an empty list is passed in.
+        self.assertEquals([], list(notification_comment_batches([])))
+
+    def test_with_one_non_comment_notification(self):
+        # Given a single non-comment notification, a single tuple is
+        # generated.
+        notification = FakeNotification(False)
+        self.assertEquals(
+            [(1, notification)],
+            list(notification_comment_batches([notification])))
+
+    def test_with_one_comment_notification(self):
+        # Given a single comment notification, a single tuple is generated.
+        notification = FakeNotification(True)
+        self.assertEquals(
+            [(1, notification)],
+            list(notification_comment_batches([notification])))
+
+    def test_with_two_notifications_comment_first(self):
+        # Given two notifications, one a comment, one not, and the comment
+        # first, two tuples are generated, both in the same group.
+        notification1 = FakeNotification(True)
+        notification2 = FakeNotification(False)
+        notifications = [notification1, notification2]
+        self.assertEquals(
+            [(1, notification1), (1, notification2)],
+            list(notification_comment_batches(notifications)))
+
+    def test_with_two_notifications_comment_last(self):
+        # Given two notifications, one a comment, one not, and the comment
+        # last, two tuples are generated, both in the same group.
+        notification1 = FakeNotification(False)
+        notification2 = FakeNotification(True)
+        notifications = [notification1, notification2]
+        self.assertEquals(
+            [(1, notification1), (1, notification2)],
+            list(notification_comment_batches(notifications)))
+
+    def test_with_three_notifications_comment_in_middle(self):
+        # Given three notifications, one a comment, two not, and the comment
+        # in the middle, three tuples are generated, all in the same group.
+        notification1 = FakeNotification(False)
+        notification2 = FakeNotification(True)
+        notification3 = FakeNotification(False)
+        notifications = [notification1, notification2, notification3]
+        self.assertEquals(
+            [(1, notification1), (1, notification2), (1, notification3)],
+            list(notification_comment_batches(notifications)))
+
+    def test_with_more_notifications(self):
+        # Given four notifications - non-comment, comment, non-comment,
+        # comment - four tuples are generated. The first three notifications
+        # are in the first group, the last notification is in a group on its
+        # own.
+        notification1 = FakeNotification(False)
+        notification2 = FakeNotification(True)
+        notification3 = FakeNotification(False)
+        notification4 = FakeNotification(True)
+        notifications = [
+            notification1, notification2,
+            notification3, notification4,
+            ]
+        self.assertEquals(
+            [(1, notification1), (1, notification2),
+             (1, notification3), (2, notification4)],
+            list(notification_comment_batches(notifications)))
+
+
+class TestNotificationBatches(unittest.TestCase):
+    """Tests of `notification_batches`."""
+
+    def test_with_nothing(self):
+        # Nothing is generated if an empty list is passed in.
+        self.assertEquals([], list(notification_batches([])))
+
+    def test_with_one_non_comment_notification(self):
+        # Given a single non-comment notification, a single batch is
+        # generated.
+        notification = FakeNotification(False)
+        self.assertEquals(
+            [[notification]],
+            list(notification_batches([notification])))
+
+    def test_with_one_comment_notification(self):
+        # Given a single comment notification, a single batch is generated.
+        notification = FakeNotification(True)
+        self.assertEquals(
+            [[notification]],
+            list(notification_batches([notification])))
+
+    def test_with_two_notifications_comment_first(self):
+        # Given two similar notifications, one a comment, one not, and the
+        # comment first, a single batch is generated.
+        notification1 = FakeNotification(True)
+        notification2 = FakeNotification(False)
+        notifications = [notification1, notification2]
+        self.assertEquals(
+            [[notification1, notification2]],
+            list(notification_batches(notifications)))
+
+    def test_with_two_notifications_comment_last(self):
+        # Given two similar notifications, one a comment, one not, and the
+        # comment last, a single batch is generated.
+        notification1 = FakeNotification(False)
+        notification2 = FakeNotification(True)
+        notifications = [notification1, notification2]
+        self.assertEquals(
+            [[notification1, notification2]],
+            list(notification_batches(notifications)))
+
+    def test_with_three_notifications_comment_in_middle(self):
+        # Given three similar notifications, one a comment, two not, and the
+        # comment in the middle, one batch is generated.
+        notification1 = FakeNotification(False)
+        notification2 = FakeNotification(True)
+        notification3 = FakeNotification(False)
+        notifications = [notification1, notification2, notification3]
+        self.assertEquals(
+            [[notification1, notification2, notification3]],
+            list(notification_batches(notifications)))
+
+    def test_with_more_notifications(self):
+        # Given four similar notifications - non-comment, comment,
+        # non-comment, comment - two batches are generated. The first three
+        # notifications are in the first batch.
+        notification1 = FakeNotification(False)
+        notification2 = FakeNotification(True)
+        notification3 = FakeNotification(False)
+        notification4 = FakeNotification(True)
+        notifications = [
+            notification1, notification2,
+            notification3, notification4,
+            ]
+        self.assertEquals(
+            [[notification1, notification2, notification3], [notification4]],
+            list(notification_batches(notifications)))
+
+    def test_notifications_for_same_bug(self):
+        # Batches are grouped by bug.
+        notifications = [FakeNotification(bug=1) for number in range(5)]
+        observed = list(notification_batches(notifications))
+        self.assertEquals([notifications], observed)
+
+    def test_notifications_for_different_bugs(self):
+        # Batches are grouped by bug.
+        notifications = [FakeNotification(bug=number) for number in range(5)]
+        expected = [[notification] for notification in notifications]
+        observed = list(notification_batches(notifications))
+        self.assertEquals(expected, observed)
+
+    def test_notifications_for_same_owner(self):
+        # Batches are grouped by owner.
+        notifications = [FakeNotification(owner=1) for number in range(5)]
+        observed = list(notification_batches(notifications))
+        self.assertEquals([notifications], observed)
+
+    def test_notifications_for_different_owners(self):
+        # Batches are grouped by owner.
+        notifications = [FakeNotification(owner=number) for number in range(5)]
+        expected = [[notification] for notification in notifications]
+        observed = list(notification_batches(notifications))
+        self.assertEquals(expected, observed)
+
+    def test_notifications_with_mixed_bugs_and_owners(self):
+        # Batches are grouped by bug and owner.
+        notifications = [
+            FakeNotification(bug=1, owner=1),
+            FakeNotification(bug=1, owner=2),
+            FakeNotification(bug=2, owner=2),
+            FakeNotification(bug=2, owner=1),
+            ]
+        expected = [[notification] for notification in notifications]
+        observed = list(notification_batches(notifications))
+        self.assertEquals(expected, observed)
+
+    def test_notifications_with_mixed_bugs_and_owners_2(self):
+        # Batches are grouped by bug and owner.
+        notifications = [
+            FakeNotification(bug=1, owner=1),
+            FakeNotification(bug=1, owner=1),
+            FakeNotification(bug=2, owner=2),
+            FakeNotification(bug=2, owner=2),
+            ]
+        expected = [notifications[0:2], notifications[2:4]]
+        observed = list(notification_batches(notifications))
+        self.assertEquals(expected, observed)
+
+    def test_notifications_with_mixed_bugs_owners_and_comments(self):
+        # Batches are grouped by bug, owner and comments.
+        notifications = [
+            FakeNotification(is_comment=False, bug=1, owner=1),
+            FakeNotification(is_comment=False, bug=1, owner=1),
+            FakeNotification(is_comment=True, bug=1, owner=1),
+            FakeNotification(is_comment=False, bug=1, owner=2),
+            ]
+        expected = [notifications[0:3], notifications[3:4]]
+        observed = list(notification_batches(notifications))
+        self.assertEquals(expected, observed)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)