Merge lp:~jtv/launchpad/pre-876594 into lp:launchpad

[Jeroen T. Vermeulen]

[r=lifeless][no-qa] Cleanups for package upload notification.

= Summary =

We need to make some changes to Soyuz upload notification. Actually this also covers copying, building, and syncing of packages.

Unfortunately the code has become a bit of a mess and some of the tests are painfully brittle.

== Proposed fix ==

Before fixing the bug itself, clean up the code. Make tests independent of arbitrary ordering of email addressee lists; eliminate needless repetition of code; make the name of the function we'll be changing a bit more specific; make complicated code a bit more concise so we can reason about it more effectively.

== Pre-implementation notes ==

Discussed the bigger picture with Julian. We're going to exempt package maintainers and authors from notification for packages that are being accepted into different archives than their releases are in. We'll also want to see if we can remove complexity from notify() that reflects knowledge inherent in the callsite.

== Implementation details ==

The recipients-gathering code no longer checks for various recipients being the same. Instead it just lumps all recipients into a set. Duplicates disappear as a matter of course.

This will affect the logging a bit: the code will now happily say it's adding both the maintainer and the last author to the set of recipients, even if they're the same person. It may actually be clearer; certainly the code becomes easier to manage.

== Tests ==

{{{
./bin/test -vvc lp.archiveuploader
./bin/test -vvc lp.soyuz
}}}

== Demo and Q/A ==

There are no functional changes, so as long as nothing oopses, it should be fine.

= Launchpad lint =

I'll address the lint in distroseriesqueue-notify.txt in a separate lint branch. The other stuff is a bit harder to clean up because it involves whitespace-sensitive tests.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/scripts/packagecopier.py
  lib/lp/soyuz/model/queue.py
  lib/lp/archiveuploader/tests/nascentupload-announcements.txt
  lib/lp/soyuz/doc/distroseriesqueue-dist-upgrader.txt
  lib/lp/soyuz/adapters/notification.py
  lib/lp/soyuz/doc/distroseriesqueue-notify.txt
  lib/lp/soyuz/adapters/tests/test_notification.py

./lib/lp/archiveuploader/tests/nascentupload-announcements.txt
     186: want exceeds 78 characters.
     187: want exceeds 78 characters.
     659: want exceeds 78 characters.
     726: want exceeds 78 characters.
     728: want exceeds 78 characters.
     774: want exceeds 78 characters.
     776: want exceeds 78 characters.
./lib/lp/soyuz/doc/distroseriesqueue-dist-upgrader.txt
     145: want exceeds 78 characters.
     148: want exceeds 78 characters.
./lib/lp/soyuz/doc/distroseriesqueue-notify.txt
       1: narrative uses a moin header.
       6: source has bad indentation.
       9: narrative has trailing whitespace.
      10: narrative has trailing whitespace.
      13: source has bad indentation.
      21: source exceeds 78 characters.
      21: source has bad indentation.
      41: source has bad indentation.
      60: source has bad indentation.
      67: source has bad indentation.
      74: source has bad indentation.
      79: source has bad indentation.
      84: source has bad indentation.
     102: narrative has trailing whitespace.
     103: narrative has trailing whitespace.
     106: source has bad indentation.
     113: source has bad indentation.
     133: source has bad indentation.
     140: source has bad indentation.
     144: source has bad indentation.
     150: source has bad indentation.
     155: source has bad indentation.
     161: source has bad indentation.
     168: source has bad indentation.
     173: source has bad indentation.
     178: source has bad indentation.
     200: source has bad indentation.
     218: source has bad indentation.
     222: source has bad indentation.
     229: source has bad indentation.
     234: source has bad indentation.
     256: source has bad indentation.
     277: source has bad indentation.
     283: source has bad indentation.
./lib/lp/soyuz/adapters/tests/test_notification.py
     217: Line exceeds 78 characters.
     218: Line exceeds 78 characters.
     222: Line exceeds 78 characters.
     217: E501 line too long (85 characters)
     218: E501 line too long (82 characters)
     222: E501 line too long (82 characters)

[Robert Collins (lifeless)]

10 + ... for entry in sorted([addr.strip() for addr in field.split(',')]):
11 + ... print entry

and also in
509 + >>> def extract_addresses(header_field):

might be better as
for entry in sorted(re.split(field, ' \t\r\n,')):

86 + # Some syncs (e.g. from Debian) will involve packages whose
87 + # changed-by person was auto-created in LP and hence does not have a
88 + # preferred email address set.
89 + changedby_person = email_to_person(info['changedby'])

The comment here now looks wonky - there is no special casing happening, so perhaps it should move to email_to_person?

I think this existing comment:
196 # Now filter list of recipients for persons only registered in
197 # Launchpad to avoid spamming the innocent.

means 'Do not email persons that have not configured a preferred email - e.g. folk listed in Launchpad who have not activated their account'. If so, and if my phrasing is clearer, feel free to update it.

For this:
231 + if fullemail is None or fullemail == '':
232 + return None
I would write 'if not fullemail:' - thats what __nonzero__ is for, and its correct here. Our style guide was updated a while ago to remove the admonition that this was bad (its not).

person_to_email looks like something other bits of code (e.g. incoming-mail-processing) will want/need; since you are here you could consider deduping.

Looks fine to me, these are all tweaks.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks for the fast review.

> 10 + ... for entry in sorted([addr.strip() for addr in
> field.split(',')]):
> 11 + ... print entry
>
> and also in
> 509 + >>> def extract_addresses(header_field):
>
> might be better as
> for entry in sorted(re.split(field, ' \t\r\n,')):

Alas, no. The field may contain a full name, so it generally contains spaces that should not be split.

> 86 + # Some syncs (e.g. from Debian) will involve packages whose
> 87 + # changed-by person was auto-created in LP and hence does not have a
> 88 + # preferred email address set.
> 89 + changedby_person = email_to_person(info['changedby'])
>
> The comment here now looks wonky - there is no special casing happening, so
> perhaps it should move to email_to_person?

Doesn't really make sense to talk about the changed-by author there. Instead, I extended the comment to say that in that situation email_to_person will return None. Puts the rest of the comment in perspective.

> I think this existing comment:
> 196 # Now filter list of recipients for persons only registered in
> 197 # Launchpad to avoid spamming the innocent.
>
> means 'Do not email persons that have not configured a preferred email - e.g.
> folk listed in Launchpad who have not activated their account'. If so, and if
> my phrasing is clearer, feel free to update it.

Did that.

> For this:
> 231 + if fullemail is None or fullemail == '':
> 232 + return None
> I would write 'if not fullemail:' - thats what __nonzero__ is for, and its
> correct here. Our style guide was updated a while ago to remove the admonition
> that this was bad (its not).

Certainly easier. Changed.

> person_to_email looks like something other bits of code (e.g. incoming-mail-
> processing) will want/need; since you are here you could consider deduping.

Probably not worth the trouble for a simple two-liner.

Jeroen

=== modified file 'lib/lp/archiveuploader/tests/nascentupload-announcements.txt'
--- lib/lp/archiveuploader/tests/nascentupload-announcements.txt	2011-08-22 04:38:02 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-announcements.txt	2011-10-27 16:44:31 +0000
@@ -46,8 +46,8 @@
     ...     return cmp(a[1], b[1])
 
     >>> def print_addrlist(field):
-    ...    for entry in field.split(','):
-    ...        print entry.strip()
+    ...    for entry in sorted([addr.strip() for addr in field.split(',')]):
+    ...        print entry
 
 Import the test keys to use 'insecure' policy.
 
@@ -443,8 +443,8 @@
     'Launchpad actually'
 
     >>> print_addrlist(notification['To'])
+    Daniel Silverstone <[e-mail]>
     Foo Bar <[e-mail]>
-    Daniel Silverstone <[e-mail]>
 
     >>> notification['Subject']
     '[ubuntu/hoary-updates] bar 1.0-2 (Waiting for approval)'
@@ -479,8 +479,8 @@
     'Launchpad actually'
 
     >>> print_addrlist(notification['To'])
+    Daniel Silverstone <[e-mail]>
     Foo Bar <[e-mail]>
-    Daniel Silverstone <[e-mail]>
 
     >>> notification['Subject']
     '[ubuntu/hoary-backports] bar 1.0-3 (Waiting for approval)'

=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py	2011-08-31 05:06:54 +0000
+++ lib/lp/soyuz/adapters/notification.py	2011-10-27 16:44:31 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 
 __all__ = [
-    'get_recipients',  # Available for testing only.
     'notify',
     ]
 
@@ -68,7 +67,7 @@
     }
     template = get_template(archive, 'rejected')
     body = template % information
-    to_addrs = get_recipients(
+    to_addrs = get_upload_notification_recipients(
         blamer, archive, distroseries, logger, changes=changes)
     debug(logger, "Sending rejection email.")
     if not to_addrs:
@@ -128,7 +127,7 @@
            summary_text=None, changes=None, changesfile_content=None,
            changesfile_object=None, action=None, dry_run=False,
            logger=None, announce_from_person=None, previous_version=None):
-    """Notify about
+    """Notify about an upload or package copy.
 
     :param blamer: The `IPerson` who is to blame for this notification.
     :param spr: The `ISourcePackageRelease` that was created.
@@ -187,7 +186,7 @@
     if not files and action != 'rejected':
         return
 
-    recipients = get_recipients(
+    recipients = get_upload_notification_recipients(
         blamer, archive, distroseries, logger, changes=changes, spr=spr,
         bprs=bprs)
 
@@ -304,13 +303,12 @@
     if distroseries.changeslist:
         information['ANNOUNCE'] = "Announcing to %s" % (
             distroseries.changeslist)
-    try:
-        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
-        # have a preferred email address set.
-        changedby_person = None
+
+    # 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'])
+
     if blamer is not None and blamer != changedby_person:
         signer_signature = person_to_email(blamer)
         if signer_signature != info['changedby']:
@@ -448,73 +446,69 @@
         return guess_encoding(s)
 
 
-def debug(logger, msg):
+def debug(logger, msg, *args, **kwargs):
     """Shorthand debug notation for publish() methods."""
     if logger is not None:
-        logger.debug(msg)
-
-
-def get_recipients(blamer, archive, distroseries, logger, changes=None,
-                   spr=None, bprs=None):
+        logger.debug(msg, *args, **kwargs)
+
+
+def is_valid_uploader(person, distribution):
+    """Is `person` an uploader for `distribution`?
+
+    A `None` person is not an uploader.
+    """
+    if person is None:
+        return None
+    else:
+        return person.isUploader(distribution)
+
+
+def get_upload_notification_recipients(blamer, archive, distroseries,
+                                       logger=None, changes=None, spr=None,
+                                       bprs=None):
     """Return a list of recipients for notification emails."""
-    candidate_recipients = []
     debug(logger, "Building recipients list.")
+    candidate_recipients = [
+        blamer,
+        ]
     info = fetch_information(spr, bprs, changes)
 
-    if info['changedby']:
-        try:
-            changer = email_to_person(info['changedby'])
-        except ParseMaintError:
-            changer = None
-    else:
-        changer = None
-
-    if info['maintainer']:
-        try:
-            maintainer = email_to_person(info['maintainer'])
-        except ParseMaintError:
-            maintainer = None
-    else:
-        maintainer = None
-
-    if blamer:
-        # This is a signed upload.
-        candidate_recipients.append(blamer)
-    else:
-        debug(logger,
-            "Changes file is unsigned, adding changer as recipient")
+    changer = email_to_person(info['changedby'])
+    maintainer = email_to_person(info['maintainer'])
+
+    if blamer is None:
+        debug(
+            logger, "Changes file is unsigned; adding changer as recipient.")
         candidate_recipients.append(changer)
 
     if archive.is_ppa:
         # For PPAs, any person or team mentioned explicitly in the
         # ArchivePermissions as uploaders for the archive will also
         # get emailed.
-        uploaders = [
-            permission.person for permission in
-                archive.getUploadersForComponent()]
-        candidate_recipients.extend(uploaders)
-
-    # If this is not a PPA, we also consider maintainer and changed-by.
-    elif blamer is not None:
-        if (maintainer and maintainer != blamer and
-                maintainer.isUploader(distroseries.distribution)):
-            debug(logger, "Adding maintainer to recipients")
-            candidate_recipients.append(maintainer)
-
-        if (changer and changer != blamer and
-                changer.isUploader(distroseries.distribution)):
-            debug(logger, "Adding changed-by to recipients")
-            candidate_recipients.append(changer)
-
-    # Now filter list of recipients for persons only registered in
-    # Launchpad to avoid spamming the innocent.
-    recipients = []
-    for person in candidate_recipients:
-        if person is None or person.preferredemail is None:
-            continue
-        recipient = format_address_for_person(person)
-        debug(logger, "Adding recipient: '%s'" % recipient)
-        recipients.append(recipient)
+        candidate_recipients.extend([
+            permission.person
+            for permission in archive.getUploadersForComponent()])
+    else:
+        # If this is not a PPA, we also consider maintainer and changed-by.
+        if blamer is not None:
+            if is_valid_uploader(maintainer, distroseries.distribution):
+                debug(logger, "Adding maintainer to recipients")
+                candidate_recipients.append(maintainer)
+
+            if is_valid_uploader(changer, distroseries.distribution):
+                debug(logger, "Adding changed-by to recipients")
+                candidate_recipients.append(changer)
+
+    # Collect email addresses to notify.  Skip persons who do not have a
+    # preferredemail set, such as people who have not activated their
+    # Launchpad accounts (and are therefore not expecting this email).
+    recipients = [
+        format_address_for_person(person)
+        for person in filter(None, set(candidate_recipients))
+            if person.preferredemail is not None]
+
+    for recipient in recipients:
+        debug(logger, "Adding recipient: '%s'", recipient)
 
     return recipients
 
@@ -572,18 +566,29 @@
 
 
 def email_to_person(fullemail):
-    """Return an IPerson given an RFC2047 email address."""
-    # The 2nd arg to s_f_m() doesn't matter as it won't fail since every-
-    # thing will have already parsed at this point.
-    (rfc822, rfc2047, name, email) = safe_fix_maintainer(
-        fullemail, "email")
-    return getUtility(IPersonSet).getByEmail(email)
+    """Return an `IPerson` given an RFC2047 email address.
+
+    :param fullemail: Potential email address.
+    :return: `IPerson` with the given email address.  None if there
+        isn't one, or if `fullemail` isn't a proper email address.
+    """
+    if not fullemail:
+        return None
+
+    try:
+        # The 2nd arg to s_f_m() doesn't matter as it won't fail since every-
+        # thing will have already parsed at this point.
+        (rfc822, rfc2047, name, email) = safe_fix_maintainer(
+            fullemail, "email")
+        return getUtility(IPersonSet).getByEmail(email)
+    except ParseMaintError:
+        return None
 
 
 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:
+        # This will use email.Header to encode any non-ASCII characters.
         return format_address_for_person(person)
 
 
@@ -595,12 +600,11 @@
     user ([e-mail]).
     """
     katie = getUtility(ILaunchpadCelebrities).katie
-    try:
-        changed_by = email_to_person(changed_by_email)
-    except ParseMaintError:
-        return False
+    changed_by = email_to_person(changed_by_email)
     return (
-        spr and not bprs and changed_by == katie and
+        spr and
+        not bprs and
+        changed_by == katie and
         pocket != PackagePublishingPocket.SECURITY)
 
 

=== modified file 'lib/lp/soyuz/adapters/tests/test_notification.py'
--- lib/lp/soyuz/adapters/tests/test_notification.py	2011-08-31 05:06:54 +0000
+++ lib/lp/soyuz/adapters/tests/test_notification.py	2011-10-27 16:44:31 +0000
@@ -6,8 +6,9 @@
 # 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
-from textwrap import dedent
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -18,24 +19,24 @@
     )
 from lp.archivepublisher.utils import get_ppa_reference
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.log.logger import BufferLogger
 from lp.services.mail.sendmail import format_address_for_person
-from lp.services.log.logger import BufferLogger
 from lp.soyuz.adapters.notification import (
     assemble_body,
     calculate_subject,
-    get_recipients,
     fetch_information,
+    get_upload_notification_recipients,
     is_auto_sync_upload,
+    notify,
+    person_to_email,
     reject_changes_file,
-    person_to_email,
-    notify,
     )
-from lp.soyuz.interfaces.component import IComponentSet
-from lp.soyuz.model.component import ComponentSelection
 from lp.soyuz.enums import (
     ArchivePurpose,
     PackageUploadCustomFormat,
     )
+from lp.soyuz.interfaces.component import IComponentSet
+from lp.soyuz.model.component import ComponentSelection
 from lp.soyuz.model.distroseriessourcepackagerelease import (
     DistroSeriesSourcePackageRelease,
     )
@@ -231,8 +232,8 @@
     def test_fetch_information_changes(self):
         changes = {
             'Date': '2001-01-01',
-            'Changed-By': 'Foo Bar <[e-mail]>',
-            'Maintainer': 'Foo Bar <[e-mail]>',
+            'Changed-By': 'Foo Bar <[e-mail]>',
+            'Maintainer': 'Foo Bar <[e-mail]>',
             'Changes': ' * Foo!',
             }
         info = fetch_information(
@@ -246,7 +247,7 @@
             info['maintainer_displayname'],
             ]
         for field in fields:
-            self.assertEqual('Foo Bar <[e-mail]>', field)
+            self.assertEqual('Foo Bar <[e-mail]>', field)
 
     def test_fetch_information_spr(self):
         creator = self.factory.makePerson(displayname=u"foø")
@@ -365,64 +366,67 @@
                     distroseries=distroseries, component=component))
         archive.newComponentUploader(maintainer, component)
         archive.newComponentUploader(changer, component)
-        return get_recipients(
+        return get_upload_notification_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(
-            '[e-mail]', displayname='Maintainer')
-        changer = self.factory.makePerson(
-            '[e-mail]', displayname='Changer')
-        changes = {
-            'Date': '2001-01-01',
-            'Changed-By': 'Changer <[e-mail]>',
-            'Maintainer': 'Maintainer <[e-mail]>',
-            '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(
-            '[e-mail]', displayname='Maintainer')
-        changer = self.factory.makePerson(
-            '[e-mail]', displayname='Changer')
-        changes = {
-            'Date': '2001-01-01',
-            'Changed-By': 'Changer <[e-mail]>',
-            '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(
-            '[e-mail]', displayname='Maintainer')
-        changer = self.factory.makePerson(
-            '[e-mail]', displayname='Changer')
-        changes = {
-            'Date': '2001-01-01',
-            'Changed-By': 'Changer <changer at canonical.com>',
-            'Maintainer': 'Maintainer <[e-mail]>',
-            '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)
+    def test_get_upload_notification_recipients_good_emails(self):
+        # Test get_upload_notification_recipients with good email addresses..
+        blamer = self.factory.makePerson()
+        maintainer = self.factory.makePerson(
+            '[e-mail]', displayname='Maintainer')
+        changer = self.factory.makePerson(
+            '[e-mail]', displayname='Changer')
+        changes = {
+            'Date': '2001-01-01',
+            'Changed-By': 'Changer <[e-mail]>',
+            'Maintainer': 'Maintainer <[e-mail]>',
+            'Changes': ' * Foo!',
+            }
+        recipients = self._run_recipients_test(
+            changes, blamer, maintainer, changer)
+        expected = [
+            format_address_for_person(person)
+            for person in (blamer, maintainer, changer)]
+        self.assertContentEqual(expected, recipients)
+
+    def test_get_upload_notification_recipients_bad_maintainer_email(self):
+        blamer = self.factory.makePerson()
+        maintainer = self.factory.makePerson(
+            '[e-mail]', displayname='Maintainer')
+        changer = self.factory.makePerson(
+            '[e-mail]', displayname='Changer')
+        changes = {
+            'Date': '2001-01-01',
+            'Changed-By': 'Changer <[e-mail]>',
+            'Maintainer': 'Maintainer <maintainer at example.com>',
+            'Changes': ' * Foo!',
+            }
+        recipients = self._run_recipients_test(
+            changes, blamer, maintainer, changer)
+        expected = [
+            format_address_for_person(person) for person in (blamer, changer)]
+        self.assertContentEqual(expected, recipients)
+
+    def test_get_upload_notification_recipients_bad_changedby_email(self):
+        # Test get_upload_notification_recipients with invalid changedby
+        # email address.
+        blamer = self.factory.makePerson()
+        maintainer = self.factory.makePerson(
+            '[e-mail]', displayname='Maintainer')
+        changer = self.factory.makePerson(
+            '[e-mail]', displayname='Changer')
+        changes = {
+            'Date': '2001-01-01',
+            'Changed-By': 'Changer <changer at example.com>',
+            'Maintainer': 'Maintainer <[e-mail]>',
+            'Changes': ' * Foo!',
+            }
+        recipients = self._run_recipients_test(
+            changes, blamer, maintainer, changer)
+        expected = [
+            format_address_for_person(person)
+            for person in (blamer, maintainer)]
+        self.assertContentEqual(expected, recipients)
 
     def test_assemble_body_handles_no_preferred_email_for_changer(self):
         # If changer has no preferred email address,

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-dist-upgrader.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-dist-upgrader.txt	2011-07-21 23:10:26 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-dist-upgrader.txt	2011-10-27 16:44:31 +0000
@@ -60,6 +60,7 @@
     DEBUG Creating queue entry
     DEBUG Setting it to ACCEPTED
     DEBUG Building recipients list.
+    ...
     DEBUG Adding recipient: 'Foo Bar <[e-mail]>'
     DEBUG Sent a mail:
     ...
@@ -235,6 +236,7 @@
     DEBUG Creating queue entry
     DEBUG Setting it to ACCEPTED
     DEBUG Building recipients list.
+    ...
     DEBUG Adding recipient: 'Foo Bar <[e-mail]>'
     DEBUG Sent a mail:
     ...

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-notify.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-notify.txt	2011-06-01 04:57:27 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-notify.txt	2011-10-27 16:44:31 +0000
@@ -45,11 +45,11 @@
   >>> netapplet_upload.notify(
   ...     changes_file_object=changes_file, logger=FakeLogger())
   DEBUG Building recipients list.
-  DEBUG Changes file is unsigned, adding changer as recipient
+  DEBUG Changes file is unsigned; adding changer as recipient.
   ...
   DEBUG Sent a mail:
   ...
-  DEBUG     Recipients: Daniel Silverstone <[e-mail]>
+  DEBUG   Recipients: ... Silverstone ...
   ...
   DEBUG above if files already exist in other distroseries.
   ...
@@ -120,7 +120,7 @@
   ...
   DEBUG Sent a mail:
   ...
-  DEBUG     Recipients: Foo Bar <[e-mail]>, Daniel Silverstone <[e-mail]>
+  DEBUG     Recipients: ... Bar ...
   ...
   DEBUG Announcing to [e-mail]
   ...
@@ -137,22 +137,41 @@
 The mail 'To:' addresses contain the signer and the changer's email.  The
 announcement email contains the serieses changeslist.
 
-  >>> [msg['To'] for msg in msgs]
-  ['Foo Bar <[e-mail]>,\n\tDaniel Silverstone <[e-mail]>', '[e-mail]']
+  >>> def to_lower(address):
+  ...     """Return lower-case version of email address."""
+  ...     return address.lower()
+
+  >>> def extract_addresses(header_field):
+  ...     """Extract and sort addresses from an email header field."""
+  ...     return sorted(
+  ...         [addr.strip() for addr in header_field.split(',')],
+  ...         key=to_lower)
+
+  >>> for addr in extract_addresses(msgs[0]['To']):
+  ...     print addr
+  Daniel Silverstone <[e-mail]>
+  Foo Bar <[e-mail]>
+
+  >>> print msgs[1]['To']
+  [e-mail]
 
 The mail 'Bcc:' address is the uploader.  The announcement has the uploader
 and the Debian derivatives address for the package uploaded.
 
-  >>> [msg['Bcc'] for msg in msgs]
-  ['Root <root@localhost>', 'Root <root@localhost>, [e-mail]']
+  >>> for msg in msgs:
+  ...     print extract_addresses(msg['Bcc'])
+  ['Root <root@localhost>']
+  ['[e-mail]', 'Root <root@localhost>']
 
 The mail 'From:' addresses are the uploader and the changer.
 
-  >>> [msg['From'] for msg in msgs]
-  ['Root <root@localhost>', 'Daniel Silverstone <[e-mail]>']
+  >>> for msg in msgs:
+  ...     print msg['From']
+  Root <root@localhost>
+  Daniel Silverstone <[e-mail]>
 
-  >>> notification['Subject']
-  '[ubuntu/breezy-autotest] netapplet 0.99.6-1 (New)'
+  >>> print notification['Subject']
+  [ubuntu/breezy-autotest] netapplet 0.99.6-1 (New)
 
 The mail body contains the same list of files again:
 
@@ -188,7 +207,7 @@
   ...
   DEBUG Sent a mail:
   ...
-  DEBUG     Recipients: Foo Bar <[e-mail]>, Daniel Silverstone <[e-mail]>
+  DEBUG     Recipients: ... Silverstone ...
   ...
   DEBUG above if files already exist in other distroseries.
   ...
@@ -200,13 +219,15 @@
 
 The mail headers are the same as before:
 
-  >>> notification['To']
-  'Foo Bar <[e-mail]>,\n\tDaniel Silverstone <[e-mail]>'
-  >>> notification['Bcc']
-  'Root <root@localhost>'
+  >>> for addr in extract_addresses(notification['To']):
+  ...     print addr
+  Daniel Silverstone <[e-mail]>
+  Foo Bar <[e-mail]>
+  >>> print notification['Bcc']
+  Root <root@localhost>
 
-  >>> notification['Subject']
-  '[ubuntu/breezy-autotest] netapplet 0.99.6-1 (New)'
+  >>> print notification['Subject']
+  [ubuntu/breezy-autotest] netapplet 0.99.6-1 (New)
 
 The mail body contains the same list of files again:
 
@@ -241,7 +262,7 @@
   ...
   DEBUG   Subject: [ubuntu/breezy-autotest] netapplet 0.99.6-1 (Rejected)
   DEBUG   Sender: Root <root@localhost>
-  DEBUG   Recipients: Foo Bar <[e-mail]>, Daniel Silverstone <[e-mail]>
+  DEBUG   Recipients: ... Bar ...
   DEBUG   Bcc: Root <root@localhost>
   DEBUG   Body:
   DEBUG Rejected:

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2011-08-25 08:29:37 +0000
+++ lib/lp/soyuz/model/queue.py	2011-10-27 16:44:31 +0000
@@ -868,7 +868,7 @@
             signer, self.sourcepackagerelease, self.builds, self.customfiles,
             self.archive, self.distroseries, self.pocket, summary_text,
             changes, changesfile_content, changes_file_object,
-            status_action[self.status], dry_run, logger)
+            status_action[self.status], dry_run=dry_run, logger=logger)
 
     def _isPersonUploader(self, person):
         """Return True if person is an uploader to the package's distro."""

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2011-09-26 06:30:07 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2011-10-27 16:44:31 +0000
@@ -533,8 +533,8 @@
     Verifies if each copy can be performed using `CopyChecker` and
     raises `CannotCopy` if one or more copies could not be performed.
 
-    When `CannotCopy`is raised call sites are in charge to rollback the
-    transaction or performed copies will be commited.
+    When `CannotCopy` is raised, call sites are responsible for rolling
+    back the transaction.  Otherwise, performed copies will be commited.
 
     Wrapper for `do_direct_copy`.
 
@@ -611,8 +611,7 @@
             # In zopeless mode this email will be sent immediately.
             notify(
                 person, source.sourcepackagerelease, [], [], archive,
-                series, pocket, summary_text=error_text,
-                action='rejected')
+                series, pocket, summary_text=error_text, action='rejected')
         raise CannotCopy(error_text)
 
     overrides_index = 0
@@ -648,8 +647,7 @@
             if send_email:
                 notify(
                     person, source.sourcepackagerelease, [], [], archive,
-                    destination_series, pocket, changes=None,
-                    action='accepted',
+                    destination_series, pocket, action='accepted',
                     announce_from_person=announce_from_person,
                     previous_version=old_version)