launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09911
[Merge] lp:~wallyworld/launchpad/view-private-stacked-branch-393566 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/view-private-stacked-branch-393566 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #393566 in Launchpad itself: "cannot view private branch if you cannot see the private branch it is stacked on"
https://bugs.launchpad.net/launchpad/+bug/393566
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/view-private-stacked-branch-393566/+merge/114784
== Implementation ==
This branch ensures that if a person is granted access to a branch, they are granted access (if necessary) to any branches on which the branch it stacked.
There were a few things to do:
1. Add a getStackedOnBranches() method to Branch
2. Add a getBranchIds() method to BranchCollection. This is similar to the BugTaskSet searchBugIds() method in that it does the search but only returns the branch ids, not the hydrated branches. I also cleaned up a bit of the code so that things were less messy and common code was used to construct the core query.
3. Add a getInvisibleArtifacts() method to the sharing service (similar to the existing getVisibleArtifacts() method).
4. The sharing service ensureAccessGrants() method finds any stacked on branches the grantee can't see and adds those to the list for which access is to be granted.
== Tests ==
Basically add a few tests for each of the new methods introduced above. Also fix some existing tests in test_branchcollection where the newly visible stacked on branches needed to be added to the expected results.
== Lint ==
I added back a couple of unused imports I removed since SteveK also removed them in a branch he is landing and I didn't want to conflict.
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/interfaces/branch.py
lib/lp/code/interfaces/branchcollection.py
lib/lp/code/model/branch.py
lib/lp/code/model/branchcollection.py
lib/lp/code/model/tests/test_branch.py
lib/lp/code/model/tests/test_branchcollection.py
lib/lp/registry/interfaces/sharingservice.py
lib/lp/registry/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
./lib/lp/code/model/branch.py
147: 'PersonVisibility' imported but unused
5: 'IProduct' imported but unused
--
https://code.launchpad.net/~wallyworld/launchpad/view-private-stacked-branch-393566/+merge/114784
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/view-private-stacked-branch-393566 into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2012-07-12 06:51:37 +0000
+++ lib/lp/code/interfaces/branch.py 2012-07-13 06:23:21 +0000
@@ -641,6 +641,9 @@
def getStackedBranches():
"""The branches that are stacked on this one."""
+ def getStackedOnBranches():
+ """The branches on which this one is stacked."""
+
def getMainlineBranchRevisions(start_date, end_date=None,
oldest_first=False):
"""Return the matching mainline branch revision objects.
=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py 2012-05-15 08:16:09 +0000
+++ lib/lp/code/interfaces/branchcollection.py 2012-07-13 06:23:21 +0000
@@ -72,6 +72,9 @@
objects in the collection.
"""
+ def getBranchIds():
+ """Return a result set of all branch ids in this collection."""
+
def getMergeProposals(statuses=None, for_branches=None,
target_branch=None, eager_load=False):
"""Return a result set of merge proposals for the branches in this
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2012-07-13 00:17:32 +0000
+++ lib/lp/code/model/branch.py 2012-07-13 06:23:21 +0000
@@ -598,6 +598,14 @@
store = Store.of(self)
return store.find(Branch, Branch.stacked_on == self)
+ def getStackedOnBranches(self):
+ """See `IBranch`."""
+ if not self.stacked_on:
+ return []
+ stacked_on_branches = [self.stacked_on]
+ stacked_on_branches.extend(self.stacked_on.getStackedOnBranches())
+ return stacked_on_branches
+
@property
def code_is_browseable(self):
"""See `IBranch`."""
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2012-07-13 00:06:59 +0000
+++ lib/lp/code/model/branchcollection.py 2012-07-13 06:23:21 +0000
@@ -140,8 +140,7 @@
def ownerCounts(self):
"""See `IBranchCollection`."""
is_team = Person.teamowner != None
- branch_owners = self._getBranchIdQuery()
- branch_owners.columns = (Branch.ownerID,)
+ branch_owners = self._getBranchSelect((Branch.ownerID,))
counts = dict(self.store.find(
(is_team, Count(Person.id)),
Person.id.is_in(branch_owners)).group_by(is_team))
@@ -198,12 +197,10 @@
asymmetric_expr,
asymmetric_tables)
- def _getBranchIdQuery(self):
- """Return a Storm 'Select' for the branch IDs in this collection."""
- branches = self.getBranches(eager_load=False)
- select = branches.get_plain_result_set()._get_select()
- select.columns = (Branch.id,)
- return select
+ def _getBranchSelect(self, columns=(Branch.id,)):
+ """Return a Storm 'Select' for columns in this collection."""
+ branches = self.getBranches(eager_load=False, find_expr=columns)
+ return branches.get_plain_result_set()._get_select()
def _getBranchExpressions(self):
"""Return the where expressions for this collection."""
@@ -289,13 +286,13 @@
cache = caches[code_import.branchID]
cache.code_import = code_import
- def getBranches(self, eager_load=False):
+ def getBranches(self, find_expr=Branch, eager_load=False):
"""See `IBranchCollection`."""
all_tables = set(
self._tables.values() + self._asymmetric_tables.values())
tables = [Branch] + list(all_tables)
expressions = self._getBranchExpressions()
- resultset = self.store.using(*tables).find(Branch, *expressions)
+ resultset = self.store.using(*tables).find(find_expr, *expressions)
def do_eager_load(rows):
branch_ids = set(branch.id for branch in rows)
@@ -315,11 +312,16 @@
set([self._user.id]))
return branch
- eager_load_hook = do_eager_load if eager_load else None
+ eager_load_hook = (
+ do_eager_load if eager_load and find_expr == Branch else None)
return DecoratedResultSet(
resultset, pre_iter_hook=eager_load_hook,
result_decorator=cache_permission)
+ def getBranchIds(self):
+ """See `IBranchCollection`."""
+ return self.getBranches(find_expr=Branch.id).get_plain_result_set()
+
def getMergeProposals(self, statuses=None, for_branches=None,
target_branch=None, merged_revnos=None,
eager_load=False):
@@ -442,8 +444,7 @@
expressions = [
CodeReviewVoteReference.reviewer == reviewer,
- BranchMergeProposal.source_branchID.is_in(
- self._getBranchIdQuery())]
+ BranchMergeProposal.source_branchID.is_in(self._getBranchSelect())]
visibility = self._getBranchVisibilityExpression()
if visibility:
expressions.append(BranchMergeProposal.target_branchID.is_in(
@@ -529,8 +530,7 @@
# BranchCollection conceptual model, but we're not quite sure how to
# fix it just yet. Perhaps when bug 337494 is fixed, we'd be able to
# sensibly be able to move this method to another utility class.
- branch_query = self._getBranchIdQuery()
- branch_query.columns = (Branch.ownerID,)
+ branch_query = self._getBranchSelect((Branch.ownerID,))
return self.store.find(
Person,
Person.id == TeamParticipation.teamID,
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2012-07-12 08:16:22 +0000
+++ lib/lp/code/model/tests/test_branch.py 2012-07-13 06:23:21 +0000
@@ -1727,6 +1727,29 @@
self.assertEqual(
set([stacked_a, stacked_b]), set(branch.getStackedBranches()))
+ def testNoBranchesStackedOn(self):
+ # getStackedBranches returns an empty collection if there are no
+ # branches stacked on it.
+ branch = self.factory.makeAnyBranch()
+ self.assertEqual(set(), set(branch.getStackedOnBranches()))
+
+ def testSingleBranchStackedOn(self):
+ # some_branch.getStackedOnBranches returns a collection of branches
+ # on which some_branch is stacked.
+ branch = self.factory.makeAnyBranch()
+ stacked_branch = self.factory.makeAnyBranch(stacked_on=branch)
+ self.assertEqual(
+ set([branch]), set(stacked_branch.getStackedOnBranches()))
+
+ def testMultipleBranchesStackedOn(self):
+ # some_branch.getStackedOnBranches returns a collection of branches
+ # on which some_branch is stacked.
+ stacked_a = self.factory.makeAnyBranch()
+ stacked_b = self.factory.makeAnyBranch(stacked_on=stacked_a)
+ branch = self.factory.makeAnyBranch(stacked_on=stacked_b)
+ self.assertEqual(
+ set([stacked_a, stacked_b]), set(branch.getStackedOnBranches()))
+
class BranchAddLandingTarget(TestCaseWithFactory):
"""Exercise all the code paths for adding a landing target."""
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2012-07-12 07:28:22 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2012-07-13 06:23:21 +0000
@@ -147,13 +147,21 @@
product=product,
information_type=InformationType.USERDATA)
someone = self.factory.makePerson()
- getUtility(IService, 'sharing').ensureAccessGrants(
- [someone], owner, branches=[branch], ignore_permissions=True)
+ with person_logged_in(owner):
+ getUtility(IService, 'sharing').ensureAccessGrants(
+ [someone], owner, branches=[branch], ignore_permissions=True)
[branch] = list(collection.visibleByUser(someone).getBranches())
with StormStatementRecorder() as recorder:
self.assertTrue(branch.visibleByUser(someone))
self.assertThat(recorder, HasQueryCount(Equals(0)))
+ def test_getBranchIds(self):
+ branch = self.factory.makeProductBranch()
+ self.factory.makeAnyBranch()
+ collection = GenericBranchCollection(
+ self.store, [Branch.product == branch.product])
+ self.assertEqual([branch.id], list(collection.getBranchIds()))
+
def test_count(self):
# The 'count' property of a collection is the number of elements in
# the collection.
@@ -650,12 +658,14 @@
self.assertEqual([self.public_branch], list(branches.getBranches()))
def test_owner_sees_own_branches(self):
- # Users can always see the branches that they own, as well as public
- # branches.
+ # Users can always see the branches that they own, those their own
+ # branches are stacked on, as well as public branches.
owner = removeSecurityProxy(self.private_branch1).owner
branches = self.all_branches.visibleByUser(owner)
self.assertEqual(
- sorted([self.public_branch, self.private_branch1]),
+ sorted([self.public_branch, self.private_branch1,
+ self.public_stacked_on_branch,
+ self.private_stacked_on_branch]),
sorted(branches.getBranches()))
def test_launchpad_services_sees_all(self):
@@ -677,7 +687,8 @@
sorted(branches.getBranches()))
def test_subscribers_can_see_branches(self):
- # A person subscribed to a branch can see it, even if it's private.
+ # A person subscribed to a branch can see it, as well as any it is
+ # stacked on, even if it's private.
subscriber = self.factory.makePerson()
removeSecurityProxy(self.private_branch1).subscribe(
subscriber, BranchSubscriptionNotificationLevel.NOEMAIL,
@@ -686,7 +697,9 @@
subscriber)
branches = self.all_branches.visibleByUser(subscriber)
self.assertEqual(
- sorted([self.public_branch, self.private_branch1]),
+ sorted([self.public_branch, self.private_branch1,
+ self.public_stacked_on_branch,
+ self.private_stacked_on_branch]),
sorted(branches.getBranches()))
def test_subscribed_team_members_can_see_branches(self):
=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py 2012-06-18 03:23:18 +0000
+++ lib/lp/registry/interfaces/sharingservice.py 2012-07-13 06:23:21 +0000
@@ -70,6 +70,18 @@
:return: a collection of artifacts the person can see.
"""
+ def getInvisibleArtifacts(person, branches=None, bugs=None):
+ """Return the artifacts which are not shared with person.
+
+ Given lists of artifacts, return those a person does not have access to
+ either via a policy grant or artifact grant.
+
+ :param person: the person whose access is being checked.
+ :param branches: the branches to check for which a person has access.
+ :param bugs: the bugs to check for which a person has access.
+ :return: a collection of artifacts the person can not see.
+ """
+
def getPeopleWithoutAccess(concrete_artifact, people):
"""Return the people who cannot access an artifact.
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-07-02 23:40:56 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-07-13 06:23:21 +0000
@@ -139,6 +139,38 @@
return visible_bugs, visible_branches
+ def getInvisibleArtifacts(self, person, branches=None, bugs=None):
+ """See `ISharingService`."""
+ bugs_by_id = {}
+ branches_by_id = {}
+ for bug in bugs or []:
+ bugs_by_id[bug.id] = bug
+ for branch in branches or []:
+ branches_by_id[branch.id] = branch
+
+ # Load the bugs.
+ visible_bug_ids = set()
+ if bugs_by_id:
+ param = BugTaskSearchParams(
+ user=person, bug=any(*bugs_by_id.keys()))
+ visible_bug_ids = set(getUtility(IBugTaskSet).searchBugIds(param))
+ invisible_bug_ids = set(bugs_by_id.keys()).difference(visible_bug_ids)
+ invisible_bugs = [bugs_by_id[bug_id] for bug_id in invisible_bug_ids]
+
+ # Load the branches.
+ invisible_branches = []
+ if branches_by_id:
+ all_branches = getUtility(IAllBranches)
+ visible_branch_ids = all_branches.visibleByUser(person).withIds(
+ *branches_by_id.keys()).getBranchIds()
+ invisible_branch_ids = (
+ set(branches_by_id.keys()).difference(visible_branch_ids))
+ invisible_branches = [
+ branches_by_id[branch_id]
+ for branch_id in invisible_branch_ids]
+
+ return invisible_bugs, invisible_branches
+
def getPeopleWithoutAccess(self, concrete_artifact, people):
"""See `ISharingService`."""
# Public artifacts allow everyone to have access.
@@ -428,6 +460,16 @@
artifacts = []
if branches:
artifacts.extend(branches)
+ # If the branch is stacked we need to ensure that the user is
+ # granted access to the stacked on branch(es) also.
+ stacked_on_branches = []
+ for branch in branches:
+ stacked_on_branches.extend(branch.getStackedOnBranches())
+ for sharee in sharees:
+ invisible_bugs, invisible_stacked_on_branches = (
+ self.getInvisibleArtifacts(
+ sharee, branches=stacked_on_branches))
+ artifacts.extend(invisible_stacked_on_branches)
if bugs:
artifacts.extend(bugs)
if not ignore_permissions:
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-07-11 01:21:56 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-07-13 06:23:21 +0000
@@ -493,7 +493,7 @@
[InformationType.USERDATA,
InformationType.EMBARGOEDSECURITY]),
]
- else:
+ else:
expected_sharee_grants = [
(sharee,
{es_policy: SharingPermission.ALL,
@@ -997,6 +997,34 @@
information_type=InformationType.USERDATA)
self._assert_ensureAccessGrants(owner, None, [branch])
+ def test_ensureAccessGrantsStackedBranches(self):
+ # Access grants created for branches also grant access to any private
+ # stacked on branches.
+ owner = self.factory.makePerson()
+ product = self.factory.makeProduct(owner=owner)
+ login_person(owner)
+ stacked_on_branch = self.factory.makeBranch(
+ product=product, owner=owner,
+ information_type=InformationType.EMBARGOEDSECURITY)
+ branch = self.factory.makeBranch(
+ product=product, owner=owner, stacked_on=stacked_on_branch,
+ information_type=InformationType.USERDATA)
+
+ # Grant access to the top level branch.
+ grantee = self.factory.makePerson()
+ with FeatureFixture(WRITE_FLAG):
+ self.service.ensureAccessGrants(
+ [grantee], owner, branches=[branch])
+
+ # Check that the expected access grants have been created.
+ shared_branches = []
+ policies = getUtility(IAccessPolicySource).findByPillar([product])
+ apgfs = getUtility(IAccessPolicyGrantFlatSource)
+ access_artifacts = apgfs.findArtifactsByGrantee(grantee, policies)
+ for a in access_artifacts:
+ shared_branches.append(a.concrete_artifact)
+ self.assertContentEqual([branch, stacked_on_branch], shared_branches)
+
def test_ensureAccessGrantsExisting(self):
# Any existing access grants are retained and new ones created.
owner = self.factory.makePerson()
@@ -1154,8 +1182,8 @@
without_access = self.service.getPeopleWithoutAccess(bug, people)
self.assertContentEqual(people[:5], without_access)
- def test_getVisibleArtifacts(self):
- # Test the getVisibleArtifacts method.
+ def _make_Artifacts(self):
+ # Make artifacts for test (in)visible artifact methods.
owner = self.factory.makePerson()
product = self.factory.makeProduct(owner=owner)
grantee = self.factory.makePerson()
@@ -1186,23 +1214,26 @@
grant_access(bug)
for branch in branches[:5]:
grant_access(branch)
- # XXX bug=1001042 wallyworld 2012-05-18
- # for now we need to subscribe users to the branch in order
- # for the underlying BranchCollection to allow access. This will
- # no longer be the case when BranchCollection supports the new
- # access policy framework.
- branch.subscribe(
- grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
- BranchSubscriptionDiffSize.NODIFF,
- CodeReviewNotificationLevel.NOEMAIL,
- owner)
+ return grantee, branches, bugs
+ def test_getVisibleArtifacts(self):
+ # Test the getVisibleArtifacts method.
+ grantee, branches, bugs = self._make_Artifacts()
# Check the results.
shared_bugs, shared_branches = self.service.getVisibleArtifacts(
grantee, branches, bugs)
self.assertContentEqual(bugs[:5], shared_bugs)
self.assertContentEqual(branches[:5], shared_branches)
+ def test_getInvisibleArtifacts(self):
+ # Test the getInvisibleArtifacts method.
+ grantee, branches, bugs = self._make_Artifacts()
+ # Check the results.
+ not_shared_bugs, not_shared_branches = (
+ self.service.getInvisibleArtifacts(grantee, branches, bugs))
+ self.assertContentEqual(bugs[5:], not_shared_bugs)
+ self.assertContentEqual(branches[5:], not_shared_branches)
+
def _assert_getVisibleArtifacts_bug_change(self, change_callback):
# Test the getVisibleArtifacts method excludes bugs after a change of
# information_type or bugtask re-targetting.
Follow ups
