[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/notify-changed-by-field into lp:launchpad

2016-11-04 Thread noreply
The proposal to merge lp:~cjwatson/launchpad/notify-changed-by-field into 
lp:launchpad has been updated.

Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/notify-changed-by-field/+merge/308587
-- 
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp


Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/notify-changed-by-field into lp:launchpad

2016-11-02 Thread William Grant
Review: Approve code



Diff comments:

> === modified file 'lib/lp/soyuz/mail/packageupload.py'
> --- lib/lp/soyuz/mail/packageupload.py2015-09-02 12:05:15 +
> +++ lib/lp/soyuz/mail/packageupload.py2016-10-16 13:55:46 +
> @@ -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.

I think the key point is this: unlike normal PPA users, we assume that a system 
that is clever enough to set Launchpad-Notify-Changed-By is probably also 
clever enough to not be unthinkingly uploading things with misleading 
Changed-Bys.

> +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.


-- 
https://code.launchpad.net/~cjwatson/launchpad/notify-changed-by-field/+merge/308587
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp


[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/notify-changed-by-field into lp:launchpad

2016-10-16 Thread Colin Watson
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 +
+++ lib/lp/soyuz/mail/packageupload.py	2016-10-16 13:55:46 +
@@ -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 +
+++ lib/lp/soyuz/mail/tests/test_packageupload.py	2016-10-16 13:55:46 +
@@ -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@example.com'), 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 ',
+'Maintainer': 'Foo Bar ',
+'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@example.com'), 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(