launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02812
[Merge] lp:~danilo/launchpad/bug-720826-emails into lp:launchpad/db-devel
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-720826-emails into lp:launchpad/db-devel with lp:~danilo/launchpad/bug-720826 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#720826 Add subscription description header for bug notifications
https://bugs.launchpad.net/bugs/720826
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-720826-emails/+merge/51747
= Bug 720826: modify emails =
This is a part of work on 720826: adding headers and email body content
describing bug subscription filters which have caused that email to go off.
This branch adds the actual headers based on the BugNotificationFilter
table linking BugNotifications with BugSubscriptionFilters.
== Proposed fix ==
* BugNotificationBuilder (which constructs the actual email text) gets a new filters parameter on the build() method
* scripts/bugnotification.py construct_email_notifications now finds all matching filters for each recipient, constructs a text to include in the body of the email (for gmail users) and adds X-Launchpad-Subscription-Filter header for each of the filters with non-empty description
== Implementation details ==
I'm also moving bug-notification*.txt email templates to lib/lp/bugs/emailtemplates and using them by adding 'bugs' to the get_email_template call.
Rest of the bits are mostly lint fixes.
== Tests ==
bin/test -cvvt TestEmailNotificationsWithFilters -t TestBugNotificationBuilder
== Demo and Q/A ==
no-qa on it's own (as illustrated by tests, appropriate records do not get created automatically yet)
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/emailtemplates/bug-notification-verbose.txt
lib/lp/bugs/emailtemplates/bug-notification.txt
lib/lp/bugs/interfaces/structuralsubscription.py
lib/lp/bugs/mail/bugnotificationbuilder.py
lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py
lib/lp/bugs/model/structuralsubscription.py
lib/lp/bugs/scripts/bugnotification.py
lib/lp/bugs/scripts/tests/test_bugnotification.py
./lib/lp/bugs/emailtemplates/bug-notification-verbose.txt
3: Line has trailing whitespace.
./lib/lp/bugs/emailtemplates/bug-notification.txt
3: Line has trailing whitespace.
--
https://code.launchpad.net/~danilo/launchpad/bug-720826-emails/+merge/51747
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-720826-emails into lp:launchpad/db-devel.
=== renamed file 'lib/canonical/launchpad/emailtemplates/bug-notification-verbose.txt' => 'lib/lp/bugs/emailtemplates/bug-notification-verbose.txt'
--- lib/canonical/launchpad/emailtemplates/bug-notification-verbose.txt 2011-01-06 14:43:46 +0000
+++ lib/lp/bugs/emailtemplates/bug-notification-verbose.txt 2011-03-01 13:41:50 +0000
@@ -1,7 +1,7 @@
%(content)s
--
-%(notification_rationale)s
+%(notification_rationale)s%(subscription_filters)s
%(bug_url)s
Title:
=== renamed file 'lib/canonical/launchpad/emailtemplates/bug-notification.txt' => 'lib/lp/bugs/emailtemplates/bug-notification.txt'
--- lib/canonical/launchpad/emailtemplates/bug-notification.txt 2011-02-14 19:45:00 +0000
+++ lib/lp/bugs/emailtemplates/bug-notification.txt 2011-03-01 13:41:50 +0000
@@ -1,7 +1,7 @@
%(content)s
--
-%(notification_rationale)s
+%(notification_rationale)s%(subscription_filters)s
%(bug_url)s
Title:
=== modified file 'lib/lp/bugs/interfaces/structuralsubscription.py'
--- lib/lp/bugs/interfaces/structuralsubscription.py 2011-03-01 13:41:49 +0000
+++ lib/lp/bugs/interfaces/structuralsubscription.py 2011-03-01 13:41:50 +0000
@@ -14,10 +14,6 @@
'IStructuralSubscriptionTargetHelper',
]
-from lazr.enum import (
- DBEnumeratedType,
- DBItem,
- )
from lazr.restful.declarations import (
call_with,
export_as_webservice_entry,
@@ -41,7 +37,6 @@
)
from zope.schema import (
Bool,
- Choice,
Datetime,
Int,
)
=== modified file 'lib/lp/bugs/mail/bugnotificationbuilder.py'
--- lib/lp/bugs/mail/bugnotificationbuilder.py 2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/mail/bugnotificationbuilder.py 2011-03-01 13:41:50 +0000
@@ -151,7 +151,7 @@
event_creator.name)))
def build(self, from_address, to_address, body, subject, email_date,
- rationale=None, references=None, message_id=None):
+ rationale=None, references=None, message_id=None, filters=None):
"""Construct the notification.
:param from_address: The From address of the notification.
@@ -192,4 +192,9 @@
if rationale is not None:
message.add_header('X-Launchpad-Message-Rationale', rationale)
+ if filters is not None:
+ for filter in filters:
+ message.add_header(
+ 'X-Launchpad-Subscription-Filter', filter)
+
return message
=== added file 'lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py'
--- lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py 2011-03-01 13:41:50 +0000
@@ -0,0 +1,51 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for BugNotificationBuilder email construction."""
+
+from datetime import datetime
+import pytz
+
+from canonical.testing.layers import ZopelessDatabaseLayer
+from lp.bugs.mail.bugnotificationbuilder import BugNotificationBuilder
+from lp.testing import TestCaseWithFactory
+
+
+class TestBugNotificationBuilder(TestCaseWithFactory):
+ """Test emails sent when subscribed by someone else."""
+
+ layer = ZopelessDatabaseLayer
+
+ def setUp(self):
+ # Run the tests as a logged-in user.
+ super(TestBugNotificationBuilder, self).setUp(
+ user='test@xxxxxxxxxxxxx')
+ self.bug = self.factory.makeBug()
+ self.builder = BugNotificationBuilder(self.bug)
+
+ def test_build_filters_empty(self):
+ """Filters are added."""
+ utc_now = datetime.now(pytz.UTC)
+ message = self.builder.build('from', 'to', 'body', 'subject',
+ utc_now, filters=[])
+ self.assertIs(None,
+ message.get("X-Subscription-Filter-Description", None))
+
+ def test_build_filters_single(self):
+ """Filters are added."""
+ utc_now = datetime.now(pytz.UTC)
+ message = self.builder.build('from', 'to', 'body', 'subject',
+ utc_now, filters=[u"Testing filter"])
+ self.assertContentEqual(
+ [u"Testing filter"],
+ message.get_all("X-Subscription-Filter-Description"))
+
+ def test_build_filters_multiple(self):
+ """Filters are added."""
+ utc_now = datetime.now(pytz.UTC)
+ message = self.builder.build(
+ 'from', 'to', 'body', 'subject', utc_now,
+ filters=[u"Testing filter", u"Second testing filter"])
+ self.assertContentEqual(
+ [u"Testing filter", u"Second testing filter"],
+ message.get_all("X-Subscription-Filter-Description"))
=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py 2011-03-01 13:41:49 +0000
+++ lib/lp/bugs/model/structuralsubscription.py 2011-03-01 13:41:50 +0000
@@ -17,7 +17,6 @@
from storm.base import Storm
from storm.expr import (
- Alias,
And,
CompoundOper,
Except,
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py 2011-02-17 17:30:22 +0000
+++ lib/lp/bugs/scripts/bugnotification.py 2011-03-01 13:41:50 +0000
@@ -33,17 +33,17 @@
def get_activity_key(notification):
"""Given a notification, return a key for the activity if it exists.
-
+
The key will be used to determine whether changes for the activity are
undone within the same batch of notifications (which are supposed to
be all for the same bug when they get to this function). Therefore,
the activity's attribute is a good start for the key.
-
+
If the activity was on a bugtask, we will also want to distinguish
by bugtask, because, for instance, changing a status from INPROGRESS
to FIXCOMMITED on one bug task is not undone if the status changes
from FIXCOMMITTED to INPROGRESS on another bugtask.
-
+
Similarly, if the activity is about adding or removing something
that we can have multiple of, like a branch or an attachment, the
key should include information on that value, because adding one
@@ -163,6 +163,18 @@
reason = recipient.reason_body
rationale = recipient.reason_header
+ filters = set()
+ for notification in filtered_notifications:
+ notification_filters = notification.getFiltersByRecipient(
+ email_person)
+ for notification_filter in notification_filters:
+ if notification_filter.description is not None:
+ filters.add(notification_filter.description)
+ if filters:
+ # There are some filters as well, add it to the email body.
+ filters_text = u"\nMatching filters: %s" % ", ".join(filters)
+ else:
+ filters_text = u""
# XXX deryck 2009-11-17 Bug #484319
# This should be refactored to add a link inside the
# code where we build `reason`. However, this will
@@ -180,7 +192,9 @@
'bug_title': data_wrapper.format(bug.title),
'bug_url': canonical_url(bug),
'unsubscribe_notice': unsubscribe_notice,
- 'notification_rationale': mail_wrapper.format(reason)}
+ 'notification_rationale': mail_wrapper.format(reason),
+ 'subscription_filters': filters_text,
+ }
# If the person we're sending to receives verbose notifications
# we include the description and status of the bug in the email
@@ -200,10 +214,11 @@
else:
email_template = 'bug-notification.txt'
- body = (get_email_template(email_template) % body_data).strip()
+ body_template = get_email_template(email_template, 'bugs')
+ body = (body_template % body_data).strip()
msg = bug_notification_builder.build(
from_address, address, body, subject, email_date,
- rationale, references, msgid)
+ rationale, references, msgid, filters=filters)
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-02-17 17:30:22 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-03-01 13:41:50 +0000
@@ -14,7 +14,10 @@
from zope.interface import implements
from canonical.config import config
-from canonical.database.sqlbase import flush_database_updates
+from canonical.database.sqlbase import (
+ flush_database_updates,
+ sqlvalues,
+ )
from canonical.launchpad.ftests import login
from canonical.launchpad.helpers import get_contact_email_addresses
from canonical.launchpad.interfaces.message import IMessageSet
@@ -136,7 +139,7 @@
class FakeNotification:
"""An even simpler fake notification.
-
+
Used by TestGetActivityKey, TestNotificationCommentBatches and
TestNotificationBatches."""
@@ -153,6 +156,7 @@
class MockBugActivity:
"""A mock BugActivity used for testing."""
+
def __init__(self, target=None, attribute=None,
oldvalue=None, newvalue=None):
self.target = target
@@ -868,3 +872,180 @@
BugAttachmentChange(
self.ten_minutes_ago, self.person, 'attachment',
item, None))
+
+from lp.bugs.model.bugnotification import (
+ BugNotification,
+ BugNotificationFilter,
+# BugNotificationRecipient,
+ )
+from lp.bugs.scripts.bugnotification import construct_email_notifications
+from storm.store import Store
+
+
+class TestEmailNotificationsWithFilters(TestCaseWithFactory):
+ """Ensure outgoing mails have corresponding mail headers.
+
+ Every filter that could have potentially caused a notification to
+ go off has a `BugNotificationFilter` record linking a `BugNotification`
+ and a `BugSubscriptionFilter`.
+
+ From those records, we include all BugSubscriptionFilter.description
+ in X-Subscription-Filter-Description headers in each email.
+ """
+
+ layer = LaunchpadZopelessLayer
+
+ def setUp(self):
+ super(TestEmailNotificationsWithFilters, self).setUp()
+ self.bug=self.factory.makeBug()
+ subscriber = self.factory.makePerson()
+ self.subscription = self.bug.default_bugtask.target.addSubscription(
+ subscriber, subscriber)
+ self.filter_count = 0
+ self.notification = self.addNotification(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()
+ notification = BugNotification(
+ message=message, activity=None, bug=self.bug,
+ is_comment=False, date_emailed=None)
+ self.addNotificationRecipient(notification, person)
+ return notification
+
+ def addFilter(self, description, subscription=None):
+ if subscription is None:
+ subscription = self.subscription
+ filter_count = self.filter_count
+ self.filter_count += 1
+ else:
+ # For a non-default subscription, always use
+ # the initial filter.
+ filter_count = 0
+
+ # If no filters have been requested before,
+ # use the initial auto-created filter for a subscription.
+ if filter_count == 0:
+ bug_filter = subscription.bug_filters.one()
+ else:
+ bug_filter = subscription.newBugFilter()
+ bug_filter.description = description
+ BugNotificationFilter(
+ bug_notification=self.notification,
+ bug_subscription_filter=bug_filter)
+
+ def getSubscriptionEmailHeaders(self, by_person=False):
+ filtered, omitted, messages = construct_email_notifications(
+ [self.notification])
+ if by_person:
+ headers = {}
+ else:
+ headers = set()
+ for message in messages:
+ if by_person:
+ headers[message['to']] = message.get_all(
+ "X-Launchpad-Subscription-Filter", [])
+ else:
+ headers = headers.union(
+ set(message.get_all(
+ "X-Launchpad-Subscription-Filter", [])))
+ return headers
+
+ def getSubscriptionEmailBody(self, by_person=False):
+ filtered, omitted, messages = construct_email_notifications(
+ [self.notification])
+ if by_person:
+ filter_texts = {}
+ else:
+ filter_texts = set()
+ for message in messages:
+ filters_line = None
+ for line in message.get_payload().splitlines():
+ if line.startswith("Matching filters: "):
+ filters_line = line
+ break
+ if filters_line is not None:
+ if by_person:
+ filter_texts[message['to']] = filters_line
+ else:
+ filter_texts.add(filters_line)
+ return filter_texts
+
+ def test_header_empty(self):
+ # An initial filter with no description doesn't cause any
+ # headers to be added.
+ self.assertContentEqual([],
+ self.getSubscriptionEmailHeaders())
+
+ def test_header_single(self):
+ # A single filter with a description makes all emails
+ # include that particular filter description in a header.
+ bug_filter = self.addFilter(u"Test filter")
+
+ self.assertContentEqual([u"Test filter"],
+ self.getSubscriptionEmailHeaders())
+
+ def test_header_multiple(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")
+
+ self.assertContentEqual([u"First filter", u"Second filter"],
+ self.getSubscriptionEmailHeaders())
+
+ def test_header_other_subscriber_by_person(self):
+ # Filters for a different subscribers are included only
+ # in email messages relevant to them, even if they might
+ # all be for the same notification.
+ other_person = self.factory.makePerson()
+ other_subscription = self.bug.default_bugtask.target.addSubscription(
+ other_person, other_person)
+ bug_filter = self.addFilter(u"Someone's filter", other_subscription)
+ self.addNotificationRecipient(self.notification, other_person)
+
+ this_filter = self.addFilter(u"Test filter")
+
+ the_subscriber = self.subscription.subscriber
+ self.assertEquals(
+ {other_person.preferredemail.email: [u"Someone's filter"],
+ the_subscriber.preferredemail.email: [u"Test filter"]},
+ self.getSubscriptionEmailHeaders(by_person=True))
+
+ def test_body_empty(self):
+ # An initial filter with no description doesn't cause any
+ # text to be added to the email body.
+ self.assertContentEqual([],
+ self.getSubscriptionEmailBody())
+
+ def test_body_single(self):
+ # A single filter with a description makes all emails
+ # include that particular filter description in the body.
+ bug_filter = self.addFilter(u"Test filter")
+
+ self.assertContentEqual([u"Matching filters: Test filter"],
+ self.getSubscriptionEmailBody())
+
+ def test_body_multiple(self):
+ # Multiple filters with description make all emails
+ # include them in the email body.
+ bug_filter = self.addFilter(u"First filter")
+ bug_filter = self.addFilter(u"Second filter")
+
+ self.assertContentEqual(
+ [u"Matching filters: First filter, Second filter"],
+ self.getSubscriptionEmailBody())
