Ian Booth has proposed merging 
lp:~wallyworld/launchpad/sharing-view-batching4-957663 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #957663 in Launchpad itself: "+sharing view needs support for batching"
  https://bugs.launchpad.net/launchpad/+bug/957663

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-view-batching4-957663/+merge/98820

== Implementation ==

Improve sharing service and access policy interfaces and rework queries to only 
load person objects once per batch.

Example of interface change:
The IAccessPolicyGrantFlatSource.findGranteePermissionsByPolicy() interface 
returned a list of tuples (IPerson, IAccessPolicy, string). The string was the 
permission name and although the name is required when hand data off to the 
view, this internal model method should have returned a tuple containing the 
SharingPermission enum object. Then it's up to the service methods to flatten 
the tuple out to the json data.

Also, the pillar sharing view had some methods changed to properties.

Before batching all the data for the view was loaded in a single query, joining 
Person, AccessPolicy and AccessPolicyGrantFlat. But this query returned 
multiple rows per person and it not suitable for batching. So the next 
iteration loaded all persons in the required batch and then made a second query 
to load the sharing permissions. The second query loaded person objects again. 
The time to do this was of the order of 10s of ms but William nagged me to 
death saying this was too expensive - loading persons in Launchpad is slow 
apparently. So I have changed the implementation of 
findGranteePermissionsByPolicy() to use a decorated result set so that the core 
batch query loads the persons and a pre_iteration hook bulk loads the 
permissions. So it's two queries (but with less joins) and only one loads 
persons. The second query is on the AccessPolicyGrantFlat table so is known to 
be fast.

SQL tracing shows that the batching infrastructure executes the core batching 
query twice due to a decorated result set being used. This is unfortunate. But 
I don't think the 2nd execution instantiates person objects.

== Tests ==

Check the existing tests run:
test_accesspolicy
test_sharingservice
test_pillar_sharing

The above ensures that external facing interfaces (eg those providing data to 
the view) work as before. Some of the tests needed tweaking due to the 
AccessPolicy model interface changes.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/pillar.py
  lib/lp/registry/browser/tests/test_pillar_sharing.py
  lib/lp/registry/interfaces/accesspolicy.py
  lib/lp/registry/interfaces/sharingservice.py
  lib/lp/registry/model/accesspolicy.py
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
  lib/lp/registry/templates/pillar-sharing.pt
  lib/lp/registry/tests/test_accesspolicy.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/sharing-view-batching4-957663/+merge/98820
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~wallyworld/launchpad/sharing-view-batching4-957663 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py	2012-03-21 14:30:16 +0000
+++ lib/lp/registry/browser/pillar.py	2012-03-22 13:04:20 +0000
@@ -303,15 +303,17 @@
             size=config.launchpad.default_batch_size,
             range_factory=StormRangeFactory(sharees))
 
-    def shareeData(self):
-        """Return an `ITableBatchNavigator` for sharees."""
+    @property
+    def sharees(self):
+        """An `IBatchNavigator` for sharees."""
         if self._batch_navigator is None:
-            unbatchedSharees = self.unbatchedShareeData()
+            unbatchedSharees = self.unbatched_sharees
             self._batch_navigator = self._getBatchNavigator(unbatchedSharees)
         return self._batch_navigator
 
-    def unbatchedShareeData(self):
-        """Return all the sharees for a pillar."""
+    @property
+    def unbatched_sharees(self):
+        """All the sharees for a pillar."""
         return self._getSharingService().getPillarSharees(self.context)
 
     def initialize(self):
@@ -334,10 +336,9 @@
         if len(view_names) != 1:
             raise AssertionError("Ambiguous view name.")
         cache.objects['view_name'] = view_names.pop()
-        batch_navigator = self.shareeData()
+        batch_navigator = self.sharees
         cache.objects['sharee_data'] = (
-            self._getSharingService().getPillarShareeData(
-                self.context, batch_navigator.batch))
+            self._getSharingService().jsonShareeData(batch_navigator.batch))
 
         def _getBatchInfo(batch):
             if batch is None:

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-03-21 14:30:16 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-03-22 13:04:20 +0000
@@ -21,6 +21,7 @@
 
 from lp.app.interfaces.services import IService
 from lp.registry.enums import InformationType
+from lp.registry.interfaces.accesspolicy import IAccessPolicyGrantFlatSource
 from lp.registry.model.pillar import PillarPerson
 from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
@@ -132,6 +133,7 @@
             owner=self.owner, driver=self.driver)
         login_person(self.driver)
 
+
 class PillarSharingViewTestMixin:
     """Test the PillarSharingView."""
 
@@ -139,7 +141,7 @@
 
     def createSharees(self):
         login_person(self.owner)
-        access_policy = self.factory.makeAccessPolicy(
+        self.access_policy = self.factory.makeAccessPolicy(
             pillar=self.pillar,
             type=InformationType.PROPRIETARY)
         self.grantees = []
@@ -148,12 +150,12 @@
             grantee = self.factory.makePerson(name=name)
             self.grantees.append(grantee)
             # Make access policy grant so that 'All' is returned.
-            self.factory.makeAccessPolicyGrant(access_policy, grantee)
+            self.factory.makeAccessPolicyGrant(self.access_policy, grantee)
             # Make access artifact grants so that 'Some' is returned.
             artifact_grant = self.factory.makeAccessArtifactGrant()
             self.factory.makeAccessPolicyArtifact(
                 artifact=artifact_grant.abstract_artifact,
-                policy=access_policy)
+                policy=self.access_policy)
         # Make grants for grantees in ascending order so we can slice off the
         # first elements in the pillar observer results to check batching.
         for x in range(10):
@@ -207,12 +209,14 @@
             cache = IJSONRequestCache(view.request)
             self.assertIsNotNone(cache.objects.get('information_types'))
             self.assertIsNotNone(cache.objects.get('sharing_permissions'))
-            aps = getUtility(IService, 'sharing')
             batch_size = config.launchpad.default_batch_size
-            observers = aps.getPillarShareeData(
-                self.pillar, grantees=self.grantees[:batch_size])
+            apgfs = getUtility(IAccessPolicyGrantFlatSource)
+            sharees = apgfs.findGranteePermissionsByPolicy(
+                [self.access_policy], self.grantees[:batch_size])
+            sharing_service = getUtility(IService, 'sharing')
+            sharee_data = sharing_service.jsonShareeData(sharees)
             self.assertContentEqual(
-                observers, cache.objects.get('sharee_data'))
+                sharee_data, cache.objects.get('sharee_data'))
 
     def test_view_batch_data(self):
         # Test the expected batching data is in the json request cache.
@@ -220,7 +224,7 @@
             view = create_initialized_view(self.pillar, name='+sharing')
             cache = IJSONRequestCache(view.request)
             # Test one expected data value (there are many).
-            next_batch = view.shareeData().batch.nextBatch()
+            next_batch = view.sharees.batch.nextBatch()
             self.assertContentEqual(
                 next_batch.range_memo, cache.objects.get('next')['memo'])
 
@@ -228,7 +232,7 @@
         # Test the view range factory is properly configured.
         with FeatureFixture(ENABLED_FLAG):
             view = create_initialized_view(self.pillar, name='+sharing')
-            range_factory = view.shareeData().batch.range_factory
+            range_factory = view.sharees.batch.range_factory
 
             def test_range_factory():
                 row = range_factory.resultset.get_plain_result_set()[0]

=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py	2012-03-21 10:39:00 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py	2012-03-22 13:04:20 +0000
@@ -232,10 +232,10 @@
         :param grantees: if not None, the result only includes people in the
             specified list of grantees.
         :return: a collection of (`IPerson`, `IAccessPolicy`, permission)
-            where permission is a SharingPermission value.
-            'ALL' means the person has an access policy grant and can see all
+            where permission is a SharingPermission enum value.
+            ALL means the person has an access policy grant and can see all
             artifacts for the associated pillar.
-            'SOME' means the person only has specified access artifact grants.
+            SOME means the person only has specified access artifact grants.
         """
 
     def findArtifactsByGrantee(grantee, policies):

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-03-21 20:39:00 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-03-22 13:04:20 +0000
@@ -60,8 +60,19 @@
     @operation_parameters(
         pillar=Reference(IPillar, title=_('Pillar'), required=True))
     @operation_for_version('devel')
-    def getPillarShareeData(pillar, grantees=None):
-        """Return people/teams who can see pillar artifacts.
+    def getPillarShareeData(pillar):
+        """Return people/teams who can see pillar artifacts.
+
+        The result records are json data which includes:
+            - person name
+            - permissions they have for each information type.
+        """
+
+    def jsonShareeData(grant_permissions):
+        """Return people/teams who can see pillar artifacts.
+
+        :param grant_permissions: a list of (grantee, accesspolicy, permission)
+            tuples.
 
         The result records are json data which includes:
             - person name

=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py	2012-03-21 10:39:00 +0000
+++ lib/lp/registry/model/accesspolicy.py	2012-03-22 13:04:20 +0000
@@ -25,7 +25,10 @@
 from zope.component import getUtility
 from zope.interface import implements
 
-from lp.registry.enums import InformationType
+from lp.registry.enums import (
+    InformationType,
+    SharingPermission,
+    )
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifact,
     IAccessArtifactGrant,
@@ -37,6 +40,7 @@
     )
 from lp.registry.model.person import Person
 from lp.services.database.bulk import create
+from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.enumcol import DBEnum
 from lp.services.database.lpstorm import IStore
 from lp.services.database.stormbase import StormBase
@@ -350,13 +354,39 @@
     def findGranteePermissionsByPolicy(cls, policies, grantees=None):
         """See `IAccessPolicyGrantFlatSource`."""
         ids = [policy.id for policy in policies]
-        sharing_permission_term = SQL("""
-            CASE(
-                MIN(COALESCE(artifact, 0)))
-            WHEN 0 THEN 'ALL'
-            ELSE 'SOME'
-            END
-        """)
+
+        # A cache for the sharing permissions, keyed on (grantee.id, policy.id)
+        permissions_cache = {}
+
+        def set_permission(row):
+            # row contains (grantee.id, policy.id, permission_placeholder)
+            # Lookup the permission from the previously loaded cache.
+            return (
+                row[0], row[1], permissions_cache.get((row[0].id, row[1].id)))
+
+        def load_permissions(rows):
+            # We now have the grantees and policies we want in the result so
+            # load any corresponding permissions and cache them.
+            person_ids = set(row[0].id for row in rows)
+            policy_ids = set(row[1].id for row in rows)
+            sharing_permission_term = SQL("""
+                CASE(
+                    MIN(COALESCE(artifact, 0)))
+                WHEN 0 THEN '%s'
+                ELSE '%s'
+                END
+            """% (SharingPermission.ALL.name, SharingPermission.SOME.name))
+            constraints = [
+                cls.grantee_id.is_in(person_ids),
+                cls.policy_id.is_in(policy_ids)]
+            result_set = IStore(cls).find(
+                (cls.grantee_id, cls.policy_id, sharing_permission_term),
+                *constraints).group_by(cls.grantee_id, cls.policy_id)
+            for (person_id, policy_id, permission) in result_set:
+                permissions_cache[(person_id, policy_id)] = (
+                    SharingPermission.items[permission])
+
+        # The main result set has a placeholder for permission.
         constraints = [
             Person.id == cls.grantee_id,
             AccessPolicy.id == cls.policy_id,
@@ -364,9 +394,13 @@
         if grantees:
             grantee_ids = [grantee.id for grantee in grantees]
             constraints.append(cls.grantee_id.is_in(grantee_ids))
-        return IStore(cls).find(
-            (Person, AccessPolicy, sharing_permission_term),
-            *constraints).group_by(Person, AccessPolicy)
+        result_set = IStore(cls).find(
+            (Person, AccessPolicy,
+             SQL("'%s' as permission" % SharingPermission.NOTHING.name)),
+            *constraints).config(distinct=True)
+        return DecoratedResultSet(
+            result_set,
+            result_decorator=set_permission, pre_iter_hook=load_permissions)
 
     @classmethod
     def findArtifactsByGrantee(cls, grantee, policies):

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-03-22 02:13:42 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-03-22 13:04:20 +0000
@@ -102,21 +102,20 @@
         # XXX 2012-03-22 wallyworld bug 961836
         # We want to use person_sort_key(Person.displayname, Person.name) but
         # StormRangeFactory doesn't support that yet.
-        grantees = ap_grant_flat.findGranteesByPolicy(
+        grant_permissions = ap_grant_flat.findGranteePermissionsByPolicy(
             policies).order_by(Person.displayname, Person.name)
-        return grantees
+        return grant_permissions
 
     @available_with_permission('launchpad.Driver', 'pillar')
-    def getPillarShareeData(self, pillar, grantees=None):
+    def getPillarShareeData(self, pillar):
         """See `ISharingService`."""
-        policies = getUtility(IAccessPolicySource).findByPillar([pillar])
-        ap_grant_flat = getUtility(IAccessPolicyGrantFlatSource)
-        # XXX 2012-03-22 wallyworld bug 961836
-        # We want to use person_sort_key(Person.displayname, Person.name) but
-        # StormRangeFactory doesn't support that yet.
-        grant_permissions = ap_grant_flat.findGranteePermissionsByPolicy(
-            policies, grantees).order_by(Person.displayname, Person.name)
+        grant_permissions = list(self.getPillarSharees(pillar))
+        if not grant_permissions:
+            return None
+        return self.jsonShareeData(grant_permissions)
 
+    def jsonShareeData(self, grant_permissions):
+        """See `ISharingService`."""
         result = []
         person_by_id = {}
         request = get_current_web_service_request()
@@ -133,7 +132,8 @@
                 person_by_id[grantee.id] = person_data
                 result.append(person_data)
             person_data = person_by_id[grantee.id]
-            person_data['permissions'][policy.type.name] = sharing_permission
+            person_data['permissions'][policy.type.name] = (
+                sharing_permission.name)
         return result
 
     @available_with_permission('launchpad.Edit', 'pillar')
@@ -198,10 +198,13 @@
             self.deletePillarSharee(pillar, sharee, info_types_for_nothing)
 
         # Return sharee data to the caller.
-        sharees = self.getPillarShareeData(pillar, [sharee])
-        if not sharees:
+        ap_grant_flat = getUtility(IAccessPolicyGrantFlatSource)
+        grant_permissions = list(ap_grant_flat.findGranteePermissionsByPolicy(
+            all_pillar_policies, [sharee]))
+        if not grant_permissions:
             return None
-        return sharees[0]
+        [sharee] = self.jsonShareeData(grant_permissions)
+        return sharee
 
     @available_with_permission('launchpad.Edit', 'pillar')
     def deletePillarSharee(self, pillar, sharee,
@@ -233,9 +236,9 @@
 
         # Second delete any access artifact grants.
         ap_grant_flat = getUtility(IAccessPolicyGrantFlatSource)
-        to_delete = ap_grant_flat.findArtifactsByGrantee(
-            sharee, pillar_policies)
-        if to_delete.count() > 0:
+        to_delete = list(ap_grant_flat.findArtifactsByGrantee(
+            sharee, pillar_policies))
+        if len(to_delete) > 0:
             accessartifact_grant_source = getUtility(
                 IAccessArtifactGrantSource)
             accessartifact_grant_source.revokeByArtifact(to_delete)

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-03-21 20:39:00 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-03-22 13:04:20 +0000
@@ -123,6 +123,19 @@
             distro,
             [InformationType.EMBARGOEDSECURITY, InformationType.USERDATA])
 
+    def test_jsonShareeData(self):
+        # jsonShareeData returns the expected data.
+        access_policy = self.factory.makeAccessPolicy()
+        grantee = self.factory.makePerson()
+        sharees = self.service.jsonShareeData(
+            [(grantee, access_policy, SharingPermission.ALL),
+            (grantee, access_policy, SharingPermission.SOME)])
+        expected_data = self._makeShareeData(
+            grantee,
+            [(access_policy.type, SharingPermission.ALL),
+            (access_policy.type, SharingPermission.SOME)])
+        self.assertContentEqual([expected_data], sharees)
+
     def _assert_getPillarShareeData(self, pillar):
         # getPillarShareeData returns the expected data.
         access_policy = self.factory.makeAccessPolicy(
@@ -160,9 +173,15 @@
         login_person(driver)
         self._assert_getPillarShareeData(distro)
 
-    def test_getPillarShareeDataQueryCount(self):
-        # getPillarShareeData only should use 2 queries regardless of how many
-        # sharees are returned.
+    def _assert_QueryCount(self, func):
+        """ getPillarSharees[Data] only should use 3 queries.
+
+        1. load access policies for pillar
+        2. load sharees
+        3. load permissions for sharee
+
+        Steps 2 and 3 are split out to allow batching on persons.
+        """
         driver = self.factory.makePerson()
         product = self.factory.makeProduct(driver=driver)
         login_person(driver)
@@ -184,37 +203,22 @@
         for x in range(5):
             makeGrants()
         with StormStatementRecorder() as recorder:
-            sharees = self.service.getPillarShareeData(product)
+            sharees = list(func(product))
         self.assertEqual(10, len(sharees))
-        self.assertThat(recorder, HasQueryCount(Equals(2)))
+        self.assertThat(recorder, HasQueryCount(Equals(3)))
         # Make some more grants and check again.
         for x in range(5):
             makeGrants()
         with StormStatementRecorder() as recorder:
-            sharees = self.service.getPillarShareeData(product)
+            sharees = list(func(product))
         self.assertEqual(20, len(sharees))
-        self.assertThat(recorder, HasQueryCount(Equals(2)))
-
-    def test_getPillarShareeData_filter_grantees(self):
-        # getPillarShareeData only returns grantees in the specified list.
-        driver = self.factory.makePerson()
-        pillar = self.factory.makeProduct(driver=driver)
-        login_person(driver)
-        access_policy = self.factory.makeAccessPolicy(
-            pillar=pillar,
-            type=InformationType.PROPRIETARY)
-        grantee_in_result = self.factory.makePerson()
-        grantee_not_in_result = self.factory.makePerson()
-        self.factory.makeAccessPolicyGrant(access_policy, grantee_in_result)
-        self.factory.makeAccessPolicyGrant(
-            access_policy, grantee_not_in_result)
-
-        sharees = self.service.getPillarShareeData(pillar, [grantee_in_result])
-        expected_sharees = [
-            self._makeShareeData(
-                grantee_in_result,
-                [(InformationType.PROPRIETARY, SharingPermission.ALL)])]
-        self.assertContentEqual(expected_sharees, sharees)
+        self.assertThat(recorder, HasQueryCount(Equals(3)))
+
+    def test_getPillarShareesQueryCount(self):
+        self._assert_QueryCount(self.service.getPillarSharees)
+
+    def test_getPillarShareeDataQueryCount(self):
+        self._assert_QueryCount(self.service.getPillarShareeData)
 
     def _assert_getPillarShareeDataUnauthorized(self, pillar):
         # getPillarShareeData raises an Unauthorized exception if the user is
@@ -251,7 +255,9 @@
             artifact=artifact_grant.abstract_artifact, policy=access_policy)
 
         sharees = self.service.getPillarSharees(pillar)
-        expected_sharees = [grantee, artifact_grant.grantee]
+        expected_sharees = [
+            (grantee, access_policy, SharingPermission.ALL),
+            (artifact_grant.grantee, access_policy, SharingPermission.SOME)]
         self.assertContentEqual(expected_sharees, sharees)
 
     def test_getProductSharees(self):
@@ -347,9 +353,14 @@
         expected_sharee_data = self._makeShareeData(
             sharee, expected_permissions)
         self.assertEqual(expected_sharee_data, sharee_data)
-        # Check that getPillarShareeData returns what we expect.
-        [sharee_data] = self.service.getPillarShareeData(pillar)
-        self.assertEqual(expected_sharee_data, sharee_data)
+        # Check that getPillarSharees returns what we expect.
+        expected_sharee_grants = [
+            (sharee, policy, permission)
+            for policy, permission in [
+                (es_policy, SharingPermission.ALL),
+                (ud_policy, SharingPermission.SOME)]]
+        sharee_grants = list(self.service.getPillarSharees(pillar))
+        self.assertContentEqual(expected_sharee_grants, sharee_grants)
 
     def test_updateProjectGroupSharee_not_allowed(self):
         # We cannot add sharees to ProjectGroups.
@@ -455,18 +466,18 @@
         if types_to_delete is not None:
             expected_information_types = (
                 set(information_types).difference(types_to_delete))
-            remaining_grantee_person_data = self._makeShareeData(
-                grantee,
-                [(info_type, SharingPermission.ALL)
-                for info_type in expected_information_types])
-
-            expected_data.append(remaining_grantee_person_data)
-        # Add the data for the other sharee.
-        another_person_data = self._makeShareeData(
-            another, [(information_types[0], SharingPermission.ALL)])
+            expected_policies = [
+                access_policy for access_policy in access_policies
+                if access_policy.type in expected_information_types]
+            expected_data = [
+                (grantee, policy, SharingPermission.ALL)
+                for policy in expected_policies]
+        # Add the expected data for the other sharee.
+        another_person_data = (
+            another, access_policies[0], SharingPermission.ALL)
         expected_data.append(another_person_data)
         self.assertContentEqual(
-            expected_data, self.service.getPillarShareeData(pillar))
+            expected_data, self.service.getPillarSharees(pillar))
 
     def test_deleteProductShareeAll(self):
         # Users with launchpad.Edit can delete all access for a sharee.

=== modified file 'lib/lp/registry/templates/pillar-sharing.pt'
--- lib/lp/registry/templates/pillar-sharing.pt	2012-03-19 14:03:51 +0000
+++ lib/lp/registry/templates/pillar-sharing.pt	2012-03-22 13:04:20 +0000
@@ -34,7 +34,7 @@
       <li><a id="audit-link" class="sprite info" href='#'>Audit sharing</a></li>
     </ul>
 
-    <div tal:define="batch_navigator view/shareeData">
+    <div tal:define="batch_navigator view/sharees">
       <tal:shareelisting content="structure batch_navigator/@@+sharee-table-view" />
     </div>
 

=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py	2012-03-21 10:39:00 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py	2012-03-22 13:04:20 +0000
@@ -7,7 +7,10 @@
 from testtools.matchers import AllMatch
 from zope.component import getUtility
 
-from lp.registry.enums import InformationType
+from lp.registry.enums import (
+    InformationType,
+    SharingPermission,
+    )
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifact,
     IAccessArtifactGrant,
@@ -467,7 +470,7 @@
             apgfs.findGranteesByPolicy([
                 policy, another_policy, policy_with_no_grantees]))
 
-    def findGranteePermissionsByPolicy(self):
+    def test_findGranteePermissionsByPolicy(self):
         # findGranteePermissionsByPolicy() returns anyone with a grant for any
         # of the policies or the policies' artifacts.
         apgfs = getUtility(IAccessPolicyGrantFlatSource)
@@ -477,14 +480,14 @@
         policy = self.factory.makeAccessPolicy()
         policy_grant = self.factory.makeAccessPolicyGrant(policy=policy)
         self.assertContentEqual(
-            [(policy_grant.grantee, policy, 'ALL')],
+            [(policy_grant.grantee, policy, SharingPermission.ALL)],
             apgfs.findGranteePermissionsByPolicy(
                 [policy, policy_with_no_grantees]))
 
         # But not people with grants on artifacts.
         artifact_grant = self.factory.makeAccessArtifactGrant()
         self.assertContentEqual(
-            [(policy_grant.grantee, policy, 'ALL')],
+            [(policy_grant.grantee, policy, SharingPermission.ALL)],
             apgfs.findGranteePermissionsByPolicy(
                 [policy, policy_with_no_grantees]))
 
@@ -493,8 +496,8 @@
         self.factory.makeAccessPolicyArtifact(
             artifact=artifact_grant.abstract_artifact, policy=another_policy)
         self.assertContentEqual(
-            [(policy_grant.grantee, policy, 'ALL'),
-            (artifact_grant.grantee, another_policy, 'SOME')],
+            [(policy_grant.grantee, policy, SharingPermission.ALL),
+            (artifact_grant.grantee, another_policy, SharingPermission.SOME)],
             apgfs.findGranteePermissionsByPolicy([
                 policy, another_policy, policy_with_no_grantees]))
 
@@ -513,7 +516,7 @@
         self.factory.makeAccessPolicyGrant(
             policy=policy, grantee=grantee_not_in_result)
         self.assertContentEqual(
-            [(policy_grant.grantee, policy, 'ALL')],
+            [(policy_grant.grantee, policy, SharingPermission.ALL)],
             apgfs.findGranteePermissionsByPolicy(
                 [policy], [grantee_in_result]))
 

_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Reply via email to