Merge lp:~wallyworld/launchpad/picker-unassign-link-803996 into lp:launchpad

Proposed by Ian Booth
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 13407
Proposed branch: lp:~wallyworld/launchpad/picker-unassign-link-803996
Merge into: lp:launchpad
Diff against target: 198 lines (+15/-149)
2 files modified
lib/lp/app/javascript/picker/picker.js (+1/-1)
lib/lp/app/javascript/picker/tests/test_personpicker.js (+14/-148)
To merge this branch: bzr merge lp:~wallyworld/launchpad/picker-unassign-link-803996
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+67710@code.launchpad.net

Commit message

[r=sinzui][bug=803996] Ensure picker assign/remove links are shown correctly after a search when the picker is reopened.

Description of the change

After using the person picker to perform a search and assign someone to a bug, the "Remove assignee" and "Assign Me" links would disappear. These links are supposed to disappear when a search is done and the results displayed. But they should reappear when the picker is closed after a result is selected.

== Implementation ==

Turned out to be a trivial fix. The picker _clear() function was doing this:
this.set(RESULTS, [{}]);

The results were "cleared" but the array still had a length of 1 . Hence the logic to show/hide the links thought the picker still was displaying results. The logic was changed to
this.set(RESULTS, []);

Also, there were a bunch of duplicate tests in test_personpicker, left over from a previous rollback. These were deleted since they live in test_pickerpatcher.js

== Tests ==

All picker related yui tests pass. A new test was added also for the specific bug: test_buttons_reappear
The picker was tested also by running lp locally.

== Lint ==

jslint: 2 files to lint.
jslint: No problem found in '/home/ian/projects/lp-branches/devel-sandbox/lib/lp/app/javascript/picker/picker.js'.

jslint: No problem found in '/home/ian/projects/lp-branches/devel-sandbox/lib/lp/app/javascript/picker/tests/test_personpicker.js'.

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank your for this fix.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/app/javascript/picker/picker.js'
--- lib/lp/app/javascript/picker/picker.js 2011-07-06 14:18:59 +0000
+++ lib/lp/app/javascript/picker/picker.js 2011-07-12 14:37:48 +0000
@@ -684,7 +684,7 @@
684 _clear: function() {684 _clear: function() {
685 this.set(CURRENT_SEARCH_STRING, '');685 this.set(CURRENT_SEARCH_STRING, '');
686 this.set(ERROR, '');686 this.set(ERROR, '');
687 this.set(RESULTS, [{}]);687 this.set(RESULTS, []);
688 this.set(BATCHES, null);688 this.set(BATCHES, null);
689 this.set(BATCH_COUNT, null);689 this.set(BATCH_COUNT, null);
690 this._search_input.set('value', '');690 this._search_input.set('value', '');
691691
=== modified file 'lib/lp/app/javascript/picker/tests/test_personpicker.js'
--- lib/lp/app/javascript/picker/tests/test_personpicker.js 2011-07-08 06:29:36 +0000
+++ lib/lp/app/javascript/picker/tests/test_personpicker.js 2011-07-12 14:37:48 +0000
@@ -2,11 +2,7 @@
2 * GNU Affero General Public License version 3 (see the file LICENSE).2 * GNU Affero General Public License version 3 (see the file LICENSE).
3 */3 */
44
5YUI({5YUI().use('test', 'console', 'plugin',
6 base: '../../../../../canonical/launchpad/icing/yui/',
7 filter: 'raw', combine: false,
8 fetchCSS: false
9 }).use('test', 'console', 'plugin',
10 'lazr.picker', 'lazr.person-picker', 'lp.app.picker',6 'lazr.picker', 'lazr.person-picker', 'lp.app.picker',
11 'node-event-simulate', function(Y) {7 'node-event-simulate', function(Y) {
128
@@ -123,149 +119,6 @@
123 config);119 config);
124 },120 },
125121
126 _check_button_state: function(btn_class, is_visible) {
127 var assign_me_button = Y.one(btn_class);
128 Assert.isNotNull(assign_me_button);
129 if (is_visible) {
130 Assert.isFalse(
131 assign_me_button.hasClass('yui3-picker-hidden'),
132 btn_class + " should be visible but is hidden");
133 } else {
134 Assert.isTrue(
135 assign_me_button.hasClass('yui3-picker-hidden'),
136 btn_class + " should be hidden but is visible");
137 }
138 },
139
140 _check_assign_me_button_state: function(is_visible) {
141 this._check_button_state('.yui-picker-assign-me-button', is_visible);
142 },
143
144 _check_remove_button_state: function(is_visible) {
145 this._check_button_state('.yui-picker-remove-button', is_visible);
146 },
147
148 test_render_with_links: function () {
149 // The extra buttons section exists when there are assign/remove links
150 this.create_picker(true, true);
151 this.picker.render();
152 this.picker.show();
153
154 Y.Assert.isNotNull(Y.one('.extra-form-buttons'));
155 Y.Assert.isNotUndefined(this.picker.assign_me_button);
156 Y.Assert.isNotUndefined(this.picker.remove_button);
157 },
158
159 test_render_without_links: function () {
160 // The extra buttons section doesn't exists when there no
161 // assign/remove links.
162 this.create_picker(false, false);
163 this.picker.render();
164 this.picker.show();
165
166 Y.Assert.isNotNull(Y.one('.extra-form-buttons'));
167 Y.Assert.isUndefined(this.picker.remove_button);
168 Y.Assert.isUndefined(this.picker.assign_me_button);
169 },
170
171 test_picker_assign_me_button_text: function() {
172 // The assign me button text is correct.
173 this.create_picker(true, true);
174 this.picker.render();
175 var assign_me_button = Y.one('.yui-picker-assign-me-button');
176 Assert.areEqual('Assign Moi', assign_me_button.get('innerHTML'));
177 },
178
179 test_picker_remove_button_text: function() {
180 // The remove button text is correct.
181 this.create_picker(true, true);
182 this.picker.render();
183 var remove_button = Y.one('.yui-picker-remove-button');
184 Assert.areEqual('Remove someone', remove_button.get('innerHTML'));
185 },
186
187 test_picker_has_assign_me_button: function() {
188 // The assign me button is shown.
189 this.create_picker(true, true);
190 this.picker.render();
191 this._check_assign_me_button_state(true);
192 },
193
194 test_picker_no_assign_me_button_unless_configured: function() {
195 // The assign me button is only rendered if show_assign_me_button
196 // config setting is true.
197 this.create_picker(false, true);
198 this.picker.render();
199 Assert.isNull(Y.one('.yui-picker-assign-me-button'));
200 },
201
202 test_picker_no_assign_me_button_if_value_is_me: function() {
203 // The assign me button is not shown if the picker is created for a
204 // field where the value is "me".
205 this.create_picker(true, true, this.ME);
206 this.picker.render();
207 this._check_assign_me_button_state(false);
208 },
209
210 test_picker_assign_me_button_hide_on_save: function() {
211 // The assign me button is shown initially but hidden if the picker
212 // saves a value equal to 'me'.
213 this.create_picker(true, true);
214 this._check_assign_me_button_state(true);
215 this.picker.set('results', this.vocabulary);
216 this.picker.render();
217 simulate(
218 this.picker.get('boundingBox').one('.yui3-picker-results'),
219 'li:nth-child(1)', 'click');
220 this._check_assign_me_button_state(false);
221 },
222
223 test_picker_no_remove_button_if_null_value: function() {
224 // The remove button is not shown if the picker is created for a field
225 // which has a null value.
226 this.create_picker(true, true);
227 this.picker.render();
228 this._check_remove_button_state(false);
229 },
230
231 test_picker_has_remove_button_if_value: function() {
232 // The remove button is shown if the picker is created for a field
233 // which has a value.
234 this.create_picker(true, true, this.ME);
235 this.picker.render();
236 this._check_remove_button_state(true);
237 },
238
239 test_picker_no_remove_button_unless_configured: function() {
240 // The remove button is only rendered if show_remove_button setting is
241 // true.
242 this.create_picker(true, false, this.ME);
243 this.picker.render();
244 Assert.isNull(Y.one('.yui-picker-remove-button'));
245 },
246
247 test_picker_remove_button_clicked: function() {
248 // The remove button is hidden once a picker value has been removed.
249 // And the assign me button is shown.
250 this.create_picker(true, true, this.ME);
251 this.picker.render();
252 var remove = Y.one('.yui-picker-remove-button');
253 remove.simulate('click');
254 this._check_remove_button_state(false);
255 this._check_assign_me_button_state(true);
256 },
257
258 test_picker_assign_me_button_clicked: function() {
259 // The assign me button is hidden once it is clicked.
260 // And the remove button is shown.
261 this.create_picker(true, true);
262 this.picker.render();
263 var remove = Y.one('.yui-picker-assign-me-button');
264 remove.simulate('click');
265 this._check_remove_button_state(true);
266 this._check_assign_me_button_state(false);
267 },
268
269 test_search_field_focus: function () {122 test_search_field_focus: function () {
270 // The search field has focus when the picker is shown.123 // The search field has focus when the picker is shown.
271 this.create_picker();124 this.create_picker();
@@ -320,6 +173,19 @@
320173
321 this._change_results(this.picker, true);174 this._change_results(this.picker, true);
322 Y.Assert.isFalse(this.picker._extra_buttons.hasClass('unseen'));175 Y.Assert.isFalse(this.picker._extra_buttons.hasClass('unseen'));
176 },
177
178 test_buttons_reappear: function () {
179 // The assign/remove links are shown again after doing a search and
180 // selecting a result. Doing a search hides the links so we need to
181 // ensure they are made visible again.
182 this.create_picker(true, true);
183 this.picker.render();
184 this._change_results(this.picker, false);
185 Y.Assert.isTrue(this.picker._extra_buttons.hasClass('unseen'));
186 simulate(
187 this.picker.get('boundingBox'), '.yui3-picker-results li', 'click');
188 Y.Assert.isFalse(this.picker._extra_buttons.hasClass('unseen'));
323 }189 }
324}));190}));
325191