launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/bug-footer into lp:launchpad.

Commit message:
Make bug notifications honour PersonSettings.expanded_notification_footers.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1474071 in Launchpad itself: "Gmail filtering improvements"
  https://bugs.launchpad.net/launchpad/+bug/1474071

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/bug-footer/+merge/266577

Make bug notifications honour PersonSettings.expanded_notification_footers.

I tried to convert bug notifications to BaseMailer, and that would still ultimately be better, but it was Hard Work.  This only took me half an hour or so and the duplication isn't all that bad.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/bug-footer into lp:launchpad.

=== modified file 'lib/lp/bugs/doc/bugnotification-email.txt'
--- lib/lp/bugs/doc/bugnotification-email.txt	2015-07-21 09:04:01 +0000
+++ lib/lp/bugs/doc/bugnotification-email.txt	2015-07-31 14:49:19 +0000
@@ -535,18 +535,19 @@
 
 The build() method of a builder accepts a number of parameters and returns
 an instance of email.mime.text.MIMEText. The most basic invocation of this
-method requires a from address, a to address, a body, a subject and a
-sending date for the mail.
+method requires a from address, a to person, a body, a subject and a sending
+date for the mail.
 
     >>> from datetime import datetime
     >>> import pytz
 
     >>> from_address = get_bugmail_from_address(lp_janitor, bug_four)
+    >>> to_person = getUtility(IPersonSet).getByEmail('foo.bar@xxxxxxxxxxxxx')
     >>> sending_date = pytz.timezone('Europe/Prague').localize(
     ...     datetime(2008, 5, 20, 11, 5, 47))
 
     >>> notification_email = bug_four_notification_builder.build(
-    ...     from_address, 'foo.bar@xxxxxxxxxxxxx',
+    ...     from_address, to_person,
     ...     "A test body.", "A test subject.", sending_date)
 
 The fields of the generated notification email will be set according to
@@ -572,7 +573,7 @@
 references and message_id.
 
     >>> notification_email = bug_four_notification_builder.build(
-    ...     from_address, 'foo.bar@xxxxxxxxxxxxx',
+    ...     from_address, to_person,
     ...     "A test body.", "A test subject.", sending_date,
     ...     rationale='Because-I-said-so',
     ...     references=['<12345@xxxxxxxxxxxxx>'],
@@ -598,7 +599,7 @@
 The message subject will always have [Bug <bug_id>] prepended to it.
 
     >>> notification_email = bug_four_notification_builder.build(
-    ...     from_address, 'foo.bar@xxxxxxxxxxxxx',
+    ...     from_address, to_person,
     ...     "A test body.", "Yet another message", sending_date)
 
     >>> print notification_email['Subject']
@@ -608,8 +609,7 @@
 <bug_id>].
 
     >>> notification_email = bug_four_notification_builder.build(
-    ...     from_address, 'foo.bar@xxxxxxxxxxxxx',
-    ...     "A test body.", None, sending_date)
+    ...     from_address, to_person, "A test body.", None, sending_date)
 
     >>> print notification_email['Subject']
     [Bug 4]

=== modified file 'lib/lp/bugs/mail/bugnotificationbuilder.py'
--- lib/lp/bugs/mail/bugnotificationbuilder.py	2015-07-13 16:14:46 +0000
+++ lib/lp/bugs/mail/bugnotificationbuilder.py	2015-07-31 14:49:19 +0000
@@ -16,6 +16,7 @@
 import rfc822
 
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import (
     PRIVATE_INFORMATION_TYPES,
@@ -25,7 +26,10 @@
 from lp.services.config import config
 from lp.services.helpers import shortlist
 from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
-from lp.services.mail.sendmail import format_address
+from lp.services.mail.sendmail import (
+    append_footer,
+    format_address,
+    )
 
 
 def format_rfc2822_date(date):
@@ -161,12 +165,13 @@
             self.common_headers.append(
                 ('X-Launchpad-Bug-Duplicate', str(bug.duplicateof.id)))
 
-    def build(self, from_address, to_address, body, subject, email_date,
+    def build(self, from_address, to_person, body, subject, email_date,
               rationale=None, references=None, message_id=None, filters=None):
         """Construct the notification.
 
         :param from_address: The From address of the notification.
-        :param to_address: The To address for the notification.
+        :param to_person: The `IPerson` to use as the To address for the
+            notification.
         :param body: The body text of the notification.
         :type body: unicode
         :param subject: The Subject of the notification.
@@ -178,34 +183,47 @@
 
         :return: An `email.mime.text.MIMEText` object.
         """
-        message = MIMEText(body.encode('utf8'), 'plain', 'utf8')
-        message['Date'] = format_rfc2822_date(email_date)
-        message['From'] = from_address
-        message['To'] = to_address
+        headers = [
+            ('Date', format_rfc2822_date(email_date)),
+            ('From', from_address),
+            ('To', str(removeSecurityProxy(to_person).preferredemail.email)),
+            ]
 
         # Add the common headers.
-        for header in self.common_headers:
-            message.add_header(*header)
+        headers.extend(self.common_headers)
 
         if references:
-            message['References'] = ' '.join(references)
+            headers.append(('References', ' '.join(references)))
         if message_id is not None:
-            message['Message-Id'] = message_id
+            headers.append(('Message-Id', message_id))
 
         subject_prefix = "[Bug %d]" % self.bug.id
         if subject is None:
-            message['Subject'] = subject_prefix
+            headers.append(('Subject', subject_prefix))
         elif subject_prefix in subject:
-            message['Subject'] = subject
+            headers.append(('Subject', subject))
         else:
-            message['Subject'] = "%s %s" % (subject_prefix, subject)
+            headers.append(('Subject', "%s %s" % (subject_prefix, subject)))
 
         if rationale is not None:
-            message.add_header('X-Launchpad-Message-Rationale', rationale)
+            headers.append(('X-Launchpad-Message-Rationale', rationale))
 
         if filters is not None:
             for filter in filters:
-                message.add_header(
-                    'X-Launchpad-Subscription', filter)
-
+                headers.append(('X-Launchpad-Subscription', filter))
+
+        # XXX cjwatson 2015-07-31: This is cloned-and-hacked from
+        # BaseMailer; it would ultimately be better to convert bug
+        # notifications to that framework.
+        if to_person.expanded_notification_footers:
+            lines = []
+            for key, value in headers:
+                if key.startswith('X-Launchpad-'):
+                    lines.append('%s: %s\n' % (key[2:], value))
+            if lines:
+                body = append_footer(body, ''.join(lines))
+
+        message = MIMEText(body.encode('utf8'), 'plain', 'utf8')
+        for header in headers:
+            message.add_header(*header)
         return message

=== modified file 'lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py'
--- lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py	2015-01-26 01:31:33 +0000
+++ lib/lp/bugs/mail/tests/test_bugnotificationbuilder.py	2015-07-31 14:49:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for BugNotificationBuilder email construction."""
@@ -27,7 +27,7 @@
     def test_build_filters_empty(self):
         """Filters are added."""
         utc_now = datetime.now(pytz.UTC)
-        message = self.builder.build('from', 'to', 'body', 'subject',
+        message = self.builder.build('from', self.bug.owner, 'body', 'subject',
                                      utc_now, filters=[])
         self.assertIs(None,
                       message.get("X-Launchpad-Subscription", None))
@@ -35,7 +35,7 @@
     def test_build_filters_single(self):
         """Filters are added."""
         utc_now = datetime.now(pytz.UTC)
-        message = self.builder.build('from', 'to', 'body', 'subject',
+        message = self.builder.build('from', self.bug.owner, 'body', 'subject',
                                      utc_now, filters=[u"Testing filter"])
         self.assertContentEqual(
             [u"Testing filter"],
@@ -45,7 +45,7 @@
         """Filters are added."""
         utc_now = datetime.now(pytz.UTC)
         message = self.builder.build(
-            'from', 'to', 'body', 'subject', utc_now,
+            'from', self.bug.owner, 'body', 'subject', utc_now,
             filters=[u"Testing filter", u"Second testing filter"])
         self.assertContentEqual(
             [u"Testing filter", u"Second testing filter"],
@@ -54,7 +54,26 @@
     def test_mails_contain_notification_type_header(self):
         utc_now = datetime.now(pytz.UTC)
         message = self.builder.build(
-            'from', 'to', 'body', 'subject', utc_now, filters=[])
+            'from', self.bug.owner, 'body', 'subject', utc_now, filters=[])
         self.assertEqual(
             "bug", message.get("X-Launchpad-Notification-Type", None))
 
+    def test_mails_no_expanded_footer(self):
+        # Recipients without expanded_notification_footers do not receive an
+        # expanded footer on messages.
+        utc_now = datetime.now(pytz.UTC)
+        message = self.builder.build(
+            'from', self.bug.owner, 'body', 'subject', utc_now, filters=[])
+        self.assertNotIn(
+            "Launchpad-Notification-Type", message.get_payload(decode=True))
+
+    def test_mails_append_expanded_footer(self):
+        # Recipients with expanded_notification_footers receive an expanded
+        # footer on messages.
+        utc_now = datetime.now(pytz.UTC)
+        self.bug.owner.expanded_notification_footers = True
+        message = self.builder.build(
+            'from', self.bug.owner, 'body', 'subject', utc_now, filters=[])
+        self.assertIn(
+            "\n-- \nLaunchpad-Notification-Type: bug\n",
+            message.get_payload(decode=True))

=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2015-07-06 17:27:09 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2015-07-31 14:49:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functions related to sending bug notifications."""
@@ -182,7 +182,6 @@
         recipients.items(), key=lambda t: t[0].preferredemail.email)
 
     for email_person, data in sorted_recipients:
-        address = str(email_person.preferredemail.email)
         # 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.
@@ -240,7 +239,7 @@
         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,
+            from_address, email_person, body, subject, email_date,
             rationale, references, msgid, filters=data['filter descriptions'])
         messages.append(msg)
 

=== modified file 'lib/lp/bugs/subscribers/bug.py'
--- lib/lp/bugs/subscribers/bug.py	2013-11-29 14:12:13 +0000
+++ lib/lp/bugs/subscribers/bug.py	2015-07-31 14:49:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -28,9 +28,9 @@
 from lp.bugs.mail.newbug import generate_bug_add_email
 from lp.bugs.model.bug import get_also_notified_subscribers
 from lp.registry.interfaces.person import IPerson
+from lp.registry.model.person import get_recipients
 from lp.services.config import config
 from lp.services.database.sqlbase import block_implicit_flushes
-from lp.services.mail.helpers import get_contact_email_addresses
 from lp.services.mail.sendmail import (
     format_address,
     sendmail,
@@ -203,11 +203,11 @@
             and not event_creator.selfgenerated_bugnotifications):
         new_subs.discard(event_creator)
 
-    to_addrs = set()
+    to_persons = set()
     for new_sub in new_subs:
-        to_addrs.update(get_contact_email_addresses(new_sub))
+        to_persons.update(get_recipients(new_sub))
 
-    if not to_addrs:
+    if not to_persons:
         return False
 
     from_addr = format_address(
@@ -225,13 +225,13 @@
     recipients = bug.getBugNotificationRecipients()
 
     bug_notification_builder = BugNotificationBuilder(bug, event_creator)
-    for to_addr in sorted(to_addrs):
-        reason, rationale = recipients.getReason(to_addr)
+    for to_person in sorted(to_persons):
+        reason, rationale = recipients.getReason(to_person)
         subject, contents = generate_bug_add_email(
             bug, new_recipients=True, subscribed_by=subscribed_by,
             reason=reason, event_creator=event_creator)
         msg = bug_notification_builder.build(
-            from_addr, to_addr, contents, subject, email_date,
+            from_addr, to_person, contents, subject, email_date,
             rationale=rationale, references=references)
         sendmail(msg)