Merge lp:~wallyworld/launchpad/sharing-view-batching4-957663 into lp:launchpad

[Ian Booth]

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

== 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

[Richard Harding (rharding)]

review: Approve (code*)

[j.c.sackett (jcsackett)]

Hey Ian--

This mostly looks good; I just have one question below.

> === 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):

I'm not sure I understand the logic of making these properties; don't we
usually avoid that for things that might be expensive or slow, like
unbatched_sharees?

review: Needs Information

[Ian Booth (wallyworld)]

>
>> === 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):
>
> I'm not sure I understand the logic of making these properties; don't we
> usually avoid that for things that might be expensive or slow, like
> unbatched_sharees?
>

Good question. The property only returns an unpopulated Storm result set
and so is not expensive to implement. The result set is lazily populated
when the first batch is retrieved. The code base seems inconsistent in
this area with regard to what's done. Since it's caused a question to be
asked, I'll change it back to avoid further confusion.

[j.c.sackett (jcsackett)]

> Since it's caused a question to be asked, I'll change it back to avoid further confusion.

Dig, per our discussion on mumble, go with your heart on this--there's no clear guideline either way for a view.

review: Approve

=== 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 22:33:25 +0000
@@ -303,15 +303,15 @@
             size=config.launchpad.default_batch_size,
             range_factory=StormRangeFactory(sharees))
 
-    def shareeData(self):
-        """Return an `ITableBatchNavigator` for sharees."""
+    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."""
+    def unbatched_sharees(self):
+        """All the sharees for a pillar."""
         return self._getSharingService().getPillarSharees(self.context)
 
     def initialize(self):
@@ -334,10 +334,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 22:33:25 +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 22:33:25 +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 22:33:25 +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 22:33:25 +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 22:33:25 +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 22:33:25 +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 22:33:25 +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 22:33:25 +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]))