launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/productseries-branch-infotype-mismatch into lp:launchpad.

Commit message:
Sets a validation on branch_location in +setbranch to ensure information types match

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1078011 in Launchpad itself: "Product series and series branch information types can mismatch"
  https://bugs.launchpad.net/launchpad/+bug/1078011

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/productseries-branch-infotype-mismatch/+merge/134250

Summary
=======
It's possible to set a branch that doesn't match the information type of a productseries as the series branch; this can lead to all manner of awkwardness and shouldn't be allowed.

Preimp
======
Aaron Bentley

Implementation
==============
A check is made on the branch info type in the validate method for the LP-Branch branch type; if the information_types don't match, an error is set on the field.

Tests
=====
bin/test -vvct test_set_branch_mismatched_info_type

QA
==
Try to set a branch that has an information type that mismatches with the productseries as the series branch (do not use the picker). The form should fail and provide an error message.

LoC
===
Part of private projects

Lint
====
This branch is lint free.
-- 
https://code.launchpad.net/~jcsackett/launchpad/productseries-branch-infotype-mismatch/+merge/134250
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/productseries-branch-infotype-mismatch into lp:launchpad.

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2012-11-08 04:22:30 +0000
+++ lib/lp/registry/browser/productseries.py	2012-11-14 17:21:21 +0000
@@ -885,6 +885,13 @@
         if 'branch_location' not in data:
             self.setFieldError(
                 'branch_location', 'The branch location must be set.')
+        branch = data.get('branch_location')
+        if (branch.information_type !=
+            self.context.product.information_type):
+            error_msg = ("A %s product series cannot have a %s series branch." 
+                % (self.context.product.information_type.title.lower(),
+                   branch.information_type.title.lower()))
+            self.setFieldError('branch_type', error_msg)
 
     def _validateImportExternal(self, data):
         """Validate data for import external case."""
@@ -995,14 +1002,13 @@
     def update_action(self, action, data):
         branch_type = data.get('branch_type')
         if branch_type == LINK_LP_BZR:
-            branch_location = data.get('branch_location')
-            if branch_location != self.context.branch:
-                self.context.branch = branch_location
+            branch = data.get('branch_location')
+            if branch != self.context.branch:
                 # Request an initial upload of translation files.
                 getUtility(IRosettaUploadJobSource).create(
                     self.context.branch, NULL_REVISION)
-            else:
-                self.context.branch = branch_location
+           
+            self.context.branch = branch
             self.request.response.addInfoNotification(
                 'Series code location updated.')
         else:

=== modified file 'lib/lp/registry/browser/tests/test_productseries_views.py'
--- lib/lp/registry/browser/tests/test_productseries_views.py	2012-10-18 17:16:49 +0000
+++ lib/lp/registry/browser/tests/test_productseries_views.py	2012-11-14 17:21:21 +0000
@@ -74,7 +74,34 @@
             privacy_portlet_proprietary))
 
 
+class TestProductSeriesSetBranchView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_set_branch_mismatched_info_type(self):
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        productseries = self.factory.makeProductSeries(product=product)
+        branch = self.factory.makeBranch(product=product, owner=owner)
+        branch_name = '~%s/%s/%s' % (owner.name, product.name, branch.name)
+        removeSecurityProxy(
+            product).information_type = InformationType.PROPRIETARY
+        form = {
+            'field.branch_type': 'link-lp-bzr',
+            'field.branch_location': branch_name, 
+            'field.actions.update': 'Update',
+            }
+        with person_logged_in(product.owner):
+            view = create_initialized_view(
+                productseries, '+setbranch', form=form)
+        self.assertEqual(
+            ['A proprietary product series cannot have a public series '
+             'branch.'],    
+            view.errors)
+
+
 class TestProductSeriesHelp(TestCaseWithFactory):
+
     layer = DatabaseFunctionalLayer
 
     def test_new_series_help(self):