Merge lp:~julian-edwards/launchpad/sync-email-from-addr-bug-817102 into lp:launchpad

[Julian Edwards]

[r=jtv][bug=817102] Sync announcement emails now come from the sync requester not the package changer.

The announcement emails for syncs into a distro were heading out with the From: address set as the Changed-by on the package being synced. This is wrong, it should be the person who requested the sync.

This branch fixes that.

[Jeroen T. Vermeulen (jtv)]

That should do the trick. May be worth documenting that the override doesn't happen if the given user has no email address; I believe you hit snags with that case in the real world.

There's a stupendous amount of setup for such a simple test. Would be nice if that could be avoided somehow, but I'll admit it's a thorny case.

review: Approve

=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py	2011-07-26 10:57:47 +0000
+++ lib/lp/soyuz/adapters/notification.py	2011-07-28 20:49:48 +0000
@@ -125,7 +125,8 @@
 
 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):
+           changesfile_object=None, action=None, dry_run=False,
+           logger=None, announce_from_person=None):
     """Notify about
 
     :param blamer: The `IPerson` who is to blame for this notification.
@@ -144,6 +145,9 @@
     :param action: A string of what action to notify for, such as 'new',
         'accepted'.
     :param dry_run: If True, only log the mail.
+    :param announce_from_person: If passed, use this `IPerson` as the From: in
+        announcement emails.  If the person has no preferred email address,
+        the person is ignored and the default From: is used instead.
     """
     # If this is a binary or mixed upload, we don't send *any* emails
     # provided it's not a rejection or a security upload:
@@ -224,6 +228,11 @@
 
     (changesfile, date, from_addr, maintainer) = fetch_information(
         spr, bprs, changes)
+    if announce_from_person is not None:
+        email = announce_from_person.preferredemail
+        if email:
+            from_addr = email.email
+
     # 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.

=== modified file 'lib/lp/soyuz/adapters/tests/test_notification.py'
--- lib/lp/soyuz/adapters/tests/test_notification.py	2011-07-26 10:55:05 +0000
+++ lib/lp/soyuz/adapters/tests/test_notification.py	2011-07-28 20:49:48 +0000
@@ -10,6 +10,7 @@
     ZopelessDatabaseLayer,
     )
 from lp.archivepublisher.utils import get_ppa_reference
+from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.mail.sendmail import format_address_for_person
 from lp.services.log.logger import BufferLogger
 from lp.soyuz.adapters.notification import (
@@ -52,6 +53,28 @@
             'accepted')
         self.assertEqual(expected_subject, subject)
 
+    def test_notify_from_person_override(self):
+        # notify() takes an optional from_person to override the calculated
+        # From: address in announcement emails.
+        spr = self.factory.makeSourcePackageRelease()
+        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()
+        from_person = self.factory.makePerson()
+        notify(
+            blamer, spr, [], [], archive, distroseries, pocket,
+            action='accepted', announce_from_person=from_person)
+        notifications = pop_notifications()
+        self.assertEqual(2, len(notifications))
+        # The first notification is to the blamer,
+        # the second is the announce list, which is the one that gets the
+        # overridden From:
+        self.assertEqual(
+            from_person.preferredemail.email, notifications[1]["From"])
+
 
 class TestNotification(TestCaseWithFactory):
 

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-07-26 11:44:28 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-07-28 20:49:48 +0000
@@ -511,7 +511,7 @@
             series=self.target_distroseries, pocket=self.target_pocket,
             include_binaries=self.include_binaries, check_permissions=True,
             person=self.requester, overrides=[override],
-            send_email=send_email)
+            send_email=send_email, announce_from_person=self.requester)
 
         if pu is not None:
             # A PackageUpload will only exist if the copy job had to be

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2011-07-22 11:12:23 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2011-07-28 20:49:48 +0000
@@ -525,7 +525,7 @@
 def do_copy(sources, archive, series, pocket, include_binaries=False,
             allow_delayed_copies=True, person=None, check_permissions=True,
             overrides=None, send_email=False, strict_binaries=True,
-            close_bugs=True, create_dsd_job=True):
+            close_bugs=True, create_dsd_job=True, announce_from_person=None):
     """Perform the complete copy of the given sources incrementally.
 
     Verifies if each copy can be performed using `CopyChecker` and
@@ -557,6 +557,8 @@
         override must be for the corresponding source in the sources list.
         Overrides will be ignored for delayed copies.
     :param send_email: Should we notify for the copy performed?
+    :param announce_from_person: If send_email is True,
+        then send announcement emails with this person as the From:
     :param strict_binaries: If 'include_binaries' is True then setting this
         to True will make the copy fail if binaries cannot be also copied.
     :param close_bugs: A boolean indicating whether or not bugs on the
@@ -616,7 +618,8 @@
                 notify(
                     person, source.sourcepackagerelease, [], [], archive,
                     destination_series, pocket, changes=None,
-                    action='accepted')
+                    action='accepted',
+                    announce_from_person=announce_from_person)
 
         overrides_index += 1
         copies.extend(sub_copies)

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2011-07-26 11:39:14 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2011-07-28 20:49:48 +0000
@@ -816,6 +816,7 @@
         self.assertEquals(2, len(emails))
         self.assertIn("[e-mail]", emails[0]['To'])
         self.assertIn("[e-mail]", emails[1]['To'])
+        self.assertEqual("[e-mail]", emails[1]['From'])
 
     def test_findMatchingDSDs_matches_all_DSDs_for_job(self):
         # findMatchingDSDs finds matching DSDs for any of the packages