launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/blueprint-moderation-api into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/blueprint-moderation-api/+merge/245626

Let ~registry edit all specifications, and allow target and information_type to be set through the API.

Specs were the last major type of object that can be anonymously created but not moderated by staff without resorting to sysadmins.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/blueprint-moderation-api into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py	2014-06-17 06:13:08 +0000
+++ lib/lp/blueprints/browser/specification.py	2015-01-06 07:28:24 +0000
@@ -978,9 +978,14 @@
     cancel_url = next_url
 
 
+class ISpecificationRetargetingSchema(Interface):
+
+    target = copy_field(ISpecification['target'], readonly=False)
+
+
 class SpecificationRetargetingView(LaunchpadFormView):
 
-    schema = ISpecification
+    schema = ISpecificationRetargetingSchema
     field_names = ['target']
     label = _('Move this blueprint to a different project')
 

=== modified file 'lib/lp/blueprints/errors.py'
--- lib/lp/blueprints/errors.py	2010-10-19 00:35:05 +0000
+++ lib/lp/blueprints/errors.py	2015-01-06 07:28:24 +0000
@@ -9,11 +9,16 @@
     'TargetAlreadyHasSpecification',
     ]
 
-
+import httplib
+
+from lazr.restful.declarations import error_status
+
+
+@error_status(httplib.BAD_REQUEST)
 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)
+        msg = "There is already a blueprint named %s for %s." % (
+                name, target.displayname)
         super(TargetAlreadyHasSpecification, self).__init__(msg)

=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py	2014-06-19 02:12:50 +0000
+++ lib/lp/blueprints/interfaces/specification.py	2015-01-06 07:28:24 +0000
@@ -276,13 +276,12 @@
     # using setTarget() as the mutator.
     target = exported(
         ReferenceChoice(
-            title=_('For'), required=True, vocabulary='DistributionOrProduct',
+            title=_('For'), required=True, readonly=True,
+            vocabulary='DistributionOrProduct',
             description=_(
                 "The project for which this proposal is being made."),
             schema=ISpecificationTarget),
-        as_of="devel",
-        readonly=True,
-        )
+        as_of="devel")
 
     productseries = Choice(
         title=_('Series Goal'), required=False,
@@ -635,12 +634,23 @@
         :param target: an IProduct or IDistribution.
         """
 
+    @mutator_for(ISpecificationView['target'])
+    @operation_parameters(
+        target=copy_field(ISpecificationView['target']))
+    @export_write_operation()
+    @operation_for_version('devel')
     def retarget(target):
         """Move the spec to the given target.
 
         The new target must be an IProduct or IDistribution.
         """
 
+    @mutator_for(ISpecificationPublic['information_type'])
+    @call_with(who=REQUEST_USER)
+    @operation_parameters(
+        information_type=copy_field(ISpecificationPublic['information_type']))
+    @export_write_operation()
+    @operation_for_version('devel')
     def transitionToInformationType(information_type, who):
         """Change the information type of the Specification."""
 

=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py	2014-06-19 10:04:55 +0000
+++ lib/lp/blueprints/tests/test_specification.py	2015-01-06 07:28:24 +0000
@@ -53,6 +53,7 @@
     IAccessArtifactGrantSource,
     IAccessPolicySource,
     )
+from lp.registry.interfaces.person import IPersonSet
 from lp.security import (
     AdminSpecification,
     EditSpecificationByRelatedPeople,
@@ -414,6 +415,27 @@
                 specification.target.owner, specification,
                 error_expected=False, attribute='name', value='foo')
 
+    def test_registry_write_access(self):
+        # Users with special privileges can change the attributes
+        # of public and private specifcations.
+        specification = self.factory.makeSpecification()
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
+        all_types = specification.getAllowedInformationTypes(
+            specification.owner)
+        expert = self.factory.makePerson(
+            member_of=[getUtility(IPersonSet).getByName('registry')])
+        for information_type in all_types:
+            with person_logged_in(specification.target.owner):
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
+            self.write_access_to_ISpecificationView(
+                expert, specification, error_expected=False,
+                attribute='whiteboard', value='foo')
+            self.write_access_to_ISpecificationView(
+                expert, specification, error_expected=False,
+                attribute='name', value='foo')
+
     def _fetch_specs_visible_for_user(self, user):
         return Store.of(self.product).find(
             Specification,

=== modified file 'lib/lp/blueprints/tests/test_webservice.py'
--- lib/lp/blueprints/tests/test_webservice.py	2014-06-19 01:21:06 +0000
+++ lib/lp/blueprints/tests/test_webservice.py	2015-01-06 07:28:24 +0000
@@ -5,15 +5,20 @@
 
 __metaclass__ = type
 
+import json
+
+from testtools.matchers import MatchesStructure
 import transaction
 from zope.security.management import endInteraction
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import InformationType
 from lp.blueprints.enums import SpecificationDefinitionStatus
+from lp.registry.enums import SpecificationSharingPolicy
 from lp.services.webapp.interaction import ANONYMOUS
 from lp.services.webapp.interfaces import OAuthPermission
-from lp.registry.enums import SpecificationSharingPolicy
 from lp.testing import (
+    admin_logged_in,
     api_url,
     launchpadlib_for,
     person_logged_in,
@@ -144,11 +149,23 @@
         self.assertEqual(response.status, 200)
         self.assertContentEqual(expected_keys, response.jsonBody().keys())
 
-    def test_representation_contains_name(self):
+    def test_representation_basics(self):
         spec = self.factory.makeSpecification()
-        spec_name = spec.name
         spec_webservice = self.getSpecOnWebservice(spec)
-        self.assertEqual(spec_name, spec_webservice.name)
+        with person_logged_in(ANONYMOUS):
+            self.assertThat(
+                spec_webservice,
+                MatchesStructure.byEquality(
+                    name=spec.name,
+                    title=spec.title,
+                    specification_url=spec.specurl,
+                    summary=spec.summary,
+                    implementation_status=spec.implementation_status.title,
+                    definition_status=spec.definition_status.title,
+                    priority=spec.priority.title,
+                    date_created=spec.datecreated,
+                    whiteboard=spec.whiteboard,
+                    workitems_text=spec.workitems_text))
 
     def test_representation_contains_target(self):
         spec = self.factory.makeSpecification(
@@ -157,39 +174,6 @@
         spec_webservice = self.getSpecOnWebservice(spec)
         self.assertEqual(spec_target_name, spec_webservice.target.name)
 
-    def test_representation_contains_title(self):
-        spec = self.factory.makeSpecification(title='Foo')
-        spec_title = spec.title
-        spec_webservice = self.getSpecOnWebservice(spec)
-        self.assertEqual(spec_title, spec_webservice.title)
-
-    def test_representation_contains_specification_url(self):
-        spec = self.factory.makeSpecification(specurl='http://example.com')
-        spec_specurl = spec.specurl
-        spec_webservice = self.getSpecOnWebservice(spec)
-        self.assertEqual(spec_specurl, spec_webservice.specification_url)
-
-    def test_representation_contains_summary(self):
-        spec = self.factory.makeSpecification(summary='Foo')
-        spec_summary = spec.summary
-        spec_webservice = self.getSpecOnWebservice(spec)
-        self.assertEqual(spec_summary, spec_webservice.summary)
-
-    def test_representation_contains_implementation_status(self):
-        spec = self.factory.makeSpecification()
-        spec_implementation_status = spec.implementation_status
-        spec_webservice = self.getSpecOnWebservice(spec)
-        self.assertEqual(
-            spec_implementation_status.title,
-            spec_webservice.implementation_status)
-
-    def test_representation_contains_definition_status(self):
-        spec = self.factory.makeSpecification()
-        spec_definition_status = spec.definition_status
-        spec_webservice = self.getSpecOnWebservice(spec)
-        self.assertEqual(
-            spec_definition_status.title, spec_webservice.definition_status)
-
     def test_representation_contains_assignee(self):
         # Hard-code the person's name or else we'd need to set up a zope
         # interaction as IPerson.name is protected.
@@ -216,30 +200,6 @@
         spec_webservice = self.getSpecOnWebservice(spec)
         self.assertEqual('test-person', spec_webservice.owner.name)
 
-    def test_representation_contains_priority(self):
-        spec = self.factory.makeSpecification()
-        spec_priority = spec.priority
-        spec_webservice = self.getSpecOnWebservice(spec)
-        self.assertEqual(spec_priority.title, spec_webservice.priority)
-
-    def test_representation_contains_date_created(self):
-        spec = self.factory.makeSpecification()
-        spec_datecreated = spec.datecreated
-        spec_webservice = self.getSpecOnWebservice(spec)
-        self.assertEqual(spec_datecreated, spec_webservice.date_created)
-
-    def test_representation_contains_whiteboard(self):
-        spec = self.factory.makeSpecification(whiteboard='Test')
-        spec_whiteboard = spec.whiteboard
-        spec_webservice = self.getSpecOnWebservice(spec)
-        self.assertEqual(spec_whiteboard, spec_webservice.whiteboard)
-
-    def test_representation_contains_workitems(self):
-        work_item = self.factory.makeSpecificationWorkItem()
-        spec_webservice = self.getSpecOnWebservice(work_item.specification)
-        self.assertEqual(work_item.specification.workitems_text,
-                         spec_webservice.workitems_text)
-
     def test_representation_contains_milestone(self):
         product = self.factory.makeProduct()
         productseries = self.factory.makeProductSeries(product=product)
@@ -278,6 +238,55 @@
         self.assertEqual(bug.id, spec_webservice.bugs[0].id)
 
 
+class SpecificationMutationTests(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_set_information_type(self):
+        product = self.factory.makeProduct(
+            specification_sharing_policy=(
+                SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY))
+        spec = self.factory.makeSpecification(product=product)
+        self.assertEqual(InformationType.PUBLIC, spec.information_type)
+        spec_url = api_url(spec)
+        webservice = webservice_for_person(
+            product.owner, permission=OAuthPermission.WRITE_PRIVATE)
+        response = webservice.patch(
+            spec_url, "application/json",
+            json.dumps(dict(information_type='Proprietary')),
+            api_version='devel')
+        self.assertEqual(209, response.status)
+        with admin_logged_in():
+            self.assertEqual(
+                InformationType.PROPRIETARY, spec.information_type)
+
+    def test_set_target(self):
+        old_target = self.factory.makeProduct()
+        spec = self.factory.makeSpecification(product=old_target, name='foo')
+        new_target = self.factory.makeProduct(displayname='Fooix')
+        spec_url = api_url(spec)
+        new_target_url = api_url(new_target)
+        webservice = webservice_for_person(
+            old_target.owner, permission=OAuthPermission.WRITE_PRIVATE)
+        response = webservice.patch(
+            spec_url, "application/json",
+            json.dumps(dict(target_link=new_target_url)), api_version='devel')
+        self.assertEqual(301, response.status)
+        with admin_logged_in():
+            self.assertEqual(new_target, spec.target)
+
+            # Moving another spec with the same name fails.
+            other_spec = self.factory.makeSpecification(
+                product=old_target, name='foo')
+            other_spec_url = api_url(other_spec)
+        response = webservice.patch(
+            other_spec_url, "application/json",
+            json.dumps(dict(target_link=new_target_url)), api_version='devel')
+        self.assertEqual(400, response.status)
+        self.assertEqual(
+            "There is already a blueprint named foo for Fooix.", response.body)
+
+
 class SpecificationTargetTests(SpecificationWebserviceTestCase):
     """Tests for accessing specifications via their targets."""
     layer = AppServerLayer

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2014-07-07 03:43:30 +0000
+++ lib/lp/security.py	2015-01-06 07:28:24 +0000
@@ -593,6 +593,7 @@
             if user.isOwner(goal) or user.isDriver(goal):
                 return True
         return (user.in_admin or
+                user.in_registry_experts or
                 user.isOwner(self.obj.target) or
                 user.isDriver(self.obj.target) or
                 user.isOneOf(
@@ -605,9 +606,11 @@
 
     def checkAuthenticated(self, user):
         assert self.obj.target
-        return (user.isOwner(self.obj.target) or
-                user.isDriver(self.obj.target) or
-                user.in_admin)
+        return (
+                user.in_admin or
+                user.in_registry_experts or
+                user.isOwner(self.obj.target) or
+                user.isDriver(self.obj.target))
 
 
 class DriverSpecification(AuthorizationBase):