launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #956634 in Launchpad itself: "Sharing view scalability"
  https://bugs.launchpad.net/launchpad/+bug/956634

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-view-scalability-956634/+merge/97793

== 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
-- 
https://code.launchpad.net/~wallyworld/launchpad/sharing-view-scalability-956634/+merge/97793
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-view-scalability-956634 into lp:launchpad.

=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js	2012-03-15 08:54:40 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js	2012-03-16 04:21:21 +0000
@@ -307,8 +307,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 04:02:44 +0000
+++ lib/lp/registry/javascript/sharing/shareetable.js	2012-03-16 04:21:21 +0000
@@ -114,7 +114,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/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-03-15 08:54:40 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-03-16 04:21:21 +0000
@@ -8,11 +8,12 @@
     '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.traversing.browser.absoluteurl import absoluteURL
 
 from lp.registry.enums import (
     InformationType,
@@ -91,9 +92,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-15 08:54:40 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-03-16 04:21:21 +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 (
@@ -26,6 +28,7 @@
 from lp.testing import (
     login,
     login_person,
+    StormStatementRecorder,
     TestCaseWithFactory,
     WebServiceTestCase,
     ws_object,
@@ -34,6 +37,7 @@
     AppServerLayer,
     DatabaseFunctionalLayer,
     )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import LaunchpadWebServiceCaller
 
 
@@ -49,8 +53,13 @@
     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,
+            '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)
@@ -135,6 +144,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()