launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02010
[Merge] lp:~salgado/launchpad/safe-blueprints-model into lp:launchpad/devel
Guilherme Salgado has proposed merging lp:~salgado/launchpad/safe-blueprints-model into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
This branch moves some Blueprint logic from views to models.
We want that before we expose Blueprints over the API so that we don't have to
duplicate the logic into multiple places.
Below is a more detailed list of all the changes:
- Get rid of propose_goal_with_automatic_approval by merging it with
proposeGoal(). This required refactoring the code of the security adapter
into a model method that is then used in proposeGoal() and the security
adapter
- Refactor retarget() to take just a target instead of a product or a
distribution.
- Start splitting ISpecification into several interfaces where each will be
protected with a different permission.
- Consolidate validation of blueprint retargeting into a validateMove()
method and use that all around.
--
https://code.launchpad.net/~salgado/launchpad/safe-blueprints-model/+merge/41722
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~salgado/launchpad/safe-blueprints-model into lp:launchpad/devel.
=== modified file 'lib/canonical/launchpad/interfaces/launchpad.py'
--- lib/canonical/launchpad/interfaces/launchpad.py 2010-10-03 15:30:06 +0000
+++ lib/canonical/launchpad/interfaces/launchpad.py 2010-11-24 12:23:35 +0000
@@ -414,6 +414,13 @@
"""
drivers = Attribute("A list of drivers")
+ def personHasDriverRights(person):
+ """Does the given person have launchpad.Driver rights on this object?
+
+ True if the person is one of this object's drivers, its owner or a
+ Launchpad admin.
+ """
+
class IHasAppointedDriver(Interface):
"""An object that has an appointed driver."""
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2010-11-12 23:30:57 +0000
+++ lib/canonical/launchpad/security.py 2010-11-24 12:23:35 +0000
@@ -998,9 +998,7 @@
usedfor = IHasDrivers
def checkAuthenticated(self, user):
- return (user.isOneOfDrivers(self.obj) or
- user.isOwner(self.obj) or
- user.in_admin)
+ return self.obj.personHasDriverRights(user)
class ViewProductSeries(AnonymousAuthorization):
=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py 2010-11-23 23:22:27 +0000
+++ lib/lp/blueprints/browser/specification.py 2010-11-24 12:23:35 +0000
@@ -96,6 +96,7 @@
ISpecification,
ISpecificationSet,
)
+from lp.blueprints.errors import TargetAlreadyHasSpecification
from lp.blueprints.interfaces.specificationbranch import ISpecificationBranch
from lp.blueprints.interfaces.sprintspecification import ISprintSpecification
from lp.code.interfaces.branchnamespace import IBranchNamespaceSet
@@ -129,7 +130,7 @@
# Propose the specification as a series goal, if specified.
series = data.get('series')
if series is not None:
- propose_goal_with_automatic_approval(spec, series, self.user)
+ spec.proposeGoal(series, self.user)
# Propose the specification as a sprint topic, if specified.
sprint = data.get('sprint')
if sprint is not None:
@@ -603,8 +604,7 @@
@action('Continue', name='continue')
def continue_action(self, action, data):
self.context.whiteboard = data['whiteboard']
- propose_goal_with_automatic_approval(
- self.context, data['distroseries'], self.user)
+ self.context.proposeGoal(data['distroseries'], self.user)
self.next_url = canonical_url(self.context)
@property
@@ -619,8 +619,7 @@
@action('Continue', name='continue')
def continue_action(self, action, data):
self.context.whiteboard = data['whiteboard']
- propose_goal_with_automatic_approval(
- self.context, data['productseries'], self.user)
+ self.context.proposeGoal(data['productseries'], self.user)
self.next_url = canonical_url(self.context)
@property
@@ -628,16 +627,6 @@
return canonical_url(self.context)
-def propose_goal_with_automatic_approval(specification, series, user):
- """Proposes the given specification as a goal for the given series. If
- the given user has permission, the proposal is approved automatically.
- """
- specification.proposeGoal(series, user)
- # If the proposer has permission, approve the goal automatically.
- if series is not None and check_permission('launchpad.Driver', series):
- specification.acceptBy(user)
-
-
class SpecificationGoalDecideView(LaunchpadFormView):
"""View used to allow the drivers of a series to accept
or decline the spec as a goal for that series. Typically they would use
@@ -688,29 +677,17 @@
self.request.form.get("field.target"))
return
- if target.getSpecification(self.context.name) is not None:
+ try:
+ self.context.validateMove(target)
+ except TargetAlreadyHasSpecification:
self.setFieldError('target',
'There is already a blueprint with this name for %s. '
'Please change the name of this blueprint and try again.' %
target.displayname)
- return
@action(_('Retarget Blueprint'), name='retarget')
- def register_action(self, action, data):
- # we need to ensure that there is not already a spec with this name
- # for this new target
- target = data['target']
- if target.getSpecification(self.context.name) is not None:
- return '%s already has a blueprint called %s' % (
- target.displayname, self.context.name)
- product = distribution = None
- if IProduct.providedBy(target):
- product = target
- elif IDistribution.providedBy(target):
- distribution = target
- else:
- raise AssertionError('Unknown target.')
- self.context.retarget(product=product, distribution=distribution)
+ def retarget_action(self, action, data):
+ self.context.retarget(data['target'])
self._nextURL = canonical_url(self.context)
@property
@@ -775,6 +752,8 @@
SUPERSEDED = SpecificationDefinitionStatus.SUPERSEDED
NEW = SpecificationDefinitionStatus.NEW
self.context.superseded_by = data['superseded_by']
+ # XXX: salgado, 2010-11-24, bug=680880: This logic should be in model
+ # code.
if data['superseded_by'] is not None:
# set the state to superseded
self.context.definition_status = SUPERSEDED
=== modified file 'lib/lp/blueprints/configure.zcml'
--- lib/lp/blueprints/configure.zcml 2010-11-09 14:35:44 +0000
+++ lib/lp/blueprints/configure.zcml 2010-11-24 12:23:35 +0000
@@ -152,7 +152,7 @@
<!-- Specification -->
<class class="lp.blueprints.model.specification.Specification">
- <allow interface="lp.blueprints.interfaces.specification.ISpecification"/>
+ <allow interface="lp.blueprints.interfaces.specification.ISpecificationPublic"/>
<!-- We allow any authenticated person to update the whiteboard -->
<require
permission="launchpad.AnyPerson"
@@ -162,6 +162,7 @@
methods -->
<require
permission="launchpad.Edit"
+ interface="lp.blueprints.interfaces.specification.ISpecificationEditRestricted"
set_attributes="name title summary definition_status specurl
superseded_by milestone product distribution
approver assignee drafter man_days
=== modified file 'lib/lp/blueprints/doc/specification.txt'
--- lib/lp/blueprints/doc/specification.txt 2010-11-01 03:32:29 +0000
+++ lib/lp/blueprints/doc/specification.txt 2010-11-24 12:23:35 +0000
@@ -435,10 +435,11 @@
'Declined'
And finally, if we propose a new goal, then the decision status is
-invalidated.
+invalidated. (Notice that we propose the goal as jdub as goals proposed by one
+of their drivers [e.g. mark] would be automatically accepted)
>>> trunk = upstream_firefox.getSeries('trunk')
- >>> e4x.proposeGoal(trunk, mark)
+ >>> e4x.proposeGoal(trunk, jdub)
>>> e4x.goalstatus.title
'Proposed'
=== added file 'lib/lp/blueprints/errors.py'
--- lib/lp/blueprints/errors.py 1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/errors.py 2010-11-24 12:23:35 +0000
@@ -0,0 +1,19 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Specification views."""
+
+__metaclass__ = type
+
+__all__ = [
+ 'TargetAlreadyHasSpecification',
+ ]
+
+
+class TargetAlreadyHasSpecification(Exception):
+ """The ISpecificationTarget already has a specification of that name."""
+
+ def __init__(self, target, name):
+ msg = "The target %s already has a specification named %s" % (
+ target, name)
+ super(TargetAlreadyHasSpecification, self).__init__(msg)
=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py 2010-11-04 03:34:54 +0000
+++ lib/lp/blueprints/interfaces/specification.py 2010-11-24 12:23:35 +0000
@@ -207,11 +207,27 @@
required=True, vocabulary='DistributionOrProduct')
-class ISpecification(INewSpecification, INewSpecificationTarget, IHasOwner,
- IHasLinkedBranches):
- """A Specification."""
-
- export_as_webservice_entry()
+class ISpecificationEditRestricted(Interface):
+ """Specification's attributes and methods protected with launchpad.Edit.
+ """
+
+ def setTarget(target):
+ """Set this specification's target.
+
+ :param target: an IProduct or IDistribution.
+ """
+
+ def retarget(target):
+ """Move the spec to the given target.
+
+ The new target must be an IProduct or IDistribution.
+ """
+
+
+class ISpecificationPublic(
+ INewSpecification, INewSpecificationTarget, IHasOwner,
+ IHasLinkedBranches):
+ """Specification's public attributes and methods."""
# TomBerger 2007-06-20: 'id' is required for
# SQLObject to be able to assign a security-proxied
@@ -344,10 +360,8 @@
default=SpecificationLifecycleStatus.NOTSTARTED,
readonly=True)
- def retarget(product=None, distribution=None):
- """Retarget the spec to a new product or distribution. One of
- product or distribution must be None (but not both).
- """
+ def validateMove(target):
+ """Check that the specification can be moved to the target."""
def getSprintSpecification(sprintname):
"""Get the record that links this spec to the named sprint."""
@@ -450,6 +464,12 @@
"""Return the SpecificationBranch link for the branch, or None."""
+class ISpecification(ISpecificationPublic, ISpecificationEditRestricted):
+ """A Specification."""
+
+ export_as_webservice_entry()
+
+
class ISpecificationSet(IHasSpecifications):
"""A container for specifications."""
=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py 2010-11-04 03:58:05 +0000
+++ lib/lp/blueprints/model/specification.py 2010-11-24 12:23:35 +0000
@@ -60,6 +60,7 @@
SpecificationPriority,
SpecificationSort,
)
+from lp.blueprints.errors import TargetAlreadyHasSpecification
from lp.blueprints.interfaces.specification import (
ISpecification,
ISpecificationSet,
@@ -77,9 +78,11 @@
from lp.blueprints.model.sprintspecification import SprintSpecification
from lp.bugs.interfaces.buglink import IBugLinkTarget
from lp.bugs.model.buglinktarget import BugLinkTargetMixin
+from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.distroseries import IDistroSeries
from lp.registry.interfaces.person import validate_public_person
from lp.registry.interfaces.productseries import IProductSeries
+from lp.registry.interfaces.product import IProduct
class Specification(SQLBase, BugLinkTargetMixin):
@@ -182,7 +185,6 @@
otherColumn='specification', orderBy='title',
intermediateTable='SpecificationDependency')
- # attributes
@property
def target(self):
"""See ISpecification."""
@@ -190,25 +192,27 @@
return self.product
return self.distribution
- def retarget(self, product=None, distribution=None):
- """See ISpecification."""
- assert not (product and distribution)
- assert (product or distribution)
-
- # we need to ensure that there is not already a spec with this name
- # for this new target
- if product:
- assert product.getSpecification(self.name) is None
- elif distribution:
- assert distribution.getSpecification(self.name) is None
-
- # if we are not changing anything, then return
- if self.product == product and self.distribution == distribution:
+ def setTarget(self, target):
+ """See ISpecification."""
+ if IProduct.providedBy(target):
+ self.product = target
+ self.distribution = None
+ elif IDistribution.providedBy(target):
+ self.product = None
+ self.distribution = target
+ else:
+ raise AssertionError("Unknown target: %s" % target)
+
+ def retarget(self, target):
+ """See ISpecification."""
+ if self.target == target:
return
- # we must lose any goal we have set and approved/declined because we
- # are moving to a different product that will have different
- # policies and drivers
+ self.validateMove(target)
+
+ # We must lose any goal we have set and approved/declined because we
+ # are moving to a different target that will have different
+ # policies and drivers.
self.productseries = None
self.distroseries = None
self.goalstatus = SpecificationGoalStatus.PROPOSED
@@ -216,12 +220,15 @@
self.date_goal_proposed = None
self.milestone = None
- # set the new values
- self.product = product
- self.distribution = distribution
+ self.setTarget(target)
self.priority = SpecificationPriority.UNDEFINED
self.direction_approved = False
+ def validateMove(self, target):
+ """See ISpecification."""
+ if target.getSpecification(self.name) is not None:
+ raise TargetAlreadyHasSpecification(target, self.name)
+
@property
def goal(self):
"""See ISpecification."""
@@ -259,6 +266,8 @@
# the goal should now also not have a decider
self.goal_decider = None
self.date_goal_decided = None
+ if goal is not None and goal.personHasDriverRights(proposer):
+ self.acceptBy(proposer)
def acceptBy(self, decider):
"""See ISpecification."""
@@ -658,7 +667,7 @@
def _specification_sort(self, sort):
"""Return the storm sort order for 'specifications'.
-
+
:param sort: As per HasSpecificationsMixin.specifications.
"""
# sort by priority descending, by default
@@ -671,7 +680,7 @@
def _preload_specifications_people(self, query):
"""Perform eager loading of people and their validity for query.
-
+
:param query: a string query generated in the 'specifications'
method.
:return: A DecoratedResultSet with Person precaching setup.
=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py 2010-11-01 03:57:52 +0000
+++ lib/lp/blueprints/model/sprint.py 2010-11-24 12:23:35 +0000
@@ -45,9 +45,10 @@
from lp.blueprints.model.sprintattendance import SprintAttendance
from lp.blueprints.model.sprintspecification import SprintSpecification
from lp.registry.interfaces.person import validate_public_person
-
-
-class Sprint(SQLBase):
+from lp.registry.model.hasdrivers import HasDriversMixin
+
+
+class Sprint(SQLBase, HasDriversMixin):
"""See `ISprint`."""
implements(ISprint, IHasLogo, IHasMugshot, IHasIcon)
=== added file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py 1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/tests/test_specification.py 2010-11-24 12:23:35 +0000
@@ -0,0 +1,88 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+
+from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.blueprints.errors import TargetAlreadyHasSpecification
+from lp.blueprints.interfaces.specification import SpecificationGoalStatus
+from lp.testing import TestCaseWithFactory
+
+
+class SpecificationTests(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_auto_accept_of_goal_for_drivers(self):
+ """Drivers of a series accept the goal when they propose."""
+ product = self.factory.makeProduct()
+ proposer = self.factory.makePerson()
+ productseries = self.factory.makeProductSeries(product=product)
+ removeSecurityProxy(productseries).driver = proposer
+ specification = self.factory.makeSpecification(product=product)
+ specification.proposeGoal(productseries, proposer)
+ self.assertEqual(
+ SpecificationGoalStatus.ACCEPTED, specification.goalstatus)
+
+ def test_goal_not_accepted_for_non_drivers(self):
+ """People who aren't drivers don't have their proposals approved."""
+ product = self.factory.makeProduct()
+ proposer = self.factory.makePerson()
+ productseries = self.factory.makeProductSeries(product=product)
+ specification = self.factory.makeSpecification(product=product)
+ specification.proposeGoal(productseries, proposer)
+ self.assertEqual(
+ SpecificationGoalStatus.PROPOSED, specification.goalstatus)
+
+ def test_retarget_existing_specification(self):
+ """An error is raised if the name is already taken."""
+ product1 = self.factory.makeProduct()
+ product2 = self.factory.makeProduct()
+ specification1 = self.factory.makeSpecification(
+ product=product1, name="foo")
+ specification2 = self.factory.makeSpecification(
+ product=product2, name="foo")
+ self.assertRaises(
+ TargetAlreadyHasSpecification,
+ removeSecurityProxy(specification1).retarget, product2)
+
+ def test_retarget_is_protected(self):
+ specification = self.factory.makeSpecification(
+ product=self.factory.makeProduct())
+ self.assertRaises(
+ Unauthorized, getattr, specification, 'retarget')
+
+ def test_validate_move_existing_specification(self):
+ """An error is raised by validateMove if the name is already taken."""
+ product1 = self.factory.makeProduct()
+ product2 = self.factory.makeProduct()
+ specification1 = self.factory.makeSpecification(
+ product=product1, name="foo")
+ specification2 = self.factory.makeSpecification(
+ product=product2, name="foo")
+ self.assertRaises(
+ TargetAlreadyHasSpecification, specification1.validateMove,
+ product2)
+
+ def test_setTarget(self):
+ product = self.factory.makeProduct()
+ specification = self.factory.makeSpecification(product=product)
+ self.assertEqual(product, specification.target)
+ self.assertIs(None, specification.distribution)
+
+ distribution = self.factory.makeDistribution()
+ removeSecurityProxy(specification).setTarget(distribution)
+
+ self.assertEqual(distribution, specification.target)
+ self.assertEqual(distribution, specification.distribution)
+ self.assertIs(None, specification.product)
+
+ def test_setTarget_is_protected(self):
+ specification = self.factory.makeSpecification(
+ product=self.factory.makeProduct())
+ self.assertRaises(
+ Unauthorized, getattr, specification, 'setTarget')
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2010-11-12 23:30:57 +0000
+++ lib/lp/registry/model/distribution.py 2010-11-24 12:23:35 +0000
@@ -146,6 +146,7 @@
Milestone,
)
from lp.registry.model.pillar import HasAliasMixin
+from lp.registry.model.hasdrivers import HasDriversMixin
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.registry.model.structuralsubscription import (
StructuralSubscriptionTargetMixin,
@@ -201,7 +202,7 @@
HasTranslationImportsMixin, KarmaContextMixin,
OfficialBugTagTargetMixin, QuestionTargetMixin,
StructuralSubscriptionTargetMixin, HasMilestonesMixin,
- HasBugHeatMixin):
+ HasBugHeatMixin, HasDriversMixin):
"""A distribution of an operating system, e.g. Debian GNU/Linux."""
implements(
IDistribution, IFAQTarget, IHasBugHeat, IHasBugSupervisor,
=== added file 'lib/lp/registry/model/hasdrivers.py'
--- lib/lp/registry/model/hasdrivers.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/model/hasdrivers.py 2010-11-24 12:23:35 +0000
@@ -0,0 +1,25 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Common implementations for IHasDrivers."""
+
+__metaclass__ = type
+
+__all__ = [
+ 'HasDriversMixin',
+ ]
+
+from canonical.launchpad.interfaces.launchpad import IPersonRoles
+
+
+class HasDriversMixin:
+
+ # XXX: Dear reviewer, I don't quite like this name but the only other
+ # reasonable name I can think of is canBeDrivenBy(person).
+ # Suggestions appreciated.
+ def personHasDriverRights(self, person):
+ """See `IHasDrivers`."""
+ person_roles = IPersonRoles(person)
+ return (person_roles.isOneOfDrivers(self) or
+ person_roles.isOwner(self) or
+ person_roles.in_admin)
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py 2010-11-19 17:27:35 +0000
+++ lib/lp/registry/model/product.py 2010-11-24 12:23:35 +0000
@@ -156,6 +156,7 @@
from lp.registry.model.productlicense import ProductLicense
from lp.registry.model.productrelease import ProductRelease
from lp.registry.model.productseries import ProductSeries
+from lp.registry.model.hasdrivers import HasDriversMixin
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.registry.model.structuralsubscription import (
StructuralSubscriptionTargetMixin,
@@ -286,7 +287,7 @@
class Product(SQLBase, BugTargetBase, MakesAnnouncements,
- HasSpecificationsMixin, HasSprintsMixin,
+ HasDriversMixin, HasSpecificationsMixin, HasSprintsMixin,
KarmaContextMixin, BranchVisibilityPolicyMixin,
QuestionTargetMixin, HasTranslationImportsMixin,
HasAliasMixin, StructuralSubscriptionTargetMixin,
=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py 2010-11-09 08:43:34 +0000
+++ lib/lp/registry/model/projectgroup.py 2010-11-24 12:23:35 +0000
@@ -99,6 +99,7 @@
from lp.registry.model.pillar import HasAliasMixin
from lp.registry.model.product import Product
from lp.registry.model.productseries import ProductSeries
+from lp.registry.model.hasdrivers import HasDriversMixin
from lp.registry.model.structuralsubscription import (
StructuralSubscriptionTargetMixin,
)
@@ -111,7 +112,7 @@
KarmaContextMixin, BranchVisibilityPolicyMixin,
StructuralSubscriptionTargetMixin,
HasBranchesMixin, HasMergeProposalsMixin, HasBugHeatMixin,
- HasMilestonesMixin):
+ HasMilestonesMixin, HasDriversMixin):
"""A ProjectGroup"""
implements(IProjectGroup, IFAQCollection, IHasBugHeat, IHasIcon, IHasLogo,
=== modified file 'lib/lp/registry/model/series.py'
--- lib/lp/registry/model/series.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/series.py 2010-11-24 12:23:35 +0000
@@ -18,9 +18,10 @@
ISeriesMixin,
SeriesStatus,
)
-
-
-class SeriesMixin:
+from lp.registry.model.hasdrivers import HasDriversMixin
+
+
+class SeriesMixin(HasDriversMixin):
"""See `ISeriesMixin`."""
implements(ISeriesMixin)
@@ -48,7 +49,7 @@
@property
def drivers(self):
- """See `ISeriesMixin`."""
+ """See `IHasDrivers`."""
drivers = set()
drivers.add(self.driver)
drivers = drivers.union(self.parent.drivers)
