launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/edit-icons-permissions into lp:launchpad.

Commit message:
Ensures widgets are not clickable on +sharing without the user having `launchpad.Edit`.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1081131 in Launchpad itself: "+sharing should not show edit icons if you don't have launchpad.Edit"
  https://bugs.launchpad.net/launchpad/+bug/1081131

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/edit-icons-permissions/+merge/140556

Summary
=======
The edit icons show up on +sharing for people who only have
`launchpad.Driver`; that's sufficient to view sharing data but not to edit, so
errors occur when you try to actually use the widget.

We shouldn't make the widgets editable for those without `launchpad.Edit`.

Preimp
======
Spoke with Deryck and Curtis.

Implementation
==============
On the sharing view, we add `has_edit_permission` to the JSON cache. This is
used in the JS to ensure that the widgets' clickable_content property is only
set if the permission exists.

Tests
=====
bin/test -vvct sharing --layer=YUI

QA
==
Go check a page you have Edit on; you should be able to use the widgets. Go to
one you only have driver on; you should see the data but be unable to alter it
and be given no indication you can do so.

LoC
===
Part of private projects.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/javascript/sharing/pillarsharingview.js
  lib/lp/registry/javascript/sharing/granteetable.js
  lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js
  lib/lp/registry/browser/pillar.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/edit-icons-permissions/+merge/140556
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/edit-icons-permissions into lp:launchpad.

=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py	2012-12-02 01:51:46 +0000
+++ lib/lp/registry/browser/pillar.py	2012-12-18 21:30:28 +0000
@@ -58,6 +58,7 @@
 from lp.services.config import config
 from lp.services.features import getFeatureFlag
 from lp.services.propertycache import cachedproperty
+from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.batching import (
     BatchNavigator,
     StormRangeFactory,
@@ -190,10 +191,10 @@
     def has_involvement(self):
         """This `IPillar` uses Launchpad."""
         return (self.official_malone
-            or service_uses_launchpad(self.answers_usage)
-            or service_uses_launchpad(self.blueprints_usage)
-            or service_uses_launchpad(self.translations_usage)
-            or service_uses_launchpad(self.codehosting_usage))
+                or service_uses_launchpad(self.answers_usage)
+                or service_uses_launchpad(self.blueprints_usage)
+                or service_uses_launchpad(self.translations_usage)
+                or service_uses_launchpad(self.codehosting_usage))
 
     @property
     def enabled_links(self):
@@ -354,9 +355,10 @@
             self.branch_sharing_policies)
         cache.objects['specification_sharing_policies'] = (
             self.specification_sharing_policies)
-
-        view_names = set(reg.name for reg
-            in iter_view_registrations(self.__class__))
+        cache.objects['has_edit_permission'] = check_permission(
+            "launchpad.Edit", self.context)
+        view_names = set(reg.name for reg in
+                         iter_view_registrations(self.__class__))
         if len(view_names) != 1:
             raise AssertionError("Ambiguous view name.")
         cache.objects['view_name'] = view_names.pop()

=== modified file 'lib/lp/registry/javascript/sharing/granteetable.js'
--- lib/lp/registry/javascript/sharing/granteetable.js	2012-09-12 18:40:35 +0000
+++ lib/lp/registry/javascript/sharing/granteetable.js	2012-12-18 21:30:28 +0000
@@ -218,7 +218,7 @@
            '<li class="nowrap">',
            '<span id="{{policy}}-permission-{{grantee_name}}">',
            '  <span class="value"></span>',
-           '  <a class="editicon sprite edit action-icon"',
+           '  <a class="editicon sprite edit action-icon hidden"',
            '  href="#">Edit</a>',
            '</span></li>',
            '{{/information_types}}'].join(' ');
@@ -249,9 +249,12 @@
         var contentBox = permission_node.one(
             '[id="' + policy + '-' + id + '"]');
         var value_location = contentBox.one('.value');
+        var editable = LP.cache.has_edit_permission;
         var editicon = permission_node.one('a.editicon');
-
-        var clickable_content = this.get('write_enabled');
+        if (editable) {
+            editicon.removeClass('hidden');
+        }
+        var clickable_content = (this.get('write_enabled') && editable);
         var permission_edit = new Y.ChoiceSource({
             clickable_content: clickable_content,
             contentBox: contentBox,

=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js	2012-09-14 00:45:29 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js	2012-12-18 21:30:28 +0000
@@ -156,7 +156,8 @@
         }
         choice_items.push.apply(
                 choice_items, this.getSharingPolicyInformation(artifact_type));
-        var editable = choice_items.length > 1;
+        var edit_permission = LP.cache.has_edit_permission;
+        var editable = (edit_permission && choice_items.length> 1);
         var policy_edit = new Y.ChoiceSource({
             flashEnabled: false,
             clickable_content: editable,

=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js	2012-10-26 10:00:20 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js	2012-12-18 21:30:28 +0000
@@ -520,6 +520,7 @@
         // When there is no sharing policy defined for a pillar, the default
         // policy becomes the legacy policy.
         test_sharing_policy_render_no_model_value: function() {
+            window.LP.cache.has_edit_permission = true;
             this.view = this._create_Widget();
             this.view.render();
             this._assert_sharing_policies_editable(true);
@@ -533,6 +534,13 @@
 
         // A pillar's sharing policy is correctly rendered.
         test_sharing_policy_render: function() {
+            // If there is no permission to edit the policy, it's not editable
+            this.view = this._create_Widget();
+            this.view.render();
+            this._assert_sharing_policies_editable(false);
+
+            // If there is permission, it is editable.
+            window.LP.cache.has_edit_permission = true;
             this.view = this._create_Widget();
             this.view.render();
             this._assert_sharing_policies_editable(true);