launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03186
[Merge] lp:~gary/launchpad/muteteamsub-email into lp:launchpad/db-devel
Gary Poster has proposed merging lp:~gary/launchpad/muteteamsub-email into lp:launchpad/db-devel with lp:~gmb/launchpad/team-subscription-opt-out as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~gary/launchpad/muteteamsub-email/+merge/56154
This branch teaches the email sending code to honor the per-person structural subscription mutes.
It does so via a single additional SQL query per batch of notifications (not per-source or per recipient).
As the interface change describes in lib/lp/bugs/interfaces/bugnotification.py, IBugNotificationSet.getRecipientFilterData now may omit recipients that it receives. This is already honored in lib/lp/bugs/scripts/bugnotification.py, construct_email_notifications, because we use the output of the method to determine what emails to send.
recipients = getUtility(IBugNotificationSet).getRecipientFilterData(
recipients, filtered_notifications)
The actual code change is in lib/lp/bugs/model/bugnotification.py. I'll leave it to try and speak for itself.
I added an __init__ to BugSubscriptionFilterMutes just for test convenience in lib/lp/bugs/model/bugsubscriptionfilter.py.
I added several new tests, most of them for the method in isolation but a couple of it as used by construct_email_notifications. I refactored some existing tests to make a couple of helper methods that they and the new tests could use.
That's pretty much it.
Thanks
Gary
--
https://code.launchpad.net/~gary/launchpad/muteteamsub-email/+merge/56154
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/muteteamsub-email into lp:launchpad/db-devel.
=== modified file 'lib/lp/bugs/interfaces/bugnotification.py'
--- lib/lp/bugs/interfaces/bugnotification.py 2011-04-04 06:11:16 +0000
+++ lib/lp/bugs/interfaces/bugnotification.py 2011-04-04 13:05:18 +0000
@@ -88,6 +88,10 @@
(BugNotificationRecipients) that represent the subscriptions that
caused the notifications to be sent.
:param notifications: the notifications that are being communicated.
+
+ The dict of recipients may have fewer recipients than were
+ provided if those users muted all of the subscription filters
+ that caused them to be sent.
"""
=== modified file 'lib/lp/bugs/model/bugnotification.py'
--- lib/lp/bugs/model/bugnotification.py 2011-04-04 06:11:16 +0000
+++ lib/lp/bugs/model/bugnotification.py 2011-04-04 13:05:18 +0000
@@ -54,7 +54,10 @@
IBugNotificationSet,
)
from lp.bugs.model.bugactivity import BugActivity
-from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
+from lp.bugs.model.bugsubscriptionfilter import (
+ BugSubscriptionFilter,
+ BugSubscriptionFilterMute,
+ )
from lp.bugs.model.structuralsubscription import StructuralSubscription
from lp.registry.interfaces.person import IPersonSet
from lp.services.database.stormbase import StormBase
@@ -191,9 +194,10 @@
if not notifications or not recipient_to_sources:
# This is a shortcut that will remove some error conditions.
return {}
- # This makes one call to the database to get all the information
- # we need. We get the filter ids and descriptions for each
- # source, and then we divide up the information per recipient.
+ # 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, and then we divide up the
+ # information per recipient.
# First we get some intermediate data structures set up.
source_person_id_map = {}
recipient_id_map = {}
@@ -241,22 +245,36 @@
source_person_id_map[source_person_id]['filters'][filter_id] = (
filter_description)
filter_ids.append(filter_id)
+ no_filter_marker = -1 # This is only necessary while production and
+ # sample data have structural subscriptions without filters.
# Assign the filters to each recipient.
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'])
+ source_person_id_map[source_person_id]['filters']
+ or {no_filter_marker: None})
+ if filter_ids:
+ # Now we get the information about subscriptions that might be
+ # filtered and take that into account.
+ mute_data = store.find(
+ (BugSubscriptionFilterMute.person_id,
+ BugSubscriptionFilterMute.filter_id),
+ In(BugSubscriptionFilterMute.person_id, recipient_id_map.keys()),
+ In(BugSubscriptionFilterMute.filter_id, filter_ids))
+ for person_id, filter_id in mute_data:
+ del recipient_id_map[person_id]['filters'][filter_id]
# Now recipient_id_map has all the information we need. Let's
# build the final result and return it.
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}
+ if recipient_data['filters']:
+ 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
=== modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py 2011-04-04 13:05:15 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py 2011-04-04 13:05:18 +0000
@@ -257,6 +257,12 @@
__storm_table__ = "BugSubscriptionFilterMute"
+ def __init__(self, person=None, filter=None):
+ if person is not None:
+ self.person = person
+ if filter is not None:
+ self.filter = filter.id
+
person_id = Int("person", allow_none=False, validator=validate_person)
person = Reference(person_id, "Person.id")
=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-04-04 06:11:16 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-04-04 13:05:18 +0000
@@ -49,6 +49,7 @@
BugNotificationFilter,
# BugNotificationRecipient,
)
+from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilterMute
from lp.bugs.model.bugtask import BugTask
from lp.bugs.scripts.bugnotification import (
construct_email_notifications,
@@ -904,7 +905,7 @@
class TestEmailNotificationsWithFilters(TestCaseWithFactory):
- """Ensure outgoing mails have corresponding mail headers.
+ """Ensure outgoing mails have corresponding headers, accounting for mutes.
Every filter that could have potentially caused a notification to
go off has a `BugNotificationFilter` record linking a `BugNotification`
@@ -912,6 +913,11 @@
From those records, we include all BugSubscriptionFilter.description
in X-Subscription-Filter-Description headers in each email.
+
+ Every team filter that caused notifications might be muted for a
+ given recipient. These can cause headers to be omitted, and if all
+ filters that caused the notifications are omitted then the
+ notification itself will not be sent.
"""
layer = LaunchpadZopelessLayer
@@ -1070,3 +1076,24 @@
self.assertContentEqual(
[u"Matching subscriptions: First filter, Second filter"],
self.getSubscriptionEmailBody())
+
+ def test_muted(self):
+ bug_filter = self.addFilter(u"Test filter")
+ BugSubscriptionFilterMute(
+ person=self.subscription.subscriber,
+ filter=self.notification.bug_filters.one())
+ filtered, omitted, messages = construct_email_notifications(
+ [self.notification])
+ self.assertEqual(list(messages), [])
+
+ def test_header_multiple_one_muted(self):
+ # Multiple filters with a description make all emails
+ # include all filter descriptions in the header.
+ bug_filter = self.addFilter(u"First filter")
+ bug_filter = self.addFilter(u"Second filter")
+ BugSubscriptionFilterMute(
+ person=self.subscription.subscriber,
+ filter=self.notification.bug_filters[0])
+
+ self.assertContentEqual([u"Second filter"],
+ self.getSubscriptionEmailHeaders())
=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py 2011-04-04 06:11:16 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py 2011-04-04 13:05:18 +0000
@@ -33,6 +33,7 @@
BugNotificationFilter,
BugNotificationSet,
)
+from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilterMute
from lp.testing import TestCaseWithFactory
from lp.testing.factory import LaunchpadObjectFactory
from lp.testing.mail_helpers import pop_notifications
@@ -175,6 +176,32 @@
self.addNotificationRecipient(notification, person)
return notification
+ def includeFilterInNotification(self, description=None, subscription=None,
+ notification=None,
+ create_new_filter=False):
+ if subscription is None:
+ subscription = self.subscription
+ if notification is None:
+ notification = self.notification
+ if create_new_filter:
+ bug_filter = subscription.newBugFilter()
+ else:
+ bug_filter = subscription.bug_filters.one()
+ if description is not None:
+ bug_filter.description = description
+ BugNotificationFilter(
+ bug_notification=notification,
+ bug_subscription_filter=bug_filter)
+
+ def prepareTwoNotificationsWithFilters(self):
+ # Set up first notification and filter.
+ self.includeFilterInNotification(description=u'Special Filter!')
+ # Set up second notification and filter.
+ self.notification2 = self.addNotification(self.subscriber)
+ self.includeFilterInNotification(description=u'Another Filter!',
+ create_new_filter=True,
+ notification=self.notification2)
+
def test_bug_filters_empty(self):
# When there are no linked bug filters, it returns a ResultSet
# with no entries.
@@ -182,11 +209,8 @@
def test_bug_filters_single(self):
# With a linked BugSubscriptionFilter, it is returned.
- bug_filter = self.subscription.bug_filters.one()
- BugNotificationFilter(
- bug_notification=self.notification,
- bug_subscription_filter=bug_filter)
- self.assertContentEqual([bug_filter],
+ self.includeFilterInNotification()
+ self.assertContentEqual([self.subscription.bug_filters.one()],
self.notification.bug_filters)
def test_bug_filters_multiple(self):
@@ -217,32 +241,29 @@
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())
+ self.includeFilterInNotification()
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)
+ notification2 = self.addNotification(subscriber2)
+ self.includeFilterInNotification(subscription=subscription2,
+ description=u'Special Filter!',
+ notification=notification2)
sources = list(self.notification.recipients)
+ sources2 = list(notification2.recipients)
self.assertEqual(
{self.subscriber: {'sources': sources,
- 'filter descriptions': []}},
+ 'filter descriptions': []},
+ subscriber2: {'sources': sources2,
+ 'filter descriptions': [u'Special Filter!']}},
BugNotificationSet().getRecipientFilterData(
- {self.subscriber: sources}, [self.notification]))
+ {self.subscriber: sources, subscriber2: sources2},
+ [self.notification, notification2]))
def test_getRecipientFilterData_match(self):
# When there are bug filters for the recipient,
# only those filters are returned.
- bug_filter = self.subscription.bug_filters.one()
- bug_filter.description = u'Special Filter!'
- BugNotificationFilter(
- bug_notification=self.notification,
- bug_subscription_filter=bug_filter)
+ self.includeFilterInNotification(description=u'Special Filter!')
sources = list(self.notification.recipients)
self.assertEqual(
{self.subscriber: {'sources': sources,
@@ -253,32 +274,89 @@
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.
- 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.
- 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)
+ self.prepareTwoNotificationsWithFilters()
+ # Perform the test.
sources = list(self.notification.recipients)
- sources.extend(notification2.recipients)
- # This first check is really just for the internal consistency of the
- # test.
- self.assertEqual(len(sources), 2)
- # Perform the test.
+ sources.extend(self.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, self.notification2]))
+
+ def test_getRecipientFilterData_mute(self):
+ # When there are bug filters for the recipient,
+ # only those filters are returned.
+ self.includeFilterInNotification(description=u'Special Filter!')
+ # Mute the first filter.
+ BugSubscriptionFilterMute(
+ person=self.subscriber,
+ filter=self.notification.bug_filters.one())
+ sources = list(self.notification.recipients)
+ self.assertEqual(
+ {},
+ BugNotificationSet().getRecipientFilterData(
+ {self.subscriber: sources}, [self.notification]))
+
+ def test_getRecipientFilterData_mute_one_person_of_two(self):
+ self.includeFilterInNotification()
+ # Mute the first filter.
+ BugSubscriptionFilterMute(
+ person=self.subscriber,
+ filter=self.notification.bug_filters.one())
+ subscriber2 = self.factory.makePerson()
+ subscription2 = self.bug.default_bugtask.target.addSubscription(
+ subscriber2, subscriber2)
+ notification2 = self.addNotification(subscriber2)
+ self.includeFilterInNotification(subscription=subscription2,
+ description=u'Special Filter!',
+ notification=notification2)
+ sources = list(self.notification.recipients)
+ sources2 = list(notification2.recipients)
+ self.assertEqual(
+ {subscriber2: {'sources': sources2,
+ 'filter descriptions': [u'Special Filter!']}},
+ BugNotificationSet().getRecipientFilterData(
+ {self.subscriber: sources, subscriber2: sources2},
[self.notification, notification2]))
+ def test_getRecipientFilterData_mute_one_filter_of_two(self):
+ self.prepareTwoNotificationsWithFilters()
+ # Mute the first filter.
+ BugSubscriptionFilterMute(
+ person=self.subscriber,
+ filter=self.notification.bug_filters.one())
+ sources = list(self.notification.recipients)
+ sources.extend(self.notification2.recipients)
+ # Perform the test.
+ self.assertEqual(
+ {self.subscriber: {'sources': sources,
+ 'filter descriptions': ['Another Filter!']}},
+ BugNotificationSet().getRecipientFilterData(
+ {self.subscriber: sources},
+ [self.notification, self.notification2]))
+
+ def test_getRecipientFilterData_mute_both_filters_mutes(self):
+ self.prepareTwoNotificationsWithFilters()
+ # Mute the first filter.
+ BugSubscriptionFilterMute(
+ person=self.subscriber,
+ filter=self.notification.bug_filters.one())
+ # Mute the second filter.
+ BugSubscriptionFilterMute(
+ person=self.subscriber,
+ filter=self.notification2.bug_filters.one())
+ sources = list(self.notification.recipients)
+ sources.extend(self.notification2.recipients)
+ # Perform the test.
+ self.assertEqual(
+ {},
+ BugNotificationSet().getRecipientFilterData(
+ {self.subscriber: sources},
+ [self.notification, self.notification2]))
+
class TestNotificationProcessingWithoutRecipients(TestCaseWithFactory):
"""Adding notificatons without any recipients does not cause any harm.
