launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-private-registrant-setbranch 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/+merge/128173

No longer set the registrant of a new code import on ProductSeries:+setbranch to the branch owner. With a private team as the registrant, the code import would cause the OOPS linked to the bug.

I have switched productseries-setbranch-views.txt to use a team as the owner of the product, and dropped all usage of product.owner from the test. I also check that the registrant of the code import is the currently logged in user.

I have re-factored part of and cleaned up productseries to get this branch to be net-negative.
-- 
https://code.launchpad.net/~stevenk/launchpad/no-private-registrant-setbranch/+merge/128173
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/no-private-registrant-setbranch into lp:launchpad.

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2012-07-03 08:04:35 +0000
+++ lib/lp/registry/browser/productseries.py	2012-10-05 07:18:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """View classes for `IProductSeries`."""
@@ -260,8 +260,7 @@
             configured = True
         else:
             configured = False
-        return [dict(link=set_branch,
-                     configured=configured)]
+        return [dict(link=set_branch, configured=configured)]
 
 
 class ProductSeriesOverviewMenu(
@@ -292,8 +291,8 @@
         text = 'Configure bug tracker'
         summary = 'Specify where bugs are tracked for this project'
         return Link(
-            canonical_url(self.context.product,
-                          view_name='+configure-bugtracker'),
+            canonical_url(
+                self.context.product, view_name='+configure-bugtracker'),
             text, summary, icon='edit')
 
     @enabled_with_permission('launchpad.Edit')
@@ -317,17 +316,21 @@
         summary = 'Someone with permission to set goals this series'
         return Link('+driver', text, summary, icon='edit')
 
+    def _fetch_branch_link(self):
+        if self.context.branch is None:
+            text = 'Link to branch'
+            icon = 'add'
+            summary = 'Set the branch for this series'
+        else:
+            text = "Change branch"
+            icon = 'edit'
+            summary = 'Change the branch for this series'
+        return (text, icon, summary)
+
     @enabled_with_permission('launchpad.Edit')
     def link_branch(self):
         """Return a link to set the bazaar branch for this series."""
-        if self.context.branch is None:
-            text = 'Link to branch'
-            icon = 'add'
-            summary = 'Set the branch for this series'
-        else:
-            text = "Change branch"
-            icon = 'edit'
-            summary = 'Change the branch for this series'
+        text, icon, summary = self._fetch_branch_link()
         return Link('+linkbranch', text, summary, icon=icon)
 
     @enabled_with_permission('launchpad.Edit')
@@ -335,14 +338,7 @@
         """Return a link to set the bazaar branch for this series."""
         # Once +setbranch has been beta tested thoroughly, it should
         # replace the +linkbranch page.
-        if self.context.branch is None:
-            text = 'Link to branch'
-            icon = 'add'
-            summary = 'Set the branch for this series'
-        else:
-            text = "Change branch"
-            icon = 'edit'
-            summary = 'Change the branch for this series'
+        text, icon, summary = self._fetch_branch_link()
         return Link('+setbranch', text, summary, icon=icon)
 
     @enabled_with_permission('launchpad.AnyPerson')
@@ -424,11 +420,8 @@
     if branch.product != product:
         return structured(
             '<a href="%s">%s</a> is not a branch of <a href="%s">%s</a>.',
-            canonical_url(branch),
-            branch.unique_name,
-            canonical_url(product),
+            canonical_url(branch), branch.unique_name, canonical_url(product),
             product.displayname)
-    return None
 
 
 class ProductSeriesView(LaunchpadView, MilestoneOverlayMixin):
@@ -1084,7 +1077,8 @@
                 rcs_item = RevisionControlSystems.items[rcs_type.name]
                 try:
                     code_import = getUtility(ICodeImportSet).new(
-                        registrant=branch_owner,
+                        owner=branch_owner,
+                        registrant=self.user,
                         target=IBranchTarget(self.context.product),
                         branch_name=branch_name,
                         rcs_type=rcs_item,
@@ -1092,8 +1086,8 @@
                         cvs_root=cvs_root,
                         cvs_module=cvs_module)
                 except BranchExists as e:
-                    self._setBranchExists(e.existing_branch,
-                                          'branch_name')
+                    self._setBranchExists(
+                        e.existing_branch, 'branch_name')
                     self.errors_in_action = True
                     # Abort transaction. This is normally handled
                     # by LaunchpadFormView, but we are already in
@@ -1102,8 +1096,7 @@
                     return
                 self.context.branch = code_import.branch
                 self.request.response.addInfoNotification(
-                    'Code import created and branch linked to the '
-                    'series.')
+                    'Code import created and branch linked to the series.')
             else:
                 raise UnexpectedFormData(branch_type)
 
@@ -1115,10 +1108,9 @@
         branch = None
         try:
             namespace = self.target.getNamespace(branch_owner)
-            branch = namespace.createBranch(branch_type=BranchType.HOSTED,
-                                            name=branch_name,
-                                            registrant=self.user,
-                                            url=repo_url)
+            branch = namespace.createBranch(
+                branch_type=BranchType.HOSTED, name=branch_name,
+                registrant=self.user, url=repo_url)
         except BranchCreationForbidden:
             self.addError(
                 "You are not allowed to create branches in %s." %
@@ -1187,10 +1179,7 @@
         return 'Administer %s %s series' % (
             self.context.product.displayname, self.context.name)
 
-    @property
-    def page_title(self):
-        """The page title."""
-        return self.label
+    page_title = label
 
     @property
     def cancel_url(self):

=== modified file 'lib/lp/registry/browser/tests/productseries-setbranch-view.txt'
--- lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2012-04-10 14:01:17 +0000
+++ lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2012-10-05 07:18:29 +0000
@@ -7,12 +7,16 @@
 that exists externally in a variety of version control systems.
 
     >>> from lp.testing.pages import find_tag_by_id
-    >>> product = factory.makeProduct(name="chevy")
+    >>> driver = factory.makePerson()
+    >>> from lp.registry.enums import TeamMembershipPolicy
+    >>> team = factory.makeTeam(
+    ...     owner=driver, membership_policy=TeamMembershipPolicy.MODERATED)
+    >>> product = factory.makeProduct(name="chevy", owner=team)
     >>> series = factory.makeProductSeries(name="impala", product=product)
     >>> transaction.commit()
-    >>> ignored = login_person(product.owner)
-    >>> view = create_initialized_view(series, name='+setbranch',
-    ...                                principal=product.owner)
+    >>> ignored = login_person(driver)
+    >>> view = create_initialized_view(
+    ...     series, name='+setbranch', principal=driver)
     >>> content = find_tag_by_id(view.render(), 'maincontent')
     >>> print content
     <div...
@@ -38,8 +42,8 @@
     ...     'field.branch_type': 'link-lp-bzr',
     ...     'field.actions.update': 'Update',
     ...     }
-    >>> view = create_initialized_view(series, name='+setbranch',
-    ...                                principal=product.owner, form=form)
+    >>> view = create_initialized_view(
+    ...    series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
     The branch location must be set.
@@ -52,8 +56,8 @@
     ...     'field.branch_location': 'foo',
     ...     'field.actions.update': 'Update',
     ...     }
-    >>> view = create_initialized_view(series, name='+setbranch',
-    ...                                principal=product.owner, form=form)
+    >>> view = create_initialized_view(
+    ...    series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
     ('Invalid value', InvalidValue("token 'foo' not found in vocabulary"))
@@ -62,15 +66,15 @@
 
     >>> series.branch is None
     True
-    >>> branch = factory.makeBranch(name='impala-branch',
-    ...                             owner=product.owner, product=product)
+    >>> branch = factory.makeBranch(
+    ...     name='impala-branch', owner=driver, product=product)
     >>> form = {
     ...     'field.branch_type': 'link-lp-bzr',
     ...     'field.branch_location': branch.unique_name,
     ...     'field.actions.update': 'Update',
     ...     }
-    >>> view = create_initialized_view(series, name='+setbranch',
-    ...                                principal=product.owner, form=form)
+    >>> view = create_initialized_view(
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
     >>> for notification in view.request.response.notifications:
@@ -83,8 +87,8 @@
 Revisiting the +setbranch page when the branch is already set will
 show the branch location pre-populated with the existing value.
 
-    >>> view = create_initialized_view(series, name='+setbranch',
-    ...                                principal=product.owner, form=form)
+    >>> view = create_initialized_view(
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> print view.widgets.get('branch_location')._getFormValue().unique_name
     ~person.../chevy/impala-branch
 
@@ -105,7 +109,7 @@
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(
-    ...     series, name='+setbranch', principal=product.owner, form=form)
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for notification in view.request.response.notifications:
     ...     print notification.message
     >>> for error in view.errors:
@@ -121,12 +125,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'BZR',
     ...     'field.branch_name': 'blazer-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'bzr+ssh://bzr.com/foo',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(
-    ...     series, name='+setbranch', principal=product.owner, form=form)
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for notification in view.request.response.notifications:
     ...     print notification.message
     >>> for error in view.errors:
@@ -141,12 +145,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'BZR',
     ...     'field.branch_name': 'blazer-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'http://bzr.com/foo',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(
-    ...     series, name='+setbranch', principal=product.owner, form=form)
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> transaction.commit()
     >>> for error in view.errors:
     ...     print error
@@ -155,6 +159,8 @@
     Code import created and branch linked to the series.
     >>> print series.branch.name
     blazer-branch
+    >>> print series.branch.registrant.name == driver.name
+    True
 
 Git branches cannnot use svn.
 
@@ -162,12 +168,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'GIT',
     ...     'field.branch_name': 'chevette-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'svn://svn.com/chevette',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(
-    ...     series, name='+setbranch', principal=product.owner, form=form)
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for notification in view.request.response.notifications:
     ...     print notification.message
     >>> for error in view.errors:
@@ -183,12 +189,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'GIT',
     ...     'field.branch_name': 'chevette-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'git://github.com/chevette',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(
-    ...     series, name='+setbranch', principal=product.owner, form=form)
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> transaction.commit()
     >>> for error in view.errors:
     ...     print error
@@ -204,12 +210,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'BZR_SVN',
     ...     'field.branch_name': 'suburban-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'git://github.com/suburban',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(
-    ...     series, name='+setbranch', principal=product.owner, form=form)
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for notification in view.request.response.notifications:
     ...     print notification.message
     >>> for error in view.errors:
@@ -225,12 +231,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'BZR_SVN',
     ...     'field.branch_name': 'suburban-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'svn://svn.com/suburban',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(
-    ...     series, name='+setbranch', principal=product.owner, form=form)
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
     >>> for notification in view.request.response.notifications:
@@ -247,12 +253,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'HG',
     ...     'field.branch_name': 'malibu-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'https://mercurial.com/branch',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(
-    ...     series, name='+setbranch', principal=product.owner, form=form)
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
     >>> for notification in view.request.response.notifications:
@@ -270,12 +276,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'CVS',
     ...     'field.branch_name': 'corvair-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'https://cvs.com/branch',
     ...     'field.actions.update': 'Update',
     ...     }
-    >>> view = create_initialized_view(series, name='+setbranch',
-    ...                                principal=product.owner, form=form)
+    >>> view = create_initialized_view(
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for notification in view.request.response.notifications:
     ...     print notification.message
     >>> for error in view.errors:
@@ -286,13 +292,13 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'CVS',
     ...     'field.branch_name': 'corvair-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'https://cvs.com/branch',
     ...     'field.cvs_module': 'root',
     ...     'field.actions.update': 'Update',
     ...     }
-    >>> view = create_initialized_view(series, name='+setbranch',
-    ...                                principal=product.owner, form=form)
+    >>> view = create_initialized_view(
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
     >>> for notification in view.request.response.notifications:
@@ -308,12 +314,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'GIT',
     ...     'field.branch_name': 'chevette-branch-dup',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'git://github.com/chevette',
     ...     'field.actions.update': 'Update',
     ...     }
-    >>> view = create_initialized_view(series, name='+setbranch',
-    ...                                principal=product.owner, form=form)
+    >>> view = create_initialized_view(
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
     <BLANKLINE>
@@ -329,15 +335,15 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'GIT',
     ...     'field.branch_name': 'chevette-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'git://github.com/different/chevette',
     ...     'field.actions.update': 'Update',
     ...     }
-    >>> view = create_initialized_view(series, name='+setbranch',
-    ...                                principal=product.owner, form=form)
+    >>> view = create_initialized_view(
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
-    You already have a branch for
+    Team Name ... already has a branch for
     <em>Chevy</em> called <em>chevette-branch</em>.
     >>> print view.errors_in_action
     True
@@ -354,12 +360,12 @@
     ...     'field.branch_type': 'import-external',
     ...     'field.rcs_type': 'BZR',
     ...     'field.branch_name': 'blazer-branch',
-    ...     'field.branch_owner': product.owner.name,
+    ...     'field.branch_owner': team.name,
     ...     'field.repo_url': 'http://bzr.com/foo',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(
-    ...     series, name='+setbranch', principal=product.owner, form=form)
+    ...     series, name='+setbranch', principal=driver, form=form)
     >>> for error in view.errors:
     ...     print error
     <BLANKLINE>