launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06805
[Merge] lp:~wallyworld/launchpad/tri-state-sharee-picker-959784 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/tri-state-sharee-picker-959784 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #959784 in Launchpad itself: "Cannot add access to new information type without destoying existing 'Some' access"
https://bugs.launchpad.net/launchpad/+bug/959784
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/tri-state-sharee-picker-959784/+merge/98556
== Implementation ==
The permissions selection on the sharing picker now uses radio buttons to allow selection of 'All', 'Some' or 'Nothing'. Adding a new sharee only shows 'All' or 'Nothing'.
The picker shows the radio buttons in the order 'All', 'Some', 'Nothing'.
The core picker changes were copied and adapted from the interactive mockup.
== Tests ==
Update pillarsharingview and shareepicker yui tests.
Add new test to check that getSharingPermissions() gives the permissions in the right order (All, Some, Nothing)
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
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/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
--
https://code.launchpad.net/~wallyworld/launchpad/tri-state-sharee-picker-959784/+merge/98556
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/tri-state-sharee-picker-959784 into lp:launchpad.
=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-03-16 23:46:20 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-03-21 02:37:19 +0000
@@ -86,9 +86,10 @@
zIndex: 1000,
visible: false,
information_types: LP.cache.information_types,
+ sharing_permissions: LP.cache.sharing_permissions,
save: function(result) {
self.save_sharing_selection(
- result.api_uri, result.sharing_permissions);
+ result.api_uri, result.selected_permissions);
}
});
var ns = Y.lp.registry.sharing.shareepicker;
@@ -347,31 +348,29 @@
* @param person_name
*/
update_sharee_interaction: function(update_link, person_uri, person_name) {
- var information_types_by_value =
- this.get('information_types_by_value');
var sharee_data = LP.cache.sharee_data;
- var selected_permissions = [];
+ var sharee_permissions = {};
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);
if (sharee.self_link !== full_person_uri) {
return false;
}
- Y.each(sharee.permissions, function(permission, info_type_value) {
- //we only support ALL for now
- if ('ALL' === permission) {
- selected_permissions.push(info_type_value);
- }
- });
+ sharee_permissions = sharee.permissions;
return true;
});
+ var allowed_permissions = [];
+ Y.Array.each(LP.cache.sharing_permissions, function(permission) {
+ allowed_permissions.push(permission.value);
+ });
this.get('sharee_picker').show({
first_step: 2,
sharee: {
person_uri: person_uri,
person_name: person_name
},
- selected_permissions: selected_permissions
+ sharee_permissions: sharee_permissions,
+ allowed_permissions: allowed_permissions
});
}
});
=== modified file 'lib/lp/registry/javascript/sharing/shareepicker.js'
--- lib/lp/registry/javascript/sharing/shareepicker.js 2012-03-15 08:54:40 +0000
+++ lib/lp/registry/javascript/sharing/shareepicker.js 2012-03-21 02:37:19 +0000
@@ -27,6 +27,9 @@
information_types: {
value: []
},
+ sharing_permissions: {
+ value: []
+ },
// Override for testing
anim_duration: {
value: 1
@@ -38,15 +41,20 @@
initializer: function(config) {
ShareePicker.superclass.initializer.apply(this, arguments);
var information_types = [];
+ var sharing_permissions = [];
if (config !== undefined) {
if (config.information_types !== undefined) {
information_types = config.information_types;
}
+ if (config.sharing_permissions !== undefined) {
+ sharing_permissions = config.sharing_permissions;
+ }
}
this.set('information_types', information_types);
+ this.set('sharing_permissions', sharing_permissions);
var self = this;
this.subscribe('save', function (e) {
- e.preventDefault();
+ e.halt();
// The step number indicates which picker step has just fired.
var step_nr = e.details[1];
if (!Y.Lang.isNumber(step_nr)) {
@@ -122,34 +130,54 @@
step_two_content.one('a.prev').remove(true);
} else {
step_two_content.one('a.prev').on('click', function(e) {
- e.preventDefault();
+ e.halt();
self._display_step_one();
});
}
// Wire up the next (ie submit) links.
step_two_content.all('.next').on('click', function(e) {
- e.preventDefault();
+ e.halt();
// Only submit if at least one info type is selected.
if (!self._all_info_choices_unticked(step_two_content)) {
self.fire('save', data, 2);
}
});
+ // By default, we only show All or Nothing.
+ var allowed_permissions = ['ALL', 'NOTHING'];
+ if (Y.Lang.isValue(data.allowed_permissions)) {
+ allowed_permissions = data.allowed_permissions;
+ }
+ var sharing_permissions = [];
+ Y.Array.each(this.get('sharing_permissions'),
+ function(permission) {
+ if (Y.Array.indexOf(
+ allowed_permissions, permission.value) >=0) {
+ sharing_permissions.push(permission);
+ }
+ });
+ var policy_selector = self._make_policy_selector(
+ sharing_permissions);
step_two_content.one('div.step-links')
- .insert(self._make_policy_selector(), 'before');
+ .insert(policy_selector, 'before');
step_one_content.insert(step_two_content, 'after');
- step_two_content.all('input[name=field.visibility]')
+ step_two_content.all('input[name^=field.permission]')
.on('click', function(e) {
self._disable_select_if_all_info_choices_unticked(
step_two_content);
});
}
- // If we have been given values for the information_type data, ensure
- // the relevant checkboxes are ticked.
- if (Y.Lang.isArray(data.information_types)) {
- Y.Array.each(data.information_types, function(info_type) {
+ // Initially set all radio buttons to Nothing.
+ step_two_content.all('input[name^=field.permission][value=NOTHING]')
+ .each(function(radio_button) {
+ radio_button.set('checked', true);
+ });
+ // Ensure the correct radio buttons are ticked according to the
+ // sharee_permissions.
+ if (Y.Lang.isObject(data.sharee_permissions)) {
+ Y.each(data.sharee_permissions, function(perm, type) {
var cb = step_two_content.one(
- 'input[name=field.visibility]' +
- '[value="' + info_type + '"]');
+ 'input[name=field.permission.'+type+']' +
+ '[value="' + perm + '"]');
if (Y.Lang.isValue(cb)) {
cb.set('checked', true);
}
@@ -160,16 +188,18 @@
},
/**
- * Are all the info type checkboxes unticked?
+ * Are all the radio buttons set to Nothing?
* @param content
* @return {Boolean}
* @private
*/
_all_info_choices_unticked: function(content) {
var all_unticked = true;
- content.all('input[name=field.visibility]')
+ content.all('input[name^=field.permission]')
.each(function(info_node) {
- all_unticked &= !info_node.get('checked');
+ if (info_node.get('value') !== 'NOTHING') {
+ all_unticked &= !info_node.get('checked');
+ }
});
return all_unticked;
},
@@ -191,50 +221,78 @@
},
_publish_result: function(data) {
- // Determine the chosen information type. data already contains the
+ // Determine the selected permisios. data already contains the
// selected person due to the base picker behaviour.
var contentBox = this.get('contentBox');
- var sharing_permissions = {};
- contentBox.all('input[name=field.visibility]')
- .each(function(node) {
- var permission = 'NOTHING';
+ var selected_permissions = {};
+ Y.Array.each(this.get('information_types'), function(info_type) {
+ contentBox.all('input[name=field.permission.'+info_type.value+']')
+ .each(function(node) {
if (node.get('checked')) {
- permission = 'ALL';
+ selected_permissions[info_type.value] = node.get('value');
}
- sharing_permissions[node.get('value')] = permission;
});
- data.sharing_permissions = sharing_permissions;
+ });
+ data.selected_permissions = selected_permissions;
// Publish the result with step_nr 0 to indicate we have finished.
this.fire('save', data, 0);
},
- _make_policy_selector: function() {
+ _sharing_permission_template: function() {
+ return [
+ '<table class="radio-button-widget"><tbody>',
+ '{{#permissions}}',
+ '<tr>',
+ ' <input type="radio"',
+ ' value="{{value}}"',
+ ' name="field.permission.{{info_type}}"',
+ ' id="field.permission.{{info_type}}.{{index}}"',
+ ' class="radioType">',
+ ' <label for="field.permission.{{info_type}}"',
+ ' title="{{description}}">',
+ ' {{title}}',
+ ' </label>',
+ '</tr>',
+ '{{/permissions}}',
+ '</tbody></table>'
+ ].join('');
+ },
+
+ _make_policy_selector: function(allowed_permissions) {
// The policy selector is a set of radio buttons.
+ var sharing_permissions_template = this._sharing_permission_template();
var html = Y.lp.mustache.to_html([
'<div class="selection-choices">',
- '<table class="radio-button-widget"><tbody>',
+ '<table><tbody>',
'{{#policies}}',
- ' <tr>',
- ' <td rowspan="2"><input type="checkbox"',
- ' value="{{value}}"',
- ' name="field.visibility"',
- ' id="field.visibility.{{index}}"',
- ' class="checkboxType">',
- ' </td>',
- ' <td><label for="field.visibility.{{index}}">',
+ '<tr>',
+ ' <td><strong>',
' <span class="accessPolicy{{value}}">{{title}}',
- ' </span></label>',
- ' </td>',
- ' </tr>',
- ' <tr>',
- ' <td class="formHelp">',
- ' {{description}}',
- ' </td>',
- ' </tr>',
+ ' </span>',
+ ' </strong></td>',
+ '</tr>',
+ '<tr>',
+ ' <td>',
+ ' {{#sharing_permissions}} {{/sharing_permissions}}',
+ ' </td>',
+ '</tr>',
+ '<tr>',
+ ' <td class="formHelp">',
+ ' {{description}}',
+ ' </td>',
+ '</tr>',
'{{/policies}}',
'</tbody></table></div>'
].join(''), {
- policies: this.get('information_types')
+ policies: this.get('information_types'),
+ sharing_permissions: function() {
+ return function(text, render) {
+ return Y.lp.mustache.to_html(sharing_permissions_template, {
+ permissions: allowed_permissions,
+ info_type: render('{{value}}')
+ });
+ };
+ }
});
return Y.Node.create(html);
},
@@ -273,7 +331,8 @@
var data = {
title: config.sharee.person_name,
api_uri: config.sharee.person_uri,
- information_types: config.selected_permissions,
+ sharee_permissions: config.sharee_permissions,
+ allowed_permissions: config.allowed_permissions,
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-03-16 08:18:37 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2012-03-21 02:37:19 +0000
@@ -20,19 +20,19 @@
{'name': 'fred', 'display_name': 'Fred Bloggs',
'role': '(Maintainer)', web_link: '~fred',
'self_link': '~fred',
- 'permissions': {'P1': 'ALL', 'P2': 's2'}},
+ 'permissions': {'P1': 'ALL', 'P2': 'SOME'}},
{'name': 'john', 'display_name': 'John Smith',
'role': '', web_link: '~john',
'self_link': 'file:///api/devel/~john',
- 'permissions': {'P1': 'ALL', 'P3': 's3'}}
+ 'permissions': {'P1': 'ALL', 'P3': 'SOME'}}
],
sharing_permissions: [
{'value': 'ALL', 'title': 'All',
'description': 'Everything'},
{'value': 'NOTHING', 'title': 'Nothing',
'description': 'Nothing'},
- {'value': 's2', 'title': 'S2',
- 'description': 'Sharing 2'}
+ {'value': 'SOME', 'title': 'Some',
+ 'description': 'Some'}
],
information_types: [
{index: '0', value: 'P1', title: 'Policy 1',
@@ -123,8 +123,12 @@
Y.Assert.areEqual(2, config.first_step);
Y.Assert.areEqual('~john', config.sharee.person_uri);
Y.Assert.areEqual('John', config.sharee.person_name);
+ Y.Assert.areEqual(2, Y.Object.size(config.sharee_permissions));
+ Y.Assert.areEqual('ALL', config.sharee_permissions.P1);
+ Y.Assert.areEqual('SOME', config.sharee_permissions.P3);
Y.ArrayAssert.itemsAreEqual(
- ['P1'], config.selected_permissions);
+ ['ALL', 'NOTHING', 'SOME'],
+ config.allowed_permissions);
show_picker_called = true;
};
var update_link =
@@ -278,7 +282,16 @@
'.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="P2"]').simulate('click');
+ // All sharing permissions should initially be set to nothing.
+ step_two_content.all('input[name^=field.permission]')
+ .each(function(radio_button) {
+ if (radio_button.get('checked')) {
+ Y.Assert.areEqual('NOTHING', radio_button.get('value'));
+ }
+ });
+ step_two_content
+ .one('input[name=field.permission.P2][value="ALL"]')
+ .simulate('click');
var select_link = step_two_content.one('a.next');
select_link.simulate('click');
// Selection made using the picker, now check the results.
@@ -328,7 +341,7 @@
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]');
+ '.yui3-ichoicelist-content a[href=#SOME]');
permission_choice.simulate('click');
// Selection made, now check the results.
@@ -345,7 +358,7 @@
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');
+ expected_url, 'permissions', 'Policy 1,Some');
Y.Assert.areEqual(expected_url, mockio.last_request.config.data);
mockio.last_request.successJSON({
'name': 'fred',
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareepicker.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareepicker.js 2012-03-15 08:54:40 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareepicker.js 2012-03-21 02:37:19 +0000
@@ -25,6 +25,14 @@
description: 'Policy 2 description'},
{index: '2', value: 'P3', title: 'Policy 3',
description: 'Policy 3 description'}];
+ this.sharing_permissions = [
+ {'index': 0, 'value': 'ALL', 'title': 'All',
+ 'description': 'Everything'},
+ {'index': 1, 'value': 'NOTHING', 'title': 'Nothing',
+ 'description': 'Nothing'},
+ {'index': 2, 'value': 'SOME', 'title': 'Some',
+ 'description': 'Some'}
+ ];
},
tearDown: function () {
@@ -54,7 +62,8 @@
"with whom to share",
zIndex: 1000,
visible: false,
- information_types: this.information_types
+ information_types: this.information_types,
+ sharing_permissions: this.sharing_permissions
};
if (overrides !== undefined) {
config = Y.merge(config, overrides);
@@ -109,11 +118,14 @@
var step_two_content = cb.one('.picker-content-two');
Y.Assert.isFalse(step_two_content.hasClass('unseen'));
// The second step ui should contain input buttons for each access
- // policy type.
+ // policy type for each sharing permission.
Y.Array.each(this.information_types, function(info_type) {
- var rb = step_two_content.one(
- 'input[value="' + info_type.value + '"]');
- Y.Assert.isNotNull(rb);
+ Y.Array.each(this.sharing_permissions, function(permission) {
+ var rb = step_two_content.one(
+ 'input[name=field.permission.' + info_type.value +
+ '][value="' + permission.value + '"]');
+ Y.Assert.isNotNull(rb);
+ });
});
// There should be a link back to previous step.
Y.Assert.isNotNull(step_two_content.one('a.prev'));
@@ -157,21 +169,34 @@
person_uri: '~/fred',
person_name: 'Fred'
},
- selected_permissions: ['P1']
+ sharee_permissions: {'P1': 'ALL'}
});
var cb = this.picker.get('contentBox');
var steptitle = cb.one('.contains-steptitle h2').getContent();
Y.Assert.areEqual(
'Select sharing policies for Fred', steptitle);
+ // By default, selections only for ALL and NOTHING are available
+ // (an no others).
+ Y.Assert.isNotNull(cb.one('input[value=ALL]'));
+ Y.Assert.isNotNull(cb.one('input[value=NOTHING]'));
+ Y.Assert.isNull(cb.one('input[value=SOME]'));
// Selected permission checkboxes should be ticked.
- cb.all('input[name=field.visibility]')
- .each(function(node) {
+ cb.all('input[name=field.permission.P1]')
+ .each(function(node) {
+ if (node.get('checked')) {
+ Y.Assert.areEqual('ALL', node.get('value'));
+ } else {
+ Y.Assert.areEqual('NOTHING', node.get('value'));
+ }
+ });
+ Y.Array.each(['P2', 'P3'], function(info_type) {
+ cb.all('input[name=field.permission.' + info_type + ']')
+ .each(function(node) {
if (node.get('checked')) {
- Y.Assert.areEqual('P1', node.get('value'));
- } else {
- Y.Assert.areNotEqual('P1', node.get('value'));
+ Y.Assert.areEqual('NOTHING', node.get('value'));
}
});
+ });
// Back link should not he shown
var back_link = cb.one('a.prev');
Y.Assert.isNull(back_link);
@@ -181,6 +206,27 @@
Y.Assert.areEqual('~/fred', selected_result.api_uri);
},
+ // Test that show() can be used to open the second picker screen and
+ // that we can filter what permissions are shown.
+ test_filtered_permissions: 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: ['SOME']
+ });
+ var cb = this.picker.get('contentBox');
+ Y.Assert.isNull(cb.one('input[value=ALL]'));
+ Y.Assert.isNull(cb.one('input[value=NOTHING]'));
+ Y.Assert.isNotNull(cb.one('input[value=SOME]'));
+ },
+
// Test that the back link goes back to step one when step two is
// active.
test_second_step_back_link: function() {
@@ -226,17 +272,19 @@
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="P2"]').simulate('click');
+ step_two_content
+ .one('input[name=field.permission.P2][value="ALL"]')
+ .simulate('click');
var select_link = step_two_content.one('a.next');
select_link.simulate('click');
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');
+ 3, Y.Object.size(selected_result.selected_permissions));
+ Y.Assert.areEqual(
+ selected_result.selected_permissions.P1, 'NOTHING');
+ Y.Assert.areEqual(
+ selected_result.selected_permissions.P2, 'ALL');
+ Y.Assert.areEqual(
+ selected_result.selected_permissions.P3, 'NOTHING');
Y.Assert.areEqual('~/fred', selected_result.api_uri);
},
@@ -268,19 +316,22 @@
select_link.simulate('click');
Y.Assert.isFalse(save_called);
// Select one info type and check submit links are enabled.
- cb.one('input[name=field.visibility]').simulate('click');
+ cb.one('input[name=field.permission.P1][value=ALL]')
+ .simulate('click');
cb.all('a.next', function(link) {
Y.Assert.isFalse(link.hasClass('invalid-link'));
});
// De-select info type and submit links are disabled again.
- cb.one('input[name=field.visibility]').simulate('click');
+ cb.one('input[name=field.permission.P1][value=NOTHING]')
+ .simulate('click');
cb.all('a.next', function(link) {
Y.Assert.isTrue(link.hasClass('invalid-link'));
});
select_link.simulate('click');
Y.Assert.isFalse(save_called);
// Once at least one info type is selected, save is called.
- cb.one('input[name=field.visibility]').simulate('click');
+ cb.one('input[name=field.permission.P1][value=ALL]')
+ .simulate('click');
select_link.simulate('click');
Y.Assert.isTrue(save_called);
}
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-03-19 05:53:39 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-03-21 02:37:19 +0000
@@ -76,12 +76,19 @@
def getSharingPermissions(self):
"""See `ISharingService`."""
+ # We want the permissions displayed in the following order.
+ ordered_permissions = [
+ SharingPermission.ALL,
+ SharingPermission.SOME,
+ SharingPermission.NOTHING
+ ]
sharing_permissions = []
- for permission in SharingPermission:
+ for x, permission in enumerate(ordered_permissions):
item = dict(
- value=permission.token,
+ index=x,
+ value=permission.name,
title=permission.title,
- description=permission.value.description
+ description=permission.description
)
sharing_permissions.append(item)
return sharing_permissions
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-03-16 08:28:51 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-03-21 02:37:19 +0000
@@ -71,6 +71,17 @@
sharee_data['permissions'] = permissions
return sharee_data
+ def test_getSharingPermissions(self):
+ # test_getSharingPermissions returns permissions in the right order.
+ permissions = self.service.getSharingPermissions()
+ expected_permissions = [
+ SharingPermission.ALL,
+ SharingPermission.SOME,
+ SharingPermission.NOTHING
+ ]
+ for x, permission in enumerate(expected_permissions):
+ self.assertEqual(permissions[x]['value'], permission.name)
+
def _test_getInformationTypes(self, pillar, expected_policies):
policy_data = self.service.getInformationTypes(pillar)
expected_data = []
