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

[Jeroen T. Vermeulen]

[r=adeuring] Show updating as well as synchronizing DSDs in the UI.

= Summary =

When a DistroSeriesDifference on the DistroSeries:+localpackagediffs page is waiting for a sync to be completed, it is marked with "updating..."

We want that for pending DSD updates as well.

== Proposed fix ==

Very similar to what we already do for the package-copy jobs (the package syncs): add a "find jobs relevant to these DSDs" method to the job source. Call it once from a cachedproperty on the view, for the entire batch. Have a simple lookup method on the view to see whether a given DSD has jobs pending.

This was also a nice opportunity to decisions from TAL to the view class. I added a view method that computes the new string (and note that a DSD may be syncing and updating at the same time).

== Pre-implementation notes ==

Discussed with Julian. Note that a pending DSD update should not stop the user from requesting a sync for that DSD. In that respect this change differs from the one for pending syncs.

== Implementation details ==

I cleaned up some "if/elif/elif" code in cached_differences, replacing it with a dict. One of the items in the code was a bit irregular (a mono-tuple instead of an item, which in this case looks like it does the same thing) but I was too lazy to check whether that could be cleaned up as well. I left it in.

The set_derived_series_difference_jobs_feature_flag in lib/lp/registry/browser/tests/test_distroseries.py conforms to other similar functions right above it in the same module.

In lib/lp/soyuz/tests.test_distroseriesdifference.py I found a few test methods that were wrongly named so that they weren't run as tests. (Probably my fault; I've caught myself doing that a few times. I've made it a point to watch tests fail before I start making them succeed.) One needed a change to the test, but not one with any semantic implications. Otherwise, no surprises.

Something I kept running into is that DistroSeriesDifferenceJobs are not aware of multi-parent derived series yet. That change is going to affect this code in several places.

== Tests ==

{{{
./bin/test -vvc lp.registry.browser.tests.test_distroseries
./bin/test -vvc lp.soyuz.tests.test_distroseriesdifferencejob
}}}

== Demo and Q/A ==

Look at a DSDs page such as https://staging.launchpad.net/ubuntu/oneiric/+localpackagediffs

Requesting asynchronous package syncs for DSDs should cause them to be marked as "synchronizing..."

Updating a package and reloading +localpackagediffs should cause the corresponding DSD to be marked as "updating..."

The two messages may also combine.

= Launchpad lint =

I added no lint and I fixed some pre-existing lint, but left some in place as well:

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/templates/distroseries-localdifferences.pt
  lib/lp/soyuz/model/distroseriesdifferencejob.py
  lib/lp/registry/browser/tests/test_distroseries.py
  lib/lp/soyuz/interfaces/distributionjob.py
  lib/lp/soyuz/tests/test_distroseriesdifferencejob.py
  lib/lp/registry/browser/distroseries.py

./lib/lp/soyuz/tests/test_distroseriesdifferencejob.py
     259: E261 at least two spaces before inline comment
     283: E261 at least two spaces before inline comment
     399: E261 at least two spaces before inline comment
     417: E261 at least two spaces before inline comment
     423: E261 at least two spaces before inline comment

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-05-24 10:08:33 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-05-27 08:21:17 +0000
@@ -104,6 +104,9 @@
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.soyuz.browser.archive import PackageCopyingMixin
 from lp.soyuz.browser.packagesearch import PackageSearchViewBase
+from lp.soyuz.interfaces.distributionjob import (
+    IDistroSeriesDifferenceJobSource,
+    )
 from lp.soyuz.interfaces.packagecopyjob import IPlainPackageCopyJobSource
 from lp.soyuz.interfaces.queue import IPackageUploadSet
 from lp.soyuz.model.packagecopyjob import specify_dsd_package
@@ -863,6 +866,21 @@
         job_source = getUtility(IPlainPackageCopyJobSource)
         return job_source.getPendingJobsPerPackage(self.context)
 
+    @cachedproperty
+    def pending_dsd_updates(self):
+        """Pending `DistroSeriesDifference` update jobs.
+
+        :return: A `set` of `DistroSeriesDifference`s that have pending
+            `DistroSeriesDifferenceJob`s.
+        """
+        job_source = getUtility(IDistroSeriesDifferenceJobSource)
+        return job_source.getPendingJobsForDifferences(
+            self.context, self.cached_differences.batch)
+
+    def hasPendingDSDUpdate(self, dsd):
+        """Have there been changes that `dsd` is still being updated for?"""
+        return dsd in self.pending_dsd_updates
+
     def hasPendingSync(self, dsd):
         """Is there a package-copying job pending to resolve `dsd`?"""
         return self.pending_syncs.get(specify_dsd_package(dsd)) is not None
@@ -898,6 +916,28 @@
         return (
             not self.isNewerThanParent(dsd) and not self.hasPendingSync(dsd))
 
+    def describeJobs(self, dsd):
+        """Describe any jobs that may be pending for `dsd`.
+
+        Shows "synchronizing..." if the entry is being synchronized, and
+        "updating..." if the DSD is being updated with package changes.
+
+        :param dsd: A `DistroSeriesDifference` on the page.
+        :return: An HTML text describing work that is pending or in
+            progress for `dsd`; or None.
+        """
+        has_pending_dsd_update = self.hasPendingDSDUpdate(dsd)
+        has_pending_sync = self.hasPendingSync(dsd)
+        if not has_pending_dsd_update and not has_pending_sync:
+            return None
+
+        description = []
+        if has_pending_dsd_update:
+            description.append("updating")
+        if has_pending_sync:
+            description.append("synchronizing")
+        return " and ".join(description) + "…"
+
     @property
     def specified_name_filter(self):
         """If specified, return the name filter from the GET form data."""
@@ -924,23 +964,18 @@
     @cachedproperty
     def cached_differences(self):
         """Return a batch navigator of filtered results."""
-        if self.specified_package_type == NON_IGNORED:
-            status = (
-                DistroSeriesDifferenceStatus.NEEDS_ATTENTION,)
-            child_version_higher = False
-        elif self.specified_package_type == IGNORED:
-            status = (
-                DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
-            child_version_higher = False
-        elif self.specified_package_type == HIGHER_VERSION_THAN_PARENT:
-            status = (
-                DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
-            child_version_higher = True
-        elif self.specified_package_type == RESOLVED:
-            status = DistroSeriesDifferenceStatus.RESOLVED
-            child_version_higher = False
-        else:
-            raise AssertionError('specified_package_type unknown')
+        package_type_dsd_status = {
+            NON_IGNORED: (
+                DistroSeriesDifferenceStatus.NEEDS_ATTENTION,),
+            IGNORED: DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
+            HIGHER_VERSION_THAN_PARENT: (
+                DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT),
+            RESOLVED: DistroSeriesDifferenceStatus.RESOLVED,
+        }
+
+        status = package_type_dsd_status[self.specified_package_type]
+        child_version_higher = (
+            self.specified_package_type == HIGHER_VERSION_THAN_PARENT)
 
         differences = getUtility(
             IDistroSeriesDifferenceSource).getForDistroSeries(
 
=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-05-24 14:22:31 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-05-27 08:21:17 +0000
@@ -66,6 +66,7 @@
     )
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.distributionjob import (
+    IDistroSeriesDifferenceJobSource,
     IInitialiseDistroSeriesJobSource,
     )
 from lp.soyuz.interfaces.packagecopyjob import IPlainPackageCopyJobSource
@@ -73,6 +74,7 @@
     ISourcePackageFormatSelectionSet,
     )
 from lp.soyuz.model.archivepermission import ArchivePermission
+from lp.soyuz.model import distroseriesdifferencejob
 from lp.soyuz.model.packagecopyjob import specify_dsd_package
 from lp.testing import (
     anonymous_logged_in,
@@ -85,6 +87,7 @@
     TestCaseWithFactory,
     with_celebrity_logged_in,
     )
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.matchers import HasQueryCount
 from lp.testing.views import create_initialized_view
 
@@ -102,6 +105,12 @@
         }))
 
 
+def set_derived_series_difference_jobs_feature_flag(test_case):
+    test_case.useFixture(FeatureFixture({
+        distroseriesdifferencejob.FEATURE_FLAG_ENABLE_MODULE: u'on',
+        }))
+
+
 class TestDistroSeriesView(TestCaseWithFactory):
     """Test the distroseries +index view."""
 
@@ -1095,6 +1104,14 @@
 
     layer = LaunchpadFunctionalLayer
 
+    def makeDSDJob(self, dsd):
+        """Create a `DistroSeriesDifferenceJob` to update `dsd`."""
+        job_source = getUtility(IDistroSeriesDifferenceJobSource)
+        jobs = job_source.createForPackagePublication(
+            dsd.derived_series, dsd.source_package_name,
+            PackagePublishingPocket.RELEASE)
+        return jobs[0]
+
     def test_higher_radio_mentions_parent(self):
         # The user is shown an option to display only the blacklisted
         # package with a higer version than in the parent.
@@ -1345,6 +1362,20 @@
 
             self.assertFalse(view.canPerformSync())
 
+    def test_hasPendingDSDUpdate_returns_False_if_no_pending_update(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        self.assertFalse(view.hasPendingDSDUpdate(dsd))
+
+    def test_hasPendingDSDUpdate_returns_True_if_pending_update(self):
+        set_derived_series_difference_jobs_feature_flag(self)
+        dsd = self.factory.makeDistroSeriesDifference()
+        self.makeDSDJob(dsd)
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        self.assertTrue(view.hasPendingDSDUpdate(dsd))
+
     def test_hasPendingSync_returns_False_if_no_pending_sync(self):
         dsd = self.factory.makeDistroSeriesDifference()
         view = create_initialized_view(
@@ -1423,6 +1454,44 @@
             dsd.derived_series, '+localpackagediffs')
         self.assertTrue(view.canRequestSync(dsd))
 
+    def test_canRequestSync_ignores_DSDJobs(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        view.hasPendingDSDUpdate = FakeMethod(result=True)
+        self.assertTrue(view.canRequestSync(dsd))
+
+    def test_describeJobs_returns_None_if_no_jobs(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        self.assertIs(None, view.describeJobs(dsd))
+
+    def test_describeJobs_reports_pending_update(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        view.hasPendingDSDUpdate = FakeMethod(result=True)
+        view.hasPendingSync = FakeMethod(result=False)
+        self.assertEqual("updating…", view.describeJobs(dsd))
+
+    def test_describeJobs_reports_pending_sync(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        view.hasPendingDSDUpdate = FakeMethod(result=False)
+        view.hasPendingSync = FakeMethod(result=True)
+        self.assertEqual("synchronizing…", view.describeJobs(dsd))
+
+    def test_describeJobs_reports_pending_sync_and_update(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        view.hasPendingDSDUpdate = FakeMethod(result=True)
+        view.hasPendingSync = FakeMethod(result=True)
+        self.assertEqual(
+            "updating and synchronizing…", view.describeJobs(dsd))
+
     def _syncAndGetView(self, derived_series, person, sync_differences,
                         difference_type=None, view_name='+localpackagediffs'):
         # A helper to get the POST'ed sync view.
 
=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2011-05-24 11:32:34 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2011-05-27 08:21:17 +0000
@@ -94,11 +94,12 @@
                    class="js-action toggle-extra"
                    tal:content="src_name">Foo</a>
 
-                <span
-                    class="lowlight"
-                    tal:condition="python: view.hasPendingSync(difference)">
-                    &nbsp;&nbsp;synchronizing&hellip;
-                </span>
+                <tal:activity
+                    define="activity python:view.describeJobs(difference)"
+                    condition="activity">
+                  &nbsp;&nbsp;
+                  <span class="lowlight" tal:content="structure activity"></span>
+                </tal:activity>
               </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/distributionjob.py'
--- lib/lp/soyuz/interfaces/distributionjob.py	2011-05-13 10:00:02 +0000
+++ lib/lp/soyuz/interfaces/distributionjob.py	2011-05-27 08:21:17 +0000
@@ -104,6 +104,17 @@
         :param pocket: The `PackagePublishingPocket` for the publication.
         """
 
+    def getPendingJobsForDifferences(derived_series, distroseriesdifferences):
+        """Find `DistroSeriesDifferenceJob`s for `DistroSeriesDifference`s.
+
+        :param derived_series: The derived `DistroSeries` that the
+            differences (and jobs) must be for.
+        :param distroseriesdifferences:
+            An iterable of `DistroSeriesDifference`s.
+        :return: A dict mapping each of `distroseriesdifferences` that has
+            pending jobs to a list of its jobs.
+        """
+
 
 class IDistroSeriesDifferenceJob(IRunnableJob):
         """A Job that performs actions related to DSDs."""
 
=== modified file 'lib/lp/soyuz/model/distroseriesdifferencejob.py'
--- lib/lp/soyuz/model/distroseriesdifferencejob.py	2011-05-20 14:36:26 +0000
+++ lib/lp/soyuz/model/distroseriesdifferencejob.py	2011-05-27 08:21:17 +0000
@@ -14,7 +14,10 @@
     implements,
     )
 
-from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from canonical.launchpad.interfaces.lpstorm import (
+    IMasterStore,
+    IStore,
+    )
 from lp.registry.interfaces.distroseriesdifference import (
     IDistroSeriesDifferenceSource,
     )
@@ -139,6 +142,29 @@
                 jobs.append(create_job(relative, sourcepackagename))
         return jobs
 
+    @classmethod
+    def getPendingJobsForDifferences(cls, derived_series,
+                                     distroseriesdifferences):
+        """See `IDistroSeriesDifferenceJobSource`."""
+        jobs = IStore(DistributionJob).find(
+            DistributionJob,
+            DistributionJob.job_type == cls.class_job_type,
+            Job.id == DistributionJob.job_id,
+            Job._status.is_in(Job.PENDING_STATUSES),
+            DistributionJob.distroseries == derived_series)
+
+        # XXX JeroenVermeulen 2011-05-26 bug=758906: Check for parent
+        # series once it becomes available.
+        keyed_dsds = dict(
+            (dsd.source_package_name.id, dsd)
+            for dsd in distroseriesdifferences)
+        jobs_by_dsd = {}
+        for job in jobs:
+            dsd = keyed_dsds.get(job.metadata["sourcepackagename"])
+            if dsd is not None:
+                jobs_by_dsd.setdefault(dsd, []).append(cls(job))
+        return jobs_by_dsd
+
     @property
     def sourcepackagename(self):
         return SourcePackageName.get(self.metadata['sourcepackagename'])
 
=== modified file 'lib/lp/soyuz/tests/test_distroseriesdifferencejob.py'
--- lib/lp/soyuz/tests/test_distroseriesdifferencejob.py	2011-05-23 10:54:01 +0000
+++ lib/lp/soyuz/tests/test_distroseriesdifferencejob.py	2011-05-27 08:21:17 +0000
@@ -9,6 +9,7 @@
 from psycopg2 import ProgrammingError
 from zope.component import getUtility
 from zope.interface.verify import verifyObject
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.scripts.tests import run_script
@@ -26,8 +27,10 @@
 from lp.services.job.interfaces.job import JobStatus
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.interfaces.distributionjob import (
+    DistributionJobType,
     IDistroSeriesDifferenceJobSource,
     )
+from lp.soyuz.model.distributionjob import DistributionJob
 from lp.soyuz.model.distroseriesdifferencejob import (
     create_job,
     DistroSeriesDifferenceJob,
@@ -108,15 +111,17 @@
         dsdjob = create_job(distroseries, package)
         self.assertEqual(JobStatus.WAITING, dsdjob.job.status)
 
-    def find_waiting_jobs_finds_waiting_jobs(self):
+    def test_find_waiting_jobs_finds_waiting_jobs(self):
         sourcepackage = self.factory.makeSourcePackage()
         distroseries, sourcepackagename = (
             sourcepackage.distroseries, sourcepackage.distroseries)
         job = create_job(distroseries, sourcepackagename)
-        self.assertContentEqual(
-            [job], find_waiting_jobs(distroseries, sourcepackagename))
+        found_jobs = find_waiting_jobs(distroseries, sourcepackagename)
+        self.assertEqual([job], [
+            DistroSeriesDifferenceJob(found_job)
+            for found_job in found_jobs])
 
-    def find_waiting_jobs_ignores_other_series(self):
+    def test_find_waiting_jobs_ignores_other_series(self):
         sourcepackage = self.factory.makeSourcePackage()
         distroseries, sourcepackagename = (
             sourcepackage.distroseries, sourcepackage.distroseries)
@@ -125,7 +130,7 @@
         self.assertContentEqual(
             [], find_waiting_jobs(other_series, sourcepackagename))
 
-    def find_waiting_jobs_ignores_other_packages(self):
+    def test_find_waiting_jobs_ignores_other_packages(self):
         sourcepackage = self.factory.makeSourcePackage()
         distroseries, sourcepackagename = (
             sourcepackage.distroseries, sourcepackage.distroseries)
@@ -134,7 +139,7 @@
         self.assertContentEqual(
             [], find_waiting_jobs(distroseries, other_spn))
 
-    def find_waiting_jobs_considers_only_waiting_jobs(self):
+    def test_find_waiting_jobs_considers_only_waiting_jobs(self):
         sourcepackage = self.factory.makeSourcePackage()
         distroseries, sourcepackagename = (
             sourcepackage.distroseries, sourcepackage.distroseries)
@@ -192,6 +197,60 @@
             distroseries, package, PackagePublishingPocket.PROPOSED)
         self.assertContentEqual([], find_waiting_jobs(distroseries, package))
 
+    def test_getPendingJobsForDifferences_finds_job(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        job = create_job(dsd.derived_series, dsd.source_package_name)
+        self.assertEqual(
+            {dsd: [job]},
+            self.getJobSource().getPendingJobsForDifferences(
+                dsd.derived_series, [dsd]))
+
+    def test_getPendingJobsForDifferences_ignores_other_package(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        create_job(dsd.derived_series, self.factory.makeSourcePackageName())
+        self.assertEqual(
+            {},
+            self.getJobSource().getPendingJobsForDifferences(
+                dsd.derived_series, [dsd]))
+
+    def test_getPendingJobsForDifferences_ignores_other_derived_series(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        create_job(self.makeDerivedDistroSeries(), dsd.source_package_name)
+        self.assertEqual(
+            {},
+            self.getJobSource().getPendingJobsForDifferences(
+                dsd.derived_series, [dsd]))
+
+    def test_getPendingJobsForDifferences_ignores_other_parent_series(self):
+        # XXX JeroenVermeulen 2011-05-26 bug=758906: Can't test this
+        # until we can specify the right parent series when creating a
+        # job.
+        return
+
+    def test_getPendingJobsForDifferences_ignores_non_pending_jobs(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        job = create_job(dsd.derived_series, dsd.source_package_name)
+        removeSecurityProxy(job).job._status = JobStatus.COMPLETED
+        self.assertEqual(
+            {},
+            self.getJobSource().getPendingJobsForDifferences(
+                dsd.derived_series, [dsd]))
+
+    def test_getPendingJobsForDifferences_ignores_other_job_types(self):
+        # XXX JeroenVermeulen 2011-05-26 bug=758906: Once parent_series
+        # is incorporated into the job type, set it to dsd.parent_series
+        # on the fake job, or this test will become silently meaningless.
+        dsd = self.factory.makeDistroSeriesDifference()
+        DistributionJob(
+            distribution=dsd.derived_series.distribution,
+            distroseries=dsd.derived_series,
+            job_type=DistributionJobType.INITIALISE_SERIES,
+            metadata={"sourcepackagename": dsd.source_package_name.id})
+        self.assertEqual(
+            {},
+            self.getJobSource().getPendingJobsForDifferences(
+                dsd.derived_series, [dsd]))
+
     def test_cronscript(self):
         derived_series = self.makeDerivedDistroSeries()
         package = self.factory.makeSourcePackageName()
@@ -285,7 +344,7 @@
         parent_packageset = self.factory.makePackageset(
             distroseries=distro_series_parent.parent_series,
             packages=packages)
-        derived_packageset = self.factory.makePackageset(
+        self.factory.makePackageset(
             distroseries=distro_series_parent.derived_series,
             packages=packages, name=parent_packageset.name,
             owner=parent_packageset.owner, related_set=parent_packageset)
@@ -414,7 +473,8 @@
         jobs = find_waiting_jobs(derived_series, source_package_name)
         self.runJob(jobs[0])
         self.assertEqual(
-            DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT, ds_diff[0].status)
+            DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
+            ds_diff[0].status)
         self.assertEqual('1.0-1', ds_diff[0].base_version)
 
     def test_child_is_synced(self):