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

[Jeroen T. Vermeulen]

Notify sync requester, not maintainer, of upload failures from synced packages for a distro.

= Summary =

We're spamming Debian maintainers with failure notifications from builds of their packages as copied to Ubuntu (or derived distros).

== Proposed fix ==

Spam the person who requested the sync instead.

== Pre-implementation notes ==

Discussed with Colin, Julian, and William. There may be opportunities to simplify other notification logic in the future, in particular where it suppresses inappropriate notifications for PPAs.

Apparently a build upload has no signing key; we're not entirely sure right now whose signing key is on the upload. We kept the existing logic for non-copied uploads.

== Implementation details ==

See the bug for call trees.

I didn't want to mess with the notify() function underlying Upload.notify(); it's got weird special cases and is used in too many code paths and data flows. It seems to be picking the wrong default victim in cases where there is no signing key.

== Tests ==

I added a new one. Giving it a file of itself allowed me to introduce lots of helpers.
{{{
./bin/test -vvc lp.archiveuploader.tests.test_sync_notification
}}}

== Demo and Q/A ==

We'll need help from the Soyuz specialists for this.

= Launchpad lint =

There's one piece of pre-existing lint that I dared not touch. Something about a getter and setter with the same name confusing the linter, I think.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archiveuploader/nascentupload.py
  lib/lp/soyuz/model/queue.py
  lib/lp/archiveuploader/tests/test_sync_notification.py
  lib/lp/soyuz/model/archive.py

./lib/lp/soyuz/model/archive.py
     348: redefinition of function 'private' from line 344

[j.c.sackett (jcsackett)]

Jeroen--

Looks good. That is some serious setup in the testcase, but I've never found a nice simple way to setup a soyuz based test.

review: Approve

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2011-12-24 13:09:35 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2012-01-06 06:18:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The processing of nascent uploads.
@@ -909,10 +909,10 @@
             # state.
             pass
 
-        changes_file_object = open(self.changes.filepath, "r")
-        self.queue_root.notify(summary_text=self.rejection_message,
-            changes_file_object=changes_file_object, logger=self.logger)
-        changes_file_object.close()
+        with open(self.changes.filepath, "r") as changes_file_object:
+            self.queue_root.notify(
+                summary_text=self.rejection_message,
+                changes_file_object=changes_file_object, logger=self.logger)
 
     def _createQueueEntry(self):
         """Return a PackageUpload object."""

=== added file 'lib/lp/archiveuploader/tests/test_sync_notification.py'
--- lib/lp/archiveuploader/tests/test_sync_notification.py	1970-01-01 00:00:00 +0000
+++ lib/lp/archiveuploader/tests/test_sync_notification.py	2012-01-06 06:18:28 +0000
@@ -0,0 +1,178 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test notification behaviour for cross-distro package syncs."""
+
+__metaclass__ = type
+
+import os.path
+
+from zope.component import getUtility
+
+from lp.archiveuploader.nascentupload import (
+    NascentUpload,
+    UploadError,
+    )
+from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.log.logger import DevNullLogger
+from lp.soyuz.enums import (
+    ArchivePermissionType,
+    SourcePackageFormat,
+    )
+from lp.soyuz.interfaces.sourcepackageformat import (
+    ISourcePackageFormatSelectionSet,
+    )
+from lp.soyuz.model.archivepermission import ArchivePermission
+from lp.soyuz.scripts.packagecopier import do_copy
+from lp.testing import (
+    login,
+    TestCaseWithFactory,
+    )
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.layers import LaunchpadZopelessLayer
+from lp.testing.mail_helpers import pop_notifications
+
+
+class FakeUploadPolicy:
+    def __init__(self, spph):
+        self.distroseries = spph.distroseries
+        self.archive = spph.distroseries.main_archive
+        self.pocket = spph.pocket
+
+    setDistroSeriesAndPocket = FakeMethod()
+    validateUploadType = FakeMethod()
+    checkUpload = FakeMethod()
+
+
+class FakeChangesFile:
+    def __init__(self, spph, file_path):
+        self.files = []
+        self.filepath = file_path
+        self.filename = os.path.basename(file_path)
+        self.architectures = ['i386']
+        self.suite_name = '-'.join([spph.distroseries.name, spph.pocket.name])
+        self.raw_content = open(file_path).read()
+        self.signingkey = None
+
+    checkFileName = FakeMethod([])
+    processAddresses = FakeMethod([])
+    processFiles = FakeMethod([])
+    verify = FakeMethod([UploadError("Deliberately broken")])
+
+
+class TestSyncNotification(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def makePersonWithEmail(self):
+        """Create a person; return (person, email)."""
+        address = "[e-mail]" % self.factory.getUniqueString()
+        person = self.factory.makePerson(email=address)
+        return person, address
+
+    def makeSPPH(self, distroseries, maintainer_address):
+        """Create a `SourcePackagePublishingHistory`."""
+        return self.factory.makeSourcePackagePublishingHistory(
+            distroseries=distroseries, pocket=PackagePublishingPocket.RELEASE,
+            dsc_maintainer_rfc822=maintainer_address)
+
+    def makeUploader(self, person, archive, component):
+        """Grant a person upload privileges for archive/component."""
+        ArchivePermission(
+            person=person, archive=archive, component=component,
+            permission=ArchivePermissionType.UPLOAD)
+
+    def syncSource(self, spph, target_distroseries, requester):
+        """Sync `spph` into `target_distroseries`."""
+        getUtility(ISourcePackageFormatSelectionSet).add(
+            target_distroseries, SourcePackageFormat.FORMAT_1_0)
+        target_archive = target_distroseries.main_archive
+        self.makeUploader(requester, target_archive, spph.component)
+        [synced_spph] = do_copy(
+            [spph], target_archive, target_distroseries,
+            pocket=spph.pocket, person=requester, allow_delayed_copies=False,
+            close_bugs=False)
+        return synced_spph
+
+    def makeChangesFile(self, spph, maintainer, maintainer_address,
+                        changer, changer_address):
+        temp_dir = self.makeTemporaryDirectory()
+        changes_file = os.path.join(
+            temp_dir, "%s.changes" % spph.source_package_name)
+        with open(changes_file, 'w') as changes:
+            changes.write(
+                "Maintainer: %s <%s>\n"
+                "Changed-By: %s <%s>\n"
+                % (
+                    maintainer.name,
+                    maintainer_address,
+                    changer.name,
+                    changer_address,
+                    ))
+        return FakeChangesFile(spph, changes_file)
+
+    def makeNascentUpload(self, spph, maintainer, maintainer_address,
+                          changer, changer_address):
+        """Create a `NascentUpload` for `spph`."""
+        changes = self.makeChangesFile(
+            spph, maintainer, maintainer_address, changer, changer_address)
+        upload = NascentUpload(
+            changes, FakeUploadPolicy(spph), DevNullLogger())
+        upload.queue_root = upload._createQueueEntry()
+        das = self.factory.makeDistroArchSeries(
+            distroseries=spph.distroseries)
+        bpb = self.factory.makeBinaryPackageBuild(
+            source_package_release=spph.sourcepackagerelease,
+            archive=spph.archive, distroarchseries=das, pocket=spph.pocket,
+            sourcepackagename=spph.sourcepackagename)
+        upload.queue_root.addBuild(bpb)
+        return upload
+
+    def processAndRejectUpload(self, nascent_upload):
+        nascent_upload.process()
+        # Obtain the required privileges for do_reject.
+        login('[e-mail]')
+        nascent_upload.do_reject(notify=True)
+
+    def getNotifiedAddresses(self):
+        """Get email addresses that were notified."""
+        return [message['to'] for message in pop_notifications()]
+
+    def test_failed_copy_builds_do_not_spam_upstream(self):
+        """Failed builds do not spam people who are not responsible for them.
+
+        We import Debian source packages, then sync them into Ubuntu (and
+        from there, into Ubuntu-derived distros).  Those syncs then trigger
+        builds that the original Debian maintainers and last-change authors
+        are not responsible for.
+
+        In a situation like that, we should not bother those people with the
+        failure.  We notify the person who requested the sync instead.
+
+        (The logic in lp.soyuz.adapters.notification may still notify the
+        author of the last change, if that person is also an uploader for the
+        archive that the failure happened in.  For this particular situation
+        we consider that not so much an intended behaviour, as an emergent one
+        that does not seem inappropriate.  It'd be hard to change if we wanted
+        to.)
+
+        This test guards against bug 876594.
+        """
+        maintainer, maintainer_address = self.makePersonWithEmail()
+        changer, changer_address = self.makePersonWithEmail()
+        dsp = self.factory.makeDistroSeriesParent()
+        original_spph = self.makeSPPH(dsp.parent_series, maintainer_address)
+        sync_requester, syncer_address = self.makePersonWithEmail()
+        synced_spph = self.syncSource(
+            original_spph, dsp.derived_series, sync_requester)
+        nascent_upload = self.makeNascentUpload(
+            synced_spph, maintainer, maintainer_address,
+            changer, changer_address)
+        pop_notifications()
+        self.processAndRejectUpload(nascent_upload)
+
+        notified_addresses = '\n'.join(self.getNotifiedAddresses())
+
+        self.assertNotIn(maintainer_address, notified_addresses)
+        self.assertNotIn(changer_address, notified_addresses)
+        self.assertIn(syncer_address, notified_addresses)

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/model/archive.py	2012-01-06 06:18:28 +0000
@@ -1288,18 +1288,14 @@
             #   - the given source package directly
             #   - a package set in the correct distro series that includes the
             #     given source package
-            source_allowed = self.checkArchivePermission(person,
-                                                         sourcepackagename)
+            source_allowed = self.checkArchivePermission(
+                person, sourcepackagename)
             set_allowed = self.isSourceUploadAllowed(
                 sourcepackagename, person, distroseries)
             if source_allowed or set_allowed:
                 return None
 
         if not self.getComponentsForUploader(person):
-            # XXX: JamesWestby 2010-08-01 bug=612351: We have to use
-            # is_empty() as we don't get an SQLObjectResultSet back, and
-            # so __nonzero__ isn't defined on it, and a straight bool
-            # check wouldn't do the right thing.
             if self.getPackagesetsForUploader(person).is_empty():
                 return NoRightsForArchive()
             else:

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/model/queue.py	2012-01-06 06:18:28 +0000
@@ -845,6 +845,27 @@
         # and uploading to any archive as the signer.
         return changes, strip_pgp_signature(changes_content).splitlines(True)
 
+    def findSourcePublication(self):
+        """Find the `SourcePackagePublishingHistory` for this build."""
+        first_build = self.builds[:1]
+        if first_build:
+            [first_build] = first_build
+            return first_build.build._getLatestPublication()
+        else:
+            return None
+
+    def findPersonToNotify(self):
+        """Find the right person to notify about this upload."""
+        spph = self.findSourcePublication()
+        if spph and self.sourcepackagerelease.upload_archive != self.archive:
+            # This is a build triggered by the syncing of a source
+            # package.  Notify the person who requested the sync.
+            return spph.creator
+        elif self.signing_key:
+            return self.signing_key.owner
+        else:
+            return None
+
     def notify(self, summary_text=None, changes_file_object=None,
                logger=None, dry_run=False):
         """See `IPackageUpload`."""
@@ -860,12 +881,9 @@
             changesfile_content = changes_file_object.read()
         else:
             changesfile_content = 'No changes file content available.'
-        if self.signing_key is not None:
-            signer = self.signing_key.owner
-        else:
-            signer = None
+        blamee = self.findPersonToNotify()
         notify(
-            signer, self.sourcepackagerelease, self.builds, self.customfiles,
+            blamee, 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=dry_run, logger=logger)