launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03756
[Merge] lp:~jtv/launchpad/db-bug-788526 into lp:launchpad/db-devel
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/db-bug-788526 into lp:launchpad/db-devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #788526 in Launchpad itself: "Display in-progress DSDJobs"
https://bugs.launchpad.net/launchpad/+bug/788526
For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-788526/+merge/62623
= 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
--
https://code.launchpad.net/~jtv/launchpad/db-bug-788526/+merge/62623
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-788526 into lp:launchpad/db-devel.
=== 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)">
- synchronizing…
- </span>
+ <tal:activity
+ define="activity python:view.describeJobs(difference)"
+ condition="activity">
+
+ <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):
