launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04421
[Merge] lp:~julian-edwards/launchpad/utf-codes-in-emails-bug-817106 into lp:launchpad
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/utf-codes-in-emails-bug-817106 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/utf-codes-in-emails-bug-817106/+merge/69758
= 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
--
https://code.launchpad.net/~julian-edwards/launchpad/utf-codes-in-emails-bug-817106/+merge/69758
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/utf-codes-in-emails-bug-817106 into lp:launchpad.
=== 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:04:37 +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,6 +228,7 @@
build_and_send_mail(action, recipients)
+<<<<<<< TREE
(changesfile, date, from_addr, maintainer) = fetch_information(
spr, bprs, changes)
if announce_from_person is not None:
@@ -233,6 +236,10 @@
if email:
from_addr = email.email
+=======
+ info = fetch_information(spr, bprs, changes)
+ from_addr = info['changedby']
+>>>>>>> MERGE-SOURCE
# If we're sending an acceptance notification for a non-PPA upload,
# announce if possible. Avoid announcing backports, binary-only
# security uploads, or autosync uploads.
@@ -261,13 +268,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 +285,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 +298,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 +306,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 +452,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 +575,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 +599,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 +618,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:04:37 +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 = "blah@xxxxxxxxxxx"
+ 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 <foo.bar@xxxxxxxxxxxxx>',
'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 <foo.bar@xxxxxxxxxxxxx>', 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)
-
