launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06707
[Merge] lp:~wallyworld/launchpad/implement-permission-popups-955682 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/implement-permission-popups-955682 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #955682 in Launchpad itself: "Connect the permission editing popups on the +sharing view"
https://bugs.launchpad.net/launchpad/+bug/955682
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/implement-permission-popups-955682/+merge/97588
== Implementation ==
Hook up the permission edit popups in the sharee table on the +sharing screen.
A change to IAccessPolicyGrantFlatSource.findGranteesByPolicy() is required. It now takes an optional list of grantees used to filter the result. This is because once sharePillarInformation() is finished, findGranteesByPolicy() is called to retrieve the resulting grant info for the grantee to return to the client.
When a permission is updated, that might result in the sharee no longer having access to the pillar. In this case, the ui treats it like a delete and removes the row from the table.
== Tests ==
Add test for new findGranteesByPolicy behaviour.
Add tests for new sharePillarInformation behvaiour.
Update and add yui tests for ShareePicker, ShareeTable, PillarSharingView
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/interfaces/accesspolicy.py
lib/lp/registry/javascript/sharing/pillarsharingview.js
lib/lp/registry/javascript/sharing/shareepicker.js
lib/lp/registry/javascript/sharing/shareetable.js
lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js
lib/lp/registry/javascript/sharing/tests/test_shareepicker.js
lib/lp/registry/javascript/sharing/tests/test_shareetable.js
lib/lp/registry/model/accesspolicy.py
lib/lp/registry/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
lib/lp/registry/tests/test_accesspolicy.py
--
https://code.launchpad.net/~wallyworld/launchpad/implement-permission-popups-955682/+merge/97588
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/implement-permission-popups-955682 into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py 2012-03-14 00:15:09 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py 2012-03-15 09:00:27 +0000
@@ -214,12 +214,14 @@
class IAccessPolicyGrantFlatSource(Interface):
"""Experimental query utility to search through the flattened schema."""
- def findGranteesByPolicy(policies):
+ def findGranteesByPolicy(policies, grantees=None):
"""Find teams or users with access grants for the policies.
This includes grants for artifacts in the policies.
:param policies: a collection of `IAccesPolicy`s.
+ :param grantees: if not None, the result only includes people in the
+ specified list of grantees.
:return: a collection of (`IPerson`, `IAccessPolicy`, permission)
where permission is a SharingPermission value.
'ALL' means the person has an access policy grant and can see all
=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-03-12 14:37:23 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-03-15 09:00:27 +0000
@@ -29,6 +29,10 @@
information_types_by_value: {
value: null
+ },
+
+ sharing_permissions_by_value: {
+ value: null
}
};
@@ -39,7 +43,14 @@
Y.Array.each(LP.cache.information_types, function(info_type) {
information_types_by_value[info_type.value] = info_type.title;
});
- this.set('information_types_by_value', information_types_by_value);
+ this.set(
+ 'information_types_by_value', information_types_by_value);
+ var sharing_permissions_by_value = {};
+ Y.Array.each(LP.cache.sharing_permissions, function(permission) {
+ sharing_permissions_by_value[permission.value] = permission.title;
+ });
+ this.set(
+ 'sharing_permissions_by_value', sharing_permissions_by_value);
var vocab = 'ValidPillarOwner';
var header = 'Grant access to project artifacts.';
@@ -66,7 +77,8 @@
visible: false,
information_types: LP.cache.information_types,
save: function(result) {
- self.save_sharing_selection(result);
+ self.save_sharing_selection(
+ result.api_uri, result.sharing_permissions);
}
});
var ns = Y.lp.registry.sharing.shareepicker;
@@ -107,6 +119,12 @@
self.update_sharee_interaction(
e.details[0], e.details[1], e.details[2]);
});
+ sharee_table.subscribe(
+ otns.ShareeTableWidget.UPDATE_PERMISSION, function(e) {
+ var permissions = {};
+ permissions[e.details[1]] = e.details[2];
+ self.save_sharing_selection(e.details[0], permissions);
+ });
},
syncUI: function() {
@@ -262,17 +280,23 @@
/**
* Make a server call to add the specified sharee and access policy.
* @method save_sharing_selection
- * @param selection_result the sharing picker
+ * @param person_uri
+ * @param permissions
*/
- save_sharing_selection: function(selection_result) {
+ save_sharing_selection: function(person_uri, permissions) {
var error_handler = new Y.lp.client.ErrorHandler();
var pillar_uri = LP.cache.context.self_link;
- var person_uri = Y.lp.client.normalize_uri(selection_result.api_uri);
+ person_uri = Y.lp.client.normalize_uri(person_uri);
person_uri = Y.lp.client.get_absolute_uri(person_uri);
- var permissions = [];
- Y.Array.each(
- selection_result.information_types, function(info_type) {
- permissions.push([info_type, 'All']);
+ var information_types_by_value =
+ this.get('information_types_by_value');
+ var sharing_permissions_by_value =
+ this.get('sharing_permissions_by_value');
+ var permission_params = [];
+ Y.each(permissions, function(permission, info_type) {
+ permission_params.push(
+ [information_types_by_value[info_type],
+ sharing_permissions_by_value[permission]]);
});
var self = this;
var y_config = {
@@ -280,15 +304,19 @@
start: Y.bind(self._show_sharing_spinner, namespace),
end: Y.bind(self._hide_hiding_spinner, namespace),
success: function(sharee_entry) {
- self.save_sharing_selection_success(
- sharee_entry.getAttrs());
+ if (!Y.Lang.isValue(sharee_entry)) {
+ self.remove_sharee_success(person_uri);
+ } else {
+ self.save_sharing_selection_success(
+ sharee_entry.getAttrs());
+ }
},
failure: error_handler.getFailureHandler()
},
parameters: {
pillar: pillar_uri,
sharee: person_uri,
- permissions: permissions
+ permissions: permission_params
}
};
this.get('lp_client').named_post(
@@ -314,10 +342,9 @@
return false;
}
Y.each(sharee.permissions, function(permission, info_type_value) {
- //we only support ALL for now (need a tri-state checkbox)
+ //we only support ALL for now
if ('ALL' === permission) {
- selected_permissions.push(
- information_types_by_value[info_type_value]);
+ selected_permissions.push(info_type_value);
}
});
return true;
=== modified file 'lib/lp/registry/javascript/sharing/shareepicker.js'
--- lib/lp/registry/javascript/sharing/shareepicker.js 2012-03-10 01:35:20 +0000
+++ lib/lp/registry/javascript/sharing/shareepicker.js 2012-03-15 09:00:27 +0000
@@ -194,14 +194,16 @@
// Determine the chosen information type. data already contains the
// selected person due to the base picker behaviour.
var contentBox = this.get('contentBox');
- var selected_info_types = [];
+ var sharing_permissions = {};
contentBox.all('input[name=field.visibility]')
.each(function(node) {
+ var permission = 'NOTHING';
if (node.get('checked')) {
- selected_info_types.push(node.get('value'));
+ permission = 'ALL';
}
+ sharing_permissions[node.get('value')] = permission;
});
- data.information_types = selected_info_types;
+ data.sharing_permissions = sharing_permissions;
// Publish the result with step_nr 0 to indicate we have finished.
this.fire('save', data, 0);
},
@@ -214,7 +216,7 @@
'{{#policies}}',
' <tr>',
' <td rowspan="2"><input type="checkbox"',
- ' value="{{title}}"',
+ ' value="{{value}}"',
' name="field.visibility"',
' id="field.visibility.{{index}}"',
' class="checkboxType">',
=== modified file 'lib/lp/registry/javascript/sharing/shareetable.js'
--- lib/lp/registry/javascript/sharing/shareetable.js 2012-03-10 01:35:20 +0000
+++ lib/lp/registry/javascript/sharing/shareetable.js 2012-03-15 09:00:27 +0000
@@ -14,6 +14,7 @@
NAME = "shareeTableWidget",
// Events
UPDATE_SHAREE = 'updateSharee',
+ UPDATE_PERMISSION = 'updatePermission',
REMOVE_SHAREE = 'removeSharee';
@@ -48,7 +49,7 @@
},
// The sharing permission choices: all, some, nothing etc.
sharing_permissions: {
- value: {}
+ value: []
},
// The node holding the sharee table.
sharee_table: {
@@ -80,6 +81,7 @@
this.set(
'sharee_policy_template', this._sharee_policy_template());
this.publish(UPDATE_SHAREE);
+ this.publish(UPDATE_PERMISSION);
this.publish(REMOVE_SHAREE);
},
@@ -189,6 +191,12 @@
backgroundColor: '#FFFF99'
});
permission_edit.render();
+ var self = this;
+ permission_edit.on('save', function(e) {
+ var permission = permission_edit.get('value');
+ self.fire(
+ UPDATE_PERMISSION, sharee.self_link, policy, permission);
+ });
},
// Render the access policy values for the sharees.
@@ -416,6 +424,7 @@
ShareeTableWidget.NAME = NAME;
ShareeTableWidget.UPDATE_SHAREE = UPDATE_SHAREE;
+ShareeTableWidget.UPDATE_PERMISSION = UPDATE_PERMISSION;
ShareeTableWidget.REMOVE_SHAREE = REMOVE_SHAREE;
namespace.ShareeTableWidget = ShareeTableWidget;
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-03-12 14:37:23 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-03-15 09:00:27 +0000
@@ -28,7 +28,9 @@
],
sharing_permissions: [
{'value': 'ALL', 'title': 'All',
- 'description': 'Sharing 1'},
+ 'description': 'Everything'},
+ {'value': 'NOTHING', 'title': 'Nothing',
+ 'description': 'Nothing'},
{'value': 's2', 'title': 'S2',
'description': 'Sharing 2'}
],
@@ -111,7 +113,7 @@
Y.Assert.areEqual('~john', config.sharee.person_uri);
Y.Assert.areEqual('John', config.sharee.person_name);
Y.ArrayAssert.itemsAreEqual(
- ['Policy 1'], config.selected_permissions);
+ ['P1'], config.selected_permissions);
show_picker_called = true;
};
var update_link =
@@ -237,7 +239,7 @@
});
},
- // The perform_add_sharee method makes the expected XHR calls.
+ // When a sharee is added, the expected XHR calls are made.
test_perform_add_sharee: function() {
var mockio = new Y.lp.testing.mockio.MockIo();
var lp_client = new Y.lp.client.Launchpad({
@@ -265,7 +267,7 @@
'.yui3-picker-results li:nth-child(1)').simulate('click');
var cb = sharee_picker.get('contentBox');
var step_two_content = cb.one('.picker-content-two');
- step_two_content.one('input[value="Policy 2"]').simulate('click');
+ step_two_content.one('input[value="P2"]').simulate('click');
var select_link = step_two_content.one('a.next');
select_link.simulate('click');
// Selection made using the picker, now check the results.
@@ -282,7 +284,11 @@
expected_url = Y.lp.client.append_qs(
expected_url, 'sharee', person_uri);
expected_url = Y.lp.client.append_qs(
+ expected_url, 'permissions', 'Policy 1,Nothing');
+ expected_url = Y.lp.client.append_qs(
expected_url, 'permissions', 'Policy 2,All');
+ expected_url = Y.lp.client.append_qs(
+ expected_url, 'permissions', 'Policy 3,Nothing');
Y.Assert.areEqual(expected_url, mockio.last_request.config.data);
mockio.last_request.successJSON({
'resource_type_link': 'entity',
@@ -291,6 +297,53 @@
Y.Assert.isTrue(save_sharing_selection_success_called);
},
+ // When a permission is updated, the expected XHR calls are made.
+ test_perform_update_permission: function() {
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ var lp_client = new Y.lp.client.Launchpad({
+ io_provider: mockio
+ });
+ this.view = this._create_Widget({
+ lp_client: lp_client,
+ anim_duration: 0
+ });
+ this.view.render();
+ var save_sharing_selection_success_called = false;
+ this.view.save_sharing_selection_success = function(sharee) {
+ Y.Assert.areEqual('fred', sharee.name);
+ save_sharing_selection_success_called = true;
+ };
+ // Use permission popup to select a new value.
+ var permission_popup =
+ Y.one('#sharee-table span[id=P1-permission-fred] a');
+ permission_popup.simulate('click');
+ var permission_choice = Y.one(
+ '.yui3-ichoicelist-content a[href=#s2]');
+ permission_choice.simulate('click');
+
+ // Selection made, now check the results.
+ Y.Assert.areEqual(
+ '/api/devel/+services/sharing',
+ mockio.last_request.url);
+ var person_uri = Y.lp.client.normalize_uri('~fred');
+ person_uri = Y.lp.client.get_absolute_uri(person_uri);
+ var expected_url;
+ expected_url = Y.lp.client.append_qs(
+ expected_url, 'ws.op', 'sharePillarInformation');
+ expected_url = Y.lp.client.append_qs(
+ expected_url, 'pillar', '~pillar');
+ expected_url = Y.lp.client.append_qs(
+ expected_url, 'sharee', person_uri);
+ expected_url = Y.lp.client.append_qs(
+ 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);
+ },
+
// The save_sharing_selection_success callback updates the model and
// syncs the UI.
test_save_sharing_selection_success: function() {
@@ -314,6 +367,28 @@
Y.Assert.isTrue(model_updated);
},
+ // If the XHR result of a sharePillarInformation call is null, the user
+ // is to be deleted.
+ test_save_sharing_selection_null_result: function() {
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ var lp_client = new Y.lp.client.Launchpad({
+ io_provider: mockio
+ });
+ this.view = this._create_Widget({
+ lp_client: lp_client,
+ anim_duration: 0
+ });
+ this.view.render();
+ var remove_sharee_success_called = false;
+ this.view.remove_sharee_success = function(sharee_uri) {
+ Y.Assert.areEqual('file:///api/devel/~fred', sharee_uri);
+ remove_sharee_success_called = true;
+ };
+ this.view.save_sharing_selection("~fred", ["P1,All"]);
+ mockio.last_request.successJSON(null);
+ Y.Assert.isTrue(remove_sharee_success_called);
+ },
+
// Test that syncUI works as expected.
test_syncUI: function() {
this.view = this._create_Widget();
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareepicker.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareepicker.js 2012-03-10 01:35:20 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareepicker.js 2012-03-15 09:00:27 +0000
@@ -112,7 +112,7 @@
// policy type.
Y.Array.each(this.information_types, function(info_type) {
var rb = step_two_content.one(
- 'input[value="' + info_type.title + '"]');
+ 'input[value="' + info_type.value + '"]');
Y.Assert.isNotNull(rb);
});
// There should be a link back to previous step.
@@ -157,7 +157,7 @@
person_uri: '~/fred',
person_name: 'Fred'
},
- selected_permissions: ['Policy 1']
+ selected_permissions: ['P1']
});
var cb = this.picker.get('contentBox');
var steptitle = cb.one('.contains-steptitle h2').getContent();
@@ -167,9 +167,9 @@
cb.all('input[name=field.visibility]')
.each(function(node) {
if (node.get('checked')) {
- Y.Assert.areEqual('Policy 1', node.get('value'));
+ Y.Assert.areEqual('P1', node.get('value'));
} else {
- Y.Assert.areNotEqual('Policy 1', node.get('value'));
+ Y.Assert.areNotEqual('P1', node.get('value'));
}
});
// Back link should not he shown
@@ -226,11 +226,17 @@
var cb = this.picker.get('contentBox');
var step_two_content = cb.one('.picker-content-two');
// Select an access policy.
- step_two_content.one('input[value="Policy 2"]').simulate('click');
+ step_two_content.one('input[value="P2"]').simulate('click');
var select_link = step_two_content.one('a.next');
select_link.simulate('click');
- Y.ArrayAssert.itemsAreEqual(
- ['Policy 2'], selected_result.information_types);
+ Y.Assert.areEqual(
+ 3, Y.Object.size(selected_result.sharing_permissions));
+ Y.Assert.areEqual(
+ selected_result.sharing_permissions.P1, 'NOTHING');
+ Y.Assert.areEqual(
+ selected_result.sharing_permissions.P2, 'ALL');
+ Y.Assert.areEqual(
+ selected_result.sharing_permissions.P3, 'NOTHING');
Y.Assert.areEqual('~/fred', selected_result.api_uri);
},
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareetable.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareetable.js 2012-03-09 05:04:21 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareetable.js 2012-03-15 09:00:27 +0000
@@ -201,6 +201,32 @@
);
},
+ // When the permission popup is clicked, the correct event is published.
+ test_permission_update_click: function() {
+ this.sharee_table = this._create_Widget();
+ this.sharee_table.render();
+ var event_fired = false;
+ var ns = Y.lp.registry.sharing.shareetable;
+ this.sharee_table.subscribe(
+ ns.ShareeTableWidget.UPDATE_PERMISSION, function(e) {
+ var sharee_uri = e.details[0];
+ var policy = e.details[1];
+ var permission = e.details[2];
+ Y.Assert.areEqual('~fred', sharee_uri);
+ Y.Assert.areEqual('P1', policy);
+ Y.Assert.areEqual(permission, 's2');
+ event_fired = true;
+ }
+ );
+ var permission_popup =
+ Y.one('#sharee-table span[id=P1-permission-fred] a');
+ permission_popup.simulate('click');
+ var permission_choice = Y.one(
+ '.yui3-ichoicelist-content a[href=#s2]');
+ permission_choice.simulate('click');
+ Y.Assert.isTrue(event_fired);
+ },
+
// Model changes are rendered correctly when syncUI() is called.
test_syncUI: function() {
this.sharee_table = this._create_Widget();
=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py 2012-03-12 11:19:45 +0000
+++ lib/lp/registry/model/accesspolicy.py 2012-03-15 09:00:27 +0000
@@ -182,6 +182,13 @@
cls,
Or(*(cls._constraintForPillar(pillar) for pillar in pillars)))
+ @classmethod
+ def findByPillarAndGrantee(cls, pillars):
+ """See `IAccessPolicySource`."""
+ return IStore(cls).find(
+ cls,
+ Or(*(cls._constraintForPillar(pillar) for pillar in pillars)))
+
class AccessPolicyArtifact(StormBase):
implements(IAccessPolicyArtifact)
@@ -331,7 +338,7 @@
grantee = Reference(grantee_id, 'Person.id')
@classmethod
- def findGranteesByPolicy(cls, policies):
+ def findGranteesByPolicy(cls, policies, grantees=None):
"""See `IAccessPolicyGrantFlatSource`."""
ids = [policy.id for policy in policies]
sharing_permission_term = SQL("""
@@ -341,11 +348,16 @@
ELSE 'SOME'
END
""")
- return IStore(cls).find(
- (Person, AccessPolicy, sharing_permission_term),
+ constraints = [
Person.id == cls.grantee_id,
AccessPolicy.id == cls.policy_id,
- cls.policy_id.is_in(ids)).group_by(Person, AccessPolicy)
+ cls.policy_id.is_in(ids)]
+ if grantees:
+ grantee_ids = [grantee.id for grantee in grantees]
+ constraints.append(cls.grantee_id.is_in(grantee_ids))
+ return IStore(cls).find(
+ (Person, AccessPolicy, sharing_permission_term),
+ *constraints).group_by(Person, AccessPolicy)
@classmethod
def findArtifactsByGrantee(cls, grantee, policies):
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-03-13 12:15:07 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-03-15 09:00:27 +0000
@@ -79,11 +79,12 @@
return sharing_permissions
@available_with_permission('launchpad.Driver', 'pillar')
- def getPillarSharees(self, pillar):
+ def getPillarSharees(self, pillar, grantees=None):
"""See `ISharingService`."""
policies = getUtility(IAccessPolicySource).findByPillar([pillar])
ap_grant_flat = getUtility(IAccessPolicyGrantFlatSource)
- grant_permissions = ap_grant_flat.findGranteesByPolicy(policies)
+ grant_permissions = ap_grant_flat.findGranteesByPolicy(
+ policies, grantees)
result = []
person_by_id = {}
@@ -124,23 +125,23 @@
for information_type in information_types
if information_type in info_types_for_all]
policy_source = getUtility(IAccessPolicySource)
- wanted_pillar_policies = policy_source.find(required_pillar_info_types)
-
- # We need to figure out which policy grants to create or delete.
policy_grant_source = getUtility(IAccessPolicyGrantSource)
- wanted_policy_grants = [(policy, sharee)
- for policy in wanted_pillar_policies
- if policy.type in info_types_for_all]
- existing_policy_grants = [
- (grant.policy, grant.grantee)
- for grant in policy_grant_source.find(wanted_policy_grants)]
- # Create any newly required policy grants.
- policy_grants_to_create = (
- set(wanted_policy_grants).difference(existing_policy_grants))
- if len(policy_grants_to_create) > 0:
- policy_grant_source.grant(
- [(policy, sharee, user)
- for policy, sharee in policy_grants_to_create])
+ if len(required_pillar_info_types) > 0:
+ wanted_pillar_policies = policy_source.find(
+ required_pillar_info_types)
+ # We need to figure out which policy grants to create or delete.
+ wanted_policy_grants = [(policy, sharee)
+ for policy in wanted_pillar_policies]
+ existing_policy_grants = [
+ (grant.policy, grant.grantee)
+ for grant in policy_grant_source.find(wanted_policy_grants)]
+ # Create any newly required policy grants.
+ policy_grants_to_create = (
+ set(wanted_policy_grants).difference(existing_policy_grants))
+ if len(policy_grants_to_create) > 0:
+ policy_grant_source.grant(
+ [(policy, sharee, user)
+ for policy, sharee in policy_grants_to_create])
# Now revoke any existing policy grants for types with
# permission 'some'.
@@ -158,17 +159,10 @@
self.deletePillarSharee(pillar, sharee, info_types_for_nothing)
# Return sharee data to the caller.
- request = get_current_web_service_request()
- resource = EntryResource(sharee, request)
- person_data = resource.toDataForJSON()
- valid_info_types = (
- set(information_types).difference(info_types_for_nothing))
- permission_data = dict(
- (information_type.name, permissions[information_type].name)
- for information_type in valid_info_types
- )
- person_data['permissions'] = permission_data
- return person_data
+ sharees = self.getPillarSharees(pillar, [sharee])
+ if not sharees:
+ return None
+ return sharees[0]
@available_with_permission('launchpad.Edit', 'pillar')
def deletePillarSharee(self, pillar, sharee,
@@ -192,11 +186,14 @@
grants = [
(grant.policy, grant.grantee)
for grant in policy_grant_source.find(policy_grants)]
- policy_grant_source.revoke(grants)
+ if len(grants) > 0:
+ policy_grant_source.revoke(grants)
# Second delete any access artifact grants.
ap_grant_flat = getUtility(IAccessPolicyGrantFlatSource)
to_delete = ap_grant_flat.findArtifactsByGrantee(
sharee, pillar_policies)
- accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
- accessartifact_grant_source.revokeByArtifact(to_delete)
+ if to_delete.count() > 0:
+ accessartifact_grant_source = getUtility(
+ IAccessArtifactGrantSource)
+ accessartifact_grant_source.revokeByArtifact(to_delete)
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-03-13 05:08:00 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-03-15 09:00:27 +0000
@@ -135,6 +135,27 @@
login_person(driver)
self._test_getPillarSharees(distro)
+ def test_getPillarSharees_filter_grantees(self):
+ # getPillarSharees only returns grantees in the specified list.
+ driver = self.factory.makePerson()
+ pillar = self.factory.makeProduct(driver=driver)
+ login_person(driver)
+ access_policy = self.factory.makeAccessPolicy(
+ pillar=pillar,
+ type=InformationType.PROPRIETARY)
+ grantee_in_result = self.factory.makePerson()
+ grantee_not_in_result = self.factory.makePerson()
+ self.factory.makeAccessPolicyGrant(access_policy, grantee_in_result)
+ self.factory.makeAccessPolicyGrant(
+ access_policy, grantee_not_in_result)
+
+ sharees = self.service.getPillarSharees(pillar, [grantee_in_result])
+ expected_sharees = [
+ self._makeShareeData(
+ grantee_in_result,
+ [(InformationType.PROPRIETARY, SharingPermission.ALL)])]
+ self.assertContentEqual(expected_sharees, sharees)
+
def _test_getPillarShareesUnauthorized(self, pillar):
# getPillarSharees raises an Unauthorized exception if the user is
# not permitted to do so.
@@ -242,6 +263,21 @@
login_person(owner)
self._test_sharePillarInformation(distro)
+ def test_updatePillarSharee_no_access_grants_remain(self):
+ # When a pillar sharee has it's only access policy permission changed
+ # to Some, test that None is returned.
+ owner = self.factory.makePerson()
+ pillar = self.factory.makeProduct(owner=owner)
+ login_person(owner)
+ sharee = self.factory.makePerson()
+ grant = self.factory.makeAccessPolicyGrant(grantee=sharee)
+
+ permissions = {
+ grant.policy.type: SharingPermission.SOME}
+ sharee_data = self.service.sharePillarInformation(
+ pillar, sharee, permissions, self.factory.makePerson())
+ self.assertIsNone(sharee_data)
+
def _test_sharePillarInformationUnauthorized(self, pillar):
# sharePillarInformation raises an Unauthorized exception if the user
# is not permitted to do so.
=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py 2012-03-13 03:27:41 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py 2012-03-15 09:00:27 +0000
@@ -453,6 +453,24 @@
apgfs.findGranteesByPolicy([
policy, another_policy, policy_with_no_grantees]))
+ def test_findGranteesByPolicy_filter_grantees(self):
+ # findGranteesByPolicy() returns anyone with a grant for any of
+ # the policies or the policies' artifacts so long as the grantee is in
+ # the specified list of grantees.
+ apgfs = getUtility(IAccessPolicyGrantFlatSource)
+
+ # People with grants on the policy show up.
+ policy = self.factory.makeAccessPolicy()
+ grantee_in_result = self.factory.makePerson()
+ grantee_not_in_result = self.factory.makePerson()
+ policy_grant = self.factory.makeAccessPolicyGrant(
+ policy=policy, grantee=grantee_in_result)
+ self.factory.makeAccessPolicyGrant(
+ policy=policy, grantee=grantee_not_in_result)
+ self.assertContentEqual(
+ [(policy_grant.grantee, policy, 'ALL')],
+ apgfs.findGranteesByPolicy([policy], [grantee_in_result]))
+
def test_findArtifactsByGrantee(self):
# findArtifactsByGrantee() returns the artifacts for grantee for any of
# the policies.
