launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03173
[Merge] lp:~gary/launchpad/bug741684-3 into lp:launchpad
Gary Poster has proposed merging lp:~gary/launchpad/bug741684-3 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~gary/launchpad/bug741684-3/+merge/55935
This branch changes how we get filter information for each email recipient one more time. Now, instead of making a query per subscription source, we make a single query for an entire batched set of notifications. We then assemble the data into the structure we actually want per recipient.
Part of the structure of the new method, and how it is used, is coming from the fact that I'm preparing to add additional functionality to it in a db-devel branch, but I think it stands up well enough on its own.
Almost all the code in this branch is about that change, either directly or indirectly (tests).
The only exception is that lib/lp/bugs/scripts/bugnotification.py also contains some changes because I was tired about being confused about the "recipients" name. The notification.recipients collection is actually a collection of objects that describe the subscriptions that will be affected by the notification. Each "recipient" has more information than just the principal involved in the subscription (including the rationale, for instance) and also, in this script, the "recipients" are exploded out into the people who actually receive the emails. Therefore, while the "recipients" collection on each notification makes some sense as a name on the notification, it was getting confusing within the script itself. I decided to call these objects "sources" and the people who would actually receive the mail "recipients".
I would have preferred to have getRecipientFilterData be a function for simplicity, but in this case it was actually nice to have it on a utility so I could use dependency injection within one of the tests. That said, given that there's no machinery to automatically undo the utility registration, it is interesting to note that there's no practical advantage to this pattern over monkeypatching, which is a bit sad.
Thank you!
--
https://code.launchpad.net/~gary/launchpad/bug741684-3/+merge/55935
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug741684-3 into lp:launchpad.
=== modified file 'lib/lp/bugs/interfaces/bugnotification.py'
--- lib/lp/bugs/interfaces/bugnotification.py 2011-03-26 01:37:30 +0000
+++ lib/lp/bugs/interfaces/bugnotification.py 2011-04-01 14:01:14 +0000
@@ -81,8 +81,15 @@
`BugNotificationRecipient` objects.
"""
- def getFiltersByRecipient(notifications, person):
- """Return filters for a particular recipient."""
+ def getRecipientFilterData(recipient_to_sources, notifications):
+ """Get non-muted recipients mapped to sources & filter descriptions.
+
+ :param recipient_to_sources:
+ A dict of people who are to receive the email to the sources
+ (BugNotificationRecipients) that represent the subscriptions that
+ caused the notifications to be sent.
+ :param notifications: the notifications that are being communicated.
+ """
class IBugNotificationRecipient(Interface):
=== modified file 'lib/lp/bugs/model/bugnotification.py'
--- lib/lp/bugs/model/bugnotification.py 2011-03-30 23:15:11 +0000
+++ lib/lp/bugs/model/bugnotification.py 2011-04-01 14:01:14 +0000
@@ -177,8 +177,36 @@
VALUES %s;""" % ', '.join(sql_values))
return bug_notification
- def getFiltersByRecipient(self, notifications, recipient):
+ def getRecipientFilterData(self, recipient_to_sources, notifications):
"""See `IBugNotificationSet`."""
+ if not notifications or not recipient_to_sources:
+ # This is a shortcut that will remove some error conditions.
+ return {}
+ # This makes two calls to the database to get all the
+ # information we need. The first call gets the filter ids and
+ # descriptions for each recipient.
+ source_person_id_map = {}
+ recipient_id_map = {}
+ for recipient, sources in recipient_to_sources.items():
+ source_person_ids = set()
+ recipient_id_map[recipient.id] = {
+ 'principal': recipient,
+ 'filters': {},
+ 'source person ids': source_person_ids,
+ 'sources': sources,
+ }
+ for source in sources:
+ person_id = source.person.id
+ source_person_ids.add(person_id)
+ data = source_person_id_map.get(person_id)
+ if data is None:
+ # The "filters" key is the only one we actually use. The
+ # rest are useful for debugging and introspecting.
+ data = {'sources': set(),
+ 'person': source.person,
+ 'filters': {}}
+ source_person_id_map[person_id] = data
+ data['sources'].add(source)
store = IStore(BugSubscriptionFilter)
source = store.using(
BugSubscriptionFilter,
@@ -187,15 +215,36 @@
BugNotificationFilter.bug_subscription_filter_id),
Join(StructuralSubscription,
BugSubscriptionFilter.structural_subscription_id ==
- StructuralSubscription.id),
- Join(TeamParticipation,
- TeamParticipation.teamID ==
- StructuralSubscription.subscriberID))
- return source.find(
- BugSubscriptionFilter,
+ StructuralSubscription.id))
+ filter_data = source.find(
+ (StructuralSubscription.subscriberID,
+ BugSubscriptionFilter.id,
+ BugSubscriptionFilter._description),
In(BugNotificationFilter.bug_notification_id,
[notification.id for notification in notifications]),
- TeamParticipation.personID == recipient.id)
+ In(StructuralSubscription.subscriberID,
+ source_person_id_map.keys()))
+ filter_ids = []
+ for source_person_id, filter_id, filter_description in filter_data:
+ source_person_id_map[source_person_id]['filters'][filter_id] = (
+ filter_description)
+ filter_ids.append(filter_id)
+ for recipient_data in recipient_id_map.values():
+ for source_person_id in recipient_data['source person ids']:
+ recipient_data['filters'].update(
+ source_person_id_map[source_person_id]['filters'])
+ # Now recipient_id_map has all the information we need. Let's build the
+ # final result.
+ result = {}
+ for recipient_data in recipient_id_map.values():
+ filter_descriptions = [
+ description for description
+ in recipient_data['filters'].values() if description]
+ filter_descriptions.sort() # This is good for tests.
+ result[recipient_data['principal']] = {
+ 'sources': recipient_data['sources'],
+ 'filter descriptions': filter_descriptions}
+ return result
class BugNotificationRecipient(SQLBase):
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py 2011-03-31 19:10:35 +0000
+++ lib/lp/bugs/scripts/bugnotification.py 2011-04-01 14:01:14 +0000
@@ -105,14 +105,25 @@
old_values[key] != new_values[key]):
# We will report this notification.
filtered_notifications.append(notification)
- for recipient in notification.recipients:
- email_people = get_recipients(recipient.person)
- for email_person in email_people:
- recipients_for_person = recipients.get(email_person)
- if recipients_for_person is None:
- recipients_for_person = []
- recipients[email_person] = recipients_for_person
- recipients_for_person.append(recipient)
+ for subscription_source in notification.recipients:
+ for recipient in get_recipients(
+ subscription_source.person):
+ # The subscription_source.person may be a person or a
+ # team. The get_recipients function gives us everyone
+ # who should actually get an email for that person.
+ # If subscription_source.person is a person or a team
+ # with a preferred email address, then the people to
+ # be emailed will only be subscription_source.person.
+ # However, if it is a team without a preferred email
+ # address, then this list will be the people and teams
+ # that comprise the team, transitively, stopping the walk
+ # at each person and at each team with a preferred email
+ # address.
+ sources_for_person = recipients.get(recipient)
+ if sources_for_person is None:
+ sources_for_person = []
+ recipients[recipient] = sources_for_person
+ sources_for_person.append(subscription_source)
else:
omitted_notifications.append(notification)
@@ -162,45 +173,24 @@
content = '\n\n'.join(text_notifications)
from_address = get_bugmail_from_address(actor, bug)
bug_notification_builder = BugNotificationBuilder(bug, actor)
+ recipients = getUtility(IBugNotificationSet).getRecipientFilterData(
+ recipients, filtered_notifications)
sorted_recipients = sorted(
recipients.items(), key=lambda t: t[0].preferredemail.email)
- cached_filters = {}
-
- bug_notification_set = getUtility(IBugNotificationSet)
-
- def get_filter_descriptions(recipients):
- "Given a list of recipients, return the filter descriptions for them."
- descriptions = set()
- people_seen = set()
- for recipient in recipients:
- person = recipient.person
- if person in people_seen:
- continue
- people_seen.add(person)
- recipient_filters = cached_filters.get(person)
- if recipient_filters is None:
- recipient_filters = set(
- filter.description for filter
- in bug_notification_set.getFiltersByRecipient(
- filtered_notifications, person)
- if filter.description)
- cached_filters[recipient] = recipient_filters
- descriptions.update(recipient_filters)
- return descriptions
-
- for email_person, recipients in sorted_recipients:
+
+ for email_person, data in sorted_recipients:
address = str(email_person.preferredemail.email)
- # Choosing the first recipient is a bit arbitrary, but it
+ # Choosing the first source is a bit arbitrary, but it
# is simple for the user to understand. We may want to reconsider
# this in the future.
- reason = recipients[0].reason_body
- rationale = recipients[0].reason_header
+ reason = data['sources'][0].reason_body
+ rationale = data['sources'][0].reason_header
- filters = get_filter_descriptions(recipients)
- if filters:
- # There are some filters as well, add it to the email body.
+ if data['filter descriptions']:
+ # There are some filter descriptions as well. Add them to
+ # the email body.
filters_text = u"\nMatching subscriptions: %s" % ", ".join(
- filters)
+ data['filter descriptions'])
else:
filters_text = u""
# XXX deryck 2009-11-17 Bug #484319
@@ -246,7 +236,7 @@
body = (body_template % body_data).strip()
msg = bug_notification_builder.build(
from_address, address, body, subject, email_date,
- rationale, references, msgid, filters=filters)
+ rationale, references, msgid, filters=data['filter descriptions'])
messages.append(msg)
return filtered_notifications, omitted_notifications, messages
=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-03-31 19:10:35 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-04-01 14:01:14 +0000
@@ -166,8 +166,10 @@
implements(IBugNotificationSet)
- def getFiltersByRecipient(self, notifications, recipient):
- return []
+ def getRecipientFilterData(self, recipient_to_sources, notifications):
+ return dict(
+ (recipient, {'sources': sources, 'filter descriptions': []})
+ for recipient, sources in recipient_to_sources.items())
class MockBugActivity:
=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py 2011-03-26 01:37:30 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py 2011-04-01 14:01:14 +0000
@@ -10,11 +10,13 @@
from lazr.lifecycle.event import ObjectModifiedEvent
from lazr.lifecycle.snapshot import Snapshot
+from storm.store import Store
from testtools.matchers import Not
from zope.event import notify
from zope.interface import providedBy
from canonical.config import config
+from canonical.database.sqlbase import sqlvalues
from canonical.launchpad.database.message import MessageSet
from canonical.launchpad.ftests import login
from canonical.testing import (
@@ -145,10 +147,33 @@
def setUp(self):
super(TestNotificationsLinkToFilters, self).setUp()
self.bug = self.factory.makeBug()
+ self.subscriber = self.factory.makePerson()
+ self.subscription = self.bug.default_bugtask.target.addSubscription(
+ self.subscriber, self.subscriber)
+ self.notification = self.addNotification(self.subscriber)
+
+ def addNotificationRecipient(self, notification, person):
+ # Manually insert BugNotificationRecipient for
+ # construct_email_notifications to work.
+ # Not sure why using SQLObject constructor doesn't work (it
+ # tries to insert a row with only the ID which fails).
+ Store.of(notification).execute("""
+ INSERT INTO BugNotificationRecipient
+ (bug_notification, person, reason_header, reason_body)
+ VALUES (%s, %s, %s, %s)""" % sqlvalues(
+ notification, person,
+ u'reason header', u'reason body'))
+
+ def addNotification(self, person):
+ # Add a notification along with recipient data.
+ # This is generally done with BugTaskSet.addNotification()
+ # but that requires a more complex set-up.
message = self.factory.makeMessage()
- self.notification = BugNotification(
- message=message, activity=None, bug=self.bug, is_comment=False,
- date_emailed=None)
+ notification = BugNotification(
+ message=message, activity=None, bug=self.bug,
+ is_comment=False, date_emailed=None)
+ self.addNotificationRecipient(notification, person)
+ return notification
def test_bug_filters_empty(self):
# When there are no linked bug filters, it returns a ResultSet
@@ -157,25 +182,18 @@
def test_bug_filters_single(self):
# With a linked BugSubscriptionFilter, it is returned.
- subscriber=self.factory.makePerson()
- subscription = self.bug.default_bugtask.target.addSubscription(
- subscriber, subscriber)
- bug_filter = subscription.newBugFilter()
+ bug_filter = self.subscription.bug_filters.one()
BugNotificationFilter(
bug_notification=self.notification,
bug_subscription_filter=bug_filter)
-
self.assertContentEqual([bug_filter],
self.notification.bug_filters)
def test_bug_filters_multiple(self):
# We can have more than one filter matched up with a single
# notification.
- subscriber=self.factory.makePerson()
- subscription = self.bug.default_bugtask.target.addSubscription(
- subscriber, subscriber)
- bug_filter1 = subscription.newBugFilter()
- bug_filter2 = subscription.newBugFilter()
+ bug_filter1 = self.subscription.bug_filters.one()
+ bug_filter2 = self.subscription.newBugFilter()
BugNotificationFilter(
bug_notification=self.notification,
bug_subscription_filter=bug_filter1)
@@ -186,72 +204,79 @@
self.assertContentEqual([bug_filter1, bug_filter2],
self.notification.bug_filters)
- def test_getFiltersByRecipient_empty(self):
- # When there are no linked bug filters, it returns a ResultSet
- # with no entries.
- subscriber = self.factory.makePerson()
- self.assertTrue(
- BugNotificationSet().getFiltersByRecipient(
- [self.notification], subscriber).is_empty())
+ def test_getRecipientFilterData_empty(self):
+ # When there is empty input, there is empty output.
+ self.assertEqual(
+ BugNotificationSet().getRecipientFilterData({}, []),
+ {})
+ self.assertEqual(
+ BugNotificationSet().getRecipientFilterData(
+ {}, [self.notification]),
+ {})
- def test_getFiltersByRecipient_other_persons(self):
- # When there is no bug filter for the recipient,
- # it returns a ResultSet with no entries.
- recipient = self.factory.makePerson()
- subscriber = self.factory.makePerson()
- subscription = self.bug.default_bugtask.target.addSubscription(
- subscriber, subscriber)
- bug_filter = subscription.newBugFilter()
+ def test_getRecipientFilterData_other_persons(self):
+ # When there is no named bug filter for the recipient,
+ # it returns the recipient but with no filter descriptions.
+ BugNotificationFilter(
+ bug_notification=self.notification,
+ bug_subscription_filter=self.subscription.bug_filters.one())
+ subscriber2 = self.factory.makePerson()
+ subscription2 = self.bug.default_bugtask.target.addSubscription(
+ subscriber2, subscriber2)
+ bug_filter = subscription2.bug_filters.one()
+ bug_filter.description = u'Special Filter!'
BugNotificationFilter(
bug_notification=self.notification,
bug_subscription_filter=bug_filter)
- self.assertTrue(
- BugNotificationSet().getFiltersByRecipient(
- [self.notification], recipient).is_empty())
+ sources = list(self.notification.recipients)
+ self.assertEqual(
+ {self.subscriber: {'sources': sources,
+ 'filter descriptions': []}},
+ BugNotificationSet().getRecipientFilterData(
+ {self.subscriber: sources}, [self.notification])
+ )
- def test_getFiltersByRecipient_match(self):
+ def test_getRecipientFilterData_match(self):
# When there are bug filters for the recipient,
# only those filters are returned.
- subscriber = self.factory.makePerson()
- subscription = self.bug.default_bugtask.target.addSubscription(
- subscriber, subscriber)
- bug_filter = subscription.newBugFilter()
+ bug_filter = self.subscription.bug_filters.one()
+ bug_filter.description = u'Special Filter!'
BugNotificationFilter(
bug_notification=self.notification,
bug_subscription_filter=bug_filter)
- self.assertContentEqual(
- [bug_filter],
- BugNotificationSet().getFiltersByRecipient(
- [self.notification], subscriber))
+ sources = list(self.notification.recipients)
+ self.assertEqual(
+ {self.subscriber: {'sources': sources,
+ 'filter descriptions': ['Special Filter!']}},
+ BugNotificationSet().getRecipientFilterData(
+ {self.subscriber: sources}, [self.notification]))
- def test_getFiltersByRecipients_multiple_notifications_match(self):
+ def test_getRecipientFilterData_multiple_notifications_match(self):
# When there are bug filters for the recipient for multiple
# notifications, return filters for all the notifications.
# Set up first notification and filter.
- subscriber = self.factory.makePerson()
- subscription = self.bug.default_bugtask.target.addSubscription(
- subscriber, subscriber)
- bug_filter = subscription.newBugFilter()
+ bug_filter = self.subscription.bug_filters.one()
+ bug_filter.description = u'Special Filter!'
BugNotificationFilter(
bug_notification=self.notification,
bug_subscription_filter=bug_filter)
- # set up second notification and filter.
- bug2 = self.factory.makeBug()
- message2 = self.factory.makeMessage()
- notification2 = BugNotification(
- message=message2, activity=None, bug=bug2, is_comment=False,
- date_emailed=None)
- subscription2 = bug2.default_bugtask.target.addSubscription(
- subscriber, subscriber)
- bug_filter2 = subscription2.newBugFilter()
+ # Set up second notification and filter.
+ notification2 = self.addNotification(self.subscriber)
+ bug_filter2 = self.subscription.newBugFilter()
+ bug_filter2.description = u'Another Filter!'
BugNotificationFilter(
bug_notification=notification2,
bug_subscription_filter=bug_filter2)
# Perform the test.
- self.assertContentEqual(
- set([bug_filter, bug_filter2]),
- set(BugNotificationSet().getFiltersByRecipient(
- [self.notification, notification2], subscriber)))
+ sources = list(self.notification.recipients)
+ sources.extend(notification2.recipients)
+ assert(len(sources)==2)
+ self.assertEqual(
+ {self.subscriber: {'sources': sources,
+ 'filter descriptions': ['Another Filter!', 'Special Filter!']}},
+ BugNotificationSet().getRecipientFilterData(
+ {self.subscriber: sources},
+ [self.notification, notification2]))
class TestNotificationProcessingWithoutRecipients(TestCaseWithFactory):
