launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/sharee-display-details-1002954 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1002954 in Launchpad itself: "Sharing table does not show team icons"
  https://bugs.launchpad.net/launchpad/+bug/1002954

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharee-display-details-1002954/+merge/107912

== Implementation ==

Add the icon url to the json data sent when the client asks for grantee data to display.
Update the mustache template used to render the grantee row.
The table now shows the icon (if there is one) plus the Luanchpad id of the grantee.

== Demo and QA ==

http://people.canonical.com/~ianb/sharee-table-icons.png

== Tests ==

Update shareetable yui test.
Update sharing service jsonData tests.
Add 1 to view query count tests since there is a new query to cache the icon info.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/tests/test_pillar_sharing.py
  lib/lp/registry/javascript/sharing/shareetable.js
  lib/lp/registry/javascript/sharing/tests/test_shareetable.js
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/sharee-display-details-1002954/+merge/107912
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharee-display-details-1002954 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-05-16 01:01:43 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-05-30 05:17:20 +0000
@@ -369,7 +369,7 @@
             view = create_view(self.pillar, name='+sharing')
             with StormStatementRecorder() as recorder:
                 view.initialize()
-            self.assertThat(recorder, HasQueryCount(LessThan(6)))
+            self.assertThat(recorder, HasQueryCount(LessThan(7)))
 
     def test_view_write_enabled_without_feature_flag(self):
         # Test that sharing_write_enabled is not set without the feature flag.

=== modified file 'lib/lp/registry/javascript/sharing/shareetable.js'
--- lib/lp/registry/javascript/sharing/shareetable.js	2012-05-11 06:29:27 +0000
+++ lib/lp/registry/javascript/sharing/shareetable.js	2012-05-30 05:17:20 +0000
@@ -150,8 +150,13 @@
     _sharee_row_template: function() {
         return [
             '<tr id="permission-{{name}}" data-name="{{name}}"><td>',
-            '    <a href="{{web_link}}" class="sprite {{meta}}">',
-            '                          {{display_name}}',
+            '{{#image_url}}',
+            '    <a href="{{web_link}}"><img src="{{image_url}}"/>',
+            '{{/image_url}}',
+            '{{^image_url}}',
+            '    <a href="{{web_link}}" class="{{sprite_css}}">',
+            '{{/image_url}}',
+            '    {{display_name}} ({{name}})',
             '    <span class="formHelp">{{role}}</span></a>',
             '</td>',
             '<td class="action-icons nowrap">',

=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareetable.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareetable.js	2012-05-11 06:29:27 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareetable.js	2012-05-30 05:17:20 +0000
@@ -18,9 +18,11 @@
                     {'name': 'fred', 'display_name': 'Fred Bloggs',
                      'role': '(Maintainer)', web_link: '~fred',
                      'self_link': '~fred',
+                     'image_url': '', 'sprite_css': 'sprite person',
                      'permissions': {'P1': 's1', 'P2': 's2'}},
                     {'name': 'john.smith', 'display_name': 'John Smith',
                      'role': '', web_link: '~smith', 'self_link': '~smith',
+                     'image_url': 'smurf.png', 'sprite_css': 'sprite person',
                      'shared_items_exist': true,
                     'permissions': {'P1': 's1', 'P3': 's3'}}
                     ]
@@ -117,6 +119,7 @@
                 'name': 'joe', 'display_name': 'Joe Smith',
                 'role': '(Maintainer)', web_link: '~joe',
                 'self_link': '~joe',
+                'image_url': 'smurf.png', 'sprite_css': 'sprite person',
                 'permissions': {'P1': 's2'}};
             this.sharee_table.update_sharees([new_sharee]);
             Y.Assert.isNull(Y.one('tr#sharee-table-not-shared'));
@@ -128,6 +131,13 @@
             var sharee_row = Y.one('#sharee-table tr[id=permission-'
                 + sharee.name + ']');
             Y.Assert.isNotNull(sharee_row);
+            // The sprite or branding icon.
+            if (sharee.image_url !== '') {
+                Y.Assert.isNotNull(sharee_row.one('img[src="smurf.png"]'));
+                Y.Assert.isNull(sharee_row.one('.sprite.person'));
+            } else {
+                Y.Assert.isNotNull(sharee_row.one('.sprite.person'));
+            }
             // The update link
             Y.Assert.isNotNull(
                 Y.one('#sharee-table span[id=update-'
@@ -205,6 +215,7 @@
                 'name': 'joe', 'display_name': 'Joe Smith',
                 'role': '(Maintainer)', web_link: '~joe',
                 'self_link': '~joe',
+                'image_url': 'smurf.png', 'sprite_css': 'sprite person',
                 'permissions': {'P1': 's2'}};
             this.sharee_table.update_sharees([new_sharee]);
             this._test_sharee_rendered(new_sharee);
@@ -218,6 +229,7 @@
                 'name': 'fred', 'display_name': 'Fred Bloggs',
                 'role': '(Maintainer)', web_link: '~fred',
                 'self_link': '~fred',
+                'image_url': '', 'sprite_css': 'sprite person',
                 'permissions': {'P1': 's2', 'P2': 's1'}};
             this.sharee_table.update_sharees([updated_sharee]);
             this._test_sharee_rendered(updated_sharee);
@@ -296,6 +308,7 @@
                 'name': 'joe', 'display_name': 'Joe Smith',
                 'role': '(Maintainer)', web_link: '~joe',
                 'self_link': '~joe',
+                'image_url': 'smurf.png', 'sprite_css': 'sprite person',
                 'permissions': {'P1': 's2'}};
             sharee_data.splice(0, 0, new_sharee);
             // Update a record.
@@ -322,6 +335,7 @@
                 'name': 'joe', 'display_name': 'Joe Smith',
                 'role': '(Maintainer)', web_link: '~joe',
                 'self_link': '~joe',
+                'image_url': 'smurf.png', 'sprite_css': 'sprite person',
                 'permissions': {'P1': 's2'}};
             sharee_data.splice(0, 0, new_sharee);
             this.sharee_table.syncUI();
@@ -355,6 +369,7 @@
                 'name': 'joe', 'display_name': 'Joe Smith',
                 'role': '(Maintainer)', web_link: '~joe',
                 'self_link': '~joe',
+                'image_url': 'smurf.png', 'sprite_css': 'sprite person',
                 'permissions': {'P1': 's2'}};
             this.sharee_table.navigator.fire('updateContent', [new_sharee]);
             this._test_sharee_rendered(new_sharee);

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-05-16 02:36:30 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-05-30 05:17:20 +0000
@@ -15,6 +15,7 @@
 from zope.security.interfaces import Unauthorized
 from zope.traversing.browser.absoluteurl import absoluteURL
 
+from lp.app.browser.tales import ObjectImageDisplayAPI
 from lp.bugs.interfaces.bugtask import (
     BugTaskSearchParams,
     IBugTaskSet,
@@ -31,7 +32,7 @@
     IAccessPolicyGrantSource,
     IAccessPolicySource,
     )
-from lp.registry.interfaces.distribution import IDistribution
+from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.registry.interfaces.sharingservice import ISharingService
@@ -156,6 +157,10 @@
         browser_request = IWebBrowserOriginatingRequest(request)
         details_enabled = bool((getFeatureFlag(
             'disclosure.enhanced_sharing_details.enabled')))
+        # We need to precache icon and validity information for the batch.
+        grantee_ids = [grantee[0].id for grantee in grant_permissions]
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            grantee_ids, need_icon=True, need_validity=True))
         for (grantee, permissions, shared_artifact_types) in grant_permissions:
             some_things_shared = (
                 details_enabled and len(shared_artifact_types) > 0)
@@ -164,9 +169,13 @@
                 sharee_permissions[policy.type.name] = permission.name
             shared_artifact_type_names = [
                 info_type.name for info_type in shared_artifact_types]
+            display_api = ObjectImageDisplayAPI(grantee)
+            image_url = display_api.custom_icon_url() or None
+            sprite_css = display_api.sprite_css() or 'sprite bullet'
             result.append({
                 'name': grantee.name,
-                'meta': 'team' if grantee.is_team else 'person',
+                'image_url': image_url or '',
+                'sprite_css': sprite_css,
                 'display_name': grantee.displayname,
                 'self_link': absoluteURL(grantee, request),
                 'web_link': absoluteURL(grantee, browser_request),

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-05-15 08:16:09 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-05-30 05:17:20 +0000
@@ -43,7 +43,7 @@
     )
 from lp.testing.layers import (
     AppServerLayer,
-    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
     )
 from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import LaunchpadWebServiceCaller
@@ -58,7 +58,7 @@
 class TestSharingService(TestCaseWithFactory):
     """Tests for the SharingService."""
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def setUp(self):
         super(TestSharingService, self).setUp()
@@ -68,9 +68,15 @@
                         shared_artifact_types):
         # Unpack a sharee into its attributes and add in permissions.
         request = get_current_web_service_request()
+        sprite_css = 'sprite ' + ('team' if sharee.is_team else 'person')
+        if sharee.icon:
+            image_url = sharee.icon.getURL()
+        else:
+            image_url = ''
         sharee_data = {
             'name': sharee.name,
-            'meta': 'team' if sharee.is_team else 'person',
+            'image_url': image_url,
+            'sprite_css': sprite_css,
             'display_name': sharee.displayname,
             'self_link': absoluteURL(sharee, request),
             'permissions': {}}
@@ -194,6 +200,24 @@
             [(policy1.type, SharingPermission.ALL)], [])
         self.assertContentEqual([expected_data], sharees)
 
+    def test_jsonShareeData_with_icon(self):
+        # jsonShareeData returns the expected data for a grantee with has an
+        # icon.
+        product = self.factory.makeProduct()
+        [policy1, policy2] = getUtility(IAccessPolicySource).findByPillar(
+            [product])
+        icon = self.factory.makeLibraryFileAlias(
+            filename='smurf.png', content_type='image/png')
+        grantee = self.factory.makeTeam(icon=icon)
+        with FeatureFixture(DETAILS_FLAG):
+            sharees = self.service.jsonShareeData(
+                [(grantee, {
+                    policy1: SharingPermission.ALL}, [])])
+        expected_data = self._makeShareeData(
+            grantee,
+            [(policy1.type, SharingPermission.ALL)], [])
+        self.assertContentEqual([expected_data], sharees)
+
     def _assert_getPillarShareeData(self, pillar):
         # getPillarShareeData returns the expected data.
         access_policy = self.factory.makeAccessPolicy(