launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07300
[Merge] lp:~wallyworld/launchpad/update-sharing-policies-some-988630 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/update-sharing-policies-some-988630 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #988630 in Launchpad itself: "Update sharing polices form shows 'Some' when no artifact grants exist"
https://bugs.launchpad.net/launchpad/+bug/988630
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/update-sharing-policies-some-988630/+merge/103626
== Implementation ==
When displaying the Update sharee policies dialog, we only want to enable the Some buttons if there are shared artifacts of that type.
The implementation of findGranteePermissionsByPolicy() is changed to provide the data necessary to stuff into the json request cache. The sharee data in the cache now contains the information types for which there are shared artifacts for the sharee.
The findGranteePermissionsByPolicy() method uses a different query to grab the info from AccessPolicyGrantFlat table. The new query, for each sharee, tells whether the sharee has All and/or Some permissions for each information type. Previously, if the user had All permission, Some was ignored.
wgrant ran the new SQL in dogfood to ensure it performed adequately.
== Tests ==
Update yui tests for the sharee picker and pillar sharing view:
test_shareepicker.js
test_pillarsharingview.js
The above test that the Some buttons are disabled correctly when required.
Update tests for AccessPolicyGrantFlat's findGranteePermissionsByPolicy() method.
Add new test test_findGranteePermissionsByPolicy_shared_artifact_types
Update sharing service tests, primarily the code which invokes findGranteePermissionsByPolicy()
== 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/tests/test_pillarsharingview.js
lib/lp/registry/javascript/sharing/tests/test_shareepicker.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/update-sharing-policies-some-988630/+merge/103626
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/update-sharing-policies-some-988630 into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py 2012-04-06 17:28:25 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py 2012-04-26 08:00:30 +0000
@@ -241,11 +241,14 @@
: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)
+ :return: a collection of
+ (`IPerson`, `IAccessPolicy`, permission, shared_artifact_types)
where permission is a SharingPermission enum value.
ALL means the person has an access policy grant and can see all
artifacts for the associated pillar.
SOME means the person only has specified access artifact grants.
+ shared_artifact_types contains the information_types for which the
+ user has been granted access one or more artifacts of that type.
"""
def findArtifactsByGrantee(grantee, policies):
=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-04-11 02:15:10 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-04-26 08:00:30 +0000
@@ -364,6 +364,7 @@
update_sharee_interaction: function(update_link, person_uri, person_name) {
var sharee_data = LP.cache.sharee_data;
var sharee_permissions = {};
+ var disabled_some_types = [];
Y.Array.some(sharee_data, function(sharee) {
var full_person_uri = Y.lp.client.normalize_uri(person_uri);
full_person_uri = Y.lp.client.get_absolute_uri(full_person_uri);
@@ -371,6 +372,14 @@
return false;
}
sharee_permissions = sharee.permissions;
+ // Do not allow the user to choose 'Some' unless there are shared
+ // artifacts of that type.
+ Y.Array.each(LP.cache.information_types, function(info_type) {
+ if (Y.Array.indexOf(
+ sharee.shared_artifact_types, info_type.value) < 0) {
+ disabled_some_types.push(info_type.value);
+ }
+ });
return true;
});
var allowed_permissions = [];
@@ -384,7 +393,8 @@
person_name: person_name
},
sharee_permissions: sharee_permissions,
- allowed_permissions: allowed_permissions
+ allowed_permissions: allowed_permissions,
+ disabled_some_types: disabled_some_types
});
}
});
=== modified file 'lib/lp/registry/javascript/sharing/shareepicker.js'
--- lib/lp/registry/javascript/sharing/shareepicker.js 2012-03-28 00:44:36 +0000
+++ lib/lp/registry/javascript/sharing/shareepicker.js 2012-04-26 08:00:30 +0000
@@ -109,7 +109,8 @@
this._fade_in(step_one_content, step_two_content);
},
- _render_step_two: function(back_enabled, allowed_permissions) {
+ _render_step_two: function(back_enabled, allowed_permissions,
+ disabled_some_types) {
var step_two_html = [
'<div class="picker-content-two transparent">',
'<div class="step-links">',
@@ -141,7 +142,8 @@
sharing_permissions.push(permission);
}
});
- var policy_selector = self._make_policy_selector(sharing_permissions);
+ var policy_selector = self._make_policy_selector(
+ sharing_permissions, disabled_some_types);
step_two_content.one('div.step-links')
.insert(policy_selector, 'before');
step_two_content.all('input[name^=field.permission]')
@@ -170,7 +172,8 @@
var step_two_content = contentBox.one('.picker-content-two');
if (step_two_content === null) {
step_two_content = this._render_step_two(
- data.back_enabled, data.allowed_permissions);
+ data.back_enabled, data.allowed_permissions,
+ data.disabled_some_types);
step_one_content.insert(step_two_content, 'after');
}
// Wire up the next (ie submit) links.
@@ -274,7 +277,8 @@
].join('');
},
- _make_policy_selector: function(allowed_permissions) {
+ _make_policy_selector: function(allowed_permissions,
+ disabled_some_types) {
// The policy selector is a set of radio buttons.
var sharing_permissions_template = this._sharing_permission_template();
var html = Y.lp.mustache.to_html([
@@ -310,7 +314,19 @@
};
}
});
- return Y.Node.create(html);
+ var policies = Y.Node.create(html);
+ if (Y.Lang.isArray(disabled_some_types)) {
+ Y.Array.each(disabled_some_types, function(info_type) {
+ policies.all('input[name=field.permission.' + info_type +
+ '][value=SOME]')
+ .each(function(permission_node) {
+ permission_node.set('disabled', true);
+ permission_node.set(
+ 'title', 'There are no shared artifacts.');
+ });
+ });
+ }
+ return policies;
},
_syncProgressUI: function() {
@@ -353,6 +369,7 @@
api_uri: config.sharee.person_uri,
sharee_permissions: config.sharee_permissions,
allowed_permissions: config.allowed_permissions,
+ disabled_some_types: config.disabled_some_types,
back_enabled: false
};
this._display_step_two(data);
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-04-11 02:15:10 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-04-26 08:00:30 +0000
@@ -21,11 +21,13 @@
{'name': 'fred', 'display_name': 'Fred Bloggs',
'role': '(Maintainer)', web_link: '~fred',
'self_link': '~fred',
- 'permissions': {'P1': 'ALL', 'P2': 'SOME'}},
+ 'permissions': {'P1': 'ALL', 'P2': 'SOME'},
+ 'shared_artifact_types': []},
{'name': 'john', 'display_name': 'John Smith',
'role': '', web_link: '~john',
'self_link': 'file:///api/devel/~john',
- 'permissions': {'P1': 'ALL', 'P3': 'SOME'}}
+ 'permissions': {'P1': 'ALL', 'P3': 'SOME'},
+ 'shared_artifact_types': ['P3']}
],
sharing_permissions: [
{'value': 'ALL', 'title': 'All',
@@ -141,6 +143,8 @@
Y.ArrayAssert.itemsAreEqual(
['ALL', 'NOTHING', 'SOME'],
config.allowed_permissions);
+ Y.ArrayAssert.itemsAreEqual(
+ ['P1', 'P2'], config.disabled_some_types);
show_picker_called = true;
};
var update_link =
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareepicker.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareepicker.js 2012-04-06 17:28:25 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareepicker.js 2012-04-26 08:00:30 +0000
@@ -237,6 +237,34 @@
Y.Assert.isNotNull(cb.one('input[value=SOME]'));
},
+ // Test that selected radio buttons for permission type 'Some' can be
+ // disabled.
+ test_some_permission_disabled: function() {
+ this.picker = this._create_picker();
+ // Select a person to trigger transition to next step.
+ this.picker.set('results', this.vocabulary);
+ this.picker.render();
+ this.picker.show({
+ first_step: 2,
+ sharee: {
+ person_uri: '~/fred',
+ person_name: 'Fred'
+ },
+ allowed_permissions: ['ALL', 'SOME', 'NOTHING'],
+ disabled_some_types: ['P1']
+ });
+ var cb = this.picker.get('contentBox');
+ cb.all('input[type=radio][name^=field.permission]').each(
+ function(permission_node) {
+ var expected_disabled =
+ permission_node.get('name')
+ === 'field.permission.P1' &&
+ permission_node.get('value') === 'SOME';
+ Y.Assert.areEqual(
+ expected_disabled, permission_node.get('disabled'));
+ });
+ },
+
// Test that the back link goes back to step one when step two is
// active.
test_second_step_back_link: function() {
=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py 2012-04-06 17:28:25 +0000
+++ lib/lp/registry/model/accesspolicy.py 2012-04-26 08:00:30 +0000
@@ -365,30 +365,39 @@
# A cache for the sharing permissions, keyed on grantee
permissions_cache = defaultdict(dict)
+ # aaa
+ shared_artifact_info_types = defaultdict(list)
def set_permission(person):
# Lookup the permissions from the previously loaded cache.
- return (person[0], permissions_cache[person[0]])
+ return (
+ person[0],
+ permissions_cache[person[0]],
+ shared_artifact_info_types[person[0]])
def load_permissions(people):
# We now have the grantees and policies we want in the result so
# load any corresponding permissions and cache them.
people_by_id = dict(
(person[0].id, person[0]) for person in people)
- sharing_permission_term = SQL(
- "CASE MIN(COALESCE(artifact, 0)) WHEN 0 THEN ? ELSE ? END",
- (SharingPermission.ALL.name, SharingPermission.SOME.name))
+ all_permission_term = SQL("bool_or(artifact IS NULL) as all")
+ some_permission_term = SQL(
+ "bool_or(artifact IS NOT NULL) as some")
constraints = [
cls.grantee_id.is_in(people_by_id.keys()),
cls.policy_id.is_in(policies_by_id.keys())]
result_set = IStore(cls).find(
- (cls.grantee_id, cls.policy_id, sharing_permission_term),
+ (cls.grantee_id, cls.policy_id, all_permission_term,
+ some_permission_term),
*constraints).group_by(cls.grantee_id, cls.policy_id)
- for (person_id, policy_id, permission) in result_set:
+ for (person_id, policy_id, has_all, has_some) in result_set:
person = people_by_id[person_id]
policy = policies_by_id[policy_id]
permissions_cache[person][policy] = (
- SharingPermission.items[str(permission)])
+ SharingPermission.ALL if has_all
+ else SharingPermission.SOME)
+ if has_some:
+ shared_artifact_info_types[person].append(policy.type)
constraints = [cls.policy_id.is_in(policies_by_id.keys())]
if grantees:
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-04-06 17:28:25 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-04-26 08:00:30 +0000
@@ -127,13 +127,14 @@
browser_request = IWebBrowserOriginatingRequest(request)
details_enabled = bool((getFeatureFlag(
'disclosure.enhanced_sharing_details.enabled')))
- for (grantee, permissions) in grant_permissions:
- some_things_sharred = False
+ for (grantee, permissions, shared_artifact_types) in grant_permissions:
+ some_things_shared = (
+ details_enabled and len(shared_artifact_types) > 0)
sharee_permissions = {}
for (policy, permission) in permissions.iteritems():
sharee_permissions[policy.type.name] = permission.name
- if details_enabled and permission == SharingPermission.SOME:
- some_things_sharred = True
+ shared_artifact_type_names = [
+ info_type.name for info_type in shared_artifact_types]
result.append({
'name': grantee.name,
'meta': 'team' if grantee.is_team else 'person',
@@ -141,7 +142,8 @@
'self_link': absoluteURL(grantee, request),
'web_link': absoluteURL(grantee, browser_request),
'permissions': sharee_permissions,
- 'shared_items_exist': some_things_sharred})
+ 'shared_artifact_types': shared_artifact_type_names,
+ 'shared_items_exist': some_things_shared})
return result
@available_with_permission('launchpad.Edit', 'pillar')
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-04-16 03:38:00 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-04-26 08:00:30 +0000
@@ -59,7 +59,8 @@
super(TestSharingService, self).setUp()
self.service = getUtility(IService, 'sharing')
- def _makeShareeData(self, sharee, policy_permissions):
+ def _makeShareeData(self, sharee, policy_permissions,
+ shared_artifact_types):
# Unpack a sharee into its attributes and add in permissions.
request = get_current_web_service_request()
sharee_data = {
@@ -77,6 +78,8 @@
if permission == SharingPermission.SOME:
shared_items_exist = True
sharee_data['shared_items_exist'] = shared_items_exist
+ sharee_data['shared_artifact_types'] = [
+ info_type.name for info_type in shared_artifact_types]
sharee_data['permissions'] = permissions
return sharee_data
@@ -143,11 +146,13 @@
sharees = self.service.jsonShareeData(
[(grantee, {
policy1: SharingPermission.ALL,
- policy2: SharingPermission.SOME})])
+ policy2: SharingPermission.SOME},
+ [policy1.type, policy2.type])])
expected_data = self._makeShareeData(
grantee,
[(policy1.type, SharingPermission.ALL),
- (policy2.type, SharingPermission.SOME)])
+ (policy2.type, SharingPermission.SOME)],
+ [policy1.type, policy2.type])
self.assertContentEqual([expected_data], sharees)
def test_jsonShareeData_with_Some_without_flag(self):
@@ -160,11 +165,11 @@
sharees = self.service.jsonShareeData(
[(grantee, {
policy1: SharingPermission.ALL,
- policy2: SharingPermission.SOME})])
+ policy2: SharingPermission.SOME}, [policy2.type])])
expected_data = self._makeShareeData(
grantee,
[(policy1.type, SharingPermission.ALL),
- (policy2.type, SharingPermission.SOME)])
+ (policy2.type, SharingPermission.SOME)], [policy2.type])
expected_data['shared_items_exist'] = False
self.assertContentEqual([expected_data], sharees)
@@ -178,10 +183,10 @@
with FeatureFixture(DETAILS_FLAG):
sharees = self.service.jsonShareeData(
[(grantee, {
- policy1: SharingPermission.ALL})])
+ policy1: SharingPermission.ALL}, [])])
expected_data = self._makeShareeData(
grantee,
- [(policy1.type, SharingPermission.ALL)])
+ [(policy1.type, SharingPermission.ALL)], [])
self.assertContentEqual([expected_data], sharees)
def _assert_getPillarShareeData(self, pillar):
@@ -202,10 +207,11 @@
expected_sharees = [
self._makeShareeData(
grantee,
- [(InformationType.PROPRIETARY, SharingPermission.ALL)]),
+ [(InformationType.PROPRIETARY, SharingPermission.ALL)], []),
self._makeShareeData(
artifact_grant.grantee,
- [(InformationType.PROPRIETARY, SharingPermission.SOME)])]
+ [(InformationType.PROPRIETARY, SharingPermission.SOME)],
+ [InformationType.PROPRIETARY])]
self.assertContentEqual(expected_sharees, sharees)
def test_getProductShareeData(self):
@@ -305,8 +311,9 @@
sharees = self.service.getPillarSharees(pillar)
expected_sharees = [
- (grantee, {access_policy: SharingPermission.ALL}),
- (artifact_grant.grantee, {access_policy: SharingPermission.SOME})]
+ (grantee, {access_policy: SharingPermission.ALL}, []),
+ (artifact_grant.grantee, {access_policy: SharingPermission.SOME},
+ [access_policy.type])]
self.assertContentEqual(expected_sharees, sharees)
def test_getProductSharees(self):
@@ -400,13 +407,15 @@
(InformationType.EMBARGOEDSECURITY, SharingPermission.ALL),
(InformationType.USERDATA, SharingPermission.SOME)]
expected_sharee_data = self._makeShareeData(
- sharee, expected_permissions)
+ sharee, expected_permissions,
+ [InformationType.USERDATA, InformationType.EMBARGOEDSECURITY])
self.assertEqual(expected_sharee_data, sharee_data)
# Check that getPillarSharees returns what we expect.
expected_sharee_grants = [
(sharee, {
es_policy: SharingPermission.ALL,
- ud_policy: SharingPermission.SOME})]
+ ud_policy: SharingPermission.SOME},
+ [InformationType.USERDATA, InformationType.EMBARGOEDSECURITY])]
sharee_grants = list(self.service.getPillarSharees(pillar))
self.assertContentEqual(expected_sharee_grants, sharee_grants)
@@ -518,11 +527,11 @@
access_policy for access_policy in access_policies
if access_policy.type in expected_information_types]
expected_data = [
- (grantee, {policy: SharingPermission.ALL})
+ (grantee, {policy: SharingPermission.ALL}, [])
for policy in expected_policies]
# Add the expected data for the other sharee.
another_person_data = (
- another, {access_policies[0]: SharingPermission.ALL})
+ another, {access_policies[0]: SharingPermission.ALL}, [])
expected_data.append(another_person_data)
self.assertContentEqual(
expected_data, self.service.getPillarSharees(pillar))
=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py 2012-04-06 17:28:25 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py 2012-04-26 08:00:30 +0000
@@ -515,7 +515,7 @@
policy = self.factory.makeAccessPolicy()
policy_grant = self.factory.makeAccessPolicyGrant(policy=policy)
self.assertContentEqual(
- [(policy_grant.grantee, {policy: SharingPermission.ALL})],
+ [(policy_grant.grantee, {policy: SharingPermission.ALL}, [])],
apgfs.findGranteePermissionsByPolicy(
[policy, policy_with_no_grantees]))
@@ -526,7 +526,7 @@
other_artifact_grant = self.factory.makeAccessArtifactGrant(
artifact=artifact)
self.assertContentEqual(
- [(policy_grant.grantee, {policy: SharingPermission.ALL})],
+ [(policy_grant.grantee, {policy: SharingPermission.ALL}, [])],
apgfs.findGranteePermissionsByPolicy(
[policy, policy_with_no_grantees]))
@@ -537,9 +537,11 @@
self.assertContentEqual(
[(policy_grant.grantee, {
policy: SharingPermission.ALL,
- another_policy: SharingPermission.SOME}),
+ another_policy: SharingPermission.SOME},
+ [another_policy.type]),
(other_artifact_grant.grantee, {
- another_policy: SharingPermission.SOME})],
+ another_policy: SharingPermission.SOME},
+ [another_policy.type])],
apgfs.findGranteePermissionsByPolicy([
policy, another_policy, policy_with_no_grantees]))
@@ -547,11 +549,34 @@
self.assertContentEqual(
[(policy_grant.grantee, {
policy: SharingPermission.ALL,
- another_policy: SharingPermission.SOME})],
+ another_policy: SharingPermission.SOME},
+ [another_policy.type])],
apgfs.findGranteePermissionsByPolicy([
policy, another_policy, policy_with_no_grantees]).order_by(
Person.id)[:1])
+ def test_findGranteePermissionsByPolicy_shared_artifact_types(self):
+ # findGranteePermissionsByPolicy() returns all information types for
+ # which grantees have been granted access one or more artifacts of that
+ # type.
+ apgfs = getUtility(IAccessPolicyGrantFlatSource)
+
+ policy_with_no_grantees = self.factory.makeAccessPolicy()
+ policy = self.factory.makeAccessPolicy()
+ policy_grant = self.factory.makeAccessPolicyGrant(policy=policy)
+
+ artifact = self.factory.makeAccessArtifact()
+ artifact_grant = self.factory.makeAccessArtifactGrant(
+ artifact=artifact, grantee=policy_grant.grantee)
+ self.factory.makeAccessPolicyArtifact(
+ artifact=artifact_grant.abstract_artifact, policy=policy)
+
+ self.assertContentEqual(
+ [(policy_grant.grantee, {policy: SharingPermission.ALL},
+ [policy.type])],
+ apgfs.findGranteePermissionsByPolicy(
+ [policy, policy_with_no_grantees]))
+
def test_findGranteePermissionsByPolicy_filter_grantees(self):
# findGranteePermissionsByPolicy() returns anyone with a grant for any
# of the policies or the policies' artifacts so long as the grantee is
@@ -567,7 +592,7 @@
self.factory.makeAccessPolicyGrant(
policy=policy, grantee=grantee_not_in_result)
self.assertContentEqual(
- [(policy_grant.grantee, {policy: SharingPermission.ALL})],
+ [(policy_grant.grantee, {policy: SharingPermission.ALL}, [])],
apgfs.findGranteePermissionsByPolicy(
[policy], [grantee_in_result]))
Follow ups
