launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/projects-shared-with-person-1053387 into lp:launchpad.

Commit message:
Add new export getSharedProducts method to sharing service.

Requested reviews:
  Curtis Hovey (sinzui)
Related bugs:
  Bug #1053387 in Launchpad itself: "Cannot get the projects that are shared with a person"
  https://bugs.launchpad.net/launchpad/+bug/1053387

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/projects-shared-with-person-1053387/+merge/125682

== Implementation ==

Implement the required query in the sharing service, starting with the AccessPolicyGrantFlat table and making the required joins to pull in the shared products.

Is the user is not an admin, use a filter to restrict the returned products to those for which the user is the owner or driver. For admins, return all shared products.

I considered using a 3 way join between product, accesspolicy, accesspolicygrantflat rather than joining just accesspolicy, accesspolicygrantflat and then doing product.id in(...). But the 3 way join requires a distinct and I think is more expensive.

The SQL is:

SELECT * FROM Product WHERE Product.id IN 
(
  SELECT AccessPolicy.product FROM AccessPolicyGrantFlat
  JOIN AccessPolicy ON AccessPolicyGrantFlat.policy = AccessPolicy.id
  WHERE AccessPolicyGrantFlat.grantee = 243655
) 
AND (Product.owner = 243652 OR Product.driver = 243652)

== Tests ==

Add tests to test_sharingservice.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/interfaces/sharingservice.py
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/projects-shared-with-person-1053387/+merge/125682
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-09-19 13:22:42 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-09-21 12:02:21 +0000
@@ -40,6 +40,7 @@
     )
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.pillar import IPillar
+from lp.registry.interfaces.product import IProduct
 
 
 class ISharingService(IService):
@@ -67,6 +68,21 @@
     @export_read_operation()
     @call_with(user=REQUEST_USER)
     @operation_parameters(
+        person=Reference(IPerson, title=_('Person'), required=True))
+    @operation_returns_collection_of(IProduct)
+    @operation_for_version('devel')
+    def getSharedProducts(person, user):
+        """Find products for which person has one or more access policy grants.
+
+        :param user: the user making the request. If the user is an admin, then
+            all products are returned, else only those for which the user is a
+            maintainer or driver.
+        :return: a collection of products
+        """
+
+    @export_read_operation()
+    @call_with(user=REQUEST_USER)
+    @operation_parameters(
         pillar=Reference(IPillar, title=_('Pillar'), required=True),
         person=Reference(IPerson, title=_('Person'), required=True))
     @operation_for_version('devel')

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-09-19 13:22:42 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-09-21 12:02:21 +0000
@@ -49,6 +49,7 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.sharingjob import (
     IRemoveArtifactSubscriptionsJobSource,
     )
@@ -63,6 +64,7 @@
     )
 from lp.registry.model.person import Person
 from lp.registry.model.teammembership import TeamParticipation
+from lp.registry.model.product import Product
 from lp.services.database.bulk import load
 from lp.services.database.lpstorm import IStore
 from lp.services.database.stormexpr import ColumnSelect
@@ -119,6 +121,31 @@
             AccessPolicy.id.is_in(ids)
         )
 
+    def getSharedProducts(self, person, user):
+        """See `ISharingService`."""
+        if user is None:
+            return []
+        if IPersonRoles(user).in_admin:
+            filter = True
+        else:
+            filter = Or(Product._owner == user, Product.driver == user)
+        store = IStore(AccessPolicyGrantFlat)
+        tables = [
+            AccessPolicyGrantFlat,
+            Join(
+                AccessPolicy,
+                AccessPolicyGrantFlat.policy_id == AccessPolicy.id)]
+        result_set = store.find(
+            Product,
+            Product.id.is_in(
+                Select(
+                    columns=AccessPolicy.product_id,
+                    tables=tables,
+                    where=(AccessPolicyGrantFlat.grantee_id == person.id)
+                )
+            ), filter)
+        return result_set
+
     @available_with_permission('launchpad.Driver', 'pillar')
     def getSharedArtifacts(self, pillar, person, user, include_bugs=True,
                            include_branches=True, include_specifications=True):

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-09-19 13:22:42 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-09-21 12:02:21 +0000
@@ -14,6 +14,7 @@
 from zope.traversing.browser.absoluteurl import absoluteURL
 
 from lp.app.enums import InformationType
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.interfaces.services import IService
 from lp.blueprints.interfaces.specification import ISpecification
 from lp.bugs.interfaces.bug import IBug
@@ -36,6 +37,7 @@
     IAccessPolicySource,
     )
 from lp.registry.interfaces.person import TeamMembershipPolicy
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.services.sharingservice import SharingService
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.tests import block_on_job
@@ -1280,6 +1282,55 @@
         self.assertContentEqual(branches[:9], shared_branches)
         self.assertContentEqual(specs[:9], shared_specs)
 
+    def _assert_getSharedProducts(self, product, who):
+        # Test that 'who' can query the shared products for a grantee.
+
+        # Make a product not related to 'who' which will be shared.
+        unrelated_product = self.factory.makeProduct()
+        # Make an unshared product.
+        self.factory.makeProduct()
+        person = self.factory.makePerson()
+        # Include more than one permission to ensure distinct works.
+        permissions = {
+            InformationType.PRIVATESECURITY: SharingPermission.ALL,
+            InformationType.USERDATA: SharingPermission.ALL}
+        with person_logged_in(product.owner):
+            self.service.sharePillarInformation(
+                product, person, product.owner, permissions)
+        with person_logged_in(unrelated_product.owner):
+            self.service.sharePillarInformation(
+                unrelated_product, person, unrelated_product.owner,
+                permissions)
+        shared = self.service.getSharedProducts(person, who)
+        expected = []
+        if who:
+            expected = [product]
+        if who and IPersonRoles(who).in_admin:
+            expected.append(unrelated_product)
+        self.assertEqual(expected, list(shared))
+
+    def test_getSharedProducts_anonymous(self):
+        # Anonymous users don't get to see any shared products.
+        product = self.factory.makeProduct()
+        self._assert_getSharedProducts(product, None)
+
+    def test_getSharedProducts_admin(self):
+        # Admins can see all shared products.
+        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
+        product = self.factory.makeProduct()
+        self._assert_getSharedProducts(product, admin)
+
+    def test_getSharedProducts_owner(self):
+        # Users only see shared products they own.
+        product = self.factory.makeProduct()
+        self._assert_getSharedProducts(product, product.owner)
+
+    def test_getSharedProducts_driver(self):
+        # Users only see shared products they are the driver for.
+        driver = self.factory.makePerson()
+        product = self.factory.makeProduct(driver=driver)
+        self._assert_getSharedProducts(product, driver)
+
     def test_getSharedBugs(self):
         # Test the getSharedBugs method.
         owner = self.factory.makePerson()
@@ -1681,6 +1732,13 @@
                 InformationType.USERDATA.title: SharingPermission.ALL.title}
         )
 
+    def test_getSharedProducts(self):
+        # Test the exported getSharedProducts() method.
+        ws_grantee = ws_object(self.launchpad, self.grantee)
+        products = self.service.getSharedProducts(person=ws_grantee)
+        self.assertEqual(1, len(products))
+        self.assertEqual(products[0].name, self.pillar.name)
+
     def test_getSharedBugs(self):
         # Test the exported getSharedBugs() method.
         ws_pillar = ws_object(self.launchpad, self.pillar)