launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00645
[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__)
