launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/branch-picker-team-branches into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #407260 Translations export branch can't be team-owned
  https://bugs.launchpad.net/bugs/407260

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/branch-picker-team-branches/+merge/52356

See bug 407260. This branch enhances the ownedBy() API on IBranchCollection to optionally return branches owned by a team of which the person is a member.

== Implementation ==

Added optional include_team_membership (defaults to False) parameter to ownedBy() API. If true, an extra clause is added to the filter to do the required work.

== Tests ==

Added new tests to code.model.tests.test_branchcollection.TestBranchCollectionFilters:
  test_ownedBy_with_team_membership
  test_ownedBy_and_inProduct_with_team_membership

Enhanced:
code.vocabularies.test.branch.txt

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/interfaces/branchcollection.py
  lib/lp/code/model/branchcollection.py
  lib/lp/code/model/tests/test_branchcollection.py
  lib/lp/code/vocabularies/branch.py
  lib/lp/code/vocabularies/tests/branch.txt

./lib/lp/code/interfaces/branchcollection.py
      50: E301 expected 1 blank line, found 2
-- 
https://code.launchpad.net/~wallyworld/launchpad/branch-picker-team-branches/+merge/52356
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/branch-picker-team-branches into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py	2011-03-03 01:13:47 +0000
+++ lib/lp/code/interfaces/branchcollection.py	2011-03-07 01:40:13 +0000
@@ -145,8 +145,12 @@
         with a sourcepackage.
         """
 
-    def ownedBy(person):
-        """Restrict the collection to branches owned by 'person'."""
+    def ownedBy(person, include_team_membership=False):
+        """Restrict the collection to branches owned by 'person'.
+
+        :param include_team_membership: If True, the result will include
+            branches owned by teams of which the person is a member.
+        """
 
     def registeredBy(person):
         """Restrict the collection to branches registered by 'person'."""

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-03-03 03:04:30 +0000
+++ lib/lp/code/model/branchcollection.py	2011-03-07 01:40:13 +0000
@@ -14,6 +14,7 @@
     And,
     Count,
     Desc,
+    In,
     Join,
     LeftJoin,
     Or,
@@ -158,6 +159,7 @@
         resultset = self.store.using(*tables).find(Branch, *expressions)
         if not eager_load:
             return resultset
+
         def do_eager_load(rows):
             branch_ids = set(branch.id for branch in rows)
             if not branch_ids:
@@ -368,9 +370,18 @@
             Branch.product == None,
             Branch.sourcepackagename == None])
 
-    def ownedBy(self, person):
+    def ownedBy(self, person, include_team_membership=False):
         """See `IBranchCollection`."""
-        return self._filterBy([Branch.owner == person])
+        filter = Branch.owner == person
+        if include_team_membership:
+            subquery = Select(
+                TeamParticipation.teamID,
+                where=TeamParticipation.personID==person.id)
+            filter = Or(
+                filter,
+                In(Branch.ownerID, subquery))
+
+        return self._filterBy([filter])
 
     def registeredBy(self, person):
         """See `IBranchCollection`."""

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2011-01-27 14:02:38 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2011-03-07 01:40:13 +0000
@@ -192,6 +192,17 @@
         collection = self.all_branches.ownedBy(branch.owner)
         self.assertEqual([branch], list(collection.getBranches()))
 
+    def test_ownedBy_with_team_membership(self):
+        # 'ownedBy' returns a new collection restricted to branches owned by
+        # any team of which the given person is a member.
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(members=[person])
+        branch = self.factory.makeAnyBranch(owner=team)
+        branch2 = self.factory.makeAnyBranch()
+        collection = self.all_branches.ownedBy(
+            person, include_team_membership=True)
+        self.assertEqual([branch], list(collection.getBranches()))
+
     def test_in_product(self):
         # 'inProduct' returns a new collection restricted to branches in the
         # given product.
@@ -230,6 +241,26 @@
         collection = self.all_branches.ownedBy(person).inProduct(product)
         self.assertEqual([branch], list(collection.getBranches()))
 
+    def test_ownedBy_and_inProduct_with_team_membership(self):
+        # 'ownedBy' and 'inProduct' can combine to form a collection that is
+        # restricted to branches of a particular product owned by a particular
+        # person or team of which the person is a member.
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(members=[person])
+        product = self.factory.makeProduct()
+        branch = self.factory.makeProductBranch(product=product, owner=person)
+        branch2 = self.factory.makeProductBranch(product=product, owner=team)
+        branch3 = self.factory.makeAnyBranch(owner=person)
+        branch4 = self.factory.makeProductBranch(product=product)
+        collection = self.all_branches.inProduct(product).ownedBy(
+            person, include_team_membership=True)
+        self.assertEqual(
+            set([branch, branch2]), set(collection.getBranches()))
+        collection = self.all_branches.ownedBy(
+            person, include_team_membership=True).inProduct(product)
+        self.assertEqual(
+            set([branch, branch2]), set(collection.getBranches()))
+
     def test_in_source_package(self):
         # 'inSourcePackage' returns a new collection that only has branches in
         # the given source package.

=== modified file 'lib/lp/code/vocabularies/branch.py'
--- lib/lp/code/vocabularies/branch.py	2011-03-03 01:13:47 +0000
+++ lib/lp/code/vocabularies/branch.py	2011-03-07 01:40:13 +0000
@@ -130,5 +130,7 @@
             self.user = getUtility(ILaunchBag).user
 
     def _getCollection(self):
-        return getUtility(IAllBranches).ownedBy(self.user).withBranchType(
+        return getUtility(IAllBranches).ownedBy(
+            self.user, include_team_membership=True,
+        ).withBranchType(
             BranchType.HOSTED)

=== modified file 'lib/lp/code/vocabularies/tests/branch.txt'
--- lib/lp/code/vocabularies/tests/branch.txt	2010-09-20 00:43:45 +0000
+++ lib/lp/code/vocabularies/tests/branch.txt	2011-03-07 01:40:13 +0000
@@ -130,6 +130,7 @@
     >>> from lp.code.enums import BranchType
 
     >>> a_user = factory.makePerson(name='a-branching-user')
+    >>> a_team = factory.makeTeam(name='a-team', members=[a_user])
     >>> product1 = factory.makeProduct(name='product-one')
     >>> mirrored_branch = factory.makeBranch(
     ...     owner=a_user, product=product1, name='mirrored',
@@ -137,12 +138,15 @@
     >>> product2 = factory.makeProduct(name='product-two')
     >>> hosted_branch = factory.makeBranch(
     ...     owner=a_user, product=product2, name='hosted')
+    >>> another_hosted_branch = factory.makeBranch(
+    ...     owner=a_team, product=product2, name='another_hosted')
     >>> foreign_branch = factory.makeBranch()
 
-It returns branches owned by the user, but not ones owned by others, nor
-ones that aren't hosted on Launchpad.
+It returns branches owned by the user, or teams a user belongs to, but not
+ones owned by others, nor ones that aren't hosted on Launchpad.
 
     >>> branch_vocabulary = vocabulary_registry.get(
     ...     a_user, "HostedBranchRestrictedOnOwner")
     >>> print_vocab_branches(branch_vocabulary, None)
     ~a-branching-user/product-two/hosted
+    ~a-team/product-two/another_hosted