Merge lp:~danilo/launchpad/bug-720826-emails into lp:launchpad/db-devel

[Данило Шеган]

[r=gmb][bug=720826][incr] Add headers/content describing filters that have caused an email notification to go off. Part of bug 720826 fix.

= 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.

[Graham Binns (gmb)]

review: Approve (code)

=== 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-02 17:29:24 +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-02 17:29:24 +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/mail/bugnotificationbuilder.py'
--- lib/lp/bugs/mail/bugnotificationbuilder.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/mail/bugnotificationbuilder.py	2011-03-02 17:29:24 +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-02 17:29:24 +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='[e-mail]')
+        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-Launchpad-Subscription-Filter", 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-Launchpad-Subscription-Filter"))
+
+    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-Launchpad-Subscription-Filter"))

=== 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-02 17:29:24 +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-02 17:29:24 +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
@@ -133,10 +136,13 @@
         self.recipients = [MockBugNotificationRecipient()]
         self.activity = None
 
+    def getFiltersByRecipient(self, recipient):
+        return []
+
 
 class FakeNotification:
     """An even simpler fake notification.
-    
+
     Used by TestGetActivityKey, TestNotificationCommentBatches and
     TestNotificationBatches."""
 
@@ -153,6 +159,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 +875,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())