launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19085
[Merge] lp:~wgrant/launchpad/soyuz-notifications-unicode into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/soyuz-notifications-unicode into lp:launchpad with lp:~wgrant/launchpad/maintainably-fix-safe-fix-maintainer as a prerequisite.
Commit message:
Rewrite Soyuz notification Changed-By/Maintainer/Signed-By handling to be less confusing and avoid unnecessarily RFC2047-encoding Signed-By in the body.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1479206 in Launchpad itself: "Upload notification body's Signed-By can be RFC2047-encoded"
https://bugs.launchpad.net/launchpad/+bug/1479206
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/soyuz-notifications-unicode/+merge/266186
Rewrite Soyuz notification Changed-By/Maintainer/Signed-By handling to be less confusing and avoid unnecessarily RFC2047-encoding Signed-By in the body.
rfc2047_encode_address, introduced in the previous branch, dies. rfc822_encode_address could probably disappear in favour of email.utils.formataddr, but they handle special characters in very different ways.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/soyuz-notifications-unicode into lp:launchpad.
=== modified file 'lib/lp/archiveuploader/tests/test_utils.py'
--- lib/lp/archiveuploader/tests/test_utils.py 2015-07-29 07:12:25 +0000
+++ lib/lp/archiveuploader/tests/test_utils.py 2015-07-29 07:12:25 +0000
@@ -147,77 +147,65 @@
"""
from lp.archiveuploader.utils import (
parse_maintainer_bytes,
- rfc2047_encode_address,
rfc822_encode_address,
)
cases = (
("No\xc3\xa8l K\xc3\xb6the <noel@xxxxxxxxxx>",
u"No\xe8l K\xf6the <noel@xxxxxxxxxx>",
- u"=?utf-8?b?Tm/DqGwgS8O2dGhl?= <noel@xxxxxxxxxx>",
u"No\xe8l K\xf6the",
u"noel@xxxxxxxxxx"),
("No\xe8l K\xf6the <noel@xxxxxxxxxx>",
u"No\xe8l K\xf6the <noel@xxxxxxxxxx>",
- u"=?utf-8?b?Tm/DqGwgS8O2dGhl?= <noel@xxxxxxxxxx>",
u"No\xe8l K\xf6the",
u"noel@xxxxxxxxxx"),
("James Troup <james@xxxxxxxxxx>",
u"James Troup <james@xxxxxxxxxx>",
- u"James Troup <james@xxxxxxxxxx>",
u"James Troup",
u"james@xxxxxxxxxx"),
("James J. Troup <james@xxxxxxxxxx>",
u"james@xxxxxxxxxx (James J. Troup)",
- u"james@xxxxxxxxxx (James J. Troup)",
u"James J. Troup",
u"james@xxxxxxxxxx"),
("James J, Troup <james@xxxxxxxxxx>",
u"james@xxxxxxxxxx (James J, Troup)",
- u"james@xxxxxxxxxx (James J, Troup)",
u"James J, Troup",
u"james@xxxxxxxxxx"),
("james@xxxxxxxxxx",
u" <james@xxxxxxxxxx>",
- u" <james@xxxxxxxxxx>",
u"",
u"james@xxxxxxxxxx"),
("<james@xxxxxxxxxx>",
u" <james@xxxxxxxxxx>",
- u" <james@xxxxxxxxxx>",
u"",
u"james@xxxxxxxxxx"),
("Cris van Pelt <\"Cris van Pelt\"@tribe.eu.org>",
u"Cris van Pelt <\"Cris van Pelt\"@tribe.eu.org>",
- u"Cris van Pelt <\"Cris van Pelt\"@tribe.eu.org>",
u"Cris van Pelt",
u"\"Cris van Pelt\"@tribe.eu.org"),
("Zak B. Elep <zakame@xxxxxxxxxx>",
u"zakame@xxxxxxxxxx (Zak B. Elep)",
- u"zakame@xxxxxxxxxx (Zak B. Elep)",
u"Zak B. Elep",
u"zakame@xxxxxxxxxx"),
("zakame@xxxxxxxxxx (Zak B. Elep)",
u" <zakame@xxxxxxxxxx (Zak B. Elep)>",
- u" <zakame@xxxxxxxxxx (Zak B. Elep)>",
u"",
u"zakame@xxxxxxxxxx (Zak B. Elep)"),
)
for case in cases:
(name, email) = parse_maintainer_bytes(case[0], 'Maintainer')
- self.assertEquals(case[3], name)
- self.assertEquals(case[4], email)
+ self.assertEquals(case[2], name)
+ self.assertEquals(case[3], email)
self.assertEquals(case[1], rfc822_encode_address(name, email))
- self.assertEquals(case[2], rfc2047_encode_address(name, email))
def testParseMaintainerRaises(self):
"""lp.archiveuploader.utils.parse_maintainer should raise on incorrect
=== modified file 'lib/lp/archiveuploader/utils.py'
--- lib/lp/archiveuploader/utils.py 2015-07-29 07:12:25 +0000
+++ lib/lp/archiveuploader/utils.py 2015-07-29 07:12:25 +0000
@@ -25,7 +25,6 @@
're_valid_pkg_name',
're_changes_file_name',
're_extract_src_version',
- 'rfc2047_encode_address',
'rfc822_encode_address',
'UploadError',
'UploadWarning',
@@ -33,7 +32,6 @@
from collections import defaultdict
-from email.header import Header
import os
import re
import signal
@@ -223,9 +221,11 @@
name and email must be Unicode. If they contain non-ASCII
characters, the result is not RFC822-compliant and you should use
- rfc2047_encode_address instead.
+ something like format_address instead.
- If the name field contains '.' or ',' the 'email (name)' format is used.
+ This is similar to email.utils.format_addr, except that it handles
+ special characters using the 'email (name)' format rather than
+ '"name" (email)'.
"""
# If the maintainer's name contains a full stop then the whole field will
# not work directly as an email address due to a misfeature in the syntax
@@ -237,22 +237,6 @@
return u"%s <%s>" % (name, email)
-def rfc2047_encode_address(name, email):
- """Return an RFC2047 encoding of a name and an email address.
-
- name and email must be Unicode strings, and email must be
- ASCII-only.
-
- If the name field contains '.' or ',' the 'email (name)' format is used.
- """
- try:
- email.encode('ascii')
- except UnicodeDecodeError:
- raise AssertionError("Email addresses must be ASCII.")
- return rfc822_encode_address(
- Header(name, 'utf-8', 998).encode().decode('ascii'), email)
-
-
def extract_dpkg_source(dsc_filepath, target, vendor=None):
"""Extract a source package by dsc file path.
=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py 2015-07-29 07:12:25 +0000
+++ lib/lp/soyuz/adapters/notification.py 2015-07-29 07:12:25 +0000
@@ -12,7 +12,6 @@
from email.mime.multipart import MIMEMultipart
from email.mime.text import MIMEText
-from email.utils import formataddr
import os
from zope.component import getUtility
@@ -23,7 +22,6 @@
from lp.archiveuploader.utils import (
parse_maintainer_bytes,
ParseMaintError,
- rfc2047_encode_address,
rfc822_encode_address,
)
from lp.registry.interfaces.person import IPersonSet
@@ -231,11 +229,11 @@
info = fetch_information(spr, bprs, changes)
from_addr = info['changedby']
- from_email = info['changedby_email']
- if announce_from_person is not None:
- if announce_from_person.preferredemail is not None:
- from_addr = format_address_for_person(announce_from_person)
- from_email = announce_from_person.preferredemail.email
+ if (announce_from_person is not None
+ and announce_from_person.preferredemail is not None):
+ from_addr = (
+ announce_from_person.displayname,
+ announce_from_person.preferredemail.email)
# If we're sending an acceptance notification for a non-PPA upload,
# announce if possible. Avoid announcing backports, binary-only
@@ -244,7 +242,7 @@
and not archive.is_ppa
and pocket != PackagePublishingPocket.BACKPORTS
and not (pocket == PackagePublishingPocket.SECURITY and spr is None)
- and not is_auto_sync_upload(spr, bprs, pocket, from_email)):
+ and not is_auto_sync_upload(spr, bprs, pocket, from_addr)):
name = None
bcc_addr = None
if spr:
@@ -257,8 +255,9 @@
bcc_addr = email_base.format(package_name=name)
build_and_send_mail(
- 'announcement', [str(distroseries.changeslist)], from_addr,
- bcc_addr, previous_version=previous_version)
+ 'announcement', [str(distroseries.changeslist)],
+ format_address(*from_addr) if from_addr else None, bcc_addr,
+ previous_version=previous_version)
def assemble_body(blamer, spr, bprs, archive, distroseries, summary, changes,
@@ -286,9 +285,22 @@
if spr:
information['SPR_URL'] = canonical_url(
distroseries.distribution.getSourcePackageRelease(spr))
- changedby_displayname = info['changedby_displayname']
- if changedby_displayname:
- information['CHANGEDBY'] = '\nChanged-By: %s' % changedby_displayname
+
+ # Some syncs (e.g. from Debian) will involve packages whose
+ # changed-by person was auto-created in LP and hence does not have a
+ # preferred email address set. We'll get a None here.
+ changedby_person = addr_to_person(info['changedby'])
+ if info['changedby']:
+ information['CHANGEDBY'] = (
+ '\nChanged-By: %s' % rfc822_encode_address(*info['changedby']))
+ if (blamer is not None and blamer != changedby_person
+ and blamer.preferredemail):
+ information['SIGNER'] = '\nSigned-By: %s' % rfc822_encode_address(
+ blamer.displayname, blamer.preferredemail.email)
+ if info['maintainer'] and info['maintainer'] != info['changedby']:
+ information['MAINTAINER'] = (
+ '\nMaintainer: %s' % rfc822_encode_address(*info['maintainer']))
+
origin = changes.get('Origin')
if origin:
information['ORIGIN'] = '\nOrigin: %s' % origin
@@ -299,20 +311,6 @@
information['ANNOUNCE'] = "Announcing to %s" % (
distroseries.changeslist)
- # Some syncs (e.g. from Debian) will involve packages whose
- # changed-by person was auto-created in LP and hence does not have a
- # preferred email address set. We'll get a None here.
- changedby_person = email_to_person(info['changedby_email'])
-
- if blamer is not None and blamer != changedby_person:
- signer_signature = person_to_email(blamer)
- if signer_signature != info['changedby']:
- information['SIGNER'] = '\nSigned-By: %s' % signer_signature
- # Add maintainer if present and different from changed-by.
- maintainer_displayname = info['maintainer_displayname']
- if (maintainer_displayname and
- maintainer_displayname != changedby_displayname):
- information['MAINTAINER'] = '\nMaintainer: %s' % maintainer_displayname
return get_template(archive, action) % information
@@ -463,8 +461,8 @@
candidate_recipients = [blamer]
info = fetch_information(spr, bprs, changes)
- changer = email_to_person(info['changedby_email'])
- maintainer = email_to_person(info['maintainer_email'])
+ changer = addr_to_person(info['changedby'])
+ maintainer = addr_to_person(info['maintainer'])
if blamer is None and not archive.is_copy:
debug(logger, "Changes file is unsigned; adding changer as recipient.")
@@ -557,55 +555,26 @@
return summary
-def email_to_person(email):
- """Return an `IPerson` given an email address (without a name).
+def addr_to_person(addr):
+ """Return an `IPerson` given a name and email address.
- :param email: Potential email address.
+ :param addr: (name, email) tuple. The name is ignored.
:return: `IPerson` with the given email address. None if there
- isn't one, or if `email` is None.
+ isn't one, or if `addr` is None.
"""
- if not email:
+ if addr is None:
return None
- return getUtility(IPersonSet).getByEmail(email)
-
-
-def person_to_email(person):
- """Return a string of full name <email address> given an IPerson."""
- if person and person.preferredemail:
- # This will use email.header to encode any non-ASCII characters.
- return format_address_for_person(person)
-
-
-def fix_email(fullemail, field_name):
- """Turn an email address from .changes into various useful forms.
-
- The input address may be None, or anything that `parse_maintainer_bytes`
- understands.
-
- :return: A tuple of (RFC2047-compatible address, Unicode
- RFC822-compatible address, email).
- """
- if not fullemail:
- return None, None, None
-
- try:
- name, email = parse_maintainer_bytes(fullemail, field_name)
- return (
- rfc2047_encode_address(name, email).encode('utf-8'),
- rfc822_encode_address(name, email),
- email.encode('ascii'))
- except ParseMaintError:
- return None, None, None
-
-
-def is_auto_sync_upload(spr, bprs, pocket, changed_by_email):
+ return getUtility(IPersonSet).getByEmail(addr[1])
+
+
+def is_auto_sync_upload(spr, bprs, pocket, changed_by):
"""Return True if this is a (Debian) auto sync upload.
Sync uploads are source-only, unsigned and not targeted to
the security pocket. The Changed-By field is also the Katie
user (archive@xxxxxxxxxx).
"""
- changed_by = email_to_person(changed_by_email)
+ changed_by = addr_to_person(changed_by)
return (
spr and
not bprs and
@@ -614,50 +583,40 @@
def fetch_information(spr, bprs, changes, previous_version=None):
- changedby = None
- changedby_displayname = None
- changedby_email = None
- maintainer = None
- maintainer_displayname = None
- maintainer_email = None
+ changelog = date = changedby = maintainer = None
if changes:
- changesfile = ChangesFile.formatChangesComment(
+ changelog = ChangesFile.formatChangesComment(
sanitize_string(changes.get('Changes')))
date = changes.get('Date')
- changedby, changedby_displayname, changedby_email = fix_email(
- changes.get('Changed-By'), 'Changed-By')
- maintainer, maintainer_displayname, maintainer_email = fix_email(
- changes.get('Maintainer'), 'Maintainer')
+ try:
+ changedby = parse_maintainer_bytes(
+ changes.get('Changed-By'), 'Changed-By')
+ except ParseMaintError:
+ pass
+ try:
+ maintainer = parse_maintainer_bytes(
+ changes.get('Maintainer'), 'Maintainer')
+ except ParseMaintError:
+ pass
elif spr or bprs:
if not spr and bprs:
spr = bprs[0].build.source_package_release
- changesfile = spr.aggregate_changelog(previous_version)
+ changelog = spr.aggregate_changelog(previous_version)
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)
- changedby_email = spr.creator.preferredemail.email
- if maintainer:
- addr = formataddr((spr.maintainer.displayname,
- spr.maintainer.preferredemail.email))
- maintainer_displayname = sanitize_string(addr)
- maintainer_email = spr.maintainer.preferredemail.email
- else:
- changesfile = date = None
+ if spr.creator and spr.creator.preferredemail:
+ changedby = (
+ spr.creator.displayname, spr.creator.preferredemail.email)
+ if spr.maintainer and spr.maintainer.preferredemail:
+ maintainer = (
+ spr.maintainer.displayname,
+ spr.maintainer.preferredemail.email)
return {
- 'changelog': changesfile,
+ 'changelog': changelog,
'date': date,
'changedby': changedby,
- 'changedby_displayname': changedby_displayname,
- 'changedby_email': changedby_email,
'maintainer': maintainer,
- 'maintainer_displayname': maintainer_displayname,
- 'maintainer_email': maintainer_email,
}
=== modified file 'lib/lp/soyuz/adapters/tests/test_notification.py'
--- lib/lp/soyuz/adapters/tests/test_notification.py 2014-11-08 23:53:17 +0000
+++ lib/lp/soyuz/adapters/tests/test_notification.py 2015-07-29 07:12:25 +0000
@@ -5,7 +5,6 @@
# Copyright 2011-2014 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 textwrap import dedent
from storm.store import Store
@@ -24,7 +23,6 @@
get_upload_notification_recipients,
is_auto_sync_upload,
notify,
- person_to_email,
reject_changes_file,
)
from lp.soyuz.enums import (
@@ -54,15 +52,14 @@
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)
+ creator = self.factory.makePerson(displayname=u"Loïc")
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()
+ blamer = self.factory.makePerson(displayname=u"Stéphane")
notify(
blamer, spr, [], [], archive, distroseries, pocket,
action='accepted')
@@ -70,7 +67,8 @@
self.assertEqual(2, len(notifications))
msg = notifications[1].get_payload(0)
body = msg.get_payload(decode=True)
- self.assertIn("Loïc", body)
+ self.assertIn("Changed-By: Loïc", body)
+ self.assertIn("Signed-By: Stéphane", body)
def test_calculate_subject_customfile(self):
lfa = self.factory.makeLibraryFileAlias()
@@ -196,14 +194,15 @@
archive = self.factory.makeArchive(owner=person, name='ppa')
pocket = self.factory.getAnyPocket()
distroseries = self.factory.makeDistroSeries()
- person = self.factory.makePerson()
+ person = self.factory.makePerson(
+ displayname=u'Blamer', email='blamer@xxxxxxxxxxx')
notify(
person, None, [bpr], [], archive, distroseries, pocket,
summary_text="Rejected by archive administrator.",
action='rejected')
[notification] = pop_notifications()
body = notification.get_payload()[0].get_payload()
- self.assertEqual(person_to_email(person), notification['To'])
+ self.assertEqual('Blamer <blamer@xxxxxxxxxxx>', notification['To'])
expected_body = dedent("""\
Rejected:
Rejected by archive administrator.
@@ -242,11 +241,9 @@
fields = [
info['changedby'],
info['maintainer'],
- info['changedby_displayname'],
- info['maintainer_displayname'],
]
for field in fields:
- self.assertEqual('Foo Bar <foo.bar@xxxxxxxxxxx>', field)
+ self.assertEqual((u'Foo Bar', u'foo.bar@xxxxxxxxxxx'), field)
def test_fetch_information_spr(self):
creator = self.factory.makePerson(displayname=u"foø")
@@ -257,15 +254,9 @@
self.assertEqual(info['date'], spr.dateuploaded)
self.assertEqual(info['changelog'], 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'])
+ (u"foø", spr.creator.preferredemail.email), info['changedby'])
+ self.assertEqual(
+ (u"bær", spr.maintainer.preferredemail.email), info['maintainer'])
def test_fetch_information_bprs(self):
bpr = self.factory.makeBinaryPackageRelease()
@@ -274,17 +265,11 @@
self.assertEqual(info['date'], spr.dateuploaded)
self.assertEqual(info['changelog'], 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)))
+ (spr.creator.displayname, spr.creator.preferredemail.email),
+ info['changedby'])
+ self.assertEqual(
+ (spr.maintainer.displayname, spr.maintainer.preferredemail.email),
+ info['maintainer'])
def test_calculate_subject_spr(self):
spr = self.factory.makeSourcePackageRelease()
@@ -461,5 +446,5 @@
# If changer has no preferred email address,
# is_auto_sync_upload should still work.
result = is_auto_sync_upload(
- spr=None, bprs=None, pocket=None, changed_by_email=None)
+ spr=None, bprs=None, pocket=None, changed_by=None)
self.assertFalse(result)
=== modified file 'lib/lp/soyuz/doc/soyuz-set-of-uploads.txt'
--- lib/lp/soyuz/doc/soyuz-set-of-uploads.txt 2015-07-08 19:20:29 +0000
+++ lib/lp/soyuz/doc/soyuz-set-of-uploads.txt 2015-07-29 07:12:25 +0000
@@ -337,7 +337,7 @@
Date: Tue, 25 Apr 2006 10:36:14 -0300
Changed-By: cprov@xxxxxxxxxx (Celso R. Providelo)
Maintainer: Launchpad team <launchpad@xxxxxxxxxxxxxxxxxxx>
- Signed-By: "Foo B. Bar" <foo.bar@xxxxxxxxxxxxx>
+ Signed-By: foo.bar@xxxxxxxxxxxxx (Foo B. Bar)
http://launchpad.dev/ubuntutest/+source/bar/1.0-4
<BLANKLINE>
=3D=3D
Follow ups
