launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03960
[Merge] lp:~bac/launchpad/bug-519857 into lp:launchpad
Brad Crittenden has proposed merging lp:~bac/launchpad/bug-519857 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #519857 in Launchpad itself: "Upload processor OOPSes with bad email addresses"
https://bugs.launchpad.net/launchpad/+bug/519857
For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-519857/+merge/64606
= Summary =
If a malformed email address appeared in an uploaded package's changes
file then an OOPS was thrown.
== Proposed fix ==
Catch the ParseMaintError and simply don't include the bad address in
the recipients list.
== Pre-implementation notes ==
Chatted with Julian.
== Tests ==
New unit tests for get_repients:
bin/test -vvm lp.soyuz -t test_get_recipients
== Demo and Q/A ==
Upload a package with a bad email address in it.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/archiveuploader/utils.py
lib/lp/soyuz/adapters/notification.py
lib/lp/soyuz/adapters/tests/test_notification.py
--
https://code.launchpad.net/~bac/launchpad/bug-519857/+merge/64606
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-519857 into lp:launchpad.
=== modified file 'lib/lp/archiveuploader/utils.py'
--- lib/lp/archiveuploader/utils.py 2011-03-21 12:55:50 +0000
+++ lib/lp/archiveuploader/utils.py 2011-06-14 20:26:07 +0000
@@ -257,7 +257,7 @@
"""Wrapper for fix_maintainer() to handle unicode and string argument.
It verifies the content type and transform it in a unicode with guess()
- before call ascii_smash(). Then we can safelly call fix_maintainer().
+ before call ascii_smash(). Then we can safely call fix_maintainer().
"""
if type(content) != unicode:
content = guess_encoding(content)
=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py 2011-06-14 10:11:12 +0000
+++ lib/lp/soyuz/adapters/notification.py 2011-06-14 20:26:07 +0000
@@ -6,6 +6,7 @@
__metaclass__ = type
__all__ = [
+ 'get_recipients', # Available for testing only.
'notify',
]
@@ -18,22 +19,25 @@
from canonical.config import config
from canonical.launchpad.helpers import get_email_template
-from canonical.launchpad.mail import (
- format_address,
- sendmail,
- )
from canonical.launchpad.webapp import canonical_url
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.archivepublisher.utils import get_ppa_reference
from lp.archiveuploader.changesfile import ChangesFile
-from lp.archiveuploader.utils import safe_fix_maintainer
+from lp.archiveuploader.utils import (
+ ParseMaintError,
+ safe_fix_maintainer,
+ )
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.services.encoding import (
ascii_smash,
guess as guess_encoding,
)
-from lp.services.mail.sendmail import format_address_for_person
+from lp.services.mail.sendmail import (
+ format_address,
+ format_address_for_person,
+ sendmail,
+ )
def reject_changes_file(blamer, changes_file_path, changes, archive,
@@ -92,7 +96,7 @@
'unapproved': 'Waiting for approval',
'rejected': 'Rejected',
'accepted': 'Accepted',
- 'announcement': 'Accepted'
+ 'announcement': 'Accepted',
}
@@ -122,7 +126,7 @@
def notify(blamer, spr, bprs, customfiles, archive, distroseries, pocket,
summary_text=None, changes=None, changesfile_content=None,
changesfile_object=None, action=None, dry_run=False, logger=None):
- """Notify about
+ """Notify about
:param blamer: The `IPerson` who is to blame for this notification.
:param spr: The `ISourcePackageRelease` that was created.
@@ -353,8 +357,8 @@
debug(logger, " Subject: %s" % subject)
debug(logger, " Sender: %s" % from_addr)
debug(logger, " Recipients: %s" % recipients)
- if extra_headers.has_key('Bcc'):
- debug(logger, " Bcc: %s" % extra_headers['Bcc'])
+ if 'Bcc' in extra_headers:
+ debug(logger, " Bcc: %s" % extra_headers['Bcc'])
debug(logger, " Body:")
for line in mail_text.splitlines():
debug(logger, line)
@@ -423,12 +427,20 @@
debug(logger, "Building recipients list.")
(changesfile, date, changedby, maint) = fetch_information(
spr, bprs, changes)
+
if changedby:
- changer = email_to_person(changedby)
+ try:
+ changer = email_to_person(changedby)
+ except ParseMaintError:
+ changer = None
else:
changer = None
+
if maint:
- maintainer = email_to_person(maint)
+ try:
+ maintainer = email_to_person(maint)
+ except ParseMaintError:
+ maintainer = None
else:
maintainer = None
@@ -450,7 +462,7 @@
candidate_recipients.extend(uploaders)
# If this is not a PPA, we also consider maintainer and changed-by.
- if blamer and not archive.is_ppa:
+ elif blamer is not None:
if (maintainer and maintainer != blamer and
maintainer.isUploader(distroseries.distribution)):
debug(logger, "Adding maintainer to recipients")
@@ -467,8 +479,7 @@
for person in candidate_recipients:
if person is None or person.preferredemail is None:
continue
- recipient = format_address(person.displayname,
- person.preferredemail.email)
+ recipient = format_address_for_person(person)
debug(logger, "Adding recipient: '%s'" % recipient)
recipients.append(recipient)
@@ -572,6 +583,8 @@
date = spr.dateuploaded
changedby = person_to_email(spr.creator)
maintainer = person_to_email(spr.maintainer)
+ else:
+ changesfile = date = None
return (changesfile, date, changedby, maintainer)
=== modified file 'lib/lp/soyuz/adapters/tests/test_notification.py'
--- lib/lp/soyuz/adapters/tests/test_notification.py 2011-06-14 10:07:54 +0000
+++ lib/lp/soyuz/adapters/tests/test_notification.py 2011-06-14 20:26:07 +0000
@@ -1,6 +1,8 @@
# Copyright 2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
+from storm.store import Store
+from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
from canonical.testing.layers import LaunchpadZopelessLayer
@@ -9,12 +11,18 @@
from lp.services.log.logger import BufferLogger
from lp.soyuz.adapters.notification import (
calculate_subject,
+ get_recipients,
fetch_information,
reject_changes_file,
person_to_email,
notify,
)
-from lp.soyuz.enums import PackageUploadCustomFormat
+from lp.soyuz.interfaces.component import IComponentSet
+from lp.soyuz.model.component import ComponentSelection
+from lp.soyuz.enums import (
+ ArchivePurpose,
+ PackageUploadCustomFormat,
+ )
from lp.testing import TestCaseWithFactory
from lp.testing.mail_helpers import pop_notifications
@@ -28,7 +36,7 @@
'Date': '2001-01-01',
'Changed-By': 'Foo Bar <foo.bar@xxxxxxxxxxxxx>',
'Maintainer': 'Foo Bar <foo.bar@xxxxxxxxxxxxx>',
- 'Changes': ' * Foo!'
+ 'Changes': ' * Foo!',
}
(changesfile, date, changedby, maintainer) = fetch_information(
None, None, changes)
@@ -65,7 +73,7 @@
distroseries = self.factory.makeDistroSeries()
expected_subject = '[PPA %s] [%s/%s] %s %s (Accepted)' % (
get_ppa_reference(archive), distroseries.distribution.name,
- distroseries.getSuite(pocket), spr.name, spr.version)
+ distroseries.getSuite(pocket), spr.name, spr.version)
subject = calculate_subject(
spr, [], [], archive, distroseries, pocket, 'accepted')
self.assertEqual(expected_subject, subject)
@@ -130,6 +138,7 @@
body = notification.as_string()
self.assertEqual(person_to_email(person), notification['To'])
self.assertIn('Rejected by archive administrator.\n\n* Foo!\n', body)
+<<<<<<< TREE
def test_reject_changes_file_no_email(self):
# If we are rejecting a mail, and the person to notify has no
@@ -146,3 +155,79 @@
logger=logger)
self.assertIn(
'No recipients have a preferred email.', logger.getLogBuffer())
+=======
+
+ def _run_recipients_test(self, changes, blamer, maintainer, changer):
+ distribution = self.factory.makeDistribution()
+ archive = self.factory.makeArchive(
+ distribution=distribution, purpose=ArchivePurpose.PRIMARY)
+ distroseries = self.factory.makeDistroSeries(
+ distribution=distribution)
+ # Now set the uploaders.
+ component = getUtility(IComponentSet).ensure('main')
+ if component not in distroseries.components:
+ store = Store.of(distroseries)
+ store.add(
+ ComponentSelection(
+ distroseries=distroseries, component=component))
+ archive.newComponentUploader(maintainer, component)
+ archive.newComponentUploader(changer, component)
+ return get_recipients(
+ blamer, archive, distroseries, logger=None, changes=changes)
+
+ def test_get_recipients_good_emails(self):
+ # Test get_recipients with good email addresses..
+ blamer = self.factory.makePerson()
+ maintainer = self.factory.makePerson(
+ 'maintainer@xxxxxxxxxxxxx', displayname='Maintainer')
+ changer = self.factory.makePerson(
+ 'changer@xxxxxxxxxxxxx', displayname='Changer')
+ changes = {
+ 'Date': '2001-01-01',
+ 'Changed-By': 'Changer <changer@xxxxxxxxxxxxx>',
+ 'Maintainer': 'Maintainer <maintainer@xxxxxxxxxxxxx>',
+ 'Changes': ' * Foo!',
+ }
+ recipients = self._run_recipients_test(
+ changes, blamer, maintainer, changer)
+ expected = [format_address_for_person(p)
+ for p in (blamer, maintainer, changer)]
+ self.assertEqual(expected, recipients)
+
+ def test_get_recipients_bad_maintainer_email(self):
+ blamer = self.factory.makePerson()
+ maintainer = self.factory.makePerson(
+ 'maintainer@xxxxxxxxxxxxx', displayname='Maintainer')
+ changer = self.factory.makePerson(
+ 'changer@xxxxxxxxxxxxx', displayname='Changer')
+ changes = {
+ 'Date': '2001-01-01',
+ 'Changed-By': 'Changer <changer@xxxxxxxxxxxxx>',
+ 'Maintainer': 'Maintainer <maintainer at canonical.com>',
+ 'Changes': ' * Foo!',
+ }
+ recipients = self._run_recipients_test(
+ changes, blamer, maintainer, changer)
+ expected = [format_address_for_person(p)
+ for p in (blamer, changer)]
+ self.assertEqual(expected, recipients)
+
+ def test_get_recipients_bad_changedby_email(self):
+ # Test get_recipients with invalid changedby email address.
+ blamer = self.factory.makePerson()
+ maintainer = self.factory.makePerson(
+ 'maintainer@xxxxxxxxxxxxx', displayname='Maintainer')
+ changer = self.factory.makePerson(
+ 'changer@xxxxxxxxxxxxx', displayname='Changer')
+ changes = {
+ 'Date': '2001-01-01',
+ 'Changed-By': 'Changer <changer at canonical.com>',
+ 'Maintainer': 'Maintainer <maintainer@xxxxxxxxxxxxx>',
+ 'Changes': ' * Foo!',
+ }
+ recipients = self._run_recipients_test(
+ changes, blamer, maintainer, changer)
+ expected = [format_address_for_person(p)
+ for p in (blamer, maintainer)]
+ self.assertEqual(expected, recipients)
+>>>>>>> MERGE-SOURCE
