Merge lp:~jtv/launchpad/db-bug-786970 into lp:launchpad/db-devel

[Jeroen T. Vermeulen]

[r=allenap][bug=786970,787462] Register CannotCopy failures as DSDComments.

= Summary =

When a PlainPackageCopyJob fails with a CannotCopy error, that's not really meant to be an oops. It's meant to be an error that the user should address. But how do we tell the user? This job is something you "see going on" in the UI, so we want the errors in the UI as well — email is not an answer.

== Proposed fix ==

Julian and I (with feedback from Colin) decided on the following error-reporting mechanism: any one of the PlainPackageCopyJobs we're dealing with resolves a DistroSeriesDifference, which is what's visible in the UI. And that in turn can have a conversation stream of DistroSeriesDifferenceComments attached. So the job can throw CannotCopy failure messages into that stream, which also solves the problem of logging historical builds.

Next we'll make the page poll, so that these messages can show up on the fly.

== Pre-implementation notes ==

Multiple package copies for multiple DistroSeriesDifferences can go into a single PlainPackageCopyJob. We initially thought we could capture enough information from failures that a CannotCopy can be neatly pegged to a DistroSeriesDifference on the page.

But alas. There are complications: it takes some relatively complicated tracking, and because the checks are to some extent phased, you might see your package failing because some other package broke the whole job. In the end we decided on a simple fix: only handle one package per job. This affects two callsites.

Another question is: what Launchpad user will the error messages be attributed to? An attempt to start a discussion about having a user identity to represent Launchpad in the database for this sort of thing failed miserably. We thought it might be easy impersonate the requesting user (which will be a little disconcerting because automatically generated messages meant primarily for the user will look as if they'd been entered by that same user). But the only current place for that information is Job.requester, which… isn't accessible in Python. In the end Julian gave me the green light to use the Janitor, which fulfills many of the roles that I had wanted a "Launchpad persona" user for.

== Implementation details ==

For once I remembered to test the new code for database privileges. Which revealed a few that were needed, hopefully saving us some agony later on.

Sadly gone are very similar bits of code that Gavin and I developed in parallel for collating packages by archive, so as to bundle them into the minimum number of PlainPackageCopyJobs.

== Tests ==

{{{
./bin/test -vvc lp.soyuz.tests.test_packagecopyjob
}}}

== Demo and Q/A ==

Request asynchronous package copies on a derived distroseries' +localpackagediffs page. (Do this by requesting more copies than the max_synchronous_syncs feature flag setting at once). If any of them fail (and I admit I have yet to figure out how to do that) its error will show up as a comment.

= Launchpad lint =

No lint, but it's complicated. I touched a file that had pre-existing lint, which I did not fix, and then I undid my changes to that file.

[Gavin Panella (allenap)]

This looks really good.

However, I have a major beef with it: PlainPackageCopyJob is no longer
a plain copy job; it's now heavily intertwined with derivative
distributions. I think PlainPackageCopyJob should be restored, and a
new subclass created, something like DerivativePackageCopyJob, that
knows about DSDs and so forth. It's unfortunate that that involves a
lot of boilerplate code.

Other than that, I think it's a shame that packages can only be copied
one at a time, but I can see this is a far from trivial problem to
solve. It does mean that there's no longer an all-or-nothing guarantee
when copying multiple packages. A *big* downside is that users must
check all DSD comments for errors when they request multiple copies. I
assume both of those are acceptable compromises for now.

[1]

+ try:
+ self.attemptCopy()
+ except CannotCopy, e:
+ self.abort()
+ self.reportFailure(e)
+ self.commit()

Fwiw, the job runner commits after run(), so only the abort() is
needed.

[2]

+ def findMatchingDSDs(self):
[...]
+ source_distro = self.source_archive.distribution
+ return [
+ dsd
+ for dsd in candidates
+ if dsd.parent_series.distribution == source_distro]

Can you do with with IDs instead?

        source_distro_id = self.source_archive.distributionID
        return [
            dsd
            for dsd in candidates
                if dsd.parent_series.distributionID == source_distro_id]

[3]

+ def test_findMatchingDSDs_matches_all_DSDs_for_job(self):
+ # findMatchingDSDs finds matching DSDs for any of the packages
+ # in the job.
+ dsd = self.factory.makeDistroSeriesDifference()
+ job = removeSecurityProxy(self.makeJob(dsd))

This should be naked_job.

Also in test_findMatchingDSDs_ignores_other_source_series.

review: Needs Fixing

[Jeroen T. Vermeulen (jtv)]

Thanks Gavin.

> However, I have a major beef with it: PlainPackageCopyJob is no longer
> a plain copy job; it's now heavily intertwined with derivative
> distributions. I think PlainPackageCopyJob should be restored, and a
> new subclass created, something like DerivativePackageCopyJob, that
> knows about DSDs and so forth. It's unfortunate that that involves a
> lot of boilerplate code.

That seems like a job for a separate branch though; it will affect a lot more than what's in this branch.

Do I understand that correctly? If so, it seems easier to land this branch first and then make a separate card of splitting up the various usages of PPCJ. Would be nice to get the present branch off my hands!

> Other than that, I think it's a shame that packages can only be copied
> one at a time, but I can see this is a far from trivial problem to
> solve. It does mean that there's no longer an all-or-nothing guarantee
> when copying multiple packages. A *big* downside is that users must
> check all DSD comments for errors when they request multiple copies. I
> assume both of those are acceptable compromises for now.

Yes— we considered that, but it turns out that multi-parent breaks that anyway. You could be selecting DSDs from different parent archives, which have to go into separate jobs regardless!

So in the final analysis we can blame this on the post-last-minute change in requirements.

> + try:
> + self.attemptCopy()
> + except CannotCopy, e:
> + self.abort()
> + self.reportFailure(e)
> + self.commit()
>
> Fwiw, the job runner commits after run(), so only the abort() is
> needed.

Nice one! That commit was there from a time when the exception was still propagated, which I preserved in this branch initially but was then rightly asked to change.

> + def findMatchingDSDs(self):
> [...]
> + source_distro = self.source_archive.distribution
> + return [
> + dsd
> + for dsd in candidates
> + if dsd.parent_series.distribution == source_distro]
>
> Can you do with with IDs instead?

Sure. Silly thing that the ORM needs to fetch the distribution there, really.

> + def test_findMatchingDSDs_matches_all_DSDs_for_job(self):
> + # findMatchingDSDs finds matching DSDs for any of the packages
> + # in the job.
> + dsd = self.factory.makeDistroSeriesDifference()
> + job = removeSecurityProxy(self.makeJob(dsd))
>
> This should be naked_job.
>
> Also in test_findMatchingDSDs_ignores_other_source_series.

Fixed.

Jeroen

[Gavin Panella (allenap)]

> Thanks Gavin.
>
> > However, I have a major beef with it: PlainPackageCopyJob is no longer
> > a plain copy job; it's now heavily intertwined with derivative
> > distributions. I think PlainPackageCopyJob should be restored, and a
> > new subclass created, something like DerivativePackageCopyJob, that
> > knows about DSDs and so forth. It's unfortunate that that involves a
> > lot of boilerplate code.
>
> That seems like a job for a separate branch though; it will affect a lot more
> than what's in this branch.
>
> Do I understand that correctly? If so, it seems easier to land this branch
> first and then make a separate card of splitting up the various usages of
> PPCJ. Would be nice to get the present branch off my hands!

As agreed otp just now (between bigjools, jtv and allenap), it's cool
to leave this branch as it is and things can be split out again if and
when a solid requirement is there.

>
>
> > Other than that, I think it's a shame that packages can only be copied
> > one at a time, but I can see this is a far from trivial problem to
> > solve. It does mean that there's no longer an all-or-nothing guarantee
> > when copying multiple packages. A *big* downside is that users must
> > check all DSD comments for errors when they request multiple copies. I
> > assume both of those are acceptable compromises for now.
>
> Yes— we considered that, but it turns out that multi-parent breaks that
> anyway. You could be selecting DSDs from different parent archives, which
> have to go into separate jobs regardless!

Of course, that's very true, I didn't think of that.

>
> So in the final analysis we can blame this on the post-last-minute change in
> requirements.

Yes! Grumble, grumble.

review: Approve

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-05-25 17:29:39 +0000
+++ database/schema/security.cfg	2011-05-28 06:24:35 +0000
@@ -984,6 +984,7 @@
 
 [sync_packages]
 groups=script
+public.account                                = SELECT
 public.archive                                = SELECT
 public.archivepermission                      = SELECT, INSERT
 public.binarypackagebuild                     = SELECT, INSERT
@@ -1001,12 +1002,17 @@
 public.distributionsourcepackage              = SELECT, INSERT, UPDATE
 public.distroarchseries                       = SELECT, INSERT
 public.distroseries                           = SELECT, UPDATE
+public.distroseriesdifference                 = SELECT
+public.distroseriesdifferencemessage          = SELECT, INSERT, UPDATE
 public.distroseriesparent                     = SELECT
+public.emailaddress                           = SELECT
 public.flatpackagesetinclusion                = SELECT, INSERT
 public.gpgkey                                 = SELECT
 public.job                                    = SELECT, INSERT, UPDATE, DELETE
 public.libraryfilealias                       = SELECT, INSERT, UPDATE, DELETE
 public.libraryfilecontent                     = SELECT, INSERT
+public.message                                = SELECT, INSERT, UPDATE
+public.messagechunk                           = SELECT, INSERT, UPDATE
 public.packagebuild                           = SELECT, INSERT
 public.packagecopyjob                         = SELECT
 public.packageset                             = SELECT, INSERT

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-05-27 08:02:37 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-05-28 06:24:35 +0000
@@ -1078,17 +1078,14 @@
         """Request sync of packages that can be easily upgraded."""
         target_distroseries = self.context
         target_archive = target_distroseries.main_archive
-        differences_by_archive = (
-            getUtility(IDistroSeriesDifferenceSource)
-                .collateDifferencesByParentArchive(self.getUpgrades()))
-        for source_archive, differences in differences_by_archive.iteritems():
-            source_package_info = [
-                (difference.source_package_name.name,
-                 difference.parent_source_version)
-                for difference in differences]
-            getUtility(IPlainPackageCopyJobSource).create(
-                source_package_info, source_archive, target_archive,
-                target_distroseries, PackagePublishingPocket.UPDATES)
+        job_source = getUtility(IPlainPackageCopyJobSource)
+
+        for dsd in self.getUpgrades():
+            job_source.create(
+                [specify_dsd_package(dsd)], dsd.parent_series.main_archive,
+                target_archive, target_distroseries,
+                PackagePublishingPocket.UPDATES)
+
         self.request.response.addInfoNotification(
             (u"Upgrades of {context.displayname} packages have been "
              u"requested. Please give Launchpad some time to complete "

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-05-27 07:50:38 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-05-28 06:24:35 +0000
@@ -1090,13 +1090,17 @@
         with StormStatementRecorder() as recorder1:
             self.makeView(derived_series).requestUpgrades()
         self.assertThat(recorder1, HasQueryCount(LessThan(10)))
-        # The query count does not increase with more differences.
-        for index in xrange(3):
+        # Creating Jobs and DistributionJobs takes 2 extra queries per
+        # requested sync.
+        requested_syncs = 3
+        for index in xrange(requested_syncs):
             self.makePackageUpgrade(derived_series=derived_series)
         flush_database_caches()
         with StormStatementRecorder() as recorder2:
             self.makeView(derived_series).requestUpgrades()
-        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+        self.assertThat(
+            recorder2,
+            HasQueryCount(Equals(recorder1.count + 2 * requested_syncs)))
 
 
 class TestDistroSeriesLocalDifferencesFunctional(TestCaseWithFactory,

=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py	2011-05-24 15:36:35 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py	2011-05-28 06:24:35 +0000
@@ -338,15 +338,3 @@
 
         Blacklisted items are excluded.
         """
-
-    def collateDifferencesByParentArchive(differences):
-        """Collate the given differences by parent archive.
-
-        The given `IDistroSeriesDifference`s are returned in a `dict`, with
-        the parent `Archive` as keys.
-
-        :param differences: An iterable sequence of `IDistroSeriesDifference`.
-
-        :return: A `dict` of iterable sequences of `IDistroSeriesDifference`
-            keyed by their parent `IArchive`.
-        """

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-05-24 11:32:34 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-05-28 06:24:35 +0000
@@ -494,17 +494,6 @@
                 DistroSeriesDifference.source_package_name_id)
         return DecoratedResultSet(differences, itemgetter(0))
 
-    @staticmethod
-    def collateDifferencesByParentArchive(differences):
-        by_archive = dict()
-        for difference in differences:
-            archive = difference.parent_series.main_archive
-            if archive in by_archive:
-                by_archive[archive].append(difference)
-            else:
-                by_archive[archive] = [difference]
-        return by_archive
-
     @cachedproperty
     def source_pub(self):
         """See `IDistroSeriesDifference`."""

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2011-05-24 15:36:35 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2011-05-28 06:24:35 +0000
@@ -1129,24 +1129,6 @@
         self.assertContentEqual(
             [], dsd_source.getSimpleUpgrades(dsd.derived_series))
 
-    def test_collateDifferencesByParentArchive(self):
-        dsp1 = self.factory.makeDistroSeriesParent()
-        dsp2 = self.factory.makeDistroSeriesParent()
-        differences = [
-            self.factory.makeDistroSeriesDifference(dsp1.derived_series),
-            self.factory.makeDistroSeriesDifference(dsp2.derived_series),
-            self.factory.makeDistroSeriesDifference(dsp1.derived_series),
-            self.factory.makeDistroSeriesDifference(dsp2.derived_series),
-            ]
-        dsd_source = getUtility(IDistroSeriesDifferenceSource)
-        observed = (
-            dsd_source.collateDifferencesByParentArchive(differences))
-        expected = {
-            dsp1.parent_series.main_archive: differences[0::2],
-            dsp2.parent_series.main_archive: differences[1::2],
-            }
-        self.assertEqual(observed, expected)
-
 
 class TestMostRecentComments(TestCaseWithFactory):
 

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2011-05-26 15:06:44 +0000
+++ lib/lp/soyuz/browser/archive.py	2011-05-28 06:24:35 +0000
@@ -1285,19 +1285,6 @@
         dest_display_name)
 
 
-def partition_pubs_by_archive(source_pubs):
-    """Group `source_pubs` by archive.
-
-    :param source_pubs: A sequence of `SourcePackagePublishingHistory`.
-    :return: A dict mapping `Archive`s to the list of entries from
-        `source_pubs` that are in that archive.
-    """
-    by_source_archive = {}
-    for spph in source_pubs:
-        by_source_archive.setdefault(spph.archive, []).append(spph)
-    return by_source_archive
-
-
 def name_pubs_with_versions(source_pubs):
     """Annotate each entry from `source_pubs` with its version.
 
@@ -1328,11 +1315,11 @@
             person, dest_archive, dest_series, dest_pocket, spns)
 
     job_source = getUtility(IPlainPackageCopyJobSource)
-    archive_pubs = partition_pubs_by_archive(source_pubs)
-    for source_archive, spphs in archive_pubs.iteritems():
+    for spph in source_pubs:
         job_source.create(
-            name_pubs_with_versions(spphs), source_archive, dest_archive,
+            name_pubs_with_versions([spph]), spph.archive, dest_archive,
             dest_series, dest_pocket, include_binaries=include_binaries)
+
     return structured("""
         <p>Requested sync of %s packages.</p>
         <p>Please allow some time for these to be processed.</p>

=== modified file 'lib/lp/soyuz/browser/tests/test_package_copying_mixin.py'
--- lib/lp/soyuz/browser/tests/test_package_copying_mixin.py	2011-05-20 20:40:19 +0000
+++ lib/lp/soyuz/browser/tests/test_package_copying_mixin.py	2011-05-28 06:24:35 +0000
@@ -19,7 +19,6 @@
     FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS,
     name_pubs_with_versions,
     PackageCopyingMixin,
-    partition_pubs_by_archive,
     render_cannotcopy_as_html,
     )
 from lp.soyuz.interfaces.archive import CannotCopy
@@ -104,30 +103,6 @@
         packages = [self.getUniqueString() for counter in range(300)]
         self.assertFalse(PackageCopyingMixin().canCopySynchronously(packages))
 
-    def test_partition_pubs_by_archive_maps_archives_to_pubs(self):
-        # partition_pubs_by_archive returns a dict mapping each archive
-        # to a list of SPPHs on that archive.
-        spph = FakeSPPH()
-        self.assertEqual(
-            {spph.archive: [spph]}, partition_pubs_by_archive([spph]))
-
-    def test_partition_pubs_by_archive_splits_by_archive(self):
-        # partition_pubs_by_archive keeps SPPHs for different archives
-        # separate.
-        spphs = [FakeSPPH() for counter in xrange(2)]
-        mapping = partition_pubs_by_archive(spphs)
-        self.assertEqual(
-            dict((spph.archive, [spph]) for spph in spphs), mapping)
-
-    def test_partition_pubs_by_archive_clusters_by_archive(self):
-        # partition_pubs_by_archive bundles SPPHs for the same archive
-        # into a single dict entry.
-        archive = FakeArchive()
-        spphs = [FakeSPPH(archive=archive) for counter in xrange(2)]
-        mapping = partition_pubs_by_archive(spphs)
-        self.assertEqual([archive], mapping.keys())
-        self.assertContentEqual(spphs, mapping[archive])
-
     def test_name_pubs_with_versions_lists_packages_and_versions(self):
         # name_pubs_with_versions returns a list of tuples of source
         # package name and source package version, one per SPPH.

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-05-19 17:20:02 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-05-28 06:24:35 +0000
@@ -17,6 +17,8 @@
     Reference,
     Unicode,
     )
+import transaction
+from zope.component import getUtility
 from zope.interface import (
     classProvides,
     implements,
@@ -26,13 +28,20 @@
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet,
     )
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.lpstorm import (
     IMasterStore,
     IStore,
     )
 from lp.app.errors import NotFoundError
+from lp.registry.interfaces.distroseriesdifference import (
+    IDistroSeriesDifferenceSource,
+    )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.distroseries import DistroSeries
+from lp.registry.interfaces.distroseriesdifferencecomment import (
+    IDistroSeriesDifferenceCommentSource,
+    )
 from lp.services.database.stormbase import StormBase
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
@@ -153,6 +162,11 @@
 
 class PlainPackageCopyJob(PackageCopyJobDerived):
     """Job that copies packages between archives."""
+    # This job type serves in different places: it supports copying
+    # packages between archives, but also the syncing of packages from
+    # parents into a derived distroseries.  We may split these into
+    # separate types at some point, but for now we (allenap, bigjools,
+    # jtv) chose to keep it as one.
 
     implements(IPlainPackageCopyJob)
 
@@ -232,6 +246,18 @@
 
     def run(self):
         """See `IRunnableJob`."""
+        try:
+            self.attemptCopy()
+        except CannotCopy, e:
+            self.abort()
+            self.reportFailure(e)
+
+    def attemptCopy(self):
+        """Attempt to perform the copy.
+
+        :raise CannotCopy: If the copy fails for a reason that the user
+            can deal with.
+        """
         if self.target_archive.is_ppa:
             if self.target_pocket != PackagePublishingPocket.RELEASE:
                 raise CannotCopy(
@@ -250,6 +276,42 @@
             series=self.target_distroseries, pocket=self.target_pocket,
             include_binaries=self.include_binaries, check_permissions=False)
 
+    def abort(self):
+        """Abort work."""
+        transaction.abort()
+
+    def findMatchingDSDs(self):
+        """Find any `DistroSeriesDifference`s that this job might resolve."""
+        dsd_source = getUtility(IDistroSeriesDifferenceSource)
+        target_series = self.target_distroseries
+        candidates = dsd_source.getForDistroSeries(
+            distro_series=target_series)
+        # The job doesn't know what distroseries a given package is
+        # coming from, and the version number in the DSD may have
+        # changed.  We can however filter out DSDs that are from
+        # different distributions, based on the job's target archive.
+        source_distro_id = self.source_archive.distributionID
+        return [
+            dsd
+            for dsd in candidates
+                if dsd.parent_series.distributionID == source_distro_id]
+
+    def reportFailure(self, cannotcopy_exception):
+        """Attempt to report failure to the user."""
+        message = unicode(cannotcopy_exception)
+        dsds = self.findMatchingDSDs()
+        comment_source = getUtility(IDistroSeriesDifferenceCommentSource)
+
+        # Register the error comment in the name of the Janitor.  Not a
+        # great choice, but we have no user identity to represent
+        # Launchpad; it's far too costly to create one; and
+        # impersonating the requester can be misleading and would also
+        # involve extra bookkeeping.
+        reporting_persona = getUtility(ILaunchpadCelebrities).janitor
+
+        for dsd in dsds:
+            comment_source.new(dsd, reporting_persona, message)
+
     def __repr__(self):
         """Returns an informative representation of the job."""
         parts = ["%s to copy" % self.__class__.__name__]

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-24 14:29:51 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-28 06:24:35 +0000
@@ -8,7 +8,12 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.config import config
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.testing import LaunchpadZopelessLayer
+from lp.registry.model.distroseriesdifferencecomment import (
+    DistroSeriesDifferenceComment,
+    )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
@@ -34,15 +39,23 @@
     run_script,
     TestCaseWithFactory,
     )
-
-
-class PlainPackageCopyJobTests(TestCaseWithFactory):
-    """Test case for PlainPackageCopyJob."""
-
-    layer = LaunchpadZopelessLayer
-
-    def makeJob(self, dsd):
+from lp.testing.fakemethod import FakeMethod
+
+
+def get_dsd_comments(dsd):
+    """Retrieve `DistroSeriesDifferenceComment`s for `dsd`."""
+    return IStore(dsd).find(
+        DistroSeriesDifferenceComment,
+        DistroSeriesDifferenceComment.distro_series_difference == dsd)
+
+
+class LocalTestHelper:
+    """Put test helpers that want to be in the test classes here."""
+
+    def makeJob(self, dsd=None):
         """Create a `PlainPackageCopyJob` that would resolve `dsd`."""
+        if dsd is None:
+            dsd = self.factory.makeDistroSeriesDifference()
         source_packages = [specify_dsd_package(dsd)]
         source_archive = dsd.parent_series.main_archive
         target_archive = dsd.derived_series.main_archive
@@ -58,6 +71,12 @@
         self.layer.switchDbUser('sync_packages')
         job.run()
 
+
+class PlainPackageCopyJobTests(TestCaseWithFactory, LocalTestHelper):
+    """Test case for PlainPackageCopyJob."""
+
+    layer = LaunchpadZopelessLayer
+
     def test_create(self):
         # A PackageCopyJob can be created and stores its arguments.
         distroseries = self.factory.makeDistroSeries()
@@ -105,23 +124,59 @@
 
     def test_getActiveJobs_only_returns_waiting_jobs(self):
         # getActiveJobs ignores jobs that aren't in the WAITING state.
-        job = self.makeJob(self.factory.makeDistroSeriesDifference())
+        job = self.makeJob()
         removeSecurityProxy(job).job._status = JobStatus.RUNNING
         source = getUtility(IPlainPackageCopyJobSource)
         self.assertContentEqual([], source.getActiveJobs(job.target_archive))
 
-    def test_run_unknown_package(self):
-        # A job properly records failure.
+    def test_run_raises_errors(self):
+        # A job reports unexpected errors as exceptions.
+        class Boom(Exception):
+            pass
+
+        job = self.makeJob()
+        removeSecurityProxy(job).attemptCopy = FakeMethod(failure=Boom())
+
+        self.assertRaises(Boom, job.run)
+
+    def test_run_posts_copy_failure_as_comment(self):
+        # If the job fails with a CannotCopy exception, it swallows the
+        # exception and posts a DistroSeriesDifferenceComment with the
+        # failure message.
+        dsd = self.factory.makeDistroSeriesDifference()
+        self.factory.makeArchive(distribution=dsd.derived_series.distribution)
+        job = self.makeJob(dsd)
+        removeSecurityProxy(job).attemptCopy = FakeMethod(
+            failure=CannotCopy("Server meltdown"))
+
+        # The job's error handling will abort, so commit the objects we
+        # created as would have happened in real life.
+        transaction.commit()
+
+        job.run()
+
+        self.assertEqual(
+            ["Server meltdown"],
+            [comment.body_text for comment in get_dsd_comments(dsd)])
+
+    def test_run_cannot_copy_unknown_package(self):
+        # Attempting to copy an unknown package is reported as a
+        # failure.
         distroseries = self.factory.makeDistroSeries()
         archive1 = self.factory.makeArchive(distroseries.distribution)
         archive2 = self.factory.makeArchive(distroseries.distribution)
-        source = getUtility(IPlainPackageCopyJobSource)
-        job = source.create(
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        job = job_source.create(
             source_packages=[("foo", "1.0-1")], source_archive=archive1,
             target_archive=archive2, target_distroseries=distroseries,
             target_pocket=PackagePublishingPocket.RELEASE,
             include_binaries=False)
-        self.assertRaises(CannotCopy, self.runJob, job)
+        naked_job = removeSecurityProxy(job)
+        naked_job.reportFailure = FakeMethod()
+
+        job.run()
+
+        self.assertEqual(1, naked_job.reportFailure.call_count)
 
     def test_target_ppa_non_release_pocket(self):
         # When copying to a PPA archive the target must be the release pocket.
@@ -134,7 +189,13 @@
             target_archive=archive2, target_distroseries=distroseries,
             target_pocket=PackagePublishingPocket.UPDATES,
             include_binaries=False)
-        self.assertRaises(CannotCopy, self.runJob, job)
+
+        naked_job = removeSecurityProxy(job)
+        naked_job.reportFailure = FakeMethod()
+
+        job.run()
+
+        self.assertEqual(1, naked_job.reportFailure.call_count)
 
     def test_run(self):
         # A proper test run synchronizes packages.
@@ -164,8 +225,9 @@
 
         source = getUtility(IPlainPackageCopyJobSource)
         job = source.create(
-            source_packages=[("libc", "2.8-1")], source_archive=breezy_archive,
-            target_archive=target_archive, target_distroseries=target_series,
+            source_packages=[("libc", "2.8-1")],
+            source_archive=breezy_archive, target_archive=target_archive,
+            target_distroseries=target_series,
             target_pocket=PackagePublishingPocket.RELEASE,
             include_binaries=False)
         self.assertContentEqual(
@@ -275,8 +337,7 @@
     def test_getPendingJobsPerPackage_ignores_other_distroseries(self):
         # getPendingJobsPerPackage only looks for jobs on the indicated
         # distroseries.
-        dsd = self.factory.makeDistroSeriesDifference()
-        self.makeJob(dsd)
+        self.makeJob()
         other_series = self.factory.makeDistroSeries()
         job_source = getUtility(IPlainPackageCopyJobSource)
         self.assertEqual(
@@ -333,3 +394,48 @@
         job_source = getUtility(IPlainPackageCopyJobSource)
         self.assertEqual(
             {}, job_source.getPendingJobsPerPackage(dsd.derived_series))
+
+    def test_findMatchingDSDs_matches_all_DSDs_for_job(self):
+        # findMatchingDSDs finds matching DSDs for any of the packages
+        # in the job.
+        dsd = self.factory.makeDistroSeriesDifference()
+        naked_job = removeSecurityProxy(self.makeJob(dsd))
+        self.assertContentEqual([dsd], naked_job.findMatchingDSDs())
+
+    def test_findMatchingDSDs_ignores_other_source_series(self):
+        # findMatchingDSDs tries to ignore DSDs that are for different
+        # parent series than the job's source series.  (This can't be
+        # done with perfect precision because the job doesn't keep track
+        # of source distroseries, but in practice it should be good
+        # enough).
+        dsd = self.factory.makeDistroSeriesDifference()
+        naked_job = removeSecurityProxy(self.makeJob(dsd))
+
+        # If the dsd differs only in parent series, that's enough to
+        # make it a non-match.
+        removeSecurityProxy(dsd).parent_series = (
+            self.factory.makeDistroSeries())
+
+        self.assertContentEqual([], naked_job.findMatchingDSDs())
+
+
+class TestPlainPackageCopyJobPrivileges(TestCaseWithFactory, LocalTestHelper):
+    """Test that `PlainPackageCopyJob` has the privileges it needs.
+
+    This test looks for errors, not failures.  It's here only to see that
+    these operations don't run into any privilege limitations.
+    """
+
+    layer = LaunchpadZopelessLayer
+
+    def test_findMatchingDSDs(self):
+        job = self.makeJob()
+        transaction.commit()
+        self.layer.switchDbUser(config.IPlainPackageCopyJobSource.dbuser)
+        removeSecurityProxy(job).findMatchingDSDs()
+
+    def test_reportFailure(self):
+        job = self.makeJob()
+        transaction.commit()
+        self.layer.switchDbUser(config.IPlainPackageCopyJobSource.dbuser)
+        removeSecurityProxy(job).reportFailure(CannotCopy("Mommy it hurts"))