launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/db-bug-781514 into lp:launchpad/db-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #781514 in Launchpad itself: "UI for async syncs: showing syncs in progress"
  https://bugs.launchpad.net/launchpad/+bug/781514

For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-781514/+merge/61557

= Summary =

The DistroSeries:+localpackagediffs page should show which of the DistroSeriesDifferences it displays are in the process of being synchronized away.

Synchronizing a package that represents a difference between a distroseries and a parent series resolves the difference.  The synchronization is performed by a PlainPackageCopyJob.


== Proposed fix ==

We're taking this in stages.  In this first stage, the static page displays which differences are being synchronized at the time the page is generated.

The entry's checkbox (normally used to request synchronization manually, which doesn't make sense when the sync is already in the pipeline) is replaced with a "please wait" clockface, and a simple notice "(Synchronizing)" is added.

It's probably not very good UI design.  We'll still want to address that.  But this will give us a first working view to criticize, and that's exactly easier than setting up test data on a development machine.


== Pre-implementation notes ==

Discussed with Robert and Julian at various stages.  At first I went down the rabbit hole of adding "this job is taking care of me" information to DistroSeriesDifference, using lazr.delegates.  But that is vastly overweight: I gave up when I found I needed to create a new interface and register it in ZCML!

Raphaël gave me a hand in finding where to disable the checkbox.  We'll also want to disable it in another case — when the difference shows a newer package in the child distroseries than in the parent.


== Implementation details ==

The view code may seem overly verbose.  But I'd like to make sure we get the version check in right away, and the new browser code now has a perfect nook for it.  (I would have included the fix in this branch but that means extra testing as well, and before you know it the branch gets out of hand).


== Tests ==

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


== Demo and Q/A ==

Find a derived distroseries, and view its +localpackagediffs page.  Hit the Upgrade button to generate asynchronous sync requests (the Sync button may generate synchronous requests, which is no good to us here).

The page should now show the affected differences as syncing.  For Upgrade they are the ones where the parent has a newer version than the derived series, and the derived series has no local changes to the package.  These entries will show clockfaces where they had checkboxes before, and be marked "(Synchronizing)."


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/templates/distroseries-localdifferences.pt
  lib/lp/soyuz/model/packagecopyjob.py
  lib/lp/soyuz/interfaces/packagecopyjob.py
  lib/lp/soyuz/tests/test_packagecopyjob.py
  lib/lp/registry/browser/distroseries.py
-- 
https://code.launchpad.net/~jtv/launchpad/db-bug-781514/+merge/61557
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-781514 into lp:launchpad/db-devel.

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-05-18 18:39:03 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-05-19 12:51:17 +0000
@@ -851,6 +851,25 @@
         return (has_perm and
                 self.cached_differences.batch.total() > 0)
 
+    @cachedproperty
+    def pending_syncs(self):
+        """Pending synchronization jobs for this distroseries.
+
+        :return: A dict mapping (name, version) package specifications to
+            pending sync jobs.
+        """
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        return job_source.getPendingJobsPerPackage(self.context)
+
+    def hasPendingSync(self, dsd):
+        """Is there a package-copying job pending to resolve `dsd`?"""
+        return self.pending_syncs.get(dsd) is None
+
+    def canRequestSync(self, dsd):
+        """Does it make sense to request a sync for this difference?"""
+        # XXX JeroenVermeulen bug=783435: Also compare versions.
+        return not self.hasPendingSync(dsd)
+
     @property
     def specified_name_filter(self):
         """If specified, return the name filter from the GET form data."""

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2011-05-13 15:13:54 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2011-05-19 12:51:17 +0000
@@ -74,14 +74,20 @@
                 tal:attributes="class src_name">
 
               <td>
-                <input tal:condition="can_perform_sync"
-                  name="field.selected_differences" type="checkbox"
-                  tal:attributes="
-                    value diff_id;
-                    id string:field.selected_differences.${diff_id}"/>
+                <tal:checkbox tal:condition="view/canRequestSync/difference">
+                  <input tal:condition="can_perform_sync"
+                    name="field.selected_differences" type="checkbox"
+                    tal:attributes="
+                      value diff_id;
+                      id string:field.selected_differences.${diff_id}"/>
 
-                <a tal:attributes="href difference/fmt:url" class="toggle-extra"
-                   tal:content="src_name">Foo</a>
+                  <a tal:attributes="href difference/fmt:url" class="toggle-extra"
+                     tal:content="src_name">Foo</a>
+                </tal:checkbox>
+                <tal:clockface condition="view/hasPendingSync/difference">
+                  <span class="sprite build_depwait"></span>
+                  <i>(Synchronizing)</i>
+                </tal:clockface>
               </td>
               <td tal:condition="python: not(view.has_unique_parent) and view.show_parent_version">
                 <a tal:attributes="href difference/parent_series/fmt:url"

=== modified file 'lib/lp/soyuz/interfaces/packagecopyjob.py'
--- lib/lp/soyuz/interfaces/packagecopyjob.py	2011-05-13 08:21:38 +0000
+++ lib/lp/soyuz/interfaces/packagecopyjob.py	2011-05-19 12:51:17 +0000
@@ -103,6 +103,21 @@
     def getActiveJobs(target_archive):
         """Retrieve all active sync jobs for an archive."""
 
+    def getPendingJobsPerPackage(target_series):
+        """Find pending jobs for each package in `target_series`.
+
+        This is meant for finding jobs that will resolve specific
+        `DistroSeriesDifference`s, so see also `specify_dsd_package`.
+
+        :param target_series: Target `DistroSeries`; this corresponds to
+            `DistroSeriesDifference.derived_series`.
+        :return: A dict containing as keys the (name, version) tuples for
+            each `DistroSeriesDifference` that has a resolving
+            `PlainPackageCopyJob` pending.  Each of these DSDs maps to its
+            oldest pending job.  The `version` corresponds to
+            `DistroSeriesDifference.parent_source_version`.
+        """
+
 
 class IPlainPackageCopyJob(IRunnableJob):
     """A no-frills job to copy packages between `IArchive`s."""

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-05-10 09:15:18 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-05-19 12:51:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -6,6 +6,7 @@
 __all__ = [
     "PackageCopyJob",
     "PlainPackageCopyJob",
+    "specify_dsd_package",
 ]
 
 from lazr.delegates import delegates
@@ -33,6 +34,7 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.distroseries import DistroSeries
 from lp.services.database.stormbase import StormBase
+from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
 from lp.soyuz.interfaces.archive import CannotCopy
@@ -46,6 +48,17 @@
 from lp.soyuz.scripts.packagecopier import do_copy
 
 
+def specify_dsd_package(dsd):
+    """Return (name, parent version) for `dsd`'s package.
+
+    This describes the package that `dsd` is for in a format suitable for
+    `PlainPackageCopyJobSource`.
+
+    :param dsd: A `DistroSeriesDifference`.
+    """
+    return (dsd.source_package_name.name, dsd.parent_source_version)
+
+
 class PackageCopyJob(StormBase):
     """Base class for package copying jobs."""
 
@@ -174,6 +187,34 @@
             PackageCopyJob.target_archive == target_archive)
         return DecoratedResultSet(jobs, cls)
 
+    @classmethod
+    def getPendingJobsForTargetSeries(cls, target_series):
+        """Get upcoming jobs for `target_series`, ordered by age."""
+        pending_states = [
+            JobStatus.WAITING,
+            JobStatus.RUNNING,
+            ]
+        raw_jobs = IStore(PackageCopyJob).find(
+            PackageCopyJob,
+            Job.id == PackageCopyJob.job_id,
+            PackageCopyJob.job_type == cls.class_job_type,
+            PackageCopyJob.target_distroseries == target_series,
+            Job._status.is_in(pending_states)).order_by(PackageCopyJob.id)
+        return DecoratedResultSet(raw_jobs, cls)
+
+    @classmethod
+    def getPendingJobsPerPackage(cls, target_series):
+        """See `IPlainPackageCopyJobSource`."""
+        result = {}
+        # Go through jobs in-order, picking the first matching job for
+        # any (package, version) tuple.  Because of how
+        # getPendingJobsForTargetSeries orders its results, the first
+        # will be the oldest and thus presumably the first to finish.
+        for job in cls.getPendingJobsForTargetSeries(target_series):
+            for package in job.metadata["source_packages"]:
+                result.setdefault(tuple(package), job)
+        return result
+
     @property
     def source_packages(self):
         getPublishedSources = self.source_archive.getPublishedSources

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-10 09:15:18 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-19 12:51:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for sync package jobs."""
@@ -10,12 +10,14 @@
 
 from canonical.testing import LaunchpadZopelessLayer
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.job.interfaces.job import JobStatus
 from lp.soyuz.interfaces.archive import CannotCopy
 from lp.soyuz.interfaces.packagecopyjob import (
     IPackageCopyJob,
     IPlainPackageCopyJobSource,
     )
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.soyuz.model.packagecopyjob import specify_dsd_package
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
     run_script,
@@ -28,6 +30,17 @@
 
     layer = LaunchpadZopelessLayer
 
+    def makeJob(self, dsd):
+        """Create a `PlainPackageCopyJob` that would resolve `dsd`."""
+        source_packages = [specify_dsd_package(dsd)]
+        source_archive = dsd.parent_series.main_archive
+        target_archive = dsd.derived_series.main_archive
+        target_distroseries = dsd.derived_series
+        target_pocket = self.factory.getAnyPocket()
+        return getUtility(IPlainPackageCopyJobSource).create(
+            source_packages, source_archive, target_archive,
+            target_distroseries, target_pocket)
+
     def test_create(self):
         # A PackageCopyJob can be created and stores its arguments.
         distroseries = self.factory.makeDistroSeries()
@@ -200,3 +213,74 @@
                 distroseries=distroseries, archive1=archive1,
                 archive2=archive2),
             repr(job))
+
+    def test_getPendingJobsPerPackage_finds_jobs(self):
+        # getPendingJobsPerPackage finds jobs, and the packages they
+        # belong to.
+        dsd = self.factory.makeDistroSeriesDifference()
+        job = self.makeJob(dsd)
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            {specify_dsd_package(dsd): job},
+            job_source.getPendingJobsPerPackage(dsd.derived_series))
+
+    def test_getPendingJobsPerPackage_ignores_other_distroseries(self):
+        # getPendingJobsPerPackage only looks for jobs on the indicated
+        # distroseries.
+        dsd = self.factory.makeDistroSeriesDifference()
+        self.makeJob(dsd)
+        other_series = self.factory.makeDistroSeries()
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            {}, job_source.getPendingJobsPerPackage(other_series))
+
+    def test_getPendingJobsPerPackage_only_returns_upcoming_jobs(self):
+        # getPendingJobsPerPackage ignores jobs that have already been
+        # run.
+        dsd = self.factory.makeDistroSeriesDifference()
+        package = specify_dsd_package(dsd)
+        job = self.makeJob(dsd)
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        found_by_state = {}
+        for status in JobStatus.items:
+            removeSecurityProxy(job).job._status = status
+            result = job_source.getPendingJobsPerPackage(dsd.derived_series)
+            if len(result) > 0:
+                found_by_state[status] = result[package]
+        expected = {
+            JobStatus.WAITING: job,
+            JobStatus.RUNNING: job,
+        }
+        self.assertEqual(expected, found_by_state)
+
+    def test_getPendingJobsPerPackage_distinguishes_jobs(self):
+        # getPendingJobsPerPackage associates the right job with the
+        # right package.
+        derived_series = self.factory.makeDistroSeries()
+        dsds = [
+            self.factory.makeDistroSeriesDifference(
+                derived_series=derived_series)
+            for counter in xrange(2)]
+        jobs = map(self.makeJob, dsds)
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            dict(zip(map(specify_dsd_package, dsds), jobs)),
+            job_source.getPendingJobsPerPackage(derived_series))
+
+    def test_getPendingJobsPerPackage_picks_oldest_job_for_dsd(self):
+        # If there are multiple jobs for one package,
+        # getPendingJobsPerPackage picks the oldest.
+        dsd = self.factory.makeDistroSeriesDifference()
+        jobs = [self.makeJob(dsd) for counter in xrange(2)]
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            {specify_dsd_package(dsd): jobs[0]},
+            job_source.getPendingJobsPerPackage(dsd.derived_series))
+
+    def test_getPendingJobsPerPackage_ignores_dsds_without_jobs(self):
+        # getPendingJobsPerPackage produces no dict entry for packages
+        # that have no pending jobs, even if they do have DSDs.
+        dsd = self.factory.makeDistroSeriesDifference()
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            {}, job_source.getPendingJobsPerPackage(dsd.derived_series))