launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/some-sharing-2-950562 into lp:launchpad with lp:~wallyworld/launchpad/some-sharing-950562 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #950562 in Launchpad itself: "Sharing service needs to support 'Some'"
  https://bugs.launchpad.net/launchpad/+bug/950562

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/some-sharing-2-950562/+merge/97144

== Implementation ==

Follow on from previous branch to support sharing permission = 'some'
Simply add the new code to sharePillarInformation()

Relevant doc:
    :param permissions: a dict of {InformationType: SharingPermission}
        if SharingPermission is ALL, then create an access policy grant
        if SharingPermission is SOME, then remove any access policy grants
        if SharingPermission is NONE, then remove all grants for the access
        policy

== Tests ==

Update the test_sharePillarInformation tests

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/some-sharing-2-950562/+merge/97144
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/some-sharing-2-950562 into lp:launchpad.

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-03-13 05:30:32 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-03-13 05:30:32 +0000
@@ -106,45 +106,67 @@
         # We do not support adding sharees to project groups.
         assert not IProjectGroup.providedBy(pillar)
 
+        # Separate out the info types according to permission.
         information_types = permissions.keys()
-        pillar_info_types = [
+        info_types_for_all = [
+                    info_type for info_type in information_types
+                    if permissions[info_type] == SharingPermission.ALL]
+        info_types_for_some = [
+                    info_type for info_type in information_types
+                    if permissions[info_type] == SharingPermission.SOME]
+        info_types_for_nothing = [
+                    info_type for info_type in information_types
+                    if permissions[info_type] == SharingPermission.NOTHING]
+
+        # The wanted policies are for the information_types in all.
+        required_pillar_info_types = [
             (pillar, information_type)
-            for information_type in information_types]
+            for information_type in information_types
+            if information_type in info_types_for_all]
         policy_source = getUtility(IAccessPolicySource)
-        pillar_policies = list(policy_source.find(pillar_info_types))
+        wanted_pillar_policies = policy_source.find(required_pillar_info_types)
 
-        # We have the policies, we need to figure out which grants we need to
-        # create. We also need to revoke any grants which are not required.
+        # We need to figure out which policy grants to create or delete.
         policy_grant_source = getUtility(IAccessPolicyGrantSource)
-        policy_grants = [(policy, sharee) for policy in pillar_policies]
-        existing_grants = [
+        wanted_policy_grants = [(policy, sharee)
+                        for policy in wanted_pillar_policies
+                        if policy.type in info_types_for_all]
+        existing_policy_grants = [
             (grant.policy, grant.grantee)
-            for grant in policy_grant_source.find(policy_grants)]
-        required_grants = set(policy_grants).difference(existing_grants)
+            for grant in policy_grant_source.find(wanted_policy_grants)]
+        # Create any newly required policy grants.
+        policy_grants_to_create = (
+            set(wanted_policy_grants).difference(existing_policy_grants))
+        if len(policy_grants_to_create) > 0:
+            policy_grant_source.grant(
+                [(policy, sharee, user)
+                for policy, sharee in policy_grants_to_create])
 
+        # Now revoke any existing policy grants for types with
+        # permission 'some'.
         all_pillar_policies = policy_source.findByPillar([pillar])
-        possible_policy_grants = [
-            (policy, sharee) for policy in all_pillar_policies]
-        possible_grants = [
-            (grant.policy, grant.grantee)
-            for grant in policy_grant_source.find(possible_policy_grants)]
+        policy_grants_to_revoke = [
+            (policy, sharee)
+            for policy in all_pillar_policies
+            if policy.type in info_types_for_some]
+        if len(policy_grants_to_revoke) > 0:
+            policy_grant_source.revoke(policy_grants_to_revoke)
 
-        grants_to_revoke = set(possible_grants).difference(policy_grants)
-        # Create any newly required grants.
-        if len(required_grants) > 0:
-            policy_grant_source.grant([(policy, sharee, user)
-                                    for policy, sharee in required_grants])
-        # Now revoke any existing grants no longer required.
-        if len(grants_to_revoke) > 0:
-            policy_grant_source.revoke(grants_to_revoke)
+        # For information types with permission 'nothing', we can simply
+        # call the deletePillarSharee method directly.
+        if len(info_types_for_nothing) > 0:
+            self.deletePillarSharee(pillar, sharee, info_types_for_nothing)
 
         # Return sharee data to the caller.
         request = get_current_web_service_request()
         resource = EntryResource(sharee, request)
         person_data = resource.toDataForJSON()
-        permission_data = {}
-        for (information_type, permission) in permissions.items():
-            permission_data[information_type.name] = permission.name
+        valid_info_types = (
+            set(information_types).difference(info_types_for_nothing))
+        permission_data = dict(
+            (information_type.name, permissions[information_type].name)
+            for information_type in valid_info_types
+        )
         person_data['permissions'] = permission_data
         return person_data
 

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-03-13 05:30:32 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-03-13 05:30:32 +0000
@@ -53,7 +53,7 @@
         sharee_data = resource.toDataForJSON()
         permissions = {}
         for (policy, permission) in policy_permissions:
-            permissions[policy.name] = permission.name
+            permissions[policy.name] = unicode(permission.name)
         sharee_data['permissions'] = permissions
         return sharee_data
 
@@ -164,35 +164,58 @@
         # Make existing grants to ensure sharePillarInformation handles those
         # cases correctly.
         # First, a grant that is in the add set - it wil be retained.
-        policy = getUtility(IAccessPolicySource).find(((
+        es_policy = getUtility(IAccessPolicySource).find(((
             pillar, InformationType.EMBARGOEDSECURITY),))[0]
+        ud_policy = getUtility(IAccessPolicySource).find(((
+            pillar, InformationType.USERDATA),))[0]
         self.factory.makeAccessPolicyGrant(
-            policy, grantee=sharee, grantor=grantor)
-        # Second, a grant that is not in the add set - it will be deleted.
-        policy = self.factory.makeAccessPolicy(
+            es_policy, grantee=sharee, grantor=grantor)
+        # Second, grants that are not in the all set - they will be deleted.
+        p_policy = self.factory.makeAccessPolicy(
             pillar=pillar, type=InformationType.PROPRIETARY)
         self.factory.makeAccessPolicyGrant(
-            policy, grantee=sharee, grantor=grantor)
+            p_policy, grantee=sharee, grantor=grantor)
+        self.factory.makeAccessPolicyGrant(
+            ud_policy, grantee=sharee, grantor=grantor)
+
+        # We also make some artifact grants.
+        # First, a grant which will be retained.
+        artifact = self.factory.makeAccessArtifact()
+        self.factory.makeAccessArtifactGrant(artifact, sharee)
+        self.factory.makeAccessPolicyArtifact(
+            artifact=artifact, policy=es_policy)
+        # Second, grants which will be deleted because their policies have
+        # information types in the 'some' or 'nothing' category.
+        artifact = self.factory.makeAccessArtifact()
+        self.factory.makeAccessArtifactGrant(artifact, sharee)
+        self.factory.makeAccessPolicyArtifact(
+            artifact=artifact, policy=p_policy)
+        artifact = self.factory.makeAccessArtifact()
+        self.factory.makeAccessArtifactGrant(artifact, sharee)
+        self.factory.makeAccessPolicyArtifact(
+            artifact=artifact, policy=ud_policy)
 
         # Now call sharePillarInformation will the grants we want.
         permissions = {
             InformationType.EMBARGOEDSECURITY: SharingPermission.ALL,
-            InformationType.USERDATA: SharingPermission.SOME}
+            InformationType.USERDATA: SharingPermission.SOME,
+            InformationType.PROPRIETARY: SharingPermission.NOTHING}
         sharee_data = self.service.sharePillarInformation(
             pillar, sharee, permissions, grantor)
         policies = getUtility(IAccessPolicySource).findByPillar([pillar])
         policy_grant_source = getUtility(IAccessPolicyGrantSource)
-        grants = policy_grant_source.findByPolicy(policies)
-        self.assertEqual(grants.count(), len(permissions))
-        for grant in grants:
-            self.assertEqual(grantor, grant.grantor)
-            self.assertEqual(sharee, grant.grantee)
+        [grant] = policy_grant_source.findByPolicy(policies)
+        self.assertEqual(grantor, grant.grantor)
+        self.assertEqual(sharee, grant.grantee)
         expected_permissions = [
             (InformationType.EMBARGOEDSECURITY, SharingPermission.ALL),
             (InformationType.USERDATA, SharingPermission.SOME)]
         expected_sharee_data = self._makeShareeData(
             sharee, expected_permissions)
         self.assertEqual(expected_sharee_data, sharee_data)
+        # Check that getPillarSharees returns what we expect.
+        [sharee_data] = self.service.getPillarSharees(pillar)
+        self.assertEqual(expected_sharee_data, sharee_data)
 
     def test_updateProjectGroupSharee_not_allowed(self):
         # We cannot add sharees to ProjectGroups.