launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/getSharedArtifacts-data-1049374 into lp:launchpad.

Requested reviews:
  Curtis Hovey (sinzui)
Related bugs:
  Bug #1049374 in Launchpad itself: "Resources returned by getSharedArtifacts not adapted to resource object type"
  https://bugs.launchpad.net/launchpad/+bug/1049374

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/getSharedArtifacts-data-1049374/+merge/124099

== Implementation ==

The lazr restful infrastructure does not support fully deserialising collections containing heterogeneous elements, or non-trivial named operation return types like tuples of lists. In such cases, the return data contains json dicts of the attribute values for each item, but not web service Entry objects constructed from said data.

There are two sharing service methods recently exported:

getSharedArtifacts
getVisibleArtifacts

These were used internally first, and exported later.

Given the issues above, and the desire to return collections of web service Entry objects:

1. getSharedArtifacts and getVisibleArtifacts were unexported
2. getSharedBugs and getSharedBranches were added to the sharing service and exported

getSharedArtifacts is used internally to populate the model for the sharing details view. getSharedBugs and getSharedBranches just call this method and only return the relevant list of items.

AFAIK, getVisibleArtifacts was not used in any scripts - it was only exported because it was there.

Scripts which call getSharedArtifacts will now need to call getSharedBugs and getSharedBranches instead.

== Tests ==

Update the sharing service tests.

== 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/getSharedArtifacts-data-1049374/+merge/124099
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-06 10:59:56 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-09-13 04:35:24 +0000
@@ -18,7 +18,7 @@
     operation_for_version,
     operation_parameters,
     REQUEST_USER,
-    )
+    operation_returns_collection_of)
 from lazr.restful.fields import Reference
 from zope.schema import (
     Choice,
@@ -62,12 +62,6 @@
         information type.
         """
 
-    @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')
     def getSharedArtifacts(pillar, person, user):
         """Return the artifacts shared between the pillar and person.
 
@@ -82,13 +76,39 @@
         """
 
     @export_read_operation()
-    @operation_parameters(
-        person=Reference(IPerson, title=_('Person'), required=True),
-        branches=List(
-            Reference(schema=IBranch), title=_('Branches'), required=False),
-        bugs=List(
-            Reference(schema=IBug), title=_('Bugs'), required=False))
-    @operation_for_version('devel')
+    @call_with(user=REQUEST_USER)
+    @operation_parameters(
+        pillar=Reference(IPillar, title=_('Pillar'), required=True),
+        person=Reference(IPerson, title=_('Person'), required=True))
+    @operation_returns_collection_of(IBug)
+    @operation_for_version('devel')
+    def getSharedBugs(pillar, person, user):
+        """Return the bugs shared between the pillar and person.
+
+        The result includes bugtasks rather than bugs since this is what the
+        pillar filtering is applied to. The shared bug can be obtained simply
+        by reading the bugtask.bug attribute.
+
+        :param user: the user making the request. Only bugs visible to the
+             user will be included in the result.
+        :return: a collection of bug tasks.
+        """
+
+    @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_returns_collection_of(IBranch)
+    @operation_for_version('devel')
+    def getSharedBranches(pillar, person, user):
+        """Return the branches shared between the pillar and person.
+
+        :param user: the user making the request. Only branches visible to the
+             user will be included in the result.
+        :return: a collection of branches
+        """
+
     def getVisibleArtifacts(person, branches=None, bugs=None):
         """Return the artifacts shared with person.
 

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-09-06 10:59:56 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-09-13 04:35:24 +0000
@@ -113,16 +113,17 @@
         )
 
     @available_with_permission('launchpad.Driver', 'pillar')
-    def getSharedArtifacts(self, pillar, person, user):
+    def getSharedArtifacts(self, pillar, person, user, include_bugs=True,
+                           include_branches=True):
         """See `ISharingService`."""
         policies = getUtility(IAccessPolicySource).findByPillar([pillar])
         flat_source = getUtility(IAccessPolicyGrantFlatSource)
         bug_ids = set()
         branch_ids = set()
         for artifact in flat_source.findArtifactsByGrantee(person, policies):
-            if artifact.bug_id:
+            if artifact.bug_id and include_bugs:
                 bug_ids.add(artifact.bug_id)
-            elif artifact.branch_id:
+            elif artifact.branch_id and include_branches:
                 branch_ids.add(artifact.branch_id)
 
         # Load the bugs.
@@ -141,6 +142,20 @@
 
         return bugtasks, branches
 
+    @available_with_permission('launchpad.Driver', 'pillar')
+    def getSharedBugs(self, pillar, person, user):
+        """See `ISharingService`."""
+        bugtasks, ignore = self.getSharedArtifacts(
+            pillar, person, user, include_branches=False)
+        return bugtasks
+
+    @available_with_permission('launchpad.Driver', 'pillar')
+    def getSharedBranches(self, pillar, person, user):
+        """See `ISharingService`."""
+        ignore, branches = self.getSharedArtifacts(
+            pillar, person, user, include_bugs=False)
+        return branches
+
     def getVisibleArtifacts(self, person, branches=None, bugs=None,
                             ignore_permissions=False):
         """See `ISharingService`."""

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-09-10 10:20:39 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-09-13 04:35:24 +0000
@@ -1167,46 +1167,52 @@
         login_person(anyone)
         self._assert_updatePillarSharingPoliciesUnauthorized(anyone)
 
-    def test_getSharedArtifacts(self):
-        # Test the getSharedArtifacts method.
-        owner = self.factory.makePerson()
-        product = self.factory.makeProduct(owner=owner)
-        login_person(owner)
-
+    def create_shared_artifacts(self, product, grantee, user):
+        # Create some shared bugs and branches.
         bugs = []
         bug_tasks = []
         for x in range(0, 10):
             bug = self.factory.makeBug(
-                target=product, owner=owner,
+                target=product, owner=product.owner,
                 information_type=InformationType.USERDATA)
             bugs.append(bug)
             bug_tasks.append(bug.default_bugtask)
         branches = []
         for x in range(0, 10):
             branch = self.factory.makeBranch(
-                product=product, owner=owner,
+                product=product, owner=product.owner,
                 information_type=InformationType.USERDATA)
             branches.append(branch)
 
         # Grant access to grantee as well as the person who will be doing the
         # query. The person who will be doing the query is not granted access
         # to the last bug/branch so those won't be in the result.
-        grantee = self.factory.makePerson()
-        user = self.factory.makePerson()
-
         def grant_access(artifact, grantee_only):
             access_artifact = self.factory.makeAccessArtifact(
                 concrete=artifact)
             self.factory.makeAccessArtifactGrant(
-                artifact=access_artifact, grantee=grantee, grantor=owner)
+                artifact=access_artifact, grantee=grantee,
+                grantor=product.owner)
             if not grantee_only:
                 self.factory.makeAccessArtifactGrant(
-                    artifact=access_artifact, grantee=user, grantor=owner)
+                    artifact=access_artifact, grantee=user,
+                    grantor=product.owner)
 
         for i, bug in enumerate(bugs):
             grant_access(bug, i == 9)
         for i, branch in enumerate(branches):
             grant_access(branch, i == 9)
+        return bug_tasks, branches
+
+    def test_getSharedArtifacts(self):
+        # Test the getSharedArtifacts method.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        login_person(owner)
+        grantee = self.factory.makePerson()
+        user = self.factory.makePerson()
+        bug_tasks, branches = self.create_shared_artifacts(
+            product, grantee, user)
 
         # Check the results.
         shared_bugtasks, shared_branches = self.service.getSharedArtifacts(
@@ -1214,6 +1220,35 @@
         self.assertContentEqual(bug_tasks[:9], shared_bugtasks)
         self.assertContentEqual(branches[:9], shared_branches)
 
+    def test_getSharedBugs(self):
+        # Test the getSharedBugs method.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        login_person(owner)
+        grantee = self.factory.makePerson()
+        user = self.factory.makePerson()
+        bug_tasks, ignored = self.create_shared_artifacts(
+            product, grantee, user)
+
+        # Check the results.
+        shared_bugtasks = self.service.getSharedBugs(product, grantee, user)
+        self.assertContentEqual(bug_tasks[:9], shared_bugtasks)
+
+    def test_getSharedBranches(self):
+        # Test the getSharedBranches method.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        login_person(owner)
+        grantee = self.factory.makePerson()
+        user = self.factory.makePerson()
+        ignored, branches = self.create_shared_artifacts(
+            product, grantee, user)
+
+        # Check the results.
+        shared_branches = self.service.getSharedBranches(
+            product, grantee, user)
+        self.assertContentEqual(branches[:9], shared_branches)
+
     def test_getPeopleWithAccessBugs(self):
         # Test the getPeopleWithoutAccess method with bugs.
         owner = self.factory.makePerson()
@@ -1534,28 +1569,20 @@
                 InformationType.USERDATA.title: SharingPermission.ALL.title}
         )
 
-    def test_getSharedArtifacts(self):
+    def test_getSharedBugs(self):
+        # Test the exported getSharedBugs() method.
+        ws_pillar = ws_object(self.launchpad, self.pillar)
+        ws_grantee = ws_object(self.launchpad, self.grantee)
+        bugtasks = self.service.getSharedBugs(
+            pillar=ws_pillar, person=ws_grantee)
+        self.assertEqual(1, len(bugtasks))
+        self.assertEqual(bugtasks[0].title, self.bug.default_bugtask.title)
+
+    def test_getSharedBranches(self):
         # Test the exported getSharedArtifacts() method.
         ws_pillar = ws_object(self.launchpad, self.pillar)
         ws_grantee = ws_object(self.launchpad, self.grantee)
-        (bugtasks, branches) = self.service.getSharedArtifacts(
+        branches = self.service.getSharedBranches(
             pillar=ws_pillar, person=ws_grantee)
-        self.assertEqual(1, len(bugtasks))
-        self.assertEqual(1, len(branches))
-        self.assertEqual(bugtasks[0]['title'], self.bug.default_bugtask.title)
-        self.assertEqual(branches[0]['unique_name'], self.branch.unique_name)
-
-    def test_getVisibleArtifacts(self):
-        # Test the exported getVisibleArtifacts() method.
-        ws_grantee = ws_object(self.launchpad, self.grantee)
-        # Sadly lazr.restful doesn't know how to marshall lists of entities
-        # so we have to use links directly.
-        ws_bug_link = ws_object(self.launchpad, self.bug).self_link
-        ws_branch_link = ws_object(self.launchpad, self.branch).self_link
-        (bugs, branches) = self.service.getVisibleArtifacts(
-            person=ws_grantee,
-            branches=[ws_branch_link], bugs=[ws_bug_link])
-        self.assertEqual(1, len(bugs))
-        self.assertEqual(1, len(branches))
-        self.assertEqual(bugs[0]['title'], self.bug.title)
-        self.assertEqual(branches[0]['unique_name'], self.branch.unique_name)
+        self.assertEqual(1, len(branches))
+        self.assertEqual(branches[0].unique_name, self.branch.unique_name)