launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06891
[Merge] lp:~wallyworld/launchpad/sharing-picker-title-959417 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/sharing-picker-title-959417 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #959417 in Launchpad itself: "sharing picker implies I am selecting a user when I am changing the policies shared"
https://bugs.launchpad.net/launchpad/+bug/959417
Bug #965197 in Launchpad itself: "sharee picker always uses first selected person"
https://bugs.launchpad.net/launchpad/+bug/965197
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-picker-title-959417/+merge/99459
== Implementation ==
The branch fixes 2 issues with the sharee picker:
1. Improve title and step title
2. After selecting a sharee, if the Back button is used and a new sharee is selected, the old sharee was still being used.
The picker displays different text for title and step title depending on if a new sharee is being added or if an existing sharee is being updated.
New sharee:
Share with a user or team
Select sharing policies for Fred
Edit sharee:
Update sharing policies
Update sharing policies for Fred
A bit of refactoring was done to move the code to render the step 2 content to a separate method.
== Tests ==
Update test_shareepicker.js to test the new title/steptitle behaviour and add a new test for the sharee selection issue.
== Lint ==
Linting changed files:
lib/lp/registry/javascript/sharing/shareepicker.js
lib/lp/registry/javascript/sharing/tests/test_shareepicker.js
--
https://code.launchpad.net/~wallyworld/launchpad/sharing-picker-title-959417/+merge/99459
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-picker-title-959417 into lp:launchpad.
=== modified file 'lib/lp/registry/javascript/sharing/shareepicker.js'
--- lib/lp/registry/javascript/sharing/shareepicker.js 2012-03-21 03:18:56 +0000
+++ lib/lp/registry/javascript/sharing/shareepicker.js 2012-03-27 02:32:03 +0000
@@ -52,6 +52,7 @@
}
this.set('information_types', information_types);
this.set('sharing_permissions', sharing_permissions);
+ this.step_one_header = this.get('headerContent');
var self = this;
this.subscribe('save', function (e) {
e.halt();
@@ -63,6 +64,8 @@
var data = e.details[Y.lazr.picker.Picker.SAVE_RESULT];
switch(step_nr) {
case 1:
+ data.sharee_name = data.title;
+ delete data.title;
self._display_step_two(data);
break;
case 2:
@@ -95,6 +98,7 @@
},
_display_step_one: function() {
+ this.set('headerContent', this.step_one_header);
this.set(
'steptitle',
'Search for user or exclusive team with whom to share');
@@ -105,67 +109,79 @@
this._fade_in(step_one_content, step_two_content);
},
+ _render_step_two: function(back_enabled, allowed_permissions) {
+ var step_two_html = [
+ '<div class="picker-content-two transparent">',
+ '<div class="step-links">',
+ '<a class="prev js-action" href="#">Back</a>',
+ '<button class="next lazr-pos lazr-btn"></button>',
+ '<a class="next js-action" href="#">Select</a>',
+ '</div></div>'
+ ].join(' ');
+ var self = this;
+ var step_two_content = Y.Node.create(step_two_html);
+ // Remove the back link if required.
+ if (Y.Lang.isBoolean(back_enabled) && !back_enabled ) {
+ step_two_content.one('a.prev').remove(true);
+ } else {
+ step_two_content.one('a.prev').on('click', function(e) {
+ e.halt();
+ self._display_step_one();
+ });
+ }
+ // By default, we only show All or Nothing.
+ if (!Y.Lang.isValue(allowed_permissions)) {
+ allowed_permissions = ['ALL', 'NOTHING'];
+ }
+ 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(policy_selector, 'before');
+ step_two_content.all('input[name^=field.permission]')
+ .on('click', function(e) {
+ self._disable_select_if_all_info_choices_nothing(step_two_content);
+ });
+ return step_two_content;
+ },
+
_display_step_two: function(data) {
- var title = Y.Lang.substitute('Select sharing policies for {name}',
- {name: data.title});
- this.set('steptitle', title);
+ if (Y.Lang.isValue(data.title)) {
+ this.set(
+ 'headerContent',
+ Y.Node.create('<h2></h2>').set('text', data.title));
+ }
+ var steptitle = data.steptitle;
+ if (!Y.Lang.isValue(steptitle)) {
+ steptitle = Y.Lang.substitute(
+ 'Select sharing policies for {name}',
+ {name: data.sharee_name});
+ }
+ this.set('steptitle', steptitle);
this.set('progress', 75);
var contentBox = this.get('contentBox');
var step_one_content = contentBox.one('.yui3-widget-bd');
var step_two_content = contentBox.one('.picker-content-two');
if (step_two_content === null) {
- var step_two_html = [
- '<div class="picker-content-two transparent">',
- '<div class="step-links">',
- '<a class="prev js-action" href="#">Back</a>',
- '<button class="next lazr-pos lazr-btn"></button>',
- '<a class="next js-action" href="#">Select</a>',
- '</div></div>'
- ].join(' ');
- step_two_content = Y.Node.create(step_two_html);
- var self = this;
- // Remove the back link if required.
- if (Y.Lang.isBoolean(data.back_enabled)
- && !data.back_enabled ) {
- step_two_content.one('a.prev').remove(true);
- } else {
- step_two_content.one('a.prev').on('click', function(e) {
- e.halt();
- self._display_step_one();
- });
- }
- // Wire up the next (ie submit) links.
- step_two_content.all('.next').on('click', function(e) {
- 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(policy_selector, 'before');
+ step_two_content = this._render_step_two(
+ data.back_enabled, data.allowed_permissions);
step_one_content.insert(step_two_content, 'after');
- step_two_content.all('input[name^=field.permission]')
- .on('click', function(e) {
- self._disable_select_if_all_info_choices_unticked(
- step_two_content);
- });
}
+ // Wire up the next (ie submit) links.
+ step_two_content.detach('click');
+ step_two_content.delegate('click', function(e) {
+ e.halt();
+ // Only submit if at least one info type is selected.
+ if (!this._all_info_choices_nothing(step_two_content)) {
+ this.fire('save', data, 2);
+ }
+ }, '.next', this);
// Initially set all radio buttons to Nothing.
step_two_content.all('input[name^=field.permission][value=NOTHING]')
.each(function(radio_button) {
@@ -183,7 +199,7 @@
}
});
}
- this._disable_select_if_all_info_choices_unticked(step_two_content);
+ this._disable_select_if_all_info_choices_nothing(step_two_content);
this._fade_in(step_two_content, step_one_content);
},
@@ -193,7 +209,7 @@
* @return {Boolean}
* @private
*/
- _all_info_choices_unticked: function(content) {
+ _all_info_choices_nothing: function(content) {
var all_unticked = true;
content.all('input[name^=field.permission]')
.each(function(info_node) {
@@ -209,8 +225,8 @@
* @param content
* @private
*/
- _disable_select_if_all_info_choices_unticked: function(content) {
- var disable_links = this._all_info_choices_unticked(content);
+ _disable_select_if_all_info_choices_nothing: function(content) {
+ var disable_links = this._all_info_choices_nothing(content);
content.all('.next').each(function(node) {
if (disable_links) {
node.addClass('invalid-link');
@@ -328,8 +344,12 @@
}
switch (config.first_step) {
case 2:
+ var steptitle = Y.Lang.substitute(
+ 'Update sharing policies for {name}',
+ {name: config.sharee.person_name});
var data = {
- title: config.sharee.person_name,
+ title: 'Update sharing policies',
+ steptitle: steptitle,
api_uri: config.sharee.person_uri,
sharee_permissions: config.sharee_permissions,
allowed_permissions: config.allowed_permissions,
@@ -350,5 +370,5 @@
ShareePicker.NAME = 'sharee_picker';
namespace.ShareePicker = ShareePicker;
-}, "0.1", { "requires": ['node', 'lp.mustache', 'lazr.picker'] });
+}, "0.1", { "requires": ['node', 'json', 'lp.mustache', 'lazr.picker'] });
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareepicker.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareepicker.js 2012-03-21 03:18:56 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareepicker.js 2012-03-27 02:32:03 +0000
@@ -57,7 +57,7 @@
anim_duration: 0,
progressbar: true,
progress: 50,
- headerContent: "<h2>Grant access</h2>",
+ headerContent: "<h2>Share with a user or team</h2>",
steptitle: "Search for user or exclusive team " +
"with whom to share",
zIndex: 1000,
@@ -96,6 +96,9 @@
this.picker.set('results', this.vocabulary);
this.picker.render();
var cb = this.picker.get('contentBox');
+ Y.Assert.areEqual(
+ 'Share with a user or team',
+ this.picker.get('headerContent').get('text'));
var steptitle = cb.one('.contains-steptitle h2').getContent();
Y.Assert.areEqual(
'Search for user or exclusive team with whom to share',
@@ -110,7 +113,10 @@
// The first step ui should be hidden.
Y.Assert.isTrue(cb.one('.yui3-widget-bd').hasClass('unseen'));
// The step title should be updated according to the selected
- // person.
+ // person. The title should remain unchanged.
+ Y.Assert.areEqual(
+ 'Share with a user or team',
+ this.picker.get('headerContent').get('text'));
steptitle = cb.one('.contains-steptitle h2').getContent();
Y.Assert.areEqual(
'Select sharing policies for Fred', steptitle);
@@ -172,9 +178,13 @@
sharee_permissions: {'P1': 'ALL'}
});
var cb = this.picker.get('contentBox');
+ // Check the title and step title are correct.
var steptitle = cb.one('.contains-steptitle h2').getContent();
Y.Assert.areEqual(
- 'Select sharing policies for Fred', steptitle);
+ 'Update sharing policies',
+ this.picker.get('headerContent').get('text'));
+ Y.Assert.areEqual(
+ 'Update sharing policies for Fred', steptitle);
// By default, selections only for ALL and NOTHING are available
// (and no others).
Y.Assert.isNotNull(cb.one('input[value=ALL]'));
@@ -244,7 +254,10 @@
Y.Assert.areEqual(50, this.picker.get('progress'));
// The first step ui should be visible.
Y.Assert.isFalse(cb.one('.yui3-widget-bd').hasClass('unseen'));
- // The step title should be updated.
+ // The title and step title should be updated.
+ Y.Assert.areEqual(
+ 'Share with a user or team',
+ this.picker.get('headerContent').get('text'));
var steptitle = cb.one('.contains-steptitle h2').getContent();
Y.Assert.areEqual(
'Search for user or exclusive team with whom to share',
@@ -288,6 +301,47 @@
Y.Assert.areEqual('~/fred', selected_result.api_uri);
},
+ // Test that a new sharee can be selected after click the Back button
+ // and the new sharee is correctly used in the final result.
+ test_sharee_reselection: function() {
+ var selected_result;
+ this.picker = this._create_picker(
+ {
+ save: function(result) {
+ selected_result = result;
+ }
+ }
+ );
+ // Select a person to trigger transition to next step.
+ this.picker.set('results', this.vocabulary);
+ this.picker.render();
+ this.picker.get('boundingBox').one(
+ '.yui3-picker-results li:nth-child(1)').simulate('click');
+ var cb = this.picker.get('contentBox');
+ var step_two_content = cb.one('.picker-content-two');
+ var back_link = step_two_content.one('a.prev');
+ back_link.simulate('click');
+ // Select a different person.
+ this.picker.get('boundingBox').one(
+ '.yui3-picker-results li:nth-child(2)').simulate('click');
+ // Select an access policy.
+ 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');
+ // Check the results.
+ Y.Assert.areEqual(
+ 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('~/frieda', selected_result.api_uri);
+ },
+
// When no info types are selected, the Submit links are disabled.
test_no_selected_info_types: function() {
var save_called = false;
