launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04014
[Merge] lp:~wallyworld/launchpad/personpicker-remove-assignee-link into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/personpicker-remove-assignee-link into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #798775 in Launchpad itself: "remove assignee link shouldn't appear in the person picker if there's no one assigned"
https://bugs.launchpad.net/launchpad/+bug/798775
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/personpicker-remove-assignee-link/+merge/65359
See bug 798775 - the assign and remove me buttons in the person picker were visible when they shouldn't be.
== Implementation ==
The cause was simple - the wrong css class name was used to indicate the buttons were hidden.
Was: yui-picker-hidden
Changed to: yui3-picker-hidden
== Tests ==
There were no javascript tests for the assign and remove buttons. So I added a bunch to test_picker.js
The tests cover the cases when the picker is first opened and when a new value is selected from the picker and also when the assign/remove buttons are used.
== Lint ==
jslint: 2 files to lint.
jslint: Lint found in '/home/ian/projects/lp-branches/devel-sandbox/lib/lp/app/javascript/tests/test_picker.js':
Line 237 character 31: Use the array literal notation [].
this.vocabulary = new Array(121);
Line 413 character 31: Use the array literal notation [].
this.vocabulary = new Array(121);
--
https://code.launchpad.net/~wallyworld/launchpad/personpicker-remove-assignee-link/+merge/65359
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/personpicker-remove-assignee-link into lp:launchpad.
=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js 2011-06-16 13:50:58 +0000
+++ lib/lp/app/javascript/picker.js 2011-06-21 15:07:34 +0000
@@ -25,7 +25,7 @@
* 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.
- * config.show_assign_me_botton: Should the 'assign me' button be shown?
+ * config.show_assign_me_button: Should the 'assign me' button be shown?
* Defaults to false, should be a boolean.
* config.show_search_box: Should the search box be shown.
* Vocabularies that are not huge should not have a search box.
@@ -89,18 +89,18 @@
var link = content_box.one('.yui3-activator-data-box a');
if (remove_button) {
if (link === null || !show_remove_button) {
- remove_button.addClass('yui-picker-hidden');
+ remove_button.addClass('yui3-picker-hidden');
} else {
- remove_button.removeClass('yui-picker-hidden');
+ remove_button.removeClass('yui3-picker-hidden');
}
}
if (assign_me_button) {
if (link !== null
&& link.get('href').indexOf(LP.links.me + '/') !== -1) {
- assign_me_button.addClass('yui-picker-hidden');
+ assign_me_button.addClass('yui3-picker-hidden');
} else {
- assign_me_button.removeClass('yui-picker-hidden');
+ assign_me_button.removeClass('yui3-picker-hidden');
}
}
};
@@ -323,7 +323,7 @@
text_input.value = select_menu.value;
};
Y.on('change', copy_selected_value, select_menu);
- };
+ }
};
/**
=== modified file 'lib/lp/app/javascript/tests/test_picker.js'
--- lib/lp/app/javascript/tests/test_picker.js 2011-06-09 13:32:43 +0000
+++ lib/lp/app/javascript/tests/test_picker.js 2011-06-21 15:07:34 +0000
@@ -30,13 +30,18 @@
}
// Kill the widget itself.
widget.destroy();
+ var data_box = Y.one('#picker_id .yui3-activator-data-box');
+ var link = data_box.one('a');
+ if (link) {
+ link.get('parentNode').removeChild(link);
+ }
}
var suite = new Y.Test.Suite("Launchpad Picker Tests");
/*
- * Test cases for a picker with a yesno validation callback.
- */
+* Test cases for a picker with a yesno validation callback.
+*/
suite.add(new Y.Test.Case({
name: 'picker_yesyno_validation',
@@ -220,6 +225,182 @@
}));
/*
+ * Test cases for assign and remove buttons.
+ */
+suite.add(new Y.Test.Case({
+
+ name: 'picker_assign_remove_button',
+
+ setUp: function() {
+ var i;
+ this.vocabulary = new Array(121);
+ for (i = 0; i <5; i++) {
+ this.vocabulary[i] = {
+ "value": "value-" + i,
+ "title": "title-" + i,
+ "css": "sprite-person",
+ "description": "description-" + i,
+ "api_uri": "~/fred-" + i};
+ }
+ window.LP = {
+ links: {
+ me: '/~me'
+ },
+ cache: {}
+ };
+ this.ME_HREF = '/~me/';
+
+ // We patch Launchpad client to return some fake data for the patch
+ // operation.
+ Y.lp.client.Launchpad = function() {};
+ Y.lp.client.Launchpad.prototype.patch =
+ function(uri, representation, config, headers) {
+ // our setup assumes success, so we just do the success
+ // callback.
+ var entry_repr = {
+ 'lp_html': {'test_link': '<a href="/~me/">Content</a>'}
+ };
+ var result = new Y.lp.client.Entry(
+ null, entry_repr, "a_self_link");
+ config.on.success(result);
+ };
+
+ },
+
+ tearDown: function() {
+ cleanup_widget(this.picker);
+ delete window.LP;
+ },
+
+ create_picker: function(show_assign_me, show_remove, 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);
+ }
+
+ var config = {
+ "step_title": "Choose someone",
+ "header": "Pick Someone",
+ "validate_callback": null,
+ "show_search_box": true,
+ "show_assign_me_button": show_assign_me,
+ "show_remove_button": show_remove
+ };
+ this.picker = Y.lp.app.picker.addPickerPatcher(
+ this.vocabulary,
+ "foo/bar",
+ "test_link",
+ "picker_id",
+ config);
+ },
+
+ _check_button_state: function(btn_class, is_visible) {
+ var assign_me_button = Y.one(btn_class);
+ Assert.isNotNull(assign_me_button);
+ if (is_visible) {
+ Assert.isFalse(
+ assign_me_button.hasClass('yui3-picker-hidden'),
+ btn_class + " should be visible but is hidden");
+ } else {
+ Assert.isTrue(
+ assign_me_button.hasClass('yui3-picker-hidden'),
+ btn_class + " should be hidden but is visible");
+ }
+ },
+
+ _check_assign_me_button_state: function(is_visible) {
+ this._check_button_state('.yui-picker-assign-me-button', is_visible);
+ },
+
+ _check_remove_button_state: function(is_visible) {
+ this._check_button_state('.yui-picker-remove-button', is_visible);
+ },
+
+ test_picker_has_assign_me_button: function() {
+ // The assign me button is shown.
+ this.create_picker(true, true);
+ this.picker.render();
+ this._check_assign_me_button_state(true);
+ },
+
+ 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.picker.render();
+ Assert.isNull(Y.one('.yui-picker-assign-me-button'));
+ },
+
+ 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_HREF);
+ this.picker.render();
+ this._check_assign_me_button_state(false);
+ },
+
+ 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._check_assign_me_button_state(true);
+ this.picker.set('results', this.vocabulary);
+ this.picker.render();
+ simulate(
+ this.picker.get('boundingBox').one('.yui3-picker-results'),
+ 'li:nth-child(1)', 'click');
+ this._check_assign_me_button_state(false);
+ },
+
+ 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.picker.render();
+ this._check_remove_button_state(false);
+ },
+
+ 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_HREF);
+ this.picker.render();
+ this._check_remove_button_state(true);
+ },
+
+ 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_HREF);
+ this.picker.render();
+ Assert.isNull(Y.one('.yui-picker-remove-button'));
+ },
+
+ 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_HREF);
+ this.picker.render();
+ var remove = Y.one('.yui-picker-remove-button');
+ remove.simulate('click');
+ this._check_remove_button_state(false);
+ this._check_assign_me_button_state(true);
+ },
+
+ 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.picker.render();
+ var remove = Y.one('.yui-picker-assign-me-button');
+ remove.simulate('click');
+ this._check_remove_button_state(true);
+ this._check_assign_me_button_state(false);
+ }
+}));
+
+/*
* Test cases for a picker with a large vocabulary.
*/
suite.add(new Y.Test.Case({
@@ -257,8 +438,7 @@
"foo/bar",
"test_link",
"picker_id",
- config
- );
+ config);
},
test_picker_displays_empty_list: function() {
