Merge lp:~ttx/launchpad/lp1193389 into lp:launchpad

[Thierry Carrez]

Expose specification goal management in specification API

- Expose proposeGoal, acceptGoal, declineGoal methods and has_accepted_goal attribute
- Under wgrant advice, remove lp.blueprints.subscribers.specification_goalstatus as it's not used and its security model blocks proxied objects
- Add unit tests

Expose specification goal management in specification API

- Expose proposeGoal, acceptGoal, declineGoal methods and has_accepted_goal attribute
- Under wgrant advice, remove lp.blueprints.subscribers.specification_goalstatus as it's not used and its security model blocks proxied objects
- Add unit tests

[William Grant (wgrant)]

40 + @operation_parameters(
41 + goal=Reference(schema=IBugTarget, title=_('Target'),
42 + required=False, default=None))

That should probably be something like Reference(schema=ISpecificationTarget, title=_('Series goal')). The indentation is also confusing; I'd say:

@operation_parameters(
    goal=Reference(
        schema=ISpecificationTarget, title=_('Series goal'),
        required=False, default=None))

Additionally, proposeGoal will currently crash with an AssertionError if you pass a goal that isn't a series. You'll need to replace that with a custom exception decorated with @error_status(httplib.BAD_REQUEST) to return a 400 Bad Request rather than an OOPS, as is done with NominationError in Bug.addNomination.

121 + def test_goal_propose_and_accept(self):
122 + # Create spec

We prefer that tests start with a short comment describing the behaviour that they're testing. I'd perhaps replace "Create spec" with "Webservice clients can propose and accept spec series goals."

review: Needs Fixing (code)

[Thierry Carrez (ttx)]

Looks like ISpecificationTarget is not a series (DistroSeries or ProductSeries), but a 'DistributionOrProduct', i.e. a project... so it doesn't sound like a good match for the series goal ? Will fix the other stuff.

[Colin Watson (cjwatson)]

Doesn't this branch still permit anyone who can view the specification to call proposeGoal/acceptGoal/declineGoal, even if they would not be able to use the corresponding web UI facilities? In lib/lp/blueprints/browser/configure.zcml, the views that expose proposeGoal require launchpad.Edit, while the view that exposes acceptBy/declineBy requires launchpad.Driver; but the interfaces here are only protected by launchpad.LimitedView, which is much broader.

[William Grant (wgrant)]

Thanks for the fixes, and your good point about ISpecificationTarget being inappropriate.

However, we'll need to add a further check in proposeGoal: the series' product/distribution must match the target of the blueprint. This is presently enforced by the FilteredProductSeries/FilteredDistroSeries vocabs on the browser fields, but they can't easily be reused at this level of the model. Manual validation of the target match is easy, and you can reuse the exception.

19 @@ -11,12 +11,16 @@
20 'ISpecificationPublic',
21 'ISpecificationSet',
22 'ISpecificationView',
23 + 'GoalProposeError',
24 ]

This should be sorted alphabetically.

101 === modified file 'lib/lp/blueprints/interfaces/webservice.py'

Two bad orderings here too.

=== modified file 'lib/lp/blueprints/model/specification.py'

And one here.

[Thierry Carrez (ttx)]

William: Blame my lexical-order-dyslexia. Will fix.

Colin: if William confirms this is a flaw and you all give me a bit more detail on how to fix that, I'll be happy to plug that one. The LP perms model is still a bit fuzzy to me :)

=== modified file 'lib/lp/blueprints/configure.zcml'
--- lib/lp/blueprints/configure.zcml	2012-09-17 18:55:17 +0000
+++ lib/lp/blueprints/configure.zcml	2013-06-27 15:14:30 +0000
@@ -204,10 +204,6 @@
   <subscriber
       for="lp.blueprints.interfaces.specification.ISpecification
            lazr.lifecycle.interfaces.IObjectModifiedEvent"
-      handler="lp.blueprints.subscribers.specification_goalstatus"/>
-  <subscriber
-      for="lp.blueprints.interfaces.specification.ISpecification
-           lazr.lifecycle.interfaces.IObjectModifiedEvent"
       handler="lp.blueprints.mail.notifications.notify_specification_modified"/>
 
   <!-- SpecificationSet -->

=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py	2013-04-11 00:51:46 +0000
+++ lib/lp/blueprints/interfaces/specification.py	2013-06-27 15:14:30 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 __all__ = [
+    'GoalProposeError',
     'ISpecification',
     'ISpecificationDelta',
     'ISpecificationPublic',
@@ -13,10 +14,13 @@
     'ISpecificationView',
     ]
 
+import httplib
 
 from lazr.restful.declarations import (
     call_with,
+    error_status,
     export_as_webservice_entry,
+    export_operation_as,
     export_write_operation,
     exported,
     mutator_for,
@@ -70,6 +74,7 @@
     )
 from lp.blueprints.interfaces.sprint import ISprint
 from lp.bugs.interfaces.buglink import IBugLinkTarget
+from lp.bugs.interfaces.bugtarget import IBugTarget
 from lp.code.interfaces.branchlink import IHasLinkedBranches
 from lp.registry.interfaces.milestone import IMilestone
 from lp.registry.interfaces.person import IPerson
@@ -86,6 +91,11 @@
 from lp.services.webapp.escaping import structured
 
 
+@error_status(httplib.BAD_REQUEST)
+class GoalProposeError(Exception):
+    """Invalid series goal for this specification."""
+
+
 class SpecNameField(ContentNameField):
 
     errormessage = _("%s is already in use by another blueprint.")
@@ -476,20 +486,40 @@
         """Return the list of email addresses that receive notifications."""
 
     # goal management
+    @call_with(proposer=REQUEST_USER)
+    @operation_parameters(
+        goal=Reference(
+            schema=IBugTarget, title=_('Target'),
+            required=False, default=None))
+    @export_write_operation()
+    @operation_for_version("devel")
     def proposeGoal(goal, proposer):
         """Propose this spec for a series or distroseries."""
 
+    @call_with(decider=REQUEST_USER)
+    @export_operation_as('acceptGoal')
+    @export_write_operation()
+    @operation_for_version("devel")
     def acceptBy(decider):
         """Mark the spec as being accepted for its current series goal."""
 
+    @call_with(decider=REQUEST_USER)
+    @export_operation_as('declineGoal')
+    @export_write_operation()
+    @operation_for_version("devel")
     def declineBy(decider):
         """Mark the spec as being declined as a goal for the proposed
         series.
         """
 
-    has_accepted_goal = Attribute('Is true if this specification has been '
-        'proposed as a goal for a specific series, '
-        'and the drivers of that series have accepted the goal.')
+    has_accepted_goal = exported(
+        Bool(title=_('Series goal is accepted'),
+             readonly=True, required=True,
+             description=_(
+                'Is true if this specification has been '
+                'proposed as a goal for a specific series, '
+                'and the drivers of that series have accepted the goal.')),
+        as_of="devel")
 
     # lifecycle management
     def updateLifecycleStatus(user):

=== modified file 'lib/lp/blueprints/interfaces/webservice.py'
--- lib/lp/blueprints/interfaces/webservice.py	2013-01-03 00:27:37 +0000
+++ lib/lp/blueprints/interfaces/webservice.py	2013-06-27 15:14:30 +0000
@@ -10,6 +10,7 @@
 """
 
 __all__ = [
+    'GoalProposeError',
     'ISpecification',
     'ISpecificationBranch',
     'ISpecificationSubscription',
@@ -18,7 +19,10 @@
 # XXX: JonathanLange 2010-11-09 bug=673083: Legacy work-around for circular
 # import bugs.  Break this up into a per-package thing.
 from lp import _schema_circular_imports
-from lp.blueprints.interfaces.specification import ISpecification
+from lp.blueprints.interfaces.specification import (
+    GoalProposeError,
+    ISpecification,
+    )
 from lp.blueprints.interfaces.specificationbranch import ISpecificationBranch
 from lp.blueprints.interfaces.specificationsubscription import (
     ISpecificationSubscription,

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2013-06-20 05:50:00 +0000
+++ lib/lp/blueprints/model/specification.py	2013-06-27 15:14:30 +0000
@@ -57,6 +57,7 @@
     )
 from lp.blueprints.errors import TargetAlreadyHasSpecification
 from lp.blueprints.interfaces.specification import (
+    GoalProposeError,
     ISpecification,
     ISpecificationSet,
     )
@@ -506,14 +507,16 @@
             # we are clearing goals
             self.productseries = None
             self.distroseries = None
-        elif IProductSeries.providedBy(goal):
+        elif (IProductSeries.providedBy(goal) and
+              goal.product == self.target):
             # set the product series as a goal
             self.productseries = goal
             self.goal_proposer = proposer
             self.date_goal_proposed = UTC_NOW
             # and make sure there is no leftover distroseries goal
             self.distroseries = None
-        elif IDistroSeries.providedBy(goal):
+        elif (IDistroSeries.providedBy(goal) and
+              goal.distribution == self.target):
             # set the distroseries goal
             self.distroseries = goal
             self.goal_proposer = proposer
@@ -521,7 +524,7 @@
             # and make sure there is no leftover distroseries goal
             self.productseries = None
         else:
-            raise AssertionError('Inappropriate goal.')
+            raise GoalProposeError('Inappropriate goal.')
         # record who made the proposal, and when
         self.goal_proposer = proposer
         self.date_goal_proposed = UTC_NOW

=== modified file 'lib/lp/blueprints/subscribers.py'
--- lib/lp/blueprints/subscribers.py	2011-12-30 06:14:56 +0000
+++ lib/lp/blueprints/subscribers.py	2013-06-27 15:14:30 +0000
@@ -9,17 +9,6 @@
 from lp.services.database.sqlbase import block_implicit_flushes
 
 
-@block_implicit_flushes
-def specification_goalstatus(spec, event):
-    """Update goalstatus if productseries or distroseries is changed."""
-    delta = spec.getDelta(
-        event.object_before_modification, IPerson(event.user))
-    if delta is None:
-        return
-    if delta.productseries is not None or delta.distroseries is not None:
-        spec.goalstatus = SpecificationGoalStatus.PROPOSED
-
-
 def specification_update_lifecycle_status(spec, event):
     """Mark the specification as started and/or complete if appropriate.
 

=== modified file 'lib/lp/blueprints/tests/test_webservice.py'
--- lib/lp/blueprints/tests/test_webservice.py	2013-01-25 03:30:08 +0000
+++ lib/lp/blueprints/tests/test_webservice.py	2013-06-27 15:14:30 +0000
@@ -401,3 +401,65 @@
 
         # Now that we've unlinked the bug, there are no linked bugs at all.
         self.assertEqual(0, spec.bugs.total_size)
+
+
+class TestSpecificationGoalHandling(SpecificationWebserviceTestCase):
+
+    layer = AppServerLayer
+
+    def setUp(self):
+        super(TestSpecificationGoalHandling, self).setUp()
+        self.driver = self.factory.makePerson()
+        self.proposer = self.factory.makePerson()
+        self.product = self.factory.makeProduct(driver=self.driver)
+        self.series = self.factory.makeProductSeries(product=self.product)
+
+    def test_goal_propose_and_accept(self):
+        # Webservice clients can propose and accept spec series goals.
+        db_spec = self.factory.makeBlueprint(product=self.product,
+                                             owner=self.proposer)
+        # Propose for series goal
+        with person_logged_in(self.proposer):
+            launchpad = self.factory.makeLaunchpadService(person=self.proposer)
+            spec = ws_object(launchpad, db_spec)
+            series = ws_object(launchpad, self.series)
+            spec.proposeGoal(goal=series)
+            transaction.commit()
+            self.assertEqual(db_spec.goal, self.series)
+            self.assertFalse(spec.has_accepted_goal)
+
+        # Accept series goal
+        with person_logged_in(self.driver):
+            launchpad = self.factory.makeLaunchpadService(person=self.driver)
+            spec = ws_object(launchpad, db_spec)
+            spec.acceptGoal()
+            transaction.commit()
+            self.assertTrue(spec.has_accepted_goal)
+
+    def test_goal_propose_decline_and_clear(self):
+        # Webservice clients can decline and clear spec series goals.
+        db_spec = self.factory.makeBlueprint(product=self.product,
+                                             owner=self.proposer)
+        # Propose for series goal
+        with person_logged_in(self.proposer):
+            launchpad = self.factory.makeLaunchpadService(person=self.proposer)
+            spec = ws_object(launchpad, db_spec)
+            series = ws_object(launchpad, self.series)
+            spec.proposeGoal(goal=series)
+            transaction.commit()
+            self.assertEqual(db_spec.goal, self.series)
+            self.assertFalse(spec.has_accepted_goal)
+
+        with person_logged_in(self.driver):
+            # Decline series goal
+            launchpad = self.factory.makeLaunchpadService(person=self.driver)
+            spec = ws_object(launchpad, db_spec)
+            spec.declineGoal()
+            transaction.commit()
+            self.assertFalse(spec.has_accepted_goal)
+            self.assertEqual(db_spec.goal, self.series)
+
+            # Clear series goal as a driver
+            spec.proposeGoal(goal=None)
+            transaction.commit()
+            self.assertIsNone(db_spec.goal)