Merge lp:~wallyworld/launchpad/sharing-view-scalability-956634 into lp:launchpad

[Ian Booth]

Ensure the sharing service getPillarSharees uses a reasonable number of queries

== Implementation ==

Don't use lazr.restful marshalling - do it ourselves.
Query count is now 2, regardless of how may sharees are returned.

Drive by - ensure team icon is rendered.

== Tests ==

Add query count test to test_sharingservice

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/javascript/sharing/pillarsharingview.js
  lib/lp/registry/javascript/sharing/shareetable.js
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js	2012-03-16 02:55:42 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js	2012-03-16 08:48:19 +0000
@@ -324,8 +324,7 @@
                     if (!Y.Lang.isValue(sharee_entry)) {
                         self.remove_sharee_success(person_uri);
                     } else {
-                        self.save_sharing_selection_success(
-                            sharee_entry.getAttrs());
+                        self.save_sharing_selection_success(sharee_entry);
                     }
                 },
                 failure: error_handler.getFailureHandler()

=== modified file 'lib/lp/registry/javascript/sharing/shareetable.js'
--- lib/lp/registry/javascript/sharing/shareetable.js	2012-03-16 07:07:19 +0000
+++ lib/lp/registry/javascript/sharing/shareetable.js	2012-03-16 08:48:19 +0000
@@ -118,7 +118,7 @@
     _sharee_row_template: function() {
         return [
             '<tr id="permission-{{name}}" data-name="{{name}}"><td>',
-            '    <a href="{{web_link}}" class="sprite person">',
+            '    <a href="{{web_link}}" class="sprite {{meta}}">',
             '                          {{display_name}}',
             '    <span class="formHelp">{{role}}</span></a>',
             '</td>',

=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js	2012-03-16 01:42:53 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js	2012-03-16 08:48:19 +0000
@@ -302,7 +302,6 @@
                 expected_url, 'permissions', 'Policy 3,Nothing');
             Y.Assert.areEqual(expected_url, mockio.last_request.config.data);
             mockio.last_request.successJSON({
-                'resource_type_link': 'entity',
                 'name': 'joe',
                 'self_link': '~joe'});
             Y.Assert.isTrue(save_sharing_selection_success_called);
@@ -349,7 +348,6 @@
                 expected_url, 'permissions', 'Policy 1,S2');
             Y.Assert.areEqual(expected_url, mockio.last_request.config.data);
             mockio.last_request.successJSON({
-                'resource_type_link': 'entity',
                 'name': 'fred',
                 'self_link': '~fred'});
             Y.Assert.isTrue(save_sharing_selection_success_called);

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-03-16 08:11:47 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-03-16 08:48:19 +0000
@@ -8,12 +8,13 @@
     'SharingService',
     ]
 
-from lazr.restful import EntryResource
+from lazr.restful.interfaces import IWebBrowserOriginatingRequest
 from lazr.restful.utils import get_current_web_service_request
 
 from zope.component import getUtility
 from zope.interface import implements
 from zope.security.interfaces import Unauthorized
+from zope.traversing.browser.absoluteurl import absoluteURL
 
 from lp.registry.enums import (
     InformationType,
@@ -98,9 +99,14 @@
         request = get_current_web_service_request()
         for (grantee, policy, sharing_permission) in grant_permissions:
             if not grantee.id in person_by_id:
-                resource = EntryResource(grantee, request)
-                person_data = resource.toDataForJSON()
-                person_data['permissions'] = {}
+                person_data = {
+                    'name': grantee.name,
+                    'meta': 'team' if grantee.is_team else 'person',
+                    'display_name': grantee.displayname,
+                    'self_link': absoluteURL(grantee, request),
+                    'permissions': {}}
+                browser_request = IWebBrowserOriginatingRequest(request)
+                person_data['web_link'] = absoluteURL(grantee, browser_request)
                 person_by_id[grantee.id] = person_data
                 result.append(person_data)
             person_data = person_by_id[grantee.id]

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-03-16 06:47:43 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-03-16 08:48:19 +0000
@@ -4,11 +4,13 @@
 __metaclass__ = type
 
 
-from lazr.restful import EntryResource
+from lazr.restful.interfaces import IWebBrowserOriginatingRequest
 from lazr.restful.utils import get_current_web_service_request
+from testtools.matchers import Equals
 import transaction
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
+from zope.traversing.browser.absoluteurl import absoluteURL
 
 from lp.app.interfaces.services import IService
 from lp.registry.enums import (
@@ -27,6 +29,7 @@
 from lp.testing import (
     login,
     login_person,
+    StormStatementRecorder,
     TestCaseWithFactory,
     WebServiceTestCase,
     ws_object,
@@ -35,6 +38,7 @@
     AppServerLayer,
     DatabaseFunctionalLayer,
     )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import LaunchpadWebServiceCaller
 
 
@@ -53,8 +57,14 @@
     def _makeShareeData(self, sharee, policy_permissions):
         # Unpack a sharee into its attributes and add in permissions.
         request = get_current_web_service_request()
-        resource = EntryResource(sharee, request)
-        sharee_data = resource.toDataForJSON()
+        sharee_data = {
+            'name': sharee.name,
+            'meta': 'team' if sharee.is_team else 'person',
+            'display_name': sharee.displayname,
+            'self_link': absoluteURL(sharee, request),
+            'permissions': {}}
+        browser_request = IWebBrowserOriginatingRequest(request)
+        sharee_data['web_link'] = absoluteURL(sharee, browser_request)
         permissions = {}
         for (policy, permission) in policy_permissions:
             permissions[policy.name] = unicode(permission.name)
@@ -139,6 +149,41 @@
         login_person(driver)
         self._test_getPillarSharees(distro)
 
+    def test_getPillarShareesQueryCount(self):
+        # getPillarSharees only should use 2 queries regardless of how many
+        # sharees are returned.
+        driver = self.factory.makePerson()
+        product = self.factory.makeProduct(driver=driver)
+        login_person(driver)
+        access_policy = self.factory.makeAccessPolicy(
+            pillar=product,
+            type=InformationType.PROPRIETARY)
+
+        def makeGrants():
+            grantee = self.factory.makePerson()
+            # Make access policy grant so that 'All' is returned.
+            self.factory.makeAccessPolicyGrant(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)
+
+        # Make some grants and check the count.
+        for x in range(5):
+            makeGrants()
+        with StormStatementRecorder() as recorder:
+            sharees = self.service.getPillarSharees(product)
+        self.assertEqual(10, len(sharees))
+        self.assertThat(recorder, HasQueryCount(Equals(2)))
+        # Make some more grants and check again.
+        for x in range(5):
+            makeGrants()
+        with StormStatementRecorder() as recorder:
+            sharees = self.service.getPillarSharees(product)
+        self.assertEqual(20, len(sharees))
+        self.assertThat(recorder, HasQueryCount(Equals(2)))
+
     def test_getPillarSharees_filter_grantees(self):
         # getPillarSharees only returns grantees in the specified list.
         driver = self.factory.makePerson()