Merge lp:~thumper/launchpad/package-branch-edit-owner into lp:launchpad

[Tim Penhey]

Allow branch editors who are not in the owner team to edit the branch details without grabbing ownership of the branch.

There is a weird edge case in the editing of a branch where the editor has upload permission on the source package, and this gives the user edit permission on the official source package branch. In these cases the current owner of the branch wasn't in the vocabulary that was used for the widget to set the owner, so the default was set to the first value (the user).

Now when this edge case occurs we add the current branch owner in at the start of the vocabulary used in this situation.

pre-impl call: rockstar

tests: xx-branch-edit.txt

As a drive by fix, I noticed that the vocabularies for IPerson were duplicating the code of unique_displayname, so I fixed it.

[Jonathan Lange (jml)]

Hey Tim,

Weird edge case. Thanks for fixing it up.

 * I wish you didn't add to the doctest, and instead added some real unit tests. Don't feel obliged to do anything about this wish.

 * If there were a function like get_allowed_owners(branch, editor) (or a method branch.get_allowed_owners(editor)) that returned a list of potential owners, then this wouldn't be an edge case. I guess what I mean is, why did you decide to fix this at the view level rather than the model level?

 * The big block of code you add in the doctest could do with some basic prose explanation, e.g. "Create an official branch owned by ~ubuntu-branches, and an editor who has nothing to do with ~ubuntu-branches but is still allowed to edit the branch."

You only need to do something about the last point as far as I'm concerned. Would very much appreciate your thoughts on the first two though.

jml

review: Needs Fixing

[Tim Penhey (thumper)]

OK, for the first, since I was testing a widget that was being rendered, it seemed easiest to use a page test as it has the results of the widget being rendered.

I was wanting to use a standard widget (most of the time) for the rendering of the owner drop-down, and I didn't feel like getting into defining a new custom widget type.

Providing a real model method may well be preferable, and if we get to having a special user selector (instead of the search popup widget), then we may well need this.

I've added a comment to the top of the block of commands in the doctest.

[Jonathan Lange (jml)]

Thanks for getting back to me. I like the new comment, and won't block on the other stuff.

review: Approve

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2010-04-21 02:50:20 +0000
+++ lib/lp/code/browser/branch.py	2010-05-24 04:30:51 +0000
@@ -44,6 +44,7 @@
 from zope.interface import Interface, implements, providedBy
 from zope.publisher.interfaces import NotFound
 from zope.schema import Bool, Choice, Text
+from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary
 from lazr.delegates import delegates
 from lazr.enum import EnumeratedType, Item
 from lazr.lifecycle.event import ObjectModifiedEvent
@@ -97,6 +98,7 @@
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.registry.interfaces.person import IPerson, IPersonSet
 from lp.registry.interfaces.productseries import IProductSeries
+from lp.registry.vocabularies import UserTeamsParticipationPlusSelfVocabulary
 
 
 def quote(text):
@@ -978,6 +980,27 @@
             # Replace the normal owner field with a more permissive vocab.
             self.form_fields = self.form_fields.omit('owner')
             self.form_fields = any_owner_field + self.form_fields
+        else:
+            # For normal users, there is an edge case with package branches
+            # where the editor may not be in the team of the branch owner.  In
+            # these cases we need to extend the vocabulary connected to the
+            # owner field.
+            if not self.user.inTeam(self.context.owner):
+                vocab = UserTeamsParticipationPlusSelfVocabulary()
+                owner = self.context.owner
+                terms = [SimpleTerm(
+                    owner, owner.name, owner.unique_displayname)]
+                terms.extend([term for term in vocab])
+                owner_field = self.schema['owner']
+                owner_choice = Choice(
+                    __name__='owner', title=owner_field.title,
+                    description = owner_field.description,
+                    required=True, vocabulary=SimpleVocabulary(terms))
+                new_owner_field = form.Fields(
+                    owner_choice, render_context=self.render_context)
+                # Replace the normal owner field with a more permissive vocab.
+                self.form_fields = self.form_fields.omit('owner')
+                self.form_fields = new_owner_field + self.form_fields
 
     def validate(self, data):
         # Check that we're not moving a team branch to the +junk

=== modified file 'lib/lp/code/stories/branches/xx-branch-edit.txt'
--- lib/lp/code/stories/branches/xx-branch-edit.txt	2010-03-18 15:39:58 +0000
+++ lib/lp/code/stories/branches/xx-branch-edit.txt	2010-05-24 04:30:51 +0000
@@ -212,7 +212,7 @@
     >>> from lp.code.enums import (
     ...     BranchSubscriptionNotificationLevel, BranchSubscriptionDiffSize,
     ...     CodeReviewNotificationLevel)
-    >>> login('[e-mail]')
+    >>> login('[e-mail]')
     >>> sample_person = getUtility(IPersonSet).getByName('name12')
     >>> foogoo = factory.makeProduct(
     ...     name='foogoo', owner=sample_person)
@@ -303,3 +303,49 @@
     >>> admin_browser.getControl('Change Branch').click()
     >>> print admin_browser.url
     http://code.launchpad.dev/~mark/firefox/main
+
+
+== Package branch editing by Uploaders ==
+
+Official branches for distro series source packages are editable by valid
+package uploaders.  The normal branch owner vocabulary is the editor and the
+teams that they are a member of.  Official branches may well have an owner
+that is different to the editor, or be a celebrity that the editor is not a
+member of, like ubuntu-branches.
+
+    >>> # Create an official branch owned by ~ubuntu-branches, and an editor
+    >>> # who has nothing to do with ~ubuntu-branches but is still allowed
+    >>> # to edit the branch.
+    >>> login('[e-mail]')
+    >>> from lp.code.tests.helpers import make_official_package_branch
+    >>> from canonical.launchpad.interfaces.launchpad import (
+    ...     ILaunchpadCelebrities)
+    >>> owner = getUtility(ILaunchpadCelebrities).ubuntu_branches
+    >>> branch = make_official_package_branch(factory, owner=owner)
+    >>> editor = factory.makePerson(
+    ...     name='editor', email='[e-mail]', password='test')
+    >>> # Give editor upload rights.
+    >>> archive = branch.distroseries.distribution.main_archive
+    >>> spn = branch.sourcepackage.sourcepackagename
+    >>> from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
+    >>> permission_set = getUtility(IArchivePermissionSet)
+    >>> ignored = permission_set.newPackageUploader(archive, editor, spn)
+    >>> branch_url = canonical_url(branch)
+    >>> logout()
+
+Even though the branch owner, Ubuntu branches in this example, is not related
+to the editor, they stay as the default owner for this branch.
+
+    >>> browser = setupBrowser(auth='Basic [e-mail]:test')
+    >>> browser.open(branch_url)
+    >>> browser.getLink('Change branch details').click()
+
+The owner is still ubuntu-branches.
+
+    >>> browser.getControl('Owner').displayValue
+    ['Ubuntu branches (ubuntu-branches)']
+
+But the editor has the option to change the owner to themselves.
+
+    >>> browser.getControl('Owner').displayOptions
+    ['Ubuntu branches (ubuntu-branches)', 'Editor (editor)']

=== modified file 'lib/lp/code/tests/helpers.py'
--- lib/lp/code/tests/helpers.py	2010-04-06 01:13:28 +0000
+++ lib/lp/code/tests/helpers.py	2010-05-24 04:30:51 +0000
@@ -6,8 +6,9 @@
 __metaclass__ = type
 __all__ = [
     'add_revision_to_branch',
+    'make_erics_fooix_project',
     'make_linked_package_branch',
-    'make_erics_fooix_project',
+    'make_official_package_branch',
     ]
 
 
@@ -19,13 +20,16 @@
 from zope.security.proxy import removeSecurityProxy
 from zope.security.proxy import isinstance as zisinstance
 
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+
 from lp.code.interfaces.branchmergeproposal import (
     IBranchMergeProposalJobSource)
+from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.code.interfaces.seriessourcepackagebranch import (
     IMakeOfficialBranchLinks)
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.testing import time_counter
+from lp.testing import run_with_login, time_counter
 
 
 def mark_all_merge_proposal_jobs_done():
@@ -224,3 +228,25 @@
             make_package_branches(
                 factory, series, name, branch_count, official_count,
                 owner=mint_team, registrant=albert)
+
+
+def make_official_package_branch(factory, owner=None):
+    """Make a branch linked to the pocket of a source package."""
+    branch = factory.makePackageBranch(owner=owner)
+    # Make sure the (distroseries, pocket) combination used allows us to
+    # upload to it.
+    stable_states = (
+        SeriesStatus.SUPPORTED, SeriesStatus.CURRENT)
+    if branch.distroseries.status in stable_states:
+        pocket = PackagePublishingPocket.BACKPORTS
+    else:
+        pocket = PackagePublishingPocket.RELEASE
+    sourcepackage = branch.sourcepackage
+    suite_sourcepackage = sourcepackage.getSuiteSourcePackage(pocket)
+    registrant = factory.makePerson()
+    ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
+    run_with_login(
+        ubuntu_branches.teamowner,
+        ICanHasLinkedBranch(suite_sourcepackage).setBranch,
+        branch, registrant)
+    return branch

=== modified file 'lib/lp/code/tests/test_branch.py'
--- lib/lp/code/tests/test_branch.py	2010-03-30 13:53:25 +0000
+++ lib/lp/code/tests/test_branch.py	2010-05-24 04:30:51 +0000
@@ -15,10 +15,8 @@
 from lp.code.enums import (
     BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel)
-from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
+from lp.code.tests.helpers import make_official_package_branch
 from lp.code.xmlrpc.branch import PublicCodehostingAPI
-from lp.registry.interfaces.series import SeriesStatus
-from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.testing import run_with_login, TestCaseWithFactory
 
@@ -243,24 +241,7 @@
 
     def makeOfficialPackageBranch(self):
         """Make a branch linked to the pocket of a source package."""
-        branch = self.factory.makePackageBranch()
-        # Make sure the (distroseries, pocket) combination used allows us to
-        # upload to it.
-        stable_states = (
-            SeriesStatus.SUPPORTED, SeriesStatus.CURRENT)
-        if branch.distroseries.status in stable_states:
-            pocket = PackagePublishingPocket.BACKPORTS
-        else:
-            pocket = PackagePublishingPocket.RELEASE
-        sourcepackage = branch.sourcepackage
-        suite_sourcepackage = sourcepackage.getSuiteSourcePackage(pocket)
-        registrant = self.factory.makePerson()
-        ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
-        run_with_login(
-            ubuntu_branches.teamowner,
-            ICanHasLinkedBranch(suite_sourcepackage).setBranch,
-            branch, registrant)
-        return branch
+        return make_official_package_branch(self.factory)
 
     def test_owner_can_write_to_official_package_branch(self):
         # The owner of an official package branch can write to it, just like a
@@ -305,7 +286,7 @@
         permission_set = getUtility(IArchivePermissionSet)
         # Only admins or techboard members can add permissions normally. That
         # restriction isn't relevant to these tests.
-        self.permission_set = removeSecurityProxy(permission_set)
+        permission_set = removeSecurityProxy(permission_set)
         branch = self.makeOfficialPackageBranch()
         package = branch.sourcepackage
         person = self.factory.makePerson()
@@ -316,7 +297,7 @@
         # Now give 'person' permission to upload to 'package'.
         archive = branch.distroseries.distribution.main_archive
         spn = package.sourcepackagename
-        self.permission_set.newPackageUploader(archive, person, spn)
+        permission_set.newPackageUploader(archive, person, spn)
         # Make sure person *is* authorised to upload the source package
         # targeted by the branch at hand.
         self.assertCanUpload(person, spn, archive, None)

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2010-05-18 17:43:58 +0000
+++ lib/lp/registry/vocabularies.py	2010-05-24 04:30:51 +0000
@@ -289,8 +289,7 @@
 
     def toTerm(self, obj):
         """See `IVocabulary`."""
-        return SimpleTerm(
-            obj, obj.name, '%s (%s)' % (obj.displayname, obj.name))
+        return SimpleTerm(obj, obj.name, obj.unique_displayname)
 
     def __iter__(self):
         kw = {}