Launchpad itself

Merge lp:~james-w/launchpad/package-merge-proposal-permissions into lp:launchpad

  1. package-merge-proposal-permissions
  2. Merge into devel
Proposed by James Westby
Status: Merged
Approved by: Jonathan Lange
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~james-w/launchpad/package-merge-proposal-permissions
Merge into: lp:launchpad
Diff against target: 97 lines (+78/-4)
2 files modified
lib/canonical/launchpad/security.py (+2/-4)
lib/lp/code/tests/test_branchmergeproposal.py (+76/-0)
To merge this branch: bzr merge lp:~james-w/launchpad/package-merge-proposal-permissions
Reviewer Review Type Date Requested Status
Jonathan Lange (community) Approve
Review via email: mp+21561@code.launchpad.net

Commit message

Edit permissions on merge proposals for source package branches now take in to account the special permissions that official branches have.

Description of the change

This allows merge proposals to work with the special permissions
of package branches.

Thanks,

James

To post a comment you must log in.
Revision history for this message
Jonathan Lange (jml) wrote :

Good catch. This is very much how the permission should have been designed in the first place. A few small points:

 * Remove the is_admin and is_bzr_expert checks from the merge proposal permission checks. They are already got with the new permission you add.
 * The new test should be test_package... not test_packge...
 * Why do you set "self.permission_set"?
 * Why do you have an "if" statement in a test? It smells like the behaviour varies from run to run, which is bad for a test.
 * Ideally, there would be two unit tests, with the second round of assertions being split out into a second test.

review: Needs Fixing
Revision history for this message
James Westby (james-w) wrote :

All fixed, thanks.

James

Revision history for this message
Jonathan Lange (jml) wrote :

Awesome. That makes the test much clearer, thanks.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/security.py'
2--- lib/canonical/launchpad/security.py 2010-03-17 13:36:27 +0000
3+++ lib/canonical/launchpad/security.py 2010-03-17 15:55:49 +0000
4@@ -1872,10 +1872,8 @@
5 """
6 return (user.inTeam(self.obj.registrant) or
7 user.inTeam(self.obj.source_branch.owner) or
8- user.inTeam(self.obj.target_branch.owner) or
9- user.inTeam(self.obj.target_branch.reviewer) or
10- user.in_admin or
11- user.in_bazaar_experts)
12+ check_permission('launchpad.Edit', self.obj.target_branch) or
13+ user.inTeam(self.obj.target_branch.reviewer))
14
15
16 class ViewEntitlement(AuthorizationBase):
17
18=== added file 'lib/lp/code/tests/test_branchmergeproposal.py'
19--- lib/lp/code/tests/test_branchmergeproposal.py 1970-01-01 00:00:00 +0000
20+++ lib/lp/code/tests/test_branchmergeproposal.py 2010-03-17 15:55:49 +0000
21@@ -0,0 +1,76 @@
22+# Copyright 2010 Canonical Ltd. This software is licensed under the
23+# GNU Affero General Public License version 3 (see the file LICENSE).
24+
25+"""Unit tests for methods of BranchMergeProposal."""
26+
27+
28+from zope.component import getUtility
29+from zope.security.proxy import removeSecurityProxy
30+
31+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
32+from canonical.testing import DatabaseFunctionalLayer
33+
34+from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
35+from lp.code.tests.test_branch import PermissionTest
36+from lp.registry.interfaces.pocket import PackagePublishingPocket
37+from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
38+from lp.testing import run_with_login
39+
40+
41+class TestEditMergeProposal(PermissionTest):
42+ """Test who can edit branchmergeproposals."""
43+
44+ layer = DatabaseFunctionalLayer
45+
46+ def makePackageProposal(self):
47+ branch = self.factory.makePackageBranch()
48+ # Make sure the (distroseries, pocket) combination used allows us to
49+ # upload to it.
50+ pocket = PackagePublishingPocket.RELEASE
51+ sourcepackage = branch.sourcepackage
52+ suite_sourcepackage = sourcepackage.getSuiteSourcePackage(pocket)
53+ registrant = self.factory.makePerson()
54+ ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
55+ run_with_login(
56+ ubuntu_branches.teamowner,
57+ ICanHasLinkedBranch(suite_sourcepackage).setBranch,
58+ branch, registrant)
59+ source_branch = self.factory.makePackageBranch(
60+ sourcepackage=branch.sourcepackage)
61+ proposal = source_branch.addLandingTarget(
62+ source_branch.registrant, branch)
63+ return proposal
64+
65+ def test_package_merge_proposal_with_no_upload_permission(self):
66+ # If you can't upload the package and have no other relationship
67+ # to the proposal or branches then you can't edit the proposal.
68+ person = self.factory.makePerson()
69+ proposal = self.makePackageProposal()
70+
71+ # Person is not allowed to edit the branch presently.
72+ self.assertCannotEdit(person, proposal.target_branch)
73+ # And so isn't allowed to edit the merge proposal
74+ self.assertCannotEdit(person, proposal)
75+
76+
77+ def test_package_upload_permissions_grant_merge_proposal_edit(self):
78+ # If you can upload to the package then you can edit merge
79+ # proposals against the official branch.
80+ person = self.factory.makePerson()
81+ proposal = self.makePackageProposal()
82+
83+ permission_set = getUtility(IArchivePermissionSet)
84+ # Only admins or techboard members can add permissions normally. That
85+ # restriction isn't relevant to these tests.
86+ permission_set = removeSecurityProxy(permission_set)
87+ # Now give 'person' permission to upload to 'package'.
88+ archive = proposal.target_branch.distroseries.distribution.main_archive
89+ package = proposal.target_branch.sourcepackage
90+ spn = package.sourcepackagename
91+ permission_set.newPackageUploader(archive, person, spn)
92+
93+ # Now person can edit the branch on the basis of the upload
94+ # permissions granted above.
95+ self.assertCanEdit(person, proposal.target_branch)
96+ # And that means they can edit the proposal too
97+ self.assertCanEdit(person, proposal)