launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04173
[Merge] lp:~allenap/launchpad/show-init-series-failure-bug-795387 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/show-init-series-failure-bug-795387 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #795387 in Launchpad itself: "UI doesn't show if a series initialization request failed, it just vanishes"
https://bugs.launchpad.net/launchpad/+bug/795387
For more details, see:
https://code.launchpad.net/~allenap/launchpad/show-init-series-failure-bug-795387/+merge/67068
This branch shows a message on DistroSeries:+index (by way of
DistroSeries:+portlet-derivation) when series initialization has
failed.
If the user has the ability to try again (i.e. they can use
DistroSeries:+initseries), then they are prompted to do so.
If the user does not have the ability to try again they are prompted
to contact the distribution owner. In reality the distribution owner
*or* the distribution driver (when the distro isn't Ubuntu) can use
+initseries, but I'm copping out on that logic.
The changes:
- InitializeDistroSeriesJob.getPendingJobsForDistroseries() is renamed
to simply .get(). Only one InitializeDistroSeriesJob can exist at a
time for a distroseries, so the plural in the old method name was
misleading. The job also no longer needs to be pending (see
Job.PENDING_STATUSES); it can have any status.
It's part of IInitializeDistroSeriesJobSource so the short name
makes more sense in that context.
- New property Job.is_pending. I originally intended to use this in
TAL, but ended up not using it. It is used in only one place -
DistroSeries.isInitializing() - because
InitializeDistroSeriesJob.get() doesn't only return pending jobs.
- New method DistroSeries.getInitializationJob() simply calls through
to IInitializeDistroSeriesJobSource.get().
- The DistroSeries:+index template changes to show the derivation
portlet whenever that is or has been initialization activity on the
series, instead of just pending or successfully completed activity.
- The derivation portlet shows the message described at the top.
--
https://code.launchpad.net/~allenap/launchpad/show-init-series-failure-bug-795387/+merge/67068
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/show-init-series-failure-bug-795387 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py 2011-06-28 17:57:34 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py 2011-07-06 18:02:13 +0000
@@ -28,9 +28,13 @@
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
+from canonical.config import config
from canonical.database.constants import UTC_NOW
from canonical.database.sqlbase import flush_database_caches
-from canonical.launchpad.testing.pages import find_tag_by_id
+from canonical.launchpad.testing.pages import (
+ extract_text,
+ find_tag_by_id,
+ )
from canonical.launchpad.webapp.authorization import check_permission
from canonical.launchpad.webapp.batching import BatchNavigator
from canonical.launchpad.webapp.interaction import get_current_principal
@@ -50,7 +54,6 @@
RESOLVED,
seriesToVocab,
)
-from canonical.config import config
from lp.registry.enum import (
DistroSeriesDifferenceStatus,
DistroSeriesDifferenceType,
@@ -93,6 +96,7 @@
)
from lp.testing.fakemethod import FakeMethod
from lp.testing.matchers import (
+ DocTestMatches,
EqualsIgnoringWhitespace,
HasQueryCount,
)
@@ -334,6 +338,28 @@
self.assertTrue(derived_series.isInitializing())
self.assertThat(html_content, portlet_display)
+ def test_differences_portlet_initialization_failed(self):
+ # The difference portlet displays a failure message if initialization
+ # for the series has failed.
+ set_derived_series_ui_feature_flag(self)
+ derived_series = self.factory.makeDistroSeries()
+ parent_series = self.factory.makeDistroSeries()
+ self.simple_user = self.factory.makePerson()
+ job_source = getUtility(IInitializeDistroSeriesJobSource)
+ job = job_source.create(derived_series, [parent_series.id])
+ job.start()
+ job.fail()
+ portlet_display = soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ 'Derived series', 'h2',
+ text='Series initialization has failed'),
+ )
+ with person_logged_in(self.simple_user):
+ view = create_initialized_view(
+ derived_series, '+index', principal=self.simple_user)
+ html_content = view()
+ self.assertThat(html_content, portlet_display)
+
def assertInitSeriesLinkPresent(self, series, person):
self._assertInitSeriesLink(series, person, True)
@@ -422,6 +448,62 @@
self.assertInitSeriesLinkNotPresent(series, 'admin')
+class TestDistroSeriesDerivationPortlet(TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ @property
+ def job_source(self):
+ return getUtility(IInitializeDistroSeriesJobSource)
+
+ def test_initialization_failed_can_retry(self):
+ # When initialization has failed and the user has the ability to retry
+ # it prompts the user to try again.
+ set_derived_series_ui_feature_flag(self)
+ series = self.factory.makeDistroSeries()
+ parent = self.factory.makeDistroSeries()
+ job = self.job_source.create(series, [parent.id])
+ job.start()
+ job.fail()
+ with person_logged_in(series.owner):
+ view = create_initialized_view(series, '+portlet-derivation')
+ html_content = view()
+ self.assertThat(
+ extract_text(html_content), DocTestMatches(
+ "Series initialization has failed\n"
+ "You can attempt initialization again."))
+
+ def test_initialization_failed_cannot_retry(self):
+ # When initialization has failed and the user does not have the
+ # ability to retry it suggests contacting someone who can.
+ set_derived_series_ui_feature_flag(self)
+ series = self.factory.makeDistroSeries()
+ parent = self.factory.makeDistroSeries()
+ job = self.job_source.create(series, [parent.id])
+ job.start()
+ job.fail()
+ with anonymous_logged_in():
+ view = create_initialized_view(series, '+portlet-derivation')
+ html_content = view()
+ self.assertThat(
+ extract_text(html_content), DocTestMatches(
+ "Series initialization has failed\n"
+ "You cannot attempt initialization again, "
+ "but Person-... may be able to help."))
+ # When the owner is a team the message differs slightly from when the
+ # owner is an individual.
+ with person_logged_in(series.distribution.owner):
+ series.distribution.owner = self.factory.makeTeam()
+ with anonymous_logged_in():
+ view = create_initialized_view(series, '+portlet-derivation')
+ html_content = view()
+ self.assertThat(
+ extract_text(html_content), DocTestMatches(
+ "Series initialization has failed\n"
+ "You cannot attempt initialization again, but "
+ "a member of Team ... may be able to help."))
+
+
class TestMilestoneBatchNavigatorAttribute(TestCaseWithFactory):
"""Test the series.milestone_batch_navigator attribute."""
=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py 2011-06-20 16:04:15 +0000
+++ lib/lp/registry/interfaces/distroseries.py 2011-07-06 18:02:13 +0000
@@ -903,6 +903,12 @@
def isInitialized():
"""Has this series been initialized?"""
+ def getInitializationJob():
+ """Get the last `IInitializeDistroSeriesJob` for this series.
+
+ :return: `None` if no job is found or an `IInitializeDistroSeriesJob`.
+ """
+
class IDistroSeriesEditRestricted(Interface):
"""IDistroSeries properties which require launchpad.Edit."""
=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py 2011-06-17 14:23:52 +0000
+++ lib/lp/registry/model/distroseries.py 2011-07-06 18:02:13 +0000
@@ -1886,15 +1886,18 @@
def isInitializing(self):
"""See `IDistroSeries`."""
- job_source = getUtility(IInitializeDistroSeriesJobSource)
- pending_jobs = job_source.getPendingJobsForDistroseries(self)
- return not pending_jobs.is_empty()
+ job = self.getInitializationJob()
+ return job is not None and job.is_pending
def isInitialized(self):
"""See `IDistroSeries`."""
published = self.main_archive.getPublishedSources(distroseries=self)
return not published.is_empty()
+ def getInitializationJob(self):
+ """See `IDistroSeries`."""
+ return getUtility(IInitializeDistroSeriesJobSource).get(self)
+
class DistroSeriesSet:
implements(IDistroSeriesSet)
=== modified file 'lib/lp/registry/templates/distroseries-index.pt'
--- lib/lp/registry/templates/distroseries-index.pt 2011-06-16 13:50:58 +0000
+++ lib/lp/registry/templates/distroseries-index.pt 2011-07-06 18:02:13 +0000
@@ -67,7 +67,7 @@
tal:condition="request/features/soyuz.derived_series_ui.enabled">
<div class="yui-u"
tal:condition="python: context.isDerivedSeries() or
- context.isInitializing()">
+ context.getInitializationJob() is not None">
<div tal:replace="structure context/@@+portlet-derivation" />
</div>
</tal:derivation>
=== modified file 'lib/lp/registry/templates/distroseries-portlet-derivation.pt'
--- lib/lp/registry/templates/distroseries-portlet-derivation.pt 2011-06-14 19:49:18 +0000
+++ lib/lp/registry/templates/distroseries-portlet-derivation.pt 2011-07-06 18:02:13 +0000
@@ -65,4 +65,23 @@
<h2>Series initialization in progress</h2>
This series is initializing.
</tal:is_initializing>
+ <tal:failed_initialization
+ define="job context/getInitializationJob"
+ condition="job/status/enumvalue:FAILED|nothing">
+ <h2>Series initialization has failed</h2>
+ <tal:retry
+ define="can_retry context/@@+initseries/required:launchpad.Edit ">
+ <tal:can_retry condition="can_retry">
+ You can <a tal:attributes="href context/fmt:url/+initseries">
+ attempt initialization again</a>.
+ </tal:can_retry>
+ <tal:cannot_retry
+ define="owner context/owner"
+ condition="not:can_retry">
+ You cannot attempt initialization again, but <tal:team
+ condition="owner/is_team"> a member of </tal:team> <tal:owner
+ replace="structure owner/fmt:link" /> may be able to help.
+ </tal:cannot_retry>
+ </tal:retry>
+ </tal:failed_initialization>
</div>
=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py 2011-06-14 19:49:18 +0000
+++ lib/lp/registry/tests/test_distroseries.py 2011-07-06 18:02:13 +0000
@@ -256,6 +256,16 @@
distroseries=distroseries, archive=distroseries.main_archive)
self.assertTrue(distroseries.isInitialized())
+ def test_getInitializationJob(self):
+ # getInitializationJob() returns the most recent
+ # `IInitializeDistroSeriesJob` for the given series.
+ distroseries = self.factory.makeDistroSeries()
+ parent_distroseries = self.factory.makeDistroSeries()
+ self.assertIs(None, distroseries.getInitializationJob())
+ job_source = getUtility(IInitializeDistroSeriesJobSource)
+ job = job_source.create(distroseries, [parent_distroseries.id])
+ self.assertEqual(job, distroseries.getInitializationJob())
+
class TestDistroSeriesPackaging(TestCaseWithFactory):
=== modified file 'lib/lp/services/job/interfaces/job.py'
--- lib/lp/services/job/interfaces/job.py 2011-07-05 09:01:53 +0000
+++ lib/lp/services/job/interfaces/job.py 2011-07-06 18:02:13 +0000
@@ -27,6 +27,7 @@
Interface,
)
from zope.schema import (
+ Bool,
Choice,
Datetime,
Int,
@@ -108,6 +109,10 @@
max_retries = Int(title=_(
'The number of retries permitted before this job permanently fails.'))
+ is_pending = Bool(
+ title=_("Whether or not this job's status is such that it "
+ "could eventually complete."))
+
def acquireLease(duration=300):
"""Acquire the lease for this Job, or raise LeaseHeld."""
=== modified file 'lib/lp/services/job/model/job.py'
--- lib/lp/services/job/model/job.py 2011-07-05 09:01:53 +0000
+++ lib/lp/services/job/model/job.py 2011-07-06 18:02:13 +0000
@@ -102,6 +102,11 @@
status = property(lambda x: x._status)
+ @property
+ def is_pending(self):
+ """See `IJob`."""
+ return self.status in self.PENDING_STATUSES
+
@classmethod
def createMultiple(self, store, num_jobs):
"""Create multiple `Job`s at once.
=== modified file 'lib/lp/services/job/tests/test_job.py'
--- lib/lp/services/job/tests/test_job.py 2011-07-05 09:01:53 +0000
+++ lib/lp/services/job/tests/test_job.py 2011-07-06 18:02:13 +0000
@@ -218,6 +218,13 @@
job = Job(_status=JobStatus.FAILED)
self.assertRaises(InvalidTransition, job.resume)
+ def test_is_pending(self):
+ """is_pending is True when the job can possibly complete."""
+ for status in JobStatus.items:
+ job = Job(_status=status)
+ self.assertEqual(
+ status in Job.PENDING_STATUSES, job.is_pending)
+
class TestReadiness(TestCase):
"""Test the implementation of readiness."""
=== modified file 'lib/lp/soyuz/interfaces/distributionjob.py'
--- lib/lp/soyuz/interfaces/distributionjob.py 2011-06-15 19:18:57 +0000
+++ lib/lp/soyuz/interfaces/distributionjob.py 2011-07-06 18:02:13 +0000
@@ -107,8 +107,10 @@
overlay_pockets, overlay_components):
"""Create a new initialization job for a distroseries."""
- def getPendingJobsForDistroseries(distroseries):
- """Retrieve pending initialization jobs for a distroseries.
+ def get(distroseries):
+ """Retrieve the initialization job for a distroseries, if any.
+
+ :return: `None` or an `IInitializeDistroSeriesJob`.
"""
=== modified file 'lib/lp/soyuz/model/initializedistroseriesjob.py'
--- lib/lp/soyuz/model/initializedistroseriesjob.py 2011-06-15 19:18:57 +0000
+++ lib/lp/soyuz/model/initializedistroseriesjob.py 2011-07-06 18:02:13 +0000
@@ -76,14 +76,13 @@
return cls(distribution_job)
@classmethod
- def getPendingJobsForDistroseries(cls, distroseries):
+ def get(cls, distroseries):
"""See `IInitializeDistroSeriesJob`."""
- return IStore(DistributionJob).find(
- DistributionJob,
- DistributionJob.job_id == Job.id,
+ distribution_job = IStore(DistributionJob).find(
+ DistributionJob, DistributionJob.job_id == Job.id,
DistributionJob.job_type == cls.class_job_type,
- DistributionJob.distroseries_id == distroseries.id,
- Job._status.is_in(Job.PENDING_STATUSES))
+ DistributionJob.distroseries_id == distroseries.id).one()
+ return None if distribution_job is None else cls(distribution_job)
@property
def parents(self):
=== modified file 'lib/lp/soyuz/tests/test_initializedistroseriesjob.py'
--- lib/lp/soyuz/tests/test_initializedistroseriesjob.py 2011-06-16 20:10:10 +0000
+++ lib/lp/soyuz/tests/test_initializedistroseriesjob.py 2011-07-06 18:02:13 +0000
@@ -142,16 +142,17 @@
naked_job = removeSecurityProxy(job)
self.assertEqual((parent.id, ), naked_job.parents)
- def test_getPendingJobsForDistroseries(self):
- # Pending initialization jobs can be retrieved per distroseries.
+ def test_get(self):
+ # InitializeDistroSeriesJob.get() returns the initialization job for
+ # the given distroseries. There should only ever be one.
parent = self.factory.makeDistroSeries()
distroseries = self.factory.makeDistroSeries()
another_distroseries = self.factory.makeDistroSeries()
+ self.assertIs(None, self.job_source.get(distroseries))
self.job_source.create(distroseries, [parent.id])
self.job_source.create(another_distroseries, [parent.id])
- initialize_utility = getUtility(IInitializeDistroSeriesJobSource)
- [job] = list(initialize_utility.getPendingJobsForDistroseries(
- distroseries))
+ job = self.job_source.get(distroseries)
+ self.assertIsInstance(job, InitializeDistroSeriesJob)
self.assertEqual(job.distroseries, distroseries)
