launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06938
[Merge] lp:~wallyworld/launchpad/audit-sharing3-933815 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/audit-sharing3-933815 into lp:launchpad with lp:~wallyworld/launchpad/audit-sharing2-933815 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #933815 in Launchpad itself: "Cannot search and filter +sharing policies"
https://bugs.launchpad.net/launchpad/+bug/933815
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/audit-sharing3-933815/+merge/100147
== Implementation ==
This branch adds the ui for the +audit-sharing view. It is implemented via simple modifications to the ShareeTable widget in shareetable.js.
The view is only available to pillar maintainers via the launchpad.Edit permission. This required that some methods in the sharing service be available for launchpad.Driver as well as launchpad.Edit. So the available_with_permission decorator was enhanced to allow an optional list of permissions to be specified.
== Tests ==
Add test to TestAvailableWithPermission in test_authorization
Add tests to test_pillar_sharing
Add yui tests to test_shareetable, test_pillarsharingview
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/browser/configure.zcml
lib/lp/registry/browser/pillar.py
lib/lp/registry/browser/tests/test_pillar_sharing.py
lib/lp/registry/javascript/sharing/pillarsharingview.js
lib/lp/registry/javascript/sharing/shareetable.js
lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js
lib/lp/registry/javascript/sharing/tests/test_shareetable.js
lib/lp/registry/services/sharingservice.py
lib/lp/services/webapp/authorization.py
lib/lp/services/webapp/tests/test_authorization.py
--
https://code.launchpad.net/~wallyworld/launchpad/audit-sharing3-933815/+merge/100147
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/audit-sharing3-933815 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml 2012-03-30 14:12:27 +0000
+++ lib/lp/registry/browser/configure.zcml 2012-03-30 14:12:28 +0000
@@ -1435,7 +1435,7 @@
template="../templates/pillar-sharing.pt"/>
<browser:page
for="lp.registry.interfaces.product.IProduct"
- permission="launchpad.Driver"
+ permission="launchpad.Edit"
name="+audit-sharing"
class="lp.registry.browser.pillar.PillarAuditSharingView"
template="../templates/pillar-sharing.pt"/>
@@ -1823,7 +1823,7 @@
template="../templates/pillar-sharing.pt"/>
<browser:page
for="lp.registry.interfaces.distribution.IDistribution"
- permission="launchpad.Driver"
+ permission="launchpad.Edit"
name="+audit-sharing"
class="lp.registry.browser.pillar.PillarAuditSharingView"
template="../templates/pillar-sharing.pt"/>
=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py 2012-03-30 14:12:27 +0000
+++ lib/lp/registry/browser/pillar.py 2012-03-30 14:12:28 +0000
@@ -6,14 +6,10 @@
__metaclass__ = type
__all__ = [
-<<<<<<< TREE
- 'InvolvedMenu', 'PillarBugsMenu', 'PillarView',
-=======
'InvolvedMenu',
'PillarAuditSharingView',
'PillarBugsMenu',
'PillarView',
->>>>>>> MERGE-SOURCE
'PillarNavigationMixin',
'PillarPersonSharingView',
'PillarSharingInformationView',
@@ -371,7 +367,8 @@
@property
def show_audit_sharing_link(self):
- return True
+ # Only pillar owners can audit sharing.
+ return check_permission('launchpad.Edit', self.context)
def unbatched_sharees(self):
"""All the sharees for a pillar."""
=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py 2012-03-30 14:12:27 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py 2012-03-30 14:12:28 +0000
@@ -28,6 +28,7 @@
from lp.services.webapp.interfaces import StormRangeFactoryError
from lp.services.webapp.publisher import canonical_url
from lp.testing import (
+ login_celebrity,
login_person,
StormStatementRecorder,
TestCaseWithFactory,
@@ -245,6 +246,23 @@
self.assertEqual('Sharing Information', view.page_title)
self.assertEqual('Sharing information', view.label)
self.assertFalse(view.show_sharing_information_link)
+
+ def test_nonowners_cannot_audit(self):
+ with FeatureFixture(ENABLED_FLAG):
+ login_person(self.driver)
+ view = create_initialized_view(self.pillar, '+sharing')
+ self.assertFalse(view.show_audit_sharing_link)
+
+ def test_owners_can_audit(self):
+ with FeatureFixture(ENABLED_FLAG):
+ login_person(self.owner)
+ view = create_initialized_view(self.pillar, '+sharing')
+ self.assertTrue(view.show_audit_sharing_link)
+
+ def test_admins_can_audit(self):
+ with FeatureFixture(ENABLED_FLAG):
+ login_celebrity('admin')
+ view = create_initialized_view(self.pillar, '+sharing')
self.assertTrue(view.show_audit_sharing_link)
def test_sharing_menu_without_feature_flag(self):
@@ -269,7 +287,7 @@
# The +audit-sharing link is rendered.
with FeatureFixture(ENABLED_FLAG):
url = canonical_url(self.pillar, view_name='+sharing')
- browser = setupBrowserForUser(user=self.driver)
+ browser = setupBrowserForUser(user=self.owner)
browser.open(url)
soup = BeautifulSoup(browser.contents)
audit_sharing_link = soup.find('a', {'href': '+audit-sharing'})
@@ -343,7 +361,8 @@
# The +sharing link is rendered.
with FeatureFixture(ENABLED_FLAG):
url = canonical_url(self.pillar, view_name='+audit-sharing')
- browser = setupBrowserForUser(user=self.driver)
+ login_person(self.owner)
+ browser = setupBrowserForUser(user=self.owner)
browser.open(url)
soup = BeautifulSoup(browser.contents)
sharing_info_link = soup.find('a', {'href': '+sharing'})
=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-03-26 01:11:56 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-03-30 14:12:28 +0000
@@ -19,6 +19,10 @@
value: new Y.lp.client.Launchpad()
},
+ show_indirect_sharees: {
+ value:false
+ },
+
write_enabled: {
value: false
},
@@ -61,6 +65,9 @@
return;
}
this.set('write_enabled', true);
+ if (LP.cache.show_indirect_sharees) {
+ this.set('show_indirect_sharees', true);
+ }
var vocab = 'ValidPillarOwner';
var header = 'Grant access to project artifacts.';
@@ -106,7 +113,8 @@
sharing_permissions:
this.get('sharing_permissions_by_value'),
information_types: this.get('information_types_by_value'),
- write_enabled: this.get('write_enabled')
+ write_enabled: this.get('write_enabled'),
+ show_indirect_sharees: this.get('show_indirect_sharees')
});
this.set('sharee_table', sharee_table);
sharee_table.render();
=== modified file 'lib/lp/registry/javascript/sharing/shareetable.js'
--- lib/lp/registry/javascript/sharing/shareetable.js 2012-03-28 03:50:33 +0000
+++ lib/lp/registry/javascript/sharing/shareetable.js 2012-03-30 14:12:28 +0000
@@ -69,6 +69,10 @@
sharee_policy_template: {
value: null
},
+ // Whether this widget is to display teams via which sharees have access.
+ show_indirect_sharees: {
+ value:false
+ },
write_enabled: {
value: false
@@ -86,6 +90,7 @@
'sharee_table_empty_row', this._sharee_table_empty_row());
this.set(
'sharee_policy_template', this._sharee_policy_template());
+ this.set('via_teams_template', this._via_teams_template());
this.navigator = this.make_navigator();
var self = this;
var ns = Y.lp.registry.sharing.shareelisting_navigator;
@@ -122,11 +127,13 @@
},
_sharee_table_template: function() {
- return [
+ var template = [
'<table class="sharing listing" id="sharee-table">',
' <thead>',
- ' <tr><th style="width: 33%" ',
+ ' <tr>',
+ ' <th style="width: 33%" ',
' colspan="2">User or Team</th>',
+ ' {via_header}',
' <th colspan="2">',
' Sharing',
' <span class="help">',
@@ -145,10 +152,15 @@
' {{/sharees}}',
' </tbody>',
'</table>'].join(' ');
+ var via_header = '';
+ if (this.get('show_indirect_sharees')) {
+ via_header = '<th>Via</th>';
+ }
+ return Y.Lang.substitute(template, {via_header: via_header});
},
_sharee_row_template: function() {
- return [
+ var template = [
'<tr id="permission-{{name}}" data-name="{{name}}"><td>',
' <a href="{{web_link}}" class="sprite {{meta}}">',
' {{display_name}}',
@@ -168,6 +180,7 @@
' data-person_name="{{display_name}}"> </a>',
'</span>',
'</td>',
+ '{via_cell}',
'<td id="td-permission-{{name}}">',
' <span class="sortkey">1</span>',
' <ul class="horizontal">',
@@ -178,6 +191,21 @@
'<td><span class="formHelp">No items shared</span>',
'</td>',
'</tr>'].join(' ');
+ var via_cell = '';
+ if (this.get('show_indirect_sharees')) {
+ via_cell = '<td id="via-teams-{{name}}">' +
+ '{{>sharee_via_teams}}</td>';
+ }
+ return Y.Lang.substitute(template, {via_cell: via_cell});
+ },
+
+ _via_teams_template: function() {
+ return [
+ '<span class="unseen"> --</span>',
+ '{{#via_teams}}',
+ ' <a href="{{web_link}}" class="sprite team">',
+ ' {{display_name}}</a>',
+ '{{/via_teams}}'].join(' ');
},
_sharee_table_empty_row: function() {
@@ -200,6 +228,15 @@
'{{/information_types}}'].join(' ');
},
+ // A sharee cannot be edited if they have access via a team.
+ _sharee_editable: function(sharee) {
+ var editable = this.get('write_enabled');
+ if (Y.Lang.isArray(sharee.via_teams) && sharee.via_teams.length > 0) {
+ editable = false;
+ }
+ return editable;
+ },
+
// Render the popup widget to pick the sharing permission for an
// access policy.
render_sharee_policy: function(
@@ -227,9 +264,9 @@
var value_location = contentBox.one('.value');
var editicon = permission_node.one('a.editicon');
- var clickable_content = this.get('write_enabled');
+ var sharee_editable = this._sharee_editable(sharee);
var permission_edit = new Y.ChoiceSource({
- clickable_content: clickable_content,
+ clickable_content: sharee_editable,
contentBox: contentBox,
value_location: value_location,
editicon: editicon,
@@ -279,8 +316,8 @@
_render_sharees: function(sharees) {
var sharee_table = this.get('sharee_table');
var partials = {
- sharee_access_policies:
- this.get('sharee_policy_template'),
+ sharee_access_policies: this.get('sharee_policy_template'),
+ sharee_via_teams: this.get('via_teams_template'),
sharee_row: this.get('sharee_row_template')
};
this._prepareShareeDisplayData(sharees);
@@ -294,7 +331,10 @@
}
sharee_table.replace(table_node);
this.render_sharing_info(sharees);
- this._update_editable_status();
+ var self = this;
+ Y.Array.each(sharees, function(sharee) {
+ self._update_editable_status(sharee);
+ });
this.set('sharees', sharees);
},
@@ -425,15 +465,21 @@
});
},
- _update_editable_status: function() {
+ _update_editable_status: function(sharee) {
var sharee_table = this.get('sharee_table');
- if (!this.get('write_enabled')) {
- sharee_table.all(
- '.sprite.add, .sprite.edit, .sprite.remove')
+ var row_node = sharee_table
+ .one('tr[id=permission-' + sharee.name + ']');
+ if (!this._sharee_editable(sharee)) {
+ row_node.all('.sprite.add, .sprite.edit, .sprite.remove')
.each(function(node) {
node.addClass('unseen');
});
}
+ row_node.all('td[id^=via-teams-]').each(function(node) {
+ if (!Y.Lang.isValue(node.one('a'))) {
+ node.one('span').removeClass('unseen');
+ }
+ });
},
// Add or update new sharees in the table.
@@ -441,8 +487,8 @@
this._prepareShareeDisplayData(sharees);
var update_node_selectors = [];
var partials = {
- sharee_access_policies:
- this.get('sharee_policy_template')
+ sharee_access_policies: this.get('sharee_policy_template'),
+ sharee_via_teams: this.get('via_teams_template')
};
var sharee_table = this.get('sharee_table');
var self = this;
@@ -471,8 +517,8 @@
update_node_selectors.push(
'tr[id=permission-' + sharee.name + ']');
self.render_sharee_sharing_info(sharee);
+ self._update_editable_status(sharee);
});
- this._update_editable_status();
var anim_duration = this.get('anim_duration');
if (anim_duration === 0) {
return;
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-03-26 01:11:56 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-03-30 14:12:28 +0000
@@ -93,6 +93,16 @@
this.view.get('sharee_table').get('write_enabled'));
},
+ // show_indirect_sharees is used to configure the sharee table.
+ test_show_indirect_sharees: function() {
+ window.LP.cache.show_indirect_sharees = true;
+ this.view = this._create_Widget();
+ this.view.render();
+ Y.Assert.isTrue(
+ this.view.get('sharee_table')
+ .get('show_indirect_sharees'));
+ },
+
// Clicking a update sharee sharee link calls
// the update_sharee_interaction method with the correct parameters.
test_update_sharee_click: function() {
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareetable.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareetable.js 2012-03-28 03:50:33 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareetable.js 2012-03-30 14:12:28 +0000
@@ -18,10 +18,15 @@
{'name': 'fred', 'display_name': 'Fred Bloggs',
'role': '(Maintainer)', web_link: '~fred',
'self_link': '~fred',
- 'permissions': {'P1': 's1', 'P2': 's2'}},
- {'name': 'john.smith', 'display_name': 'John Smith',
+ 'permissions': {'P1': 's1', 'P2': 's2'},
+ 'via_teams': [
+ {'web_link': '~team1',
+ 'displayname': 'Team One'},
+ {'web_link': '~team2',
+ 'displayname': 'Team Two'}]},
+ {'name': 'john.smith', 'display_name': 'John Smith',
'role': '', web_link: '~smith', 'self_link': '~smith',
- 'permissions': {'P1': 's1', 'P3': 's3'}}
+ 'permissions': {'P1': 's1', 'P3': 's2'}}
]
}
};
@@ -160,6 +165,49 @@
Y.Array.each(this.sharee_data, function(sharee) {
self._test_sharee_rendered(sharee);
});
+ // No indirect sharees info by default.
+ Y.Assert.isNull(Y.one('#sharee-table td[id^=via-teams-]'));
+ },
+
+ // Test that sharees with via_teams data are rendered correctly.
+ test_indirect_sharees_render: function() {
+ this.sharee_table = this._create_Widget(
+ {'show_indirect_sharees': true});
+ this.sharee_table.render();
+ var self = this;
+ Y.Array.each(window.LP.cache.sharee_data, function(sharee) {
+ self._test_sharee_rendered(sharee);
+ Y.Assert.isNotNull(
+ Y.one('td[id=via-teams-' + sharee.name + ']'));
+ Y.all('td[id=via-teams-' + sharee.name + ']')
+ .each(function(cell) {
+ var row = cell.ancestor('tr');
+ if (Y.Lang.isArray(sharee.via_teams)
+ && sharee.via_teams.length > 0) {
+ // Check the via_teams rendering.
+ Y.Array.each(sharee.via_teams, function(team) {
+ Y.Assert.isNotNull(
+ cell.one('a[href=' + team.web_link+ ']'));
+ });
+ // Indirect sharees are not editable.
+ row.all('.sprite.add, .sprite.edit, ' +
+ '.sprite.remove')
+ .each(function(node) {
+ Y.Assert.isTrue(node.hasClass('unseen'));
+ });
+ } else {
+ // Direct sharees have '--' as the sharee text.
+ Y.Assert.areEqual('--', cell.get('textContent')
+ .replace(/(^[\s\xA0]+|[\s\xA0]+$)/g, ''));
+ // Direct sharees are editable.
+ row.all(
+ '.sprite.add, .sprite.edit, .sprite.remove')
+ .each(function(node) {
+ Y.Assert.isFalse(node.hasClass('unseen'));
+ });
+ }
+ });
+ });
},
// When the update link is clicked, the correct event is published.
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-03-30 14:12:27 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-03-30 14:12:28 +0000
@@ -99,7 +99,8 @@
sharing_permissions.append(item)
return sharing_permissions
- @available_with_permission('launchpad.Driver', 'pillar')
+ @available_with_permission(
+ ['launchpad.Driver', 'launchpad.Edit'], 'pillar')
def getPillarSharees(self, pillar, include_indirect=False):
"""See `ISharingService`."""
policies = getUtility(IAccessPolicySource).findByPillar([pillar])
@@ -116,7 +117,8 @@
policies).order_by(Person.displayname, Person.name)
return grant_permissions
- @available_with_permission('launchpad.Driver', 'pillar')
+ @available_with_permission(
+ ['launchpad.Driver', 'launchpad.Edit'], 'pillar')
def getPillarShareeData(self, pillar, include_indirect=False):
"""See `ISharingService`."""
grant_permissions = list(
=== modified file 'lib/lp/services/webapp/authorization.py'
--- lib/lp/services/webapp/authorization.py 2012-03-22 23:21:24 +0000
+++ lib/lp/services/webapp/authorization.py 2012-03-30 14:12:28 +0000
@@ -380,18 +380,22 @@
"""
- def __init__(self, permission, context_parameter):
+ def __init__(self, permissions, context_parameter):
"""Make a new available_with_permission function decorator.
`permission` is the string permission name, like 'launchpad.Edit'.
+ An iterable of permissions may also be specified.
`context_parameter` is the name of the function argument which
contains the context object.
"""
- self.permission = permission
+ if not getattr(permissions, '__iter__', False):
+ self.permissions = [permissions]
+ else:
+ self.permissions = permissions
self.context_parameter = context_parameter
def __call__(self, func):
- permission = self.permission
+ permissions = self.permissions
context_parameter = self.context_parameter
def permission_checker(self, *args, **kwargs):
@@ -399,9 +403,14 @@
context = kwargs[context_parameter]
else:
context = args[0]
- if not check_permission(permission, context):
+ all_disallowed = True
+ for permission in permissions:
+ if check_permission(permission, context):
+ all_disallowed = False
+ break
+ if all_disallowed:
raise Unauthorized(
- "Permission %s required on %s."
- % (permission, context))
+ "One of permission %s required on %s."
+ % (permissions, context))
return func(self, *args, **kwargs)
return permission_checker
=== modified file 'lib/lp/services/webapp/tests/test_authorization.py'
--- lib/lp/services/webapp/tests/test_authorization.py 2012-03-18 01:26:52 +0000
+++ lib/lp/services/webapp/tests/test_authorization.py 2012-03-30 14:12:28 +0000
@@ -688,6 +688,11 @@
def test_function_bar(self, foo, bar=None):
pass
+ @available_with_permission(
+ ['launchpad.Edit', 'launchpad.Driver'], 'bar')
+ def test_function_multi(self, foo, bar=None):
+ pass
+
class TestAvailableWithPermission(TestCase):
"""Test the available_with_permission decorator."""
@@ -714,6 +719,17 @@
foo = Object()
obj_to_invoke.test_function_bar(foo=foo, bar=bar)
+ def test_multi_permission(self):
+ # Method invocation with more than one allowed permission.
+ bar = Object()
+ request = LaunchpadTestRequest()
+ login(ANONYMOUS, request)
+ for permission in ['launchpad.Edit', 'launchpad.Driver']:
+ precache_permission_for_objects(request, permission, [bar])
+ obj_to_invoke = AvailableWithPermissionObject()
+ foo = Object()
+ obj_to_invoke.test_function_multi(foo=foo, bar=bar)
+
def test_unauthorized(self):
# Unauthorized method invocation.
foo = Object()
Follow ups
