launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04045
[Merge] lp:~jtv/launchpad/bug-800652 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-800652 into lp:launchpad with lp:~jtv/launchpad/pre-800634-800652 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #800634 in Launchpad itself: "+queue page doesn't show the overrides for syncs"
https://bugs.launchpad.net/launchpad/+bug/800634
Bug #800652 in Launchpad itself: "+queue page broken for accepting syncs"
https://bugs.launchpad.net/launchpad/+bug/800652
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-800652/+merge/65675
= Summary =
This makes the +queue page show overrides for sync uploads. It also simplifies the handling of the overrides in various ways.
== Implementation details ==
You'll see a better separation of display logic and database logic for components and sections: the model knows that components and sections are only relevant for source and sync uploads, and the display logic merely cares about representing Nones as empty strings.
Since ComplegePackageUpload.package_sets is now the empty list when it does not apply, there's no need to special-case it now.
I split up both of the functions that accept an upload and add a source override, each into one function for sync uploads, one for non-sync uploads, plus a simple abstract master method to choose between the two. I hope that makes it a little easier to follow the gist of things.
In the case of the source overrides, I hoisted the check for "is the new component an allowed one?" up into the master method. If that check fails, the error message mentioned both the old component and the new component without saying which was the problem. I hopefully made it a little clearer by stating just the new component in the case where that is the forbidden one.
The check for the old component went into the source-upload method, but I added a similar one for the sync-upload case. It works slightly differently because the package copy job knows its component name but not its Component object.
In order to fix irrelevant test breakage, I fixed the factory method for creating sync uploads to add the override that the upload would normally have. This also made it necessary for one test to change the way it defined the metadata it expected from its job: the newly-initialized override data was hiding the data that the test was trying to add!
== Tests ==
I ran:
{{{
./bin/test -vvc lp.soyuz -t queue -t packageupload -t packagecopyjob
}}}
== Demo and Q/A ==
Create a PCJ upload. View it on the +queue page, without folding open its details (since there won't be any, actually). Its component and section will be visible.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/soyuz/browser/tests/test_queue.py
lib/lp/soyuz/model/queue.py
lib/lp/soyuz/stories/soyuz/xx-queue-pages-motu.txt
lib/lp/soyuz/interfaces/queue.py
lib/lp/soyuz/browser/queue.py
lib/lp/soyuz/model/packagecopyjob.py
lib/lp/soyuz/configure.zcml
lib/lp/soyuz/interfaces/packagecopyjob.py
lib/lp/testing/factory.py
lib/lp/soyuz/tests/test_packagecopyjob.py
lib/lp/soyuz/tests/test_packageupload.py
lib/lp/soyuz/adapters/overrides.py
lib/lp/soyuz/doc/distroseriesqueue.txt
./lib/lp/soyuz/stories/soyuz/xx-queue-pages-motu.txt
1: narrative uses a moin header.
20: narrative uses a moin header.
41: want exceeds 78 characters.
42: want exceeds 78 characters.
43: want exceeds 78 characters.
44: want exceeds 78 characters.
45: want exceeds 78 characters.
46: want exceeds 78 characters.
47: want exceeds 78 characters.
73: source exceeds 78 characters.
74: source exceeds 78 characters.
--
https://code.launchpad.net/~jtv/launchpad/bug-800652/+merge/65675
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-800652 into lp:launchpad.
=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py 2011-06-22 07:13:29 +0000
+++ lib/lp/soyuz/browser/queue.py 2011-06-23 14:18:09 +0000
@@ -46,7 +46,6 @@
QueueInconsistentStateError,
)
from lp.soyuz.interfaces.section import ISectionSet
-from lp.soyuz.model.queue import PackageUploadSource
QUEUE_SIZE = 30
@@ -199,6 +198,7 @@
in the +queue template.
"""
# Avoid circular imports.
+ from lp.soyuz.model.queue import PackageUploadSource
from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
uploads = list(self.batchnav.currentBatch())
@@ -396,14 +396,13 @@
(queue_item.displayname, info))
else:
if source_overridden:
- success.append("OK: %(name)s(%(component)s/%(section)s)" %
- feedback_interpolations)
+ desc = "%(name)s(%(component)s/%(section)s)"
elif binary_overridden:
- success.append(
- "OK: %(name)s(%(component)s/%(section)s/%(priority)s)"
- % feedback_interpolations)
+ desc = "%(name)s(%(component)s/%(section)s/%(priority)s)"
else:
- success.append('OK: %s' % queue_item.displayname)
+ desc = "%(name)s"
+ success.append(
+ "OK: " + desc % feedback_interpolations)
for message in success:
self.request.response.addInfoNotification(message)
@@ -480,11 +479,8 @@
# Create a list of source files if this is a source upload.
self.source_files = source_upload_files.get(self.id, None)
- # Pre-fetch the sourcepackagerelease if it exists.
if self.contains_source:
self.sourcepackagerelease = self.sources[0].sourcepackagerelease
- else:
- self.sourcepackagerelease = None
if self.contains_source:
self.package_sets = package_sets.get(
@@ -513,27 +509,26 @@
@property
def display_package_sets(self):
"""Package sets, if any, for display on the +queue page."""
- if self.contains_source:
- return ' '.join(sorted(
- packageset.name for packageset in self.package_sets))
- else:
- return ""
+ return ' '.join(sorted(
+ packageset.name for packageset in self.package_sets))
@property
def display_component(self):
"""Component name, if any, for display on the +queue page."""
- if self.contains_source:
- return self.component_name.lower()
+ component_name = self.component_name
+ if component_name is None:
+ return ""
else:
- return ""
+ return component_name.lower()
@property
def display_section(self):
"""Section name, if any, for display on the +queue page."""
- if self.contains_source:
- return self.section_name.lower()
+ section_name = self.section_name
+ if section_name is None:
+ return ""
else:
- return ""
+ return section_name.lower()
@property
def display_priority(self):
=== modified file 'lib/lp/soyuz/browser/tests/test_queue.py'
--- lib/lp/soyuz/browser/tests/test_queue.py 2011-06-21 05:30:19 +0000
+++ lib/lp/soyuz/browser/tests/test_queue.py 2011-06-23 14:18:09 +0000
@@ -272,7 +272,7 @@
package_sets.values()[0][0].name,
complete_upload.display_package_sets)
- def test_display_package_sets_returns_empty_for_non_source_upload(self):
+ def test_display_package_sets_returns_empty_for_other_upload(self):
upload = self.factory.makeBuildPackageUpload()
complete_upload = self.makeCompletePackageUpload(
upload, package_sets=self.mapPackageSets(upload))
@@ -287,34 +287,51 @@
self.assertEqual("aaa bbb ccc", complete_upload.display_package_sets)
def test_display_component_returns_source_upload_component_name(self):
- complete_upload = self.makeCompletePackageUpload()
- self.assertEqual(
- complete_upload.sourcepackagerelease.component.name.lower(),
- complete_upload.display_component)
-
- def test_display_component_returns_empty_for_non_source_upload(self):
+ upload = self.factory.makeSourcePackageUpload()
+ complete_upload = self.makeCompletePackageUpload(upload)
+ self.assertEqual(
+ upload.sourcepackagerelease.component.name.lower(),
+ complete_upload.display_component)
+
+ def test_display_component_returns_copy_job_upload_component_name(self):
+ copy_job_upload = self.factory.makeCopyJobPackageUpload()
+ complete_upload = self.makeCompletePackageUpload(copy_job_upload)
+ self.assertEqual(
+ copy_job_upload.component_name.lower(),
+ complete_upload.display_component)
+
+ def test_display_component_returns_empty_for_other_upload(self):
complete_upload = self.makeCompletePackageUpload(
self.factory.makeBuildPackageUpload())
self.assertEqual('', complete_upload.display_component)
def test_display_section_returns_source_upload_section_name(self):
- complete_upload = self.makeCompletePackageUpload()
- self.assertEqual(
- complete_upload.sourcepackagerelease.section.name.lower(),
- complete_upload.display_section)
-
- def test_display_section_returns_empty_for_non_source_upload(self):
+ upload = self.factory.makeSourcePackageUpload()
+ complete_upload = self.makeCompletePackageUpload(upload)
+ self.assertEqual(
+ upload.sourcepackagerelease.section.name.lower(),
+ complete_upload.display_section)
+
+ def test_display_section_returns_copy_job_upload_section_name(self):
+ copy_job_upload = self.factory.makeCopyJobPackageUpload()
+ complete_upload = self.makeCompletePackageUpload(copy_job_upload)
+ self.assertEqual(
+ copy_job_upload.section_name.lower(),
+ complete_upload.display_section)
+
+ def test_display_section_returns_empty_for_other_upload(self):
complete_upload = self.makeCompletePackageUpload(
self.factory.makeBuildPackageUpload())
self.assertEqual('', complete_upload.display_section)
def test_display_priority_returns_source_upload_priority(self):
- complete_upload = self.makeCompletePackageUpload()
+ upload = self.factory.makeSourcePackageUpload()
+ complete_upload = self.makeCompletePackageUpload(upload)
self.assertEqual(
- complete_upload.sourcepackagerelease.urgency.name.lower(),
+ upload.sourcepackagerelease.urgency.name.lower(),
complete_upload.display_priority)
- def test_display_priority_returns_empty_for_non_source_upload(self):
+ def test_display_priority_returns_empty_for_other_upload(self):
complete_upload = self.makeCompletePackageUpload(
self.factory.makeBuildPackageUpload())
self.assertEqual('', complete_upload.display_priority)
=== modified file 'lib/lp/soyuz/doc/distroseriesqueue.txt'
--- lib/lp/soyuz/doc/distroseriesqueue.txt 2011-06-16 09:38:04 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue.txt 2011-06-23 14:18:09 +0000
@@ -676,7 +676,7 @@
... allowed_components=(universe,))
Traceback (most recent call last):
...
- QueueInconsistentStateError: No rights to override from main to restricted
+ QueueInconsistentStateError: No rights to override to restricted
Allowing "restricted" still won't work because the original component
is "main":
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py 2011-06-23 14:18:06 +0000
+++ lib/lp/soyuz/model/queue.py 2011-06-23 14:18:09 +0000
@@ -65,8 +65,8 @@
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.services.mail.signedmessage import strip_pgp_signature
from lp.services.propertycache import cachedproperty
+from lp.soyuz.adapters.notification import notify
from lp.soyuz.adapters.overrides import SourceOverride
-from lp.soyuz.adapters.notification import notify
from lp.soyuz.enums import (
PackageUploadCustomFormat,
PackageUploadStatus,
@@ -469,25 +469,39 @@
self._closeBugs(changesfile_path, logger)
self._giveKarma()
- def acceptFromQueue(self, logger=None, dry_run=False):
- """See `IPackageUpload`."""
- assert not self.is_delayed_copy, 'Cannot process delayed copies.'
-
- if self.package_copy_job is not None:
- if self.status == PackageUploadStatus.REJECTED:
- raise QueueInconsistentStateError(
- "Can't resurrect rejected syncs")
- # Circular imports :(
- from lp.soyuz.model.packagecopyjob import PlainPackageCopyJob
- # Release the job hounds, Smithers.
- self.setAccepted()
- job = PlainPackageCopyJob.get(self.package_copy_job_id)
- job.resume()
- # The copy job will send emails as appropriate. We don't
- # need to worry about closing bugs from syncs, although we
- # should probably give karma but that needs more work to
- # fix here.
- return
+ def _acceptSyncFromQueue(self):
+ """Accept a sync upload from the queue."""
+ # Circular imports :(
+ from lp.soyuz.model.packagecopyjob import PlainPackageCopyJob
+
+ assert self.package_copy_job is not None, (
+ "This method is for copy-job uploads only.")
+ assert not self.is_delayed_copy, (
+ "This method is not for delayed copies.")
+
+ if self.status == PackageUploadStatus.REJECTED:
+ raise QueueInconsistentStateError(
+ "Can't resurrect rejected syncs")
+
+ # Release the job hounds, Smithers.
+ self.setAccepted()
+ job = PlainPackageCopyJob.get(self.package_copy_job_id)
+ job.resume()
+ # The copy job will send emails as appropriate. We don't
+ # need to worry about closing bugs from syncs, although we
+ # should probably give karma but that needs more work to
+ # fix here.
+
+ def _acceptNonSyncFromQueue(self, logger=None, dry_run=False):
+ """Accept a "regular" upload from the queue.
+
+ This is the normal case, for uploads that are not delayed and are not
+ attached to package copy jobs.
+ """
+ assert self.package_copy_job is None, (
+ "This method is not for copy-job uploads.")
+ assert not self.is_delayed_copy, (
+ "This method is not for delayed copies.")
self.setAccepted()
@@ -517,6 +531,15 @@
# Give some karma!
self._giveKarma()
+ def acceptFromQueue(self, logger=None, dry_run=False):
+ """See `IPackageUpload`."""
+ assert not self.is_delayed_copy, 'Cannot process delayed copies.'
+
+ if self.package_copy_job is None:
+ self._acceptNonSyncFromQueue(logger, dry_run)
+ else:
+ self._acceptSyncFromQueue()
+
def acceptFromCopy(self):
"""See `IPackageUpload`."""
assert self.is_delayed_copy, 'Can only process delayed-copies.'
@@ -617,7 +640,7 @@
def package_version(self):
"""See `IPackageUpload`."""
if self.package_copy_job_id is not None:
- return self.package_copy_job.metadata["package_version"]
+ return self.package_copy_job.package_version
elif self.sourcepackagerelease is not None:
return self.sourcepackagerelease.version
else:
@@ -627,8 +650,8 @@
def component_name(self):
"""See `IPackageUpload`."""
if self.package_copy_job_id is not None:
- return self.package_copy_job.metadata["component_override"]
- elif self.sourcepackagerelease is not None:
+ return self.package_copy_job.component_name
+ elif self.contains_source:
return self.sourcepackagerelease.component.name
else:
return None
@@ -637,8 +660,8 @@
def section_name(self):
"""See `IPackageUpload`."""
if self.package_copy_job_id is not None:
- return self.package_copy_job.metadata["section_override"]
- elif self.sourcepackagerelease is not None:
+ return self.package_copy_job.section_name
+ elif self.contains_source:
return self.sourcepackagerelease.section.name
else:
return None
@@ -860,39 +883,63 @@
"""See `IPackageUpload`."""
return getUtility(IPackageCopyJobSource).wrap(self.package_copy_job)
- def overrideSource(self, new_component, new_section, allowed_components):
- """See `IPackageUpload`."""
- if new_component is None and new_section is None:
- # Nothing needs overriding, bail out.
- return False
-
- if self.package_copy_job is not None:
- self.concrete_package_copy_job.addSourceOverride(SourceOverride(
- self.package_name, new_component, new_section))
- return True
-
- if not self.contains_source:
- return False
+ def _overrideSyncSource(self, new_component, new_section,
+ allowed_components):
+ """Override source on the upload's `PackageCopyJob`, if any."""
+ if self.package_copy_job is None:
+ return False
+
+ copy_job = self.concrete_package_copy_job
+ allowed_component_names = [
+ component.name for component in allowed_components]
+ if copy_job.component_name not in allowed_component_names:
+ raise QueueInconsistentStateError(
+ "No rights to override from %s to %s" % (
+ copy_job.component_name, new_component.name))
+ copy_job.addSourceOverride(SourceOverride(
+ copy_job.package_name, new_component, new_section))
+
+ return True
+
+ def _overrideNonSyncSource(self, new_component, new_section,
+ allowed_components):
+ """Override sources on a source upload."""
+ made_changes = False
for source in self.sources:
- if (new_component not in allowed_components or
- source.sourcepackagerelease.component not in
- allowed_components):
- # The old or the new component is not in the list of
- # allowed components to override.
+ old_component = source.sourcepackagerelease.component
+ if old_component not in allowed_components:
+ # The old component is not in the list of allowed components
+ # to override.
raise QueueInconsistentStateError(
"No rights to override from %s to %s" % (
- source.sourcepackagerelease.component.name,
- new_component.name))
+ old_component.name, new_component.name))
source.sourcepackagerelease.override(
component=new_component, section=new_section)
+ made_changes = True
# We override our own archive too, as it is used to create
# the SPPH during publish().
self.archive = self.distroseries.distribution.getArchiveByComponent(
new_component.name)
- return True
+ return made_changes
+
+ def overrideSource(self, new_component, new_section, allowed_components):
+ """See `IPackageUpload`."""
+ if new_component is None and new_section is None:
+ # Nothing needs overriding, bail out.
+ return False
+
+ if new_component not in allowed_components:
+ raise QueueInconsistentStateError(
+ "No rights to override to %s" % new_component.name)
+
+ return (
+ self._overrideSyncSource(
+ new_component, new_section, allowed_components) or
+ self._overrideNonSyncSource(
+ new_component, new_section, allowed_components))
def overrideBinaries(self, new_component, new_section, new_priority,
allowed_components):
=== modified file 'lib/lp/soyuz/stories/soyuz/xx-queue-pages-motu.txt'
--- lib/lp/soyuz/stories/soyuz/xx-queue-pages-motu.txt 2010-11-06 12:45:26 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-queue-pages-motu.txt 2011-06-23 14:18:09 +0000
@@ -110,7 +110,7 @@
>>> motu_browser.getControl(name="Accept").click()
>>> for message in get_feedback_messages(motu_browser.contents):
... print message
- FAILED: alsa-utils (No rights to override from universe to main)
+ FAILED: alsa-utils (No rights to override to main)
The same applies to the binary:
@@ -120,7 +120,7 @@
>>> motu_browser.getControl(name="Accept").click()
>>> for message in get_feedback_messages(motu_browser.contents):
... print message
- FAILED: pmount (No rights to override from universe to main)
+ FAILED: pmount (No rights to override to main)
Our user is able to override to multiverse, however. Let's do that
with pmount:
=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py 2011-06-22 12:03:31 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py 2011-06-23 14:18:09 +0000
@@ -370,14 +370,11 @@
def makeUploadWithPackageCopyJob(self, sourcepackagename=None):
"""Create a `PackageUpload` plus attached `PlainPackageCopyJob`."""
- job_factory_args = {}
- if sourcepackagename is not None:
- job_factory_args['package_name'] = sourcepackagename.name
- job_factory_args['package_version'] = '1.0'
- job = self.factory.makePlainPackageCopyJob(**job_factory_args)
- naked_job = removeSecurityProxy(job).context
- upload = self.factory.makePackageUpload(package_copy_job=naked_job)
- return upload, job
+ from lp.soyuz.model.packagecopyjob import IPackageCopyJobSource
+ upload = self.factory.makeCopyJobPackageUpload(
+ sourcepackagename=sourcepackagename)
+ return upload, getUtility(IPackageCopyJobSource).wrap(
+ upload.package_copy_job)
def test_package_copy_job_property(self):
# Test that we can set and get package_copy_job.
@@ -395,25 +392,48 @@
def test_overrideSource_with_copy_job(self):
# The overrides should be stored in the job's metadata.
pu, pcj = self.makeUploadWithPackageCopyJob()
+ old_component = getUtility(IComponentSet)[pcj.component_name]
component = getUtility(IComponentSet)['restricted']
section = getUtility(ISectionSet)['games']
- expected_metadata = {
+ expected_metadata = {}
+ expected_metadata.update(pcj.metadata)
+ expected_metadata.update({
'component_override': component.name,
'section_override': section.name,
- }
- expected_metadata.update(pcj.metadata)
-
- pu.overrideSource(component, section, allowed_components=[component])
-
+ })
+
+ result = pu.overrideSource(
+ component, section, allowed_components=[component, old_component])
+
+ self.assertTrue(result)
self.assertEqual(expected_metadata, pcj.metadata)
+ def test_overrideSource_checks_permission_for_old_component(self):
+ pu = self.factory.makeCopyJobPackageUpload()
+ only_allowed_component = self.factory.makeComponent()
+ section = self.factory.makeSection()
+ self.assertRaises(
+ QueueInconsistentStateError,
+ pu.overrideSource,
+ only_allowed_component, section, [only_allowed_component])
+
+ def test_overrideSource_checks_permission_for_new_component(self):
+ pu, pcj = self.makeUploadWithPackageCopyJob()
+ current_component = getUtility(IComponentSet)[pcj.component_name]
+ disallowed_component = self.factory.makeComponent()
+ section = self.factory.makeSection()
+ self.assertRaises(
+ QueueInconsistentStateError,
+ pu.overrideSource,
+ disallowed_component, section, [current_component])
+
def test_acceptFromQueue_with_copy_job(self):
# acceptFromQueue should accept the upload and resume the copy
# job.
pu, pcj = self.makeUploadWithPackageCopyJob()
+ pu.pocket = PackagePublishingPocket.RELEASE
self.assertEqual(PackageUploadStatus.NEW, pu.status)
- pcj.suspend()
pu.acceptFromQueue()
@@ -423,7 +443,6 @@
def test_rejectFromQueue_with_copy_job(self):
# rejectFromQueue will reject the upload and fail the copy job.
pu, pcj = self.makeUploadWithPackageCopyJob()
- pcj.suspend()
pu.rejectFromQueue()
@@ -451,11 +470,10 @@
# An upload with a copy job takes its component and section
# names from the job.
spn = self.factory.makeSourcePackageName()
- upload, job = self.makeUploadWithPackageCopyJob(sourcepackagename=spn)
+ upload, pcj = self.makeUploadWithPackageCopyJob(sourcepackagename=spn)
component = self.factory.makeComponent()
section = self.factory.makeSection()
- job.addSourceOverride(SourceOverride(
- source_package_name=spn, component=component, section=section))
+ pcj.addSourceOverride(SourceOverride(spn, component, section))
self.assertEqual(component.name, upload.component_name)
def test_displayname_is_package_name(self):
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-06-14 20:43:01 +0000
+++ lib/lp/testing/factory.py 2011-06-23 14:18:09 +0000
@@ -243,6 +243,7 @@
from lp.services.utils import AutoDecorate
from lp.services.worlddata.interfaces.country import ICountrySet
from lp.services.worlddata.interfaces.language import ILanguageSet
+from lp.soyuz.adapters.overrides import SourceOverride
from lp.soyuz.adapters.packagelocation import PackageLocation
from lp.soyuz.enums import (
ArchivePurpose,
@@ -3442,6 +3443,8 @@
source_archive=spph.archive,
target_archive=distroseries.main_archive,
target_distroseries=distroseries)
+ job.addSourceOverride(SourceOverride(
+ spr.sourcepackagename, spr.component, spr.section))
try:
job.run()
except SuspendJobException:
