launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/notify-changed-by-field into lp:launchpad.

Commit message:
Notify the Changed-By address for PPA uploads if the .changes contains "Launchpad-Notify-Changed-By: yes".

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1633608 in Launchpad itself: "Bileto PPA upload rejections are lost to the ether"
  https://bugs.launchpad.net/launchpad/+bug/1633608

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/notify-changed-by-field/+merge/308587
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/notify-changed-by-field into lp:launchpad.

=== modified file 'lib/lp/soyuz/mail/packageupload.py'
--- lib/lp/soyuz/mail/packageupload.py	2015-09-02 12:05:15 +0000
+++ lib/lp/soyuz/mail/packageupload.py	2016-10-16 13:55:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -133,6 +133,7 @@
 
 def fetch_information(spr, bprs, changes, previous_version=None):
     changelog = date = changedby = maintainer = None
+    notify_changed_by = False
 
     if changes:
         changelog = ChangesFile.formatChangesComment(
@@ -148,6 +149,8 @@
                 changes.get('Maintainer'), 'Maintainer')
         except ParseMaintError:
             pass
+        notify_changed_by = changes.get(
+            'Launchpad-Notify-Changed-By', '') == 'yes'
     elif spr or bprs:
         if not spr and bprs:
             spr = bprs[0].build.source_package_release
@@ -166,6 +169,7 @@
         'date': date,
         'changedby': changedby,
         'maintainer': maintainer,
+        'notify_changed_by': notify_changed_by,
         }
 
 
@@ -338,6 +342,17 @@
                 add_recipient(
                     recipients, permission.person,
                     PackageUploadRecipientReason.forPPAUploader, logger=logger)
+
+            # If the "Launchpad-Notify-Changed-By: yes" field is set in the
+            # .changes file, we also consider changed-by.  The latter is
+            # intended for use by systems that automatically sign uploads on
+            # behalf of developers, in which case we want to make sure to
+            # notify the developer in question.
+            if info['notify_changed_by']:
+                debug(logger, "Adding changed-by to recipients")
+                add_recipient(
+                    recipients, changer,
+                    PackageUploadRecipientReason.forChangedBy, logger=logger)
         elif archive.is_copy:
             # For copy archives, notifying anyone else will probably only
             # confuse them.

=== modified file 'lib/lp/soyuz/mail/tests/test_packageupload.py'
--- lib/lp/soyuz/mail/tests/test_packageupload.py	2015-09-11 12:20:23 +0000
+++ lib/lp/soyuz/mail/tests/test_packageupload.py	2016-10-16 13:55:46 +0000
@@ -2,7 +2,7 @@
 # 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-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from textwrap import dedent
@@ -267,6 +267,26 @@
             ]
         for field in fields:
             self.assertEqual((u'Foo Bar', u'foo.bar@xxxxxxxxxxx'), field)
+        self.assertFalse(info['notify_changed_by'])
+
+    def test_fetch_information_changes_notify_changed_by(self):
+        changes = {
+            'Date': '2001-01-01',
+            'Changed-By': 'Foo Bar <foo.bar@xxxxxxxxxxx>',
+            'Maintainer': 'Foo Bar <foo.bar@xxxxxxxxxxx>',
+            'Changes': ' * Foo!',
+            'Launchpad-Notify-Changed-By': 'yes',
+            }
+        info = fetch_information(None, None, changes)
+        self.assertEqual('2001-01-01', info['date'])
+        self.assertEqual(' * Foo!', info['changelog'])
+        fields = [
+            info['changedby'],
+            info['maintainer'],
+            ]
+        for field in fields:
+            self.assertEqual((u'Foo Bar', u'foo.bar@xxxxxxxxxxx'), field)
+        self.assertTrue(info['notify_changed_by'])
 
     def test_fetch_information_spr(self):
         creator = self.factory.makePerson(displayname=u"foø")
@@ -280,6 +300,7 @@
             (u"foø", spr.creator.preferredemail.email), info['changedby'])
         self.assertEqual(
             (u"bær", spr.maintainer.preferredemail.email), info['maintainer'])
+        self.assertFalse(info['notify_changed_by'])
 
     def test_fetch_information_bprs(self):
         bpr = self.factory.makeBinaryPackageRelease()
@@ -293,6 +314,7 @@
         self.assertEqual(
             (spr.maintainer.displayname, spr.maintainer.preferredemail.email),
             info['maintainer'])
+        self.assertFalse(info['notify_changed_by'])
 
     def test_calculate_subject_spr(self):
         spr = self.factory.makeSourcePackageRelease()
@@ -433,6 +455,35 @@
             [], changes, None, maintainer, changer,
             purpose=ArchivePurpose.COPY)
 
+    def test_getRecipientsForAction_ppa(self):
+        # Notifications for PPA uploads normally only go to the person who
+        # signed the upload.
+        blamer, maintainer, changer = self._setup_recipients()
+        changes = {
+            'Date': '2001-01-01',
+            'Changed-By': 'Changer <changer@xxxxxxxxxxx>',
+            'Maintainer': 'Maintainer <maintainer@xxxxxxxxxxx>',
+            'Changes': ' * Foo!',
+            }
+        self.assertRecipientsEqual(
+            [blamer], changes, blamer, maintainer, changer,
+            purpose=ArchivePurpose.PPA)
+
+    def test_getRecipientsForAction_ppa_notify_changed_by(self):
+        # If the .changes file contains "Launchpad-Notify-Changed-By: yes",
+        # notifications go to the changer even for PPA uploads.
+        blamer, maintainer, changer = self._setup_recipients()
+        changes = {
+            'Date': '2001-01-01',
+            'Changed-By': 'Changer <changer@xxxxxxxxxxx>',
+            'Maintainer': 'Maintainer <maintainer@xxxxxxxxxxx>',
+            'Changes': ' * Foo!',
+            'Launchpad-Notify-Changed-By': 'yes',
+            }
+        self.assertRecipientsEqual(
+            [blamer, changer], changes, blamer, maintainer, changer,
+            purpose=ArchivePurpose.PPA)
+
     def test__getHeaders_primary(self):
         # _getHeaders returns useful values for headers used for filtering.
         # For a primary archive, this includes the maintainer and changer.