launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/avoid-copy-archive-spam into lp:launchpad.

Commit message:
Don't mail the maintainer and changer for uploads to copy archives.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1103491 in Launchpad itself: "Natively-synced packages in copy archives spam Debian developers with disallowed-component warnings"
  https://bugs.launchpad.net/launchpad/+bug/1103491

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/avoid-copy-archive-spam/+merge/144611

== Summary ==

This is the first part of a fix for bug 1103491, which is currently spamming at least one Debian developer for no good reason.  It doesn't fix the reason the uploads in question are being rejected, but that will take a little longer to disentangle; for now, I'd at least like to squash the e-mails.

== Proposed fix ==

Don't send mail to maintainer/changed-by for copy archive uploads.

== Implementation details ==

The change itself is trivial.  I noticed that utilities/update-copyright wasn't correctly handling lib/lp/soyuz/adapters/tests/test_notification.py and fixed that along the way, and I did some refactoring to bring this branch to LoC-neutrality.

== Tests ==

bin/test -vvct lp.soyuz.adapters.tests.test_notification

== Demo and Q/A ==

Not sure if there's a lightweight way; we'll probably have to create a small copy archive on dogfood with just lha in it, attempt a build upload, and make sure its failure doesn't send mail.  If we still have no dogfood builders, then we could inject the upload that Tom attached to the bug.

== Lint ==

Pre-existing lint relating to an indented e-mail message, so I think not worth fixing:

./lib/lp/soyuz/adapters/tests/test_notification.py
     218: E501 line too long (85 characters)
     219: E501 line too long (82 characters)
     223: E501 line too long (82 characters)
-- 
https://code.launchpad.net/~cjwatson/launchpad/avoid-copy-archive-spam/+merge/144611
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/avoid-copy-archive-spam into lp:launchpad.

=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py	2012-02-10 09:31:39 +0000
+++ lib/lp/soyuz/adapters/notification.py	2013-01-23 23:59:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Notification for uploads and copies."""
@@ -471,17 +471,14 @@
                                        bprs=None):
     """Return a list of recipients for notification emails."""
     debug(logger, "Building recipients list.")
-    candidate_recipients = [
-        blamer,
-        ]
+    candidate_recipients = [blamer]
     info = fetch_information(spr, bprs, changes)
 
     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.")
+        debug(logger, "Changes file is unsigned; adding changer as recipient.")
         candidate_recipients.append(changer)
 
     if archive.is_ppa:
@@ -491,6 +488,10 @@
         candidate_recipients.extend([
             permission.person
             for permission in archive.getUploadersForComponent()])
+    elif archive.is_copy:
+        # For copy archives, notifying anyone else will probably only
+        # confuse them.
+        pass
     else:
         # If this is not a PPA, we also consider maintainer and changed-by.
         if blamer is not None:
@@ -548,8 +549,7 @@
 
     if customfiles:
         files.extend(
-            [(file.libraryfilealias.filename, '', '')
-            for file in customfiles])
+            [(file.libraryfilealias.filename, '', '') for file in customfiles])
 
     return files
 
@@ -581,8 +581,7 @@
     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")
+        rfc822, rfc2047, name, email = safe_fix_maintainer(fullemail, "email")
         return getUtility(IPersonSet).getByEmail(email)
     except ParseMaintError:
         return None

=== modified file 'lib/lp/soyuz/adapters/tests/test_notification.py'
--- lib/lp/soyuz/adapters/tests/test_notification.py	2012-02-10 09:31:39 +0000
+++ lib/lp/soyuz/adapters/tests/test_notification.py	2013-01-23 23:59:22 +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 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from email.utils import formataddr
@@ -127,8 +127,7 @@
         # to the announce list, which is the one that gets the overridden
         # From:
         self.assertEqual(
-            "Lemmy Kilmister <lemmy@xxxxxxxxxxx>",
-            notifications[1]["From"])
+            "Lemmy Kilmister <lemmy@xxxxxxxxxxx>", notifications[1]["From"])
 
     def test_notify_from_person_override_with_unicode_names(self):
         # notify() takes an optional from_person to override the calculated
@@ -238,8 +237,7 @@
             'Maintainer': 'Foo Bar <foo.bar@xxxxxxxxxxx>',
             'Changes': ' * Foo!',
             }
-        info = fetch_information(
-            None, None, changes)
+        info = fetch_information(None, None, changes)
         self.assertEqual('2001-01-01', info['date'])
         self.assertEqual(' * Foo!', info['changelog'])
         fields = [
@@ -353,12 +351,20 @@
         notifications = pop_notifications()
         self.assertEqual(0, len(notifications))
 
-    def _run_recipients_test(self, changes, blamer, maintainer, changer):
+    def _setup_recipients(self):
+        blamer = self.factory.makePerson()
+        maintainer = self.factory.makePerson(
+            'maintainer@xxxxxxxxxxx', displayname='Maintainer')
+        changer = self.factory.makePerson(
+            'changer@xxxxxxxxxxx', displayname='Changer')
+        return blamer, maintainer, changer
+
+    def assertRecipientsEqual(self, expected, changes, blamer, maintainer,
+                              changer, purpose=ArchivePurpose.PRIMARY):
         distribution = self.factory.makeDistribution()
         archive = self.factory.makeArchive(
-            distribution=distribution, purpose=ArchivePurpose.PRIMARY)
-        distroseries = self.factory.makeDistroSeries(
-            distribution=distribution)
+            distribution=distribution, purpose=purpose)
+        distroseries = self.factory.makeDistroSeries(distribution=distribution)
         # Now set the uploaders.
         component = getUtility(IComponentSet).ensure('main')
         if component not in distroseries.components:
@@ -366,69 +372,64 @@
             store.add(
                 ComponentSelection(
                     distroseries=distroseries, component=component))
-        archive.newComponentUploader(maintainer, component)
-        archive.newComponentUploader(changer, component)
-        return get_upload_notification_recipients(
+        distribution.main_archive.newComponentUploader(maintainer, component)
+        distribution.main_archive.newComponentUploader(changer, component)
+        observed = get_upload_notification_recipients(
             blamer, archive, distroseries, logger=None, changes=changes)
+        self.assertContentEqual(
+            [format_address_for_person(person) for person in expected],
+            observed)
 
     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(
-            'maintainer@xxxxxxxxxxx', displayname='Maintainer')
-        changer = self.factory.makePerson(
-            'changer@xxxxxxxxxxx', displayname='Changer')
+        blamer, maintainer, changer = self._setup_recipients()
         changes = {
             'Date': '2001-01-01',
             'Changed-By': 'Changer <changer@xxxxxxxxxxx>',
             'Maintainer': 'Maintainer <maintainer@xxxxxxxxxxx>',
             'Changes': ' * Foo!',
             }
-        recipients = self._run_recipients_test(
+        self.assertRecipientsEqual(
+            [blamer, maintainer, changer],
             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(
-            'maintainer@xxxxxxxxxxx', displayname='Maintainer')
-        changer = self.factory.makePerson(
-            'changer@xxxxxxxxxxx', displayname='Changer')
+        blamer, maintainer, changer = self._setup_recipients()
         changes = {
             'Date': '2001-01-01',
             'Changed-By': 'Changer <changer@xxxxxxxxxxx>',
             '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)
+        self.assertRecipientsEqual(
+            [blamer, changer], changes, blamer, maintainer, changer)
 
     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(
-            'maintainer@xxxxxxxxxxx', displayname='Maintainer')
-        changer = self.factory.makePerson(
-            'changer@xxxxxxxxxxx', displayname='Changer')
+        blamer, maintainer, changer = self._setup_recipients()
         changes = {
             'Date': '2001-01-01',
             'Changed-By': 'Changer <changer at example.com>',
             'Maintainer': 'Maintainer <maintainer@xxxxxxxxxxx>',
             '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)
+        self.assertRecipientsEqual(
+            [blamer, maintainer], changes, blamer, maintainer, changer)
+
+    def test_get_upload_notification_recipients_copy_archive(self):
+        # Notifications for uploads to copy archives only go to the archive
+        # owner.
+        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.COPY)
 
     def test_assemble_body_handles_no_preferred_email_for_changer(self):
         # If changer has no preferred email address,

=== modified file 'utilities/update-copyright'
--- utilities/update-copyright	2012-01-01 03:10:25 +0000
+++ utilities/update-copyright	2013-01-23 23:59:22 +0000
@@ -1,6 +1,6 @@
 #!/usr/bin/python
 #
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Update the year in copyright notices.
@@ -33,7 +33,7 @@
 
 def update_copyright(lines):
     """Update the copyright notice in the given file lines."""
-    for line in range(min(len(lines), 3)):
+    for line in range(min(len(lines), 5)):
         match = copyright_pattern.search(lines[line])
         if match is not None:
             old_years = match.group('years')