Merge lp:~julian-edwards/launchpad/utf-codes-in-emails-bug-817106 into lp:launchpad

[Julian Edwards]

[r=adeuring][bug=817106] Make notification emails cope with UTF8 in the message.

= Summary =
Make notification emails cope with UTF8.

== Proposed fix ==
Look at https://lists.ubuntu.com/archives/oneiric-changes/2011-July/005553.html
which was the first ever sync done in production - the notification email
has mangled the Changed-By: in the body of the email. We need to fix that!

== Implementation details ==
Apart from Python being an utter PITA when dealing with unicode, the fix was
mostly straightforward apart from changing the fetch_information() function
to return a dictionary instead of that crazy tuple.

Using a dict makes it easier to add more returned data without affecting all
the callsites that don't care about that extra data.

The basic problem was that fetch_information() was assuming that the email
addresses it was returning would only ever be used in the email headers and
encoding them accordingly. I've added extra keys in the dict that contain
the email addresses in unicode strings instead of escaped ascii for headers,
then the whole email body is converted to utf8 before sending.

== Tests ==
bin/test -cvv test_notification

== Demo and Q/A ==
I'll be doing some syncs on staging/dogfood and examining the emails in
the staging inbox.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/adapters/notification.py
  lib/lp/soyuz/adapters/tests/test_notification.py

[Abel Deuring (adeuring)]

nice branch!

Just a remark:

> +# -*- coding: utf-8 -*-
> +# NOTE: The first line above must stay first; do not move the copyright
> +# notice to the top. See http://www.python.org/dev/peps/pep-0263/.
[...]
> + RANDOM_UNICODE = u"Loïc"

I think we avoided to add the "coding: utf-8" line up to now and used string constants like u'Lo\u1234c' instead. But I think it is safe to assume that all editors we use work properly with utf-8 encoded files.

review: Approve (code)

[Julian Edwards (julian-edwards)]

Thanks Abel.

test_ppauploadprocessor.py does this trick too, I think I prefer seeing the real unicode compared to the crazy escape chars! And as you say, I would hope most editors deal with that now.

[Robert Collins (lifeless)]

FTR I'm totally fine with utf8 file encoding; I think our docs /
standards say otherwise somewhere - care to have a quick look on the
wiki / in tree and refresh them with this change?

=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py	2011-07-28 16:06:14 +0000
+++ lib/lp/soyuz/adapters/notification.py	2011-07-29 09:21:46 +0000
@@ -13,6 +13,7 @@
 
 from email.mime.multipart import MIMEMultipart
 from email.mime.text import MIMEText
+from email.utils import formataddr
 import os
 
 from zope.component import getUtility
@@ -218,6 +219,7 @@
         body = assemble_body(
             blamer, spr, bprs, archive, distroseries, summarystring, changes,
             action)
+        body = body.encode("utf8")
         send_mail(
             spr, archive, recipients, subject, body, dry_run,
             changesfile_content=changesfile_content,
@@ -226,8 +228,8 @@
 
     build_and_send_mail(action, recipients)
 
-    (changesfile, date, from_addr, maintainer) = fetch_information(
-        spr, bprs, changes)
+    info = fetch_information(spr, bprs, changes)
+    from_addr = info['changedby']
     if announce_from_person is not None:
         email = announce_from_person.preferredemail
         if email:
@@ -261,13 +263,12 @@
     """Assemble the e-mail notification body."""
     if changes is None:
         changes = {}
-    (changesfile, date, changedby, maintainer) = fetch_information(
-        spr, bprs, changes)
+    info = fetch_information(spr, bprs, changes)
     information = {
         'STATUS': ACTION_DESCRIPTIONS[action],
         'SUMMARY': summary,
-        'DATE': 'Date: %s' % date,
-        'CHANGESFILE': changesfile,
+        'DATE': 'Date: %s' % info['date'],
+        'CHANGESFILE': info['changesfile'],
         'DISTRO': distroseries.distribution.title,
         'ANNOUNCE': 'No announcement sent',
         'CHANGEDBY': '',
@@ -279,8 +280,9 @@
         }
     if spr:
         information['SPR_URL'] = canonical_url(spr)
-    if changedby:
-        information['CHANGEDBY'] = '\nChanged-By: %s' % changedby
+    changedby_displayname = info['changedby_displayname']
+    if changedby_displayname:
+        information['CHANGEDBY'] = '\nChanged-By: %s' % changedby_displayname
     origin = changes.get('Origin')
     if origin:
         information['ORIGIN'] = '\nOrigin: %s' % origin
@@ -291,7 +293,7 @@
         information['ANNOUNCE'] = "Announcing to %s" % (
             distroseries.changeslist)
     try:
-        changedby_person = email_to_person(changedby)
+        changedby_person = email_to_person(info['changedby'])
     except ParseMaintError:
         # Some syncs (e.g. from Debian) will involve packages whose
         # changed-by person was auto-created in LP and hence does not
@@ -299,9 +301,11 @@
         changedby_person = None
     if blamer is not None and blamer != changedby_person:
         signer_signature = person_to_email(blamer)
-        if signer_signature != changedby:
+        if signer_signature != info['changedby']:
             information['SIGNER'] = '\nSigned-By: %s' % signer_signature
     # Add maintainer if present and different from changed-by.
+    maintainer = info['maintainer']
+    changedby = info['changedby']
     if maintainer and maintainer != changedby:
         information['MAINTAINER'] = '\nMaintainer: %s' % maintainer
     return get_template(archive, action) % information
@@ -443,20 +447,19 @@
     """Return a list of recipients for notification emails."""
     candidate_recipients = []
     debug(logger, "Building recipients list.")
-    (changesfile, date, changedby, maint) = fetch_information(
-        spr, bprs, changes)
+    info = fetch_information(spr, bprs, changes)
 
-    if changedby:
+    if info['changedby']:
         try:
-            changer = email_to_person(changedby)
+            changer = email_to_person(info['changedby'])
         except ParseMaintError:
             changer = None
     else:
         changer = None
 
-    if maint:
+    if info['maintainer']:
         try:
-            maintainer = email_to_person(maint)
+            maintainer = email_to_person(info['maintainer'])
         except ParseMaintError:
             maintainer = None
     else:
@@ -567,6 +570,7 @@
 
 def person_to_email(person):
     """Return a string of full name <e-mail address> given an IPerson."""
+    # This will use email.Header to encode any unicode.
     if person and person.preferredemail:
         return format_address_for_person(person)
 
@@ -590,13 +594,18 @@
 
 def fetch_information(spr, bprs, changes):
     changedby = None
+    changedby_displayname = None
     maintainer = None
+    maintainer_displayname = None
+
     if changes:
         changesfile = ChangesFile.formatChangesComment(
             sanitize_string(changes.get('Changes')))
         date = changes.get('Date')
         changedby = sanitize_string(changes.get('Changed-By'))
         maintainer = sanitize_string(changes.get('Maintainer'))
+        changedby_displayname = changedby
+        maintainer_displayname = maintainer
     elif spr or bprs:
         if not spr and bprs:
             spr = bprs[0].build.source_package_release
@@ -604,9 +613,25 @@
         date = spr.dateuploaded
         changedby = person_to_email(spr.creator)
         maintainer = person_to_email(spr.maintainer)
+        if changedby:
+            addr = formataddr((spr.creator.displayname,
+                               spr.creator.preferredemail.email))
+            changedby_displayname = sanitize_string(addr)
+        if maintainer:
+            addr = formataddr((spr.maintainer.displayname,
+                               spr.maintainer.preferredemail.email))
+            maintainer_displayname = sanitize_string(addr)
     else:
         changesfile = date = None
-    return (changesfile, date, changedby, maintainer)
+
+    return {
+        'changesfile': changesfile,
+        'date': date,
+        'changedby': changedby,
+        'changedby_displayname': changedby_displayname,
+        'maintainer': maintainer,
+        'maintainer_displayname': maintainer_displayname,
+        }
 
 
 class LanguagePackEncountered(Exception):
 
=== modified file 'lib/lp/soyuz/adapters/tests/test_notification.py'
--- lib/lp/soyuz/adapters/tests/test_notification.py	2011-07-28 14:02:39 +0000
+++ lib/lp/soyuz/adapters/tests/test_notification.py	2011-07-29 09:21:46 +0000
@@ -1,6 +1,11 @@
+# -*- coding: utf-8 -*-
+# NOTE: The first line above must stay first; do not move the copyright
+# notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
+#
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from email.utils import formataddr
 from storm.store import Store
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -37,6 +42,27 @@
 
     layer = LaunchpadZopelessLayer
 
+    def test_notify_from_unicode_names(self):
+        # People with unicode in their names should appear correctly in the
+        # email and not get smashed to ASCII or otherwise transliterated.
+        RANDOM_UNICODE = u"Loïc"
+        creator = self.factory.makePerson(displayname=RANDOM_UNICODE)
+        spr = self.factory.makeSourcePackageRelease(creator=creator)
+        self.factory.makeSourcePackageReleaseFile(sourcepackagerelease=spr)
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
+        pocket = PackagePublishingPocket.RELEASE
+        distroseries = self.factory.makeDistroSeries()
+        distroseries.changeslist = "[e-mail]"
+        blamer = self.factory.makePerson()
+        notify(
+            blamer, spr, [], [], archive, distroseries, pocket,
+            action='accepted')
+        notifications = pop_notifications()
+        self.assertEqual(2, len(notifications))
+        msg = notifications[1].get_payload(0)
+        body = msg.get_payload(decode=True)
+        self.assertIn("Loïc", body)
+
     def test_calculate_subject_customfile(self):
         lfa = self.factory.makeLibraryFileAlias()
         package_upload = self.factory.makePackageUpload()
@@ -87,33 +113,56 @@
             'Maintainer': 'Foo Bar <[e-mail]>',
             'Changes': ' * Foo!',
             }
-        (changesfile, date, changedby, maintainer) = fetch_information(
+        info = fetch_information(
             None, None, changes)
-        self.assertEqual('2001-01-01', date)
-        self.assertEqual(' * Foo!', changesfile)
-        for field in (changedby, maintainer):
+        self.assertEqual('2001-01-01', info['date'])
+        self.assertEqual(' * Foo!', info['changesfile'])
+        fields = [
+            info['changedby'],
+            info['maintainer'],
+            info['changedby_displayname'],
+            info['maintainer_displayname'],
+            ]
+        for field in fields:
             self.assertEqual('Foo Bar <[e-mail]>', field)
 
     def test_fetch_information_spr(self):
-        spr = self.factory.makeSourcePackageRelease()
-        (changesfile, date, changedby, maintainer) = fetch_information(
-            spr, None, None)
-        self.assertEqual(date, spr.dateuploaded)
-        self.assertEqual(changesfile, spr.changelog_entry)
-        self.assertEqual(changedby, format_address_for_person(spr.creator))
-        self.assertEqual(
-            maintainer, format_address_for_person(spr.maintainer))
+        creator = self.factory.makePerson(displayname=u"foø")
+        maintainer = self.factory.makePerson(displayname=u"bær")
+        spr = self.factory.makeSourcePackageRelease(
+            creator=creator, maintainer=maintainer)
+        info = fetch_information(spr, None, None)
+        self.assertEqual(info['date'], spr.dateuploaded)
+        self.assertEqual(info['changesfile'], spr.changelog_entry)
+        self.assertEqual(
+            info['changedby'], format_address_for_person(spr.creator))
+        self.assertEqual(
+            info['maintainer'], format_address_for_person(spr.maintainer))
+        self.assertEqual(
+            u"foø <%s>" % spr.creator.preferredemail.email,
+            info['changedby_displayname'])
+        self.assertEqual(
+            u"bær <%s>" % spr.maintainer.preferredemail.email,
+            info['maintainer_displayname'])
 
     def test_fetch_information_bprs(self):
         bpr = self.factory.makeBinaryPackageRelease()
-        (changesfile, date, changedby, maintainer) = fetch_information(
-            None, [bpr], None)
+        info = fetch_information(None, [bpr], None)
         spr = bpr.build.source_package_release
-        self.assertEqual(date, spr.dateuploaded)
-        self.assertEqual(changesfile, spr.changelog_entry)
-        self.assertEqual(changedby, format_address_for_person(spr.creator))
-        self.assertEqual(
-            maintainer, format_address_for_person(spr.maintainer))
+        self.assertEqual(info['date'], spr.dateuploaded)
+        self.assertEqual(info['changesfile'], spr.changelog_entry)
+        self.assertEqual(
+            info['changedby'], format_address_for_person(spr.creator))
+        self.assertEqual(
+            info['maintainer'], format_address_for_person(spr.maintainer))
+        self.assertEqual(
+            info['changedby_displayname'],
+            formataddr((spr.creator.displayname,
+                        spr.creator.preferredemail.email)))
+        self.assertEqual(
+            info['maintainer_displayname'],
+            formataddr((spr.maintainer.displayname,
+                        spr.maintainer.preferredemail.email)))
 
     def test_calculate_subject_spr(self):
         spr = self.factory.makeSourcePackageRelease()
@@ -291,4 +340,3 @@
         result = is_auto_sync_upload(
             spr=None, bprs=None, pocket=None, changed_by_email=None)
         self.assertFalse(result)
-