Merge lp:~stevenk/launchpad/no-private-registrant-setbranch-redux into lp:launchpad

[Steve Kowalik]

Show a field error if we attempt to create a code import owned by a private team in ProductSeries:+setbranch.

While the first branch that attempted to fix this bug was fine in it's own right, it replaced one OOPS with another. This one wasn't deep in the guts of Storm, and was quite easy to ascertain meaning from. Code imports are not allowed to be owned by private teams, so we check for that and set a field error if so.

My CDO prevented me from putting up a branch that wasn't net-negative, so it contains more whitespace cleanups. I also removed a quote function that didn't seem to be called by anything.

[Curtis Hovey (sinzui)]

Francis reset the squad's Lines when we started Maintenance. Your count is -954 as of today.
    ~/Work/lp-dev-utils/loc-contributions --start-rev=15931 <email address hidden>

[Ian Booth (wallyworld)]

Looks good

review: Approve

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2012-10-05 07:10:15 +0000
+++ lib/lp/registry/browser/productseries.py	2012-10-08 05:07:24 +0000
@@ -29,7 +29,6 @@
     'ProductSeriesView',
     ]
 
-import cgi
 from operator import attrgetter
 
 from bzrlib.revision import NULL_REVISION
@@ -145,11 +144,6 @@
     )
 
 
-def quote(text):
-    """Escape and quote text."""
-    return cgi.escape(text, quote=True)
-
-
 class ProductSeriesNavigation(Navigation, BugTargetTraversalMixin,
     StructuralSubscriptionTargetTraversalMixin):
     """A class to navigate `IProductSeries` URLs."""
@@ -683,10 +677,7 @@
         """See `LaunchpadFormView`."""
         return canonical_url(self.context)
 
-    @property
-    def cancel_url(self):
-        """See `LaunchpadFormView`."""
-        return canonical_url(self.context)
+    cancel_url = next_url
 
 
 class ProductSeriesDeleteView(RegistryDeleteViewMixin, LaunchpadEditFormView):
@@ -813,9 +804,7 @@
 class SetBranchForm(Interface):
     """The fields presented on the form for setting a branch."""
 
-    use_template(
-        ICodeImport,
-        ['cvs_module'])
+    use_template(ICodeImport, ['cvs_module'])
 
     rcs_type = Choice(title=_("Type of RCS"),
         required=False, vocabulary=RevisionControlSystems,
@@ -826,42 +815,27 @@
         title=_("Branch URL"), required=True,
         description=_("The URL of the branch."),
         allowed_schemes=["http", "https"],
-        allow_userinfo=False,
-        allow_port=True,
-        allow_query=False,
-        allow_fragment=False,
-        trailing_slash=False)
+        allow_userinfo=False, allow_port=True, allow_query=False,
+        allow_fragment=False, trailing_slash=False)
 
     branch_location = copy_field(
-        IProductSeries['branch'],
-        __name__='branch_location',
+        IProductSeries['branch'], __name__='branch_location',
         title=_('Branch'),
         description=_(
             "The Bazaar branch for this series in Launchpad, "
-            "if one exists."),
-        )
+            "if one exists."))
 
     branch_type = Choice(
-        title=_('Import type'),
-        vocabulary=BRANCH_TYPE_VOCABULARY,
-        description=_("The type of import"),
-        required=True)
+        title=_('Import type'), vocabulary=BRANCH_TYPE_VOCABULARY,
+        description=_("The type of import"), required=True)
 
     branch_name = copy_field(
-        IBranch['name'],
-        __name__='branch_name',
-        title=_('Branch name'),
-        description=_(''),
-        required=True,
-        )
+        IBranch['name'], __name__='branch_name', title=_('Branch name'),
+        description=_(''), required=True)
 
     branch_owner = copy_field(
-        IBranch['owner'],
-        __name__='branch_owner',
-        title=_('Branch owner'),
-        description=_(''),
-        required=True,
-        )
+        IBranch['owner'], __name__='branch_owner', title=_('Branch owner'),
+        description=_(''), required=True)
 
 
 class ProductSeriesSetBranchView(ReturnToReferrerMixin, LaunchpadFormView,
@@ -936,9 +910,16 @@
         rcs_type = data.get('rcs_type')
         repo_url = data.get('repo_url')
 
+        # Private teams are forbidden from owning code imports.
+        branch_owner = data.get('branch_owner')
+        if branch_owner is not None and branch_owner.private:
+            self.setFieldError(
+                'branch_owner', 'Private teams are forbidden from owning '
+                'external imports.')
+
         if repo_url is None:
-            self.setFieldError('repo_url',
-                               'You must set the external repository URL.')
+            self.setFieldError(
+                'repo_url', 'You must set the external repository URL.')
         else:
             # Ensure this URL has not been imported before.
             code_import = getUtility(ICodeImportSet).getByURL(repo_url)
@@ -969,12 +950,10 @@
         """Validate that branch name and owner are set."""
         if 'branch_name' not in data:
             self.setFieldError(
-                'branch_name',
-                'The branch name must be set.')
+                'branch_name', 'The branch name must be set.')
         if 'branch_owner' not in data:
             self.setFieldError(
-                'branch_owner',
-                'The branch owner must be set.')
+                'branch_owner', 'The branch owner must be set.')
 
     def _setRequired(self, names, value):
         """Mark the widget field as optional."""

=== modified file 'lib/lp/registry/browser/tests/productseries-setbranch-view.txt'
--- lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2012-10-05 07:29:54 +0000
+++ lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2012-10-08 05:07:24 +0000
@@ -377,3 +377,22 @@
     http://launchpad.dev/chevy/corvair
     >>> for notification in view.request.response.notifications:
     ...     print notification.message
+
+If the owner is set to a private team, an error is raised.
+
+    >>> from lp.registry.enums import PersonVisibility
+    >>> private_team = factory.makeTeam(
+    ...     visibility=PersonVisibility.PRIVATE, members=[driver])
+    >>> form = {
+    ...     'field.branch_type': 'import-external',
+    ...     'field.rcs_type': 'BZR',
+    ...     'field.branch_name': 'sport-branch',
+    ...     'field.branch_owner': private_team.name,
+    ...     'field.repo_url': 'http://bzr.com/sporty',
+    ...     'field.actions.update': 'Update',
+    ...     }
+    >>> view = create_initialized_view(
+    ...     series, name='+setbranch', principal=driver, form=form)
+    >>> for error in view.errors:
+    ...     print error
+    Private teams are forbidden from owning external imports.