launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04044
[Merge] lp:~jtv/launchpad/bug-800652 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-800652 into lp:launchpad.
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/65674
= 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/65674
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/adapters/overrides.py'
--- lib/lp/soyuz/adapters/overrides.py 2011-06-05 08:17:40 +0000
+++ lib/lp/soyuz/adapters/overrides.py 2011-06-23 14:17:28 +0000
@@ -40,13 +40,7 @@
from lp.soyuz.model.binarypackagename import BinaryPackageName
from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
from lp.soyuz.model.component import Component
-from lp.soyuz.model.distroarchseries import DistroArchSeries
-from lp.soyuz.model.publishing import (
- BinaryPackagePublishingHistory,
- SourcePackagePublishingHistory,
- )
from lp.soyuz.model.section import Section
-from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
class IOverride(Interface):
@@ -130,7 +124,7 @@
def __repr__(self):
return ("<BinaryOverride at %x component=%r section=%r "
- "binary_package_name=%r distro_arch_series=%r priority=%r>" %
+ "binary_package_name=%r distro_arch_series=%r priority=%r>" %
(id(self), self.component, self.section, self.binary_package_name,
self.distro_arch_series, self.priority))
@@ -187,7 +181,7 @@
class FromExistingOverridePolicy(BaseOverridePolicy):
"""Override policy that only searches for existing publications.
-
+
Override policy that returns the SourcePackageName, component and
section for the latest published source publication, or the
BinaryPackageName, DistroArchSeries, component, section and priority
@@ -195,10 +189,15 @@
"""
def calculateSourceOverrides(self, archive, distroseries, pocket, spns):
- store = IStore(SourcePackagePublishingHistory)
+ # Avoid circular imports.
+ from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+ from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+
def eager_load(rows):
bulk.load(Component, (row[1] for row in rows))
bulk.load(Section, (row[2] for row in rows))
+
+ store = IStore(SourcePackagePublishingHistory)
already_published = DecoratedResultSet(
store.find(
(SourcePackageRelease.sourcepackagenameID,
@@ -226,10 +225,15 @@
def calculateBinaryOverrides(self, archive, distroseries, pocket,
binaries):
- store = IStore(BinaryPackagePublishingHistory)
+ # Avoid circular imports.
+ from lp.soyuz.model.distroarchseries import DistroArchSeries
+ from lp.soyuz.model.publishing import BinaryPackagePublishingHistory
+
def eager_load(rows):
bulk.load(Component, (row[2] for row in rows))
bulk.load(Section, (row[3] for row in rows))
+
+ store = IStore(BinaryPackagePublishingHistory)
expanded = calculate_target_das(distroseries, binaries)
candidates = (
make_package_condition(archive, das, bpn)
@@ -266,11 +270,11 @@
class UnknownOverridePolicy(BaseOverridePolicy):
"""Override policy that returns defaults.
-
+
Override policy that assumes everything passed in doesn't exist, so
returns the defaults.
"""
-
+
def calculateSourceOverrides(self, archive, distroseries, pocket,
sources):
default_component = archive.default_component or getUtility(
@@ -291,9 +295,9 @@
class UbuntuOverridePolicy(FromExistingOverridePolicy,
UnknownOverridePolicy):
"""Override policy for Ubuntu.
-
- An override policy that incorporates both the from existing policy
- and the unknown policy.
+
+ An override policy that incorporates both the existing policy and the
+ unknown policy.
"""
def calculateSourceOverrides(self, archive, distroseries, pocket,
@@ -314,9 +318,12 @@
total = set(binaries)
overrides = FromExistingOverridePolicy.calculateBinaryOverrides(
self, archive, distroseries, pocket, binaries)
- existing = set((
- overide.binary_package_name, overide.distro_arch_series.architecturetag)
- for overide in overrides)
+ existing = set(
+ (
+ overide.binary_package_name,
+ overide.distro_arch_series.architecturetag,
+ )
+ for overide in overrides)
missing = total.difference(existing)
if missing:
unknown = UnknownOverridePolicy.calculateBinaryOverrides(
@@ -340,6 +347,8 @@
def make_package_condition(archive, das, bpn):
+ # Avoid circular imports.
+ from lp.soyuz.model.publishing import BinaryPackagePublishingHistory
return And(
BinaryPackagePublishingHistory.archiveID == archive.id,
BinaryPackagePublishingHistory.distroarchseriesID == das.id,
@@ -347,9 +356,13 @@
def id_resolver(lookups):
+ # Avoid circular imports.
+ from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+
def _resolve(row):
store = IStore(SourcePackagePublishingHistory)
return tuple(
(value if cls is None else store.get(cls, value))
for value, cls in zip(row, lookups))
+
return _resolve
=== 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:17:28 +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:17:28 +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/configure.zcml'
--- lib/lp/soyuz/configure.zcml 2011-06-15 02:41:34 +0000
+++ lib/lp/soyuz/configure.zcml 2011-06-23 14:17:28 +0000
@@ -914,6 +914,11 @@
<!-- PackageCopyJobSource -->
<securedutility
+ component=".model.packagecopyjob.PackageCopyJob"
+ provides=".interfaces.packagecopyjob.IPackageCopyJobSource">
+ <allow interface=".interfaces.packagecopyjob.IPackageCopyJobSource"/>
+ </securedutility>
+ <securedutility
component=".model.packagecopyjob.PlainPackageCopyJob"
provides=".interfaces.packagecopyjob.IPlainPackageCopyJobSource">
<allow interface=".interfaces.packagecopyjob.IPlainPackageCopyJobSource"/>
=== 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:17:28 +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/interfaces/packagecopyjob.py'
--- lib/lp/soyuz/interfaces/packagecopyjob.py 2011-06-14 06:01:14 +0000
+++ lib/lp/soyuz/interfaces/packagecopyjob.py 2011-06-23 14:17:28 +0000
@@ -5,6 +5,7 @@
__all__ = [
"IPackageCopyJob",
+ "IPackageCopyJobSource",
"IPlainPackageCopyJob",
"IPlainPackageCopyJobSource",
"PackageCopyJobType",
@@ -37,6 +38,21 @@
from lp.soyuz.interfaces.archive import IArchive
+class IPackageCopyJobSource(Interface):
+ """Utility for `IPackageCopyJob`-implementing types."""
+
+ def wrap(package_copy_job):
+ """Wrap a `PackageCopyJob` in its concrete implementation type.
+
+ As a special case, `None` produces `None`.
+
+ :param package_copy_job: A `PackageCopyJob`.
+ :return: An `IPackageCopyJob` implementation based on
+ `package_copy_job`, but of the job's specific concrete type
+ (such as `PlainPackageCopyJob`).
+ """
+
+
class IPackageCopyJob(Interface):
"""A job that copies packages between `IArchive`s."""
@@ -74,6 +90,12 @@
schema=IJob, title=_('The common Job attributes'),
required=True, readonly=True)
+ component_name = TextLine(
+ title=_("Component override name"), required=False, readonly=True)
+
+ section_name = TextLine(
+ title=_("Section override name"), required=False, readonly=True)
+
metadata = Attribute('A dict of data about the job.')
=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py 2011-06-17 05:23:56 +0000
+++ lib/lp/soyuz/interfaces/queue.py 2011-06-23 14:17:28 +0000
@@ -140,6 +140,11 @@
package_copy_job = Reference(
schema=IPackageCopyJob,
description=_("The PackageCopyJob for this upload, if it has one."),
+ title=_("Raw Package Copy Job"), required=False, readonly=True)
+
+ concrete_package_copy_job = Reference(
+ schema=IPackageCopyJob,
+ description=_("Concrete IPackageCopyJob implementation, if any."),
title=_("Package Copy Job"), required=False, readonly=True)
archive = exported(
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py 2011-06-16 13:21:14 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py 2011-06-23 14:17:28 +0000
@@ -22,6 +22,7 @@
classProvides,
implements,
)
+from zope.security.proxy import removeSecurityProxy
from canonical.database.enumcol import EnumCol
from canonical.database.sqlbase import sqlvalues
@@ -61,6 +62,7 @@
from lp.soyuz.interfaces.copypolicy import ICopyPolicy
from lp.soyuz.interfaces.packagecopyjob import (
IPackageCopyJob,
+ IPackageCopyJobSource,
IPlainPackageCopyJob,
IPlainPackageCopyJobSource,
PackageCopyJobType,
@@ -75,6 +77,7 @@
"""Base class for package copying jobs."""
implements(IPackageCopyJob)
+ classProvides(IPackageCopyJobSource)
__storm_table__ = 'PackageCopyJob'
@@ -99,6 +102,31 @@
_json_data = Unicode('json_data')
+ # Derived concrete classes. The entire class gets one dict for
+ # this; it's not meant to be on an instance.
+ concrete_classes = {}
+
+ @classmethod
+ def registerConcreteClass(cls, new_class):
+ """Register a concrete `IPackageCopyJob`-implementing class."""
+ assert new_class.class_job_type not in cls.concrete_classes, (
+ "Class %s is already registered." % new_class)
+ cls.concrete_classes[new_class.class_job_type] = new_class
+
+ @classmethod
+ def wrap(cls, package_copy_job):
+ """See `IPackageCopyJobSource`."""
+ if package_copy_job is None:
+ return None
+ job_type = removeSecurityProxy(package_copy_job).job_type
+ concrete_class = cls.concrete_classes[job_type]
+ return concrete_class(package_copy_job)
+
+ @classmethod
+ def getByID(cls, pcj_id):
+ """See `IPackageCopyJobSource`."""
+ return cls.wrap(IStore(PackageCopyJob).get(PackageCopyJob, pcj_id))
+
def __init__(self, source_archive, target_archive, target_distroseries,
job_type, metadata, package_name=None, copy_policy=None):
super(PackageCopyJob, self).__init__()
@@ -130,6 +158,16 @@
existing.update(metadata_dict)
self._json_data = self.serializeMetadata(existing)
+ @property
+ def component_name(self):
+ """See `IPackageCopyJob`."""
+ return self.metadata.get("component_override")
+
+ @property
+ def section_name(self):
+ """See `IPackageCopyJob`."""
+ return self.metadata.get("section_override")
+
class PackageCopyJobDerived(BaseRunnableJob):
"""Abstract class for deriving from PackageCopyJob."""
@@ -339,13 +377,13 @@
metadata_dict = dict(
component_override=component,
section_override=section)
- self.context.extendMetadata(metadata_dict)
+ removeSecurityProxy(self.context).extendMetadata(metadata_dict)
def getSourceOverride(self):
"""Fetch an `ISourceOverride` from the metadata."""
name = self.package_name
- component_name = self.metadata.get("component_override")
- section_name = self.metadata.get("section_override")
+ component_name = self.component_name
+ section_name = self.section_name
source_package_name = getUtility(ISourcePackageNameSet)[name]
try:
component = getUtility(IComponentSet)[component_name]
@@ -508,3 +546,6 @@
def getPolicyImplementation(self):
"""Return the `ICopyPolicy` applicable to this job."""
return ICopyPolicy(self.copy_policy)
+
+
+PackageCopyJob.registerConcreteClass(PlainPackageCopyJob)
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py 2011-06-22 12:03:31 +0000
+++ lib/lp/soyuz/model/queue.py 2011-06-23 14:17:28 +0000
@@ -66,11 +66,13 @@
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.enums import (
PackageUploadCustomFormat,
PackageUploadStatus,
)
from lp.soyuz.interfaces.archive import MAIN_ARCHIVE_PURPOSES
+from lp.soyuz.interfaces.packagecopyjob import IPackageCopyJobSource
from lp.soyuz.interfaces.publishing import (
IPublishingSet,
ISourcePackagePublishingHistory,
@@ -467,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()
@@ -515,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.'
@@ -615,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:
@@ -625,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
@@ -635,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
@@ -853,45 +878,68 @@
existing_components.add(binary.component)
return existing_components
- def overrideSource(self, new_component, new_section, allowed_components):
+ @cachedproperty
+ def concrete_package_copy_job(self):
"""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:
- # We just need to add the required component/section to the
- # job metadata.
- extra_data = {}
- if new_component is not None:
- extra_data['component_override'] = new_component.name
- if new_section is not None:
- extra_data['section_override'] = new_section.name
- self.package_copy_job.extendMetadata(extra_data)
- return
-
- if not self.contains_source:
- return False
+ return getUtility(IPackageCopyJobSource).wrap(self.package_copy_job)
+
+ 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:17:28 +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_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py 2011-06-10 11:17:39 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2011-06-23 14:17:28 +0000
@@ -17,7 +17,10 @@
from canonical.config import config
from canonical.launchpad.interfaces.lpstorm import IStore
from canonical.launchpad.webapp.testing import verifyObject
-from canonical.testing import LaunchpadZopelessLayer
+from canonical.testing import (
+ LaunchpadZopelessLayer,
+ ZopelessDatabaseLayer,
+ )
from lp.registry.model.distroseriesdifferencecomment import (
DistroSeriesDifferenceComment,
)
@@ -43,6 +46,7 @@
from lp.soyuz.interfaces.component import IComponentSet
from lp.soyuz.interfaces.packagecopyjob import (
IPackageCopyJob,
+ IPackageCopyJobSource,
IPlainPackageCopyJob,
IPlainPackageCopyJobSource,
)
@@ -52,12 +56,14 @@
from lp.soyuz.interfaces.sourcepackageformat import (
ISourcePackageFormatSelectionSet,
)
+from lp.soyuz.model.packagecopyjob import PackageCopyJob
from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
from lp.testing import (
run_script,
TestCaseWithFactory,
)
from lp.testing.mail_helpers import pop_notifications
+from lp.testing.matchers import Provides
from lp.testing.fakemethod import FakeMethod
@@ -298,7 +304,7 @@
archive=breezy_archive)
# The target archive needs ancestry so the package is
# auto-accepted.
- ancestry = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=target_series, sourcename="libc",
version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
archive=target_archive)
@@ -489,7 +495,7 @@
# Publish a package in the source archive with some overridable
# properties set to known values.
- source_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="libc",
component='universe', section='web',
version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
@@ -497,7 +503,7 @@
# Now put the same named package in the target archive with
# different override values.
- ancestry_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="libc",
component='restricted', section='games',
version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
@@ -594,7 +600,7 @@
# Publish a package in the source archive with some overridable
# properties set to known values.
- source_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="copyme",
component='multiverse', section='web',
version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
@@ -645,7 +651,7 @@
source_archive = self.factory.makeArchive()
# Publish a package in the source archive.
- source_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="copyme",
version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
component='multiverse', section='web',
@@ -653,7 +659,7 @@
# Now put the same named package in the target archive so it has
# ancestry.
- ancestry_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="copyme",
version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
component='main', section='games',
@@ -698,7 +704,7 @@
source_archive = self.factory.makeArchive()
# Publish a package in the source archive.
- source_package = publisher.getPubSource(
+ publisher.getPubSource(
distroseries=distroseries, sourcename="copyme",
version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
archive=source_archive)
@@ -838,3 +844,25 @@
transaction.commit()
self.layer.switchDbUser(self.dbuser)
removeSecurityProxy(job).reportFailure(CannotCopy("Mommy it hurts"))
+
+
+class TestPackageCopyJobSource(TestCaseWithFactory):
+ """Test the `IPackageCopyJob` utility."""
+
+ layer = ZopelessDatabaseLayer
+
+ def test_implements_interface(self):
+ job_source = getUtility(IPackageCopyJobSource)
+ self.assertThat(job_source, Provides(IPackageCopyJobSource))
+
+ def test_wrap_accepts_None(self):
+ job_source = getUtility(IPackageCopyJobSource)
+ self.assertIs(None, job_source.wrap(None))
+
+ def test_wrap_wraps_PlainPackageCopyJob(self):
+ original_ppcj = self.factory.makePlainPackageCopyJob()
+ IStore(PackageCopyJob).flush()
+ pcj = IStore(PackageCopyJob).get(PackageCopyJob, original_ppcj.id)
+ self.assertNotEqual(None, pcj)
+ job_source = getUtility(IPackageCopyJobSource)
+ self.assertEqual(original_ppcj, job_source.wrap(pcj))
=== 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:17:28 +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:17:28 +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:
