launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-private-registrant-setbranch-redux into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1031751 in Launchpad itself: "KeyError: 'primary_vars'  raised setting branch for a project"
  https://bugs.launchpad.net/launchpad/+bug/1031751

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/no-private-registrant-setbranch-redux/+merge/128415

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.
-- 
https://code.launchpad.net/~stevenk/launchpad/no-private-registrant-setbranch-redux/+merge/128415
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/no-private-registrant-setbranch-redux into lp:launchpad.

=== 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:05: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:05: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.