launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04281
[Merge] lp:~wallyworld/launchpad/confusing-picker-text-809651 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/confusing-picker-text-809651 into lp:launchpad with lp:~wallyworld/launchpad/picker-widget-meta as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #809651 in Launchpad itself: "Person picker link text is confusing"
https://bugs.launchpad.net/launchpad/+bug/809651
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/confusing-picker-text-809651/+merge/67962
This branch is 3 of 3. Once reviewed, I will be landing this branch only.
The end goal is to dynamically update the "Remove" link text on the person picker to say "Remove team" is the current field value is a team and "Remove person" if the current field value is a person. The text can be specified in the picker config if required, as is the case for the bug assignee picker.
== Implementation ==
The javascript used for person pickers has been changed to:
- add a 'selected_value_meta' attribute to PersonPicker
- set the text for the "remove" link to the specified remove_person_text value if the widget selected_value_meta is 'person', or remove_team_text value if the widget selected_value_meta is 'team'
- the above is done when the picker first loads (using data supplied by the view) and when a new item is selected and saved (using the meta attribute from the selected item).
The picker_patcher.js already had a show_hide_buttons() method which was called at the right times. This method was updated to also set the correct text on the "Remove" link.
== Demo and QA ==
The easier way to look at the new functionality is to assign a person or team to a bug and open the picker again and observe the text.
== Tests ==
Tests were added to test_picker.js, test_personpicker.js and test_picker_patcher.js
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/picker/person_picker.js
lib/lp/app/javascript/picker/picker.js
lib/lp/app/javascript/picker/picker_patcher.js
lib/lp/app/javascript/picker/tests/test_personpicker.js
lib/lp/app/javascript/picker/tests/test_picker.js
lib/lp/app/javascript/picker/tests/test_picker_patcher.js
lib/lp/bugs/browser/bugtask.py
lib/lp/bugs/javascript/bugtask_index.js
./lib/lp/app/javascript/picker/person_picker.js
15: 'PersonPicker' has not been fully defined yet.
./lib/lp/app/javascript/picker/picker.js
65: 'Picker' has not been fully defined yet.
245: Move 'var' declarations to the top of the function.
245: Stopping. (23% scanned).
-1: JSLINT had a fatal error.
585: Line exceeds 78 characters.
1020: Line exceeds 78 characters.
./lib/lp/app/javascript/picker/picker_patcher.js
103: Line exceeds 78 characters.
./lib/lp/app/javascript/picker/tests/test_personpicker.js
170: Line exceeds 78 characters.
./lib/lp/app/javascript/picker/tests/test_picker.js
194: Move 'var' declarations to the top of the function.
194: Stopping. (20% scanned).
-1: JSLINT had a fatal error.
48: Line exceeds 78 characters.
463: Line exceeds 78 characters.
514: Line exceeds 78 characters.
958: Line exceeds 78 characters.
ian@wallyworld:~/projects/lp-branches/
--
https://code.launchpad.net/~wallyworld/launchpad/confusing-picker-text-809651/+merge/67962
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/confusing-picker-text-809651 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/picker/person_picker.js'
--- lib/lp/app/javascript/picker/person_picker.js 2011-07-06 14:13:04 +0000
+++ lib/lp/app/javascript/picker/person_picker.js 2011-07-15 03:33:10 +0000
@@ -23,8 +23,9 @@
var show_assign_me_button = true;
var show_remove_button = true;
- var assign_button_text = 'Pick Me';
- var remove_button_text = 'Remove Person';
+ var assign_me_text = 'Pick me';
+ var remove_person_text = 'Remove person';
+ var remove_team_text = 'Remove team';
if (cfg !== undefined) {
if (cfg.show_assign_me_button !== undefined) {
@@ -33,17 +34,21 @@
if (cfg.show_remove_button !== undefined) {
show_remove_button = cfg.show_remove_button;
}
- if (cfg.assign_button_text !== undefined) {
- assign_button_text = cfg.assign_button_text;
- }
- if (cfg.remove_button_text !== undefined) {
- remove_button_text = cfg.remove_button_text;
+ if (cfg.assign_me_text !== undefined) {
+ assign_me_text = cfg.assign_me_text;
+ }
+ if (cfg.remove_person_text !== undefined) {
+ remove_person_text = cfg.remove_person_text;
+ }
+ if (cfg.remove_team_text !== undefined) {
+ remove_team_text = cfg.remove_team_text;
}
}
this._show_assign_me_button = show_assign_me_button;
this._show_remove_button = show_remove_button;
- this._assign_me_button_text = assign_button_text;
- this._remove_button_text = remove_button_text;
+ this._assign_me_text = assign_me_text;
+ this._remove_person_text = remove_person_text;
+ this._remove_team_text = remove_team_text;
},
hide: function() {
@@ -73,7 +78,7 @@
'<a class="yui-picker-remove-button bg-image" ' +
'href="javascript:void(0)" ' +
'style="background-image: url(/@@/remove); padding-right: ' +
- '1em">' + this._remove_button_text + '</a>');
+ '1em">' + this._remove_person_text + '</a>');
remove_button.on('click', this.remove, this);
this._extra_buttons.appendChild(remove_button);
this.remove_button = remove_button;
@@ -84,7 +89,7 @@
'<a class="yui-picker-assign-me-button bg-image" ' +
'href="javascript:void(0)" ' +
'style="background-image: url(/@@/person)">' +
- this._assign_me_button_text + '</a>');
+ this._assign_me_text + '</a>');
assign_me_button.on('click', this.assign_me, this);
this._extra_buttons.appendChild(assign_me_button);
this.assign_me_button = assign_me_button;
=== modified file 'lib/lp/app/javascript/picker/picker.js'
--- lib/lp/app/javascript/picker/picker.js 2011-07-12 14:22:14 +0000
+++ lib/lp/app/javascript/picker/picker.js 2011-07-15 03:33:10 +0000
@@ -58,7 +58,7 @@
NO_RESULTS_SEARCH_MESSAGE = 'no_results_search_message',
RENDERUI = "renderUI",
BINDUI = "bindUI",
- SYNCUI = "syncUI";
+ SELECTED_VALUE_META = 'selected_value_meta';
var Picker = function () {
@@ -181,6 +181,13 @@
element.addClass(this.get('picker_activator_css_class'));
}
+ if (!Y.Lang.isUndefined(cfg)) {
+ // Meta information associated with the picker's associated
+ // field's current value.
+ if (Y.Lang.isValue(cfg[SELECTED_VALUE_META])) {
+ this.set(SELECTED_VALUE_META, cfg[SELECTED_VALUE_META]);
+ }
+ }
},
/**
@@ -854,6 +861,15 @@
current_search_string: {value: ''},
/**
+ * Meta information about the current state of the associated field,
+ * whose value is selected by using this picker.
+ *
+ * @attribute selected_value_meta
+ * @type String
+ */
+ selected_value_meta: {value: null},
+
+ /**
* Results currently displayed by the widget. Updating this value
* automatically updates the display.
*
@@ -991,7 +1007,8 @@
initializer: function(config) {
var input = Y.one(config.input_element);
this.doAfter('save', function (e) {
- var result = e.details[Y.lazr.picker.Picker.SAVE_RESULT];
+ var result = e.details[Picker.SAVE_RESULT];
+ this.get('host').set(SELECTED_VALUE_META, result.meta);
input.set("value", result.value);
// If the search input isn't blurred before it is focused,
// then the I-beam disappears.
=== modified file 'lib/lp/app/javascript/picker/picker_patcher.js'
--- lib/lp/app/javascript/picker/picker_patcher.js 2011-07-14 06:02:51 +0000
+++ lib/lp/app/javascript/picker/picker_patcher.js 2011-07-15 03:33:10 +0000
@@ -21,8 +21,6 @@
* @param {Object} config Object literal of config name/value pairs.
* config.header: a line of text at the top of the widget.
* config.step_title: overrides the subtitle.
- * config.assign_button_text: Override the default 'Assign' text.
- * config.remove_button_text: Override the default 'Remove' text.
* config.null_display_value: Override the default 'None' text.
* config.show_remove_button: Should the remove button be shown?
* Defaults to false, should be a boolean.
@@ -81,6 +79,14 @@
'</div>'));
};
+ var update_button_text = function() {
+ var link_text = picker._remove_person_text;
+ if (picker.get('selected_value_meta')==='team') {
+ link_text = picker._remove_team_text;
+ }
+ remove_button.set('innerHTML', link_text);
+ };
+
var show_hide_buttons = function () {
var link = content_box.one('.yui3-activator-data-box a');
if (remove_button) {
@@ -88,12 +94,13 @@
remove_button.addClass('yui3-picker-hidden');
} else {
remove_button.removeClass('yui3-picker-hidden');
+ update_button_text();
}
}
if (assign_me_button) {
if (link !== null
- && link.get('href').match(LP.links.me + "$") == LP.links.me) {
+ && link.get('href').match(LP.links.me + "$") === LP.links.me) {
assign_me_button.addClass('yui3-picker-hidden');
} else {
assign_me_button.removeClass('yui3-picker-hidden');
@@ -104,6 +111,7 @@
var save = function (picker_result) {
activator.renderProcessing();
var success_handler = function (entry) {
+ picker.set('selected_value_meta', picker_result.meta);
activator.renderSuccess(entry.getHTML(attribute_name));
show_hide_buttons();
};
=== modified file 'lib/lp/app/javascript/picker/tests/test_personpicker.js'
--- lib/lp/app/javascript/picker/tests/test_personpicker.js 2011-07-12 14:22:14 +0000
+++ lib/lp/app/javascript/picker/tests/test_personpicker.js 2011-07-15 03:33:10 +0000
@@ -86,29 +86,12 @@
delete window.LP;
},
- create_picker: function(
- show_assign_me_button, show_remove_button, field_value) {
- if (field_value !== undefined) {
- var data_box = Y.one('#picker_id .yui3-activator-data-box');
- data_box.appendChild(Y.Node.create('<a>Content</a>'));
- data_box.one('a').set('href', field_value);
- }
- if (show_assign_me_button === undefined) {
- show_assign_me_button = false;
- }
- if (show_remove_button === undefined) {
- show_remove_button = false;
- }
-
+ create_picker: function() {
var config = {
"step_title": "Choose someone",
"header": "Pick Someone",
"validate_callback": null,
"show_search_box": true,
- "show_assign_me_button": show_assign_me_button,
- "show_remove_button": show_remove_button,
- "assign_button_text": "Assign Moi",
- "remove_button_text": "Remove someone",
"picker_type": 'person'
};
this.picker = Y.lp.app.picker.addPickerPatcher(
@@ -166,7 +149,7 @@
test_buttons_vanish: function () {
// The assign/remove links are hidden when a search is performed.
- this.create_picker(true, true);
+ this.create_picker();
this.picker.render();
this._change_results(this.picker, false);
Y.Assert.isTrue(this.picker._extra_buttons.hasClass('unseen'));
@@ -179,7 +162,7 @@
// The assign/remove links are shown again after doing a search and
// selecting a result. Doing a search hides the links so we need to
// ensure they are made visible again.
- this.create_picker(true, true);
+ this.create_picker();
this.picker.render();
this._change_results(this.picker, false);
Y.Assert.isTrue(this.picker._extra_buttons.hasClass('unseen'));
=== modified file 'lib/lp/app/javascript/picker/tests/test_picker.js'
--- lib/lp/app/javascript/picker/tests/test_picker.js 2011-07-08 05:12:39 +0000
+++ lib/lp/app/javascript/picker/tests/test_picker.js 2011-07-15 03:33:10 +0000
@@ -34,7 +34,9 @@
name: 'picker_basics',
setUp: function() {
- this.picker = new Y.lazr.picker.Picker();
+ this.picker = new Y.lazr.picker.Picker({
+ 'selected_value_meta': 'foobar'
+ });
},
tearDown: function() {
@@ -46,6 +48,10 @@
Y.lazr.picker.Picker, this.picker, "Picker failed to be instantiated");
},
+ test_picker_initialisation: function() {
+ Assert.areEqual('foobar', this.picker.get('selected_value_meta'));
+ },
+
test_picker_is_stackable: function() {
// We should probably define an Assert.hasExtension.
Assert.areSame(
@@ -941,7 +947,8 @@
},
test_TextFieldPickerPlugin_selected_item_is_saved: function () {
- this.picker.set('results', [{'title': 'Object 1', value: 'first'}]);
+ this.picker.set('results', [{
+ 'title': 'Object 1', value: 'first', meta: 'newmeta'}]);
this.picker.render();
var got_focus = false;
this.search_input.on('focus', function(e) {
@@ -951,6 +958,7 @@
this.picker.get('boundingBox'), '.yui3-picker-results li', 'click');
Assert.areEqual(
'first', Y.one('[id="field.initval"]').get("value"));
+ Assert.areEqual('newmeta', this.picker.get('selected_value_meta'));
Assert.isTrue(got_focus, "focus didn't go to the search input.");
}
=== modified file 'lib/lp/app/javascript/picker/tests/test_picker_patcher.js'
--- lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2011-07-14 06:02:51 +0000
+++ lib/lp/app/javascript/picker/tests/test_picker_patcher.js 2011-07-15 03:33:10 +0000
@@ -45,9 +45,11 @@
setUp: function() {
this.vocabulary = [
{"value": "fred", "title": "Fred", "css": "sprite-person",
- "description": "fred@xxxxxxxxxxx", "api_uri": "~/fred"},
- {"value": "frieda", "title": "Frieda", "css": "sprite-person",
- "description": "frieda@xxxxxxxxxxx", "api_uri": "~/frieda"}
+ "description": "fred@xxxxxxxxxxx", "api_uri": "~/fred",
+ "meta": "person"},
+ {"value": "frieda", "title": "Frieda", "css": "sprite-team",
+ "description": "frieda@xxxxxxxxxxx", "api_uri": "~/frieda",
+ "meta": "team"}
];
this.picker = null;
this.text_input = null;
@@ -148,9 +150,10 @@
});
simulate(
this.picker.get('boundingBox').one('.yui3-picker-results'),
- 'li:nth-child(1)', 'click');
+ 'li:nth-child(2)', 'click');
Assert.areEqual(
- 'fred', Y.one('[id="field.testfield"]').get("value"));
+ 'frieda', Y.one('[id="field.testfield"]').get("value"));
+ Assert.areEqual('team', this.picker.get('selected_value_meta'));
Assert.isTrue(got_focus, "focus didn't go to the search input.");
},
@@ -243,6 +246,7 @@
"title": "title-" + i,
"css": "sprite-person",
"description": "description-" + i,
+ "meta": "team",
"api_uri": "~/fred-" + i};
}
this.ME = '/~me';
@@ -275,8 +279,7 @@
delete window.LP;
},
- create_picker: function(
- show_assign_me_button, show_remove_button, field_value) {
+ create_picker: function(params, field_value) {
if (field_value !== undefined) {
var data_box = Y.one('#picker_id .yui3-activator-data-box');
data_box.appendChild(Y.Node.create('<a>Content</a>'));
@@ -288,17 +291,28 @@
"header": "Pick Someone",
"validate_callback": null,
"show_search_box": true,
+ "show_assign_me_button": params.show_assign_me_button,
+ "show_remove_button": params.show_remove_button,
+ "selected_value_meta": params.selected_value_meta,
+ "assign_me_text": "Assign Moi",
+ "remove_person_text": "Remove someone",
+ "remove_team_text": "Remove some team"
+ };
+ this.picker = Y.lp.app.picker.addPickerPatcher(
+ this.vocabulary,
+ "foo/bar",
+ "test_link",
+ "picker_id",
+ config);
+ },
+
+ _picker_params: function(
+ show_assign_me_button, show_remove_button, selected_value_meta) {
+ return {
"show_assign_me_button": show_assign_me_button,
"show_remove_button": show_remove_button,
- "assign_button_text": "Assign Moi",
- "remove_button_text": "Remove someone"
- };
- this.picker = Y.lp.app.picker.addPickerPatcher(
- this.vocabulary,
- "foo/bar",
- "test_link",
- "picker_id",
- config);
+ "selected_value_meta": selected_value_meta
+ };
},
_check_button_state: function(btn_class, is_visible) {
@@ -325,23 +339,31 @@
test_picker_assign_me_button_text: function() {
// The assign me button text is correct.
- this.create_picker(true, true);
+ this.create_picker(this._picker_params(true, true));
this.picker.render();
var assign_me_button = Y.one('.yui-picker-assign-me-button');
Assert.areEqual('Assign Moi', assign_me_button.get('innerHTML'));
},
- test_picker_remove_button_text: function() {
+ test_picker_remove_person_button_text: function() {
// The remove button text is correct.
- this.create_picker(true, true);
+ this.create_picker(this._picker_params(true, true, "person"));
this.picker.render();
var remove_button = Y.one('.yui-picker-remove-button');
Assert.areEqual('Remove someone', remove_button.get('innerHTML'));
},
+ test_picker_remove_team_button_text: function() {
+ // The remove button text is correct.
+ this.create_picker(this._picker_params(true, true, "team"), this.ME);
+ this.picker.render();
+ var remove_button = Y.one('.yui-picker-remove-button');
+ Assert.areEqual('Remove some team', remove_button.get('innerHTML'));
+ },
+
test_picker_has_assign_me_button: function() {
// The assign me button is shown.
- this.create_picker(true, true);
+ this.create_picker(this._picker_params(true, true));
this.picker.render();
this._check_assign_me_button_state(true);
},
@@ -349,7 +371,7 @@
test_picker_no_assign_me_button_unless_configured: function() {
// The assign me button is only rendered if show_assign_me_button
// config setting is true.
- this.create_picker(false, true);
+ this.create_picker(this._picker_params(false, true));
this.picker.render();
Assert.isNull(Y.one('.yui-picker-assign-me-button'));
},
@@ -357,7 +379,7 @@
test_picker_no_assign_me_button_if_value_is_me: function() {
// The assign me button is not shown if the picker is created for a
// field where the value is "me".
- this.create_picker(true, true, this.ME);
+ this.create_picker(this._picker_params(true, true), this.ME);
this.picker.render();
this._check_assign_me_button_state(false);
},
@@ -365,7 +387,7 @@
test_picker_assign_me_button_hide_on_save: function() {
// The assign me button is shown initially but hidden if the picker
// saves a value equal to 'me'.
- this.create_picker(true, true);
+ this.create_picker(this._picker_params(true, true));
this._check_assign_me_button_state(true);
this.picker.set('results', this.vocabulary);
this.picker.render();
@@ -378,7 +400,7 @@
test_picker_no_remove_button_if_null_value: function() {
// The remove button is not shown if the picker is created for a field
// which has a null value.
- this.create_picker(true, true);
+ this.create_picker(this._picker_params(true, true));
this.picker.render();
this._check_remove_button_state(false);
},
@@ -386,7 +408,7 @@
test_picker_has_remove_button_if_value: function() {
// The remove button is shown if the picker is created for a field
// which has a value.
- this.create_picker(true, true, this.ME);
+ this.create_picker(this._picker_params(true, true), this.ME);
this.picker.render();
this._check_remove_button_state(true);
},
@@ -394,7 +416,7 @@
test_picker_no_remove_button_unless_configured: function() {
// The remove button is only rendered if show_remove_button setting is
// true.
- this.create_picker(true, false, this.ME);
+ this.create_picker(this._picker_params(true, false), this.ME);
this.picker.render();
Assert.isNull(Y.one('.yui-picker-remove-button'));
},
@@ -402,7 +424,7 @@
test_picker_remove_button_clicked: function() {
// The remove button is hidden once a picker value has been removed.
// And the assign me button is shown.
- this.create_picker(true, true, this.ME);
+ this.create_picker(this._picker_params(true, true), this.ME);
this.picker.render();
var remove = Y.one('.yui-picker-remove-button');
remove.simulate('click');
@@ -413,12 +435,38 @@
test_picker_assign_me_button_clicked: function() {
// The assign me button is hidden once it is clicked.
// And the remove button is shown.
- this.create_picker(true, true);
+ this.create_picker(this._picker_params(true, true));
this.picker.render();
- var remove = Y.one('.yui-picker-assign-me-button');
- remove.simulate('click');
+ var assign_me = Y.one('.yui-picker-assign-me-button');
+ assign_me.simulate('click');
this._check_remove_button_state(true);
this._check_assign_me_button_state(false);
+ },
+
+ test_picker_assign_me_updates_remove_text: function() {
+ // When Assign me is used, the Remove button text is updated from
+ // the team removal text to the person removal text.
+ this.create_picker(this._picker_params(true, true, "team"), this.ME);
+ this.picker.render();
+ var remove_button = Y.one('.yui-picker-remove-button');
+ Assert.areEqual('Remove some team', remove_button.get('innerHTML'));
+ var assign_me = Y.one('.yui-picker-assign-me-button');
+ assign_me.simulate('click');
+ Assert.areEqual('Remove someone', remove_button.get('innerHTML'));
+ },
+
+ test_picker_save_updates_remove_text: function() {
+ // When save is called, the Remove button text is updated according to
+ // the newly saved value.
+ this.create_picker(this._picker_params(true, true), this.ME);
+ var remove_button = Y.one('.yui-picker-remove-button');
+ Assert.areEqual('Remove someone', remove_button.get('innerHTML'));
+ this.picker.set('results', this.vocabulary);
+ this.picker.render();
+ simulate(
+ this.picker.get('boundingBox').one('.yui3-picker-results'),
+ 'li:nth-child(2)', 'click');
+ Assert.areEqual('Remove some team', remove_button.get('innerHTML'));
}
}));
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-06-20 07:03:18 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-07-15 03:33:10 +0000
@@ -74,7 +74,6 @@
from lazr.uri import URI
from pytz import utc
from simplejson import dumps
-from storm.expr import SQL
from z3c.ptcompat import ViewPageTemplateFile
from zope import (
component,
@@ -252,8 +251,6 @@
from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
from lp.bugs.interfaces.cve import ICveSet
from lp.bugs.interfaces.malone import IMaloneApplication
-from lp.bugs.model.bug import Bug
-from lp.bugs.model.bugtask import BugTask
from lp.registry.interfaces.distribution import (
IDistribution,
IDistributionSet,
@@ -3599,6 +3596,8 @@
'bugtask_path': '/'.join(
[''] + canonical_url(self.context).split('/')[3:]),
'prefix': get_prefix(self.context),
+ 'assignee_is_team': self.context.assignee
+ and self.context.assignee.is_team,
'assignee_vocabulary': assignee_vocabulary,
'hide_assignee_team_selection': hide_assignee_team_selection,
'user_can_unassign': self.context.userCanUnassign(user),
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2011-07-06 14:13:04 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2011-07-15 03:33:10 +0000
@@ -894,8 +894,10 @@
assignee_content.get('id'),
{"step_title": step_title,
"header": "Change assignee",
- "assign_button_text": "Assign Me",
- "remove_button_text": "Remove assignee",
+ "selected_value_meta": conf.assignee_is_team?"team":"person",
+ "assign_me_text": "Assign me",
+ "remove_person_text": "Remove assignee",
+ "remove_team_text": "Remove assigned team",
"null_display_value": "Unassigned",
"show_remove_button": conf.user_can_unassign,
"show_assign_me_button": true,
