launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Richard Harding has proposed merging lp:~rharding/launchpad/bp_validate_type_1062198 into lp:launchpad.

Commit message:
Add validation of the information type to addspec views.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1062198 in Launchpad itself: "OOPS changing the InformationType of a blueprint"
  https://bugs.launchpad.net/launchpad/+bug/1062198

For more details, see:
https://code.launchpad.net/~rharding/launchpad/bp_validate_type_1062198/+merge/129270

= Summary =

The information type isn't validated to be a valid value in all views creating
new blueprints. This branch adds some sanity checking and creates validation
errors if it find the value selected isn't valid.


== Implementation Notes ==

There are a lot of ways to create a blueprint and several rules to know about
when trying to determine if validation should occur.

1) Validation should generally be done against the current context if
possible.

2) Not all are possible. Some views are from non-target and you have to check
the information type against values allowed from the target.

3) If there is only one information type value allowed, the form field just
isn't shown and is empty.

To add this in we add form validation and only validate if the
information_type is specified. This auto works with the feature flag since the
value is missing if the flag isn't set, but also works for the case where only
one value is permitted and the form field isn't shown.

The first test_allowed_info_type_validated method is written out because for
the Root case, the view is +new vs +addspec like the rest. So there's some
dupe code there.

Finally, there's a drive by lint fix of a trailing space I hit during
debugging things by looking at the factory code.

== Q/A ==

Check out the linked bug. If you go to create a new blueprint from a sprint,
select launchpad as the project, and verify you get a validation error if you
select Proprietary as the information type.

== Tests ==

test_specification.py



-- 
https://code.launchpad.net/~rharding/launchpad/bp_validate_type_1062198/+merge/129270
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/bp_validate_type_1062198 into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py	2012-10-10 16:57:06 +0000
+++ lib/lp/blueprints/browser/specification.py	2012-10-12 17:28:20 +0000
@@ -221,7 +221,6 @@
     label = "Register a new blueprint"
 
     custom_widget('specurl', TextWidget, displayWidth=60)
-
     custom_widget('information_type', LaunchpadRadioWidgetWithDescription)
 
     def append_info_type(self, fields):
@@ -320,6 +319,35 @@
         values = {'information_type': information_type}
         return values
 
+    def validate(self, data):
+        """See `LaunchpadFormView`.`"""
+        super(NewSpecificationView, self).validate(data)
+        self.validate_information_type(data)
+
+    def validate_information_type(self, data):
+        """Validate the information type is allowed for this context."""
+        information_type = data.get('information_type', None)
+        if information_type is None:
+            # We rely on the model to set the correct default value.
+            return
+        else:
+            # In the case of views outside a target context it's part of the
+            # form.
+            product = IProduct(data.get('target', None), None)
+
+            if (IProduct.providedBy(self.context) or
+                IProductSeries.providedBy(self.context)):
+                product = self.context
+
+            if product:
+                allowed = product.getAllowedSpecificationInformationTypes()
+                # Check that the information type is a valid one for this
+                # Product.
+                if information_type not in allowed:
+                    error = ('This information type is not permitted for '
+                             'this product')
+                    self.setFieldError('information_type', error)
+
 
 class NewSpecificationFromTargetView(NewSpecificationView):
     """An abstract view for creating a specification from a target context.
@@ -399,6 +427,7 @@
 
         The name must be unique within the context of the chosen target.
         """
+        super(NewSpecificationFromNonTargetView, self).validate(data)
         name = data.get('name')
         target = data.get('target')
         if name is not None and target is not None:

=== modified file 'lib/lp/blueprints/browser/tests/test_specification.py'
--- lib/lp/blueprints/browser/tests/test_specification.py	2012-10-11 12:41:43 +0000
+++ lib/lp/blueprints/browser/tests/test_specification.py	2012-10-12 17:28:20 +0000
@@ -345,6 +345,24 @@
 
     expected_keys = set(['PROPRIETARY', 'PUBLIC', 'EMBARGOED'])
 
+    def _create_form_data(self, context):
+        return {
+            'field.actions.register': 'Register Blueprint',
+            'field.definition_status': 'NEW',
+            'field.target': context,
+            'field.name': 'TestBlueprint',
+            'field.title': 'Test Blueprint',
+            'field.summary': 'Test Blueprint Summary',
+        }
+
+    def _assert_information_type_validation_error(self, context, form, owner):
+        """Helper to check for invalid information type on submit."""
+        with person_logged_in(owner):
+            view = create_initialized_view(context, '+addspec', form=form)
+            expected = (u'This information type is not permitted for'
+                        u' this product')
+            self.assertIn(expected, view.errors)
+
     def test_cache_contains_information_type(self):
         view = self.createInitializedView()
         cache = IJSONRequestCache(view.request)
@@ -367,8 +385,19 @@
     layer = DatabaseFunctionalLayer
 
     def createInitializedView(self):
-        specs = getUtility(ISpecificationSet)
-        return create_initialized_view(specs, '+new')
+        context = getUtility(ISpecificationSet)
+        return create_initialized_view(context, '+new')
+
+    def test_allowed_info_type_validated(self):
+        """information_type must be validated against context"""
+        set_blueprint_information_type(self, True)
+        context = getUtility(ISpecificationSet)
+        product = self.factory.makeProduct()
+        form = self._create_form_data(product.name)
+        form['field.information_type'] = 'PROPRIETARY'
+        view = create_initialized_view(context, '+new', form=form)
+        expected = u'This information type is not permitted for this product'
+        self.assertIn(expected, view.errors)
 
 
 class TestNewSpecificationFromSprintView(TestCaseWithFactory,
@@ -380,6 +409,18 @@
         sprint = self.factory.makeSprint()
         return create_initialized_view(sprint, '+addspec')
 
+    def test_allowed_info_type_validated(self):
+        """information_type must be validated against context"""
+        set_blueprint_information_type(self, True)
+        sprint = self.factory.makeSprint()
+        product = self.factory.makeProduct(owner=sprint.owner)
+        form = self._create_form_data(product.name)
+        form['field.information_type'] = 'PROPRIETARY'
+        self._assert_information_type_validation_error(
+            sprint,
+            form,
+            sprint.owner)
+
 
 class TestNewSpecificationFromProjectView(TestCaseWithFactory,
                                           NewSpecificationTests):
@@ -390,6 +431,18 @@
         project = self.factory.makeProject()
         return create_initialized_view(project, '+addspec')
 
+    def test_allowed_info_type_validated(self):
+        """information_type must be validated against context"""
+        set_blueprint_information_type(self, True)
+        project = self.factory.makeProject()
+        product = self.factory.makeProduct(project=project)
+        form = self._create_form_data(product.name)
+        form['field.information_type'] = 'PROPRIETARY'
+        self._assert_information_type_validation_error(
+            project,
+            form,
+            project.owner)
+
 
 class TestNewSpecificationFromProductView(TestCaseWithFactory,
                                           NewSpecificationTests):

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-10-11 04:21:07 +0000
+++ lib/lp/testing/factory.py	2012-10-12 17:28:20 +0000
@@ -2234,7 +2234,7 @@
 
     def makeCodeImport(self, svn_branch_url=None, cvs_root=None,
                        cvs_module=None, target=None, branch_name=None,
-                       git_repo_url=None, 
+                       git_repo_url=None,
                        bzr_branch_url=None, registrant=None,
                        rcs_type=None, review_status=None):
         """Create and return a new, arbitrary code import.