Merge lp:~henninge/launchpad/bug-410864-undefined-in-picker into lp:launchpad

Proposed by Henning Eggers on 2011-06-08
Status: Merged
Approved by: Curtis Hovey on 2011-06-08
Approved revision: no longer in the source branch.
Merged at revision: 13196
Proposed branch: lp:~henninge/launchpad/bug-410864-undefined-in-picker
Merge into: lp:launchpad
Diff against target: 472 lines (+155/-94)
9 files modified
lib/canonical/launchpad/webapp/configure.zcml (+2/-2)
lib/lp/app/javascript/picker.js (+21/-44)
lib/lp/app/javascript/tests/test_personpicker.html (+1/-1)
lib/lp/app/javascript/tests/test_personpicker.js (+4/-4)
lib/lp/app/javascript/tests/test_picker.html (+1/-0)
lib/lp/app/javascript/tests/test_picker.js (+36/-9)
lib/lp/app/javascript/widgets.js (+75/-32)
lib/lp/app/widgets/popup.py (+13/-1)
lib/lp/app/widgets/templates/form-picker-macros.pt (+2/-1)
To merge this branch: bzr merge lp:~henninge/launchpad/bug-410864-undefined-in-picker
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code 2011-06-08 Approve on 2011-06-08
Review via email: mp+63911@code.launchpad.net

Commit message

[r=sinzui][bug=410864,787595] Work on the picker widget: Retargets the personpicker yui widget to extend the new lp-picker yui widget. Fixes a regression that let the result batches run out of proportion. Fixes the 'too many results' display.

Description of the change

= Summary =

Apart from fixing the linked bug this branch also piggybacks a fix for a
regression introduced by the fix for bug 778129 (my bad).

This regression caused the picker widget to display all the batches of
results even if a search box was available. Instead it should display
the "Too many search results" message and not display any results.

The reason for bug 410864 lies in lazr-js assuming that an empty result
list indicates that a search returned no results. But enforcing the
batch limit also creates empty result list in which case the
message "No items matched ..." produced by lazr-js is not fitting.
To prevent that message, a one-item result list with an empty item
was created which caused "undefined" to appear as the title of that
one item.

== Proposed fix ==

For the regression: The code used "config.show_search_box" in the
create() method but config might not always have such a member. So
create() needs to mimic the behavior of the createPickerPatcher()
method and default show_search_box to true when it is undefined.

For the actual bug: The render method needs to be aware that it
could get an empty data as a result and produce an empty result
element accordingly. A better fix would be to make lazr-js's
behavior regarding empty result lists configurable but I went for the
least-impact solution. Another squad is doing work on the picker and
they may well touch this.

== Pre-implementation notes ==

Chatted a big with deryck, did not catch Curtis on IRC.

== Tests ==

lib/lp/app/javascript/tests/test_picker.html

== Demo and Q/A ==

Go to a bug page, open the picker for the bug target and enter
"linux". You should get a "Too many results" message but no
"undefined" in the empty list below.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/picker.js
  lib/lp/app/javascript/tests/test_picker.js

./lib/lp/app/javascript/picker.js
     337: 'Picker' has not been fully defined yet.

To post a comment you must log in.
Curtis Hovey (sinzui) wrote :

Thank you for this change. I think the branch conflicts with https://code.launchpad.net/~jcsackett/launchpad/oh-oh-pick-me-pick-me-3 which moved modules and chunks of code. I recognise the area you made changes. You may need to make these same changes to lp.app.javascript.widgets

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
2--- lib/canonical/launchpad/webapp/configure.zcml 2011-05-27 21:03:22 +0000
3+++ lib/canonical/launchpad/webapp/configure.zcml 2011-06-09 08:46:18 +0000
4@@ -396,10 +396,10 @@
5 permission="zope.Public"
6 />
7
8- <!-- Define the widget used by PublicPersonChoice fields. -->
9+ <!-- Define the widget used by PersonChoice fields. -->
10 <view
11 type="zope.publisher.interfaces.browser.IBrowserRequest"
12- for="lp.services.fields.PublicPersonChoice
13+ for="lp.services.fields.PersonChoice
14 canonical.launchpad.webapp.vocabulary.IHugeVocabulary"
15 provides="zope.app.form.interfaces.IInputWidget"
16 factory="lp.app.widgets.popup.PersonPickerWidget"
17
18=== modified file 'lib/lp/app/javascript/picker.js'
19--- lib/lp/app/javascript/picker.js 2011-06-06 03:24:06 +0000
20+++ lib/lp/app/javascript/picker.js 2011-06-09 08:46:18 +0000
21@@ -328,47 +328,6 @@
22 Y.on('change', copy_selected_value, select_menu);
23 };
24
25-
26-/**
27- * Extend the lazr-js Picker.
28- */
29-var Picker = function() {
30- Picker.superclass.constructor.apply(this, arguments);
31-};
32-
33-Y.extend(Picker, Y.lazr.Picker, {
34- // We want to render alt title slightly differently.
35- _renderTitleUI: function(data) {
36- var li_title = Y.Node.create(
37- '<span></span>').addClass(Y.lazr.Picker.C_RESULT_TITLE);
38- var title = this._text_or_link(
39- data.title, data.title_link, data.link_css);
40- li_title.appendChild(title);
41- if (data.alt_title) {
42- var alt_link = null;
43- if (data.alt_title_link) {
44- alt_link =Y.Node.create('<a></a>')
45- .addClass(data.link_css)
46- .addClass('discreet');
47- alt_link.set('text', " Details...")
48- .set('href', data.alt_title_link);
49- Y.on('click', function(e) {
50- e.halt();
51- window.open(data.alt_title_link);
52- }, alt_link);
53- }
54- li_title.appendChild('&nbsp;(');
55- li_title.appendChild(document.createTextNode(data.alt_title));
56- if (alt_link !== null)
57- li_title.appendChild(alt_link);
58- li_title.appendChild(')');
59- }
60- return li_title;
61- }
62-});
63-Picker.NAME = 'picker';
64-namespace.Picker = Picker;
65-
66 /**
67 * Creates a picker widget that has already been rendered and hidden.
68 *
69@@ -381,6 +340,8 @@
70 * config.step_title overrides the subtitle.
71 * config.save is a Function (optional) which takes
72 * a single string argument.
73+ * config.show_search_box: Should the search box be
74+ * shown.
75 */
76 namespace.create = function (vocabulary, config, activator) {
77 if (Y.UA.ie) {
78@@ -389,6 +350,8 @@
79
80 var header = 'Choose an item.';
81 var step_title = "Enter search terms";
82+ var show_search_box = true;
83+ var picker_type = "default";
84 if (config !== undefined) {
85 if (config.header !== undefined) {
86 header = config.header;
87@@ -397,6 +360,14 @@
88 if (config.step_title !== undefined) {
89 step_title = config.step_title;
90 }
91+
92+ if (config.show_search_box !== undefined) {
93+ show_search_box = config.show_search_box;
94+ }
95+
96+ if (config.picker_type !== undefined) {
97+ picker_type = config.picker_type;
98+ }
99 }
100
101 if (typeof vocabulary !== 'string' && typeof vocabulary !== 'object') {
102@@ -417,7 +388,13 @@
103 zIndex: 1000,
104 visible: false
105 });
106- var picker = new Picker(new_config);
107+
108+ var picker = null;
109+ if (picker_type === 'person') {
110+ picker = new Y.lp.app.widgets.PersonPicker(new_config);
111+ } else {
112+ picker = new Y.lp.app.widgets.Picker(new_config);
113+ }
114
115 // We don't want the Y.lazr.Picker default save to fire since this hides
116 // the form. We want to do this ourselves after any validation has had a
117@@ -457,7 +434,7 @@
118 var batch = 0;
119 var display_vocabulary = function(results, total_size, start) {
120 var max_size = MAX_BATCHES * BATCH_SIZE;
121- if (config.show_search_box && total_size > max_size) {
122+ if (show_search_box && total_size > max_size) {
123 picker.set('error',
124 'Too many matches. Please try to narrow your search.');
125 // Display a single empty result item so that the picker
126@@ -540,5 +517,5 @@
127
128 }, "0.1", {"requires": [
129 "io", "dom", "dump", "event", "lazr.picker", "lazr.activator",
130- "json-parse", "lp.client"
131+ "json-parse", "lp.client", "lp.app.widgets"
132 ]});
133
134=== renamed file 'lib/lp/registry/javascript/tests/test_personpicker.html' => 'lib/lp/app/javascript/tests/test_personpicker.html'
135--- lib/lp/registry/javascript/tests/test_personpicker.html 2011-06-01 13:59:43 +0000
136+++ lib/lp/app/javascript/tests/test_personpicker.html 2011-06-09 08:46:18 +0000
137@@ -17,7 +17,7 @@
138 <script type="text/javascript" src="../../../app/javascript/lp.js"></script>
139
140 <!-- The module under test -->
141- <script type="text/javascript" src="../personpicker.js"></script>
142+ <script type="text/javascript" src="../widgets.js"></script>
143
144 <!-- The test suite -->
145 <script type="text/javascript" src="test_personpicker.js"></script>
146
147=== renamed file 'lib/lp/registry/javascript/tests/test_personpicker.js' => 'lib/lp/app/javascript/tests/test_personpicker.js'
148--- lib/lp/registry/javascript/tests/test_personpicker.js 2011-06-06 17:43:55 +0000
149+++ lib/lp/app/javascript/tests/test_personpicker.js 2011-06-09 08:46:18 +0000
150@@ -6,10 +6,10 @@
151 base: '../../../../canonical/launchpad/icing/yui/',
152 filter: 'raw', combine: false,
153 fetchCSS: false
154- }).use('test', 'console', 'plugin', 'lp.registry.personpicker',
155+ }).use('test', 'console', 'plugin', 'lp.app.widgets',
156 'node-event-simulate', function(Y) {
157
158- var suite = new Y.Test.Suite("lp.registry.personpicker Tests");
159+ var suite = new Y.Test.Suite("lp.app.widgets.PersonPicker Tests");
160
161 suite.add(new Y.Test.Case({
162 name: 'personpicker',
163@@ -22,7 +22,7 @@
164 },
165
166 test_render: function () {
167- var personpicker = new Y.lp.registry.personpicker.PersonPicker();
168+ var personpicker = new Y.lp.app.widgets.PersonPicker();
169 personpicker.render();
170 personpicker.show();
171
172@@ -33,7 +33,7 @@
173 },
174
175 test_buttons: function () {
176- var personpicker = new Y.lp.registry.personpicker.PersonPicker();
177+ var personpicker = new Y.lp.app.widgets.PersonPicker();
178 personpicker.render();
179 personpicker.show();
180
181
182=== modified file 'lib/lp/app/javascript/tests/test_picker.html'
183--- lib/lp/app/javascript/tests/test_picker.html 2011-04-19 11:27:26 +0000
184+++ lib/lp/app/javascript/tests/test_picker.html 2011-06-09 08:46:18 +0000
185@@ -19,6 +19,7 @@
186 <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/anim/anim.js"></script>
187 <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/lazr/lazr.js"></script>
188 <script type="text/javascript" src="../client.js"></script>
189+ <script type="text/javascript" src="../widgets.js"></script>
190
191 <!-- The module under test -->
192 <script type="text/javascript" src="../picker.js"></script>
193
194=== modified file 'lib/lp/app/javascript/tests/test_picker.js'
195--- lib/lp/app/javascript/tests/test_picker.js 2011-06-03 22:46:35 +0000
196+++ lib/lp/app/javascript/tests/test_picker.js 2011-06-09 08:46:18 +0000
197@@ -6,8 +6,8 @@
198 combine: false,
199 fetchCSS: false
200 }).use('test', 'console', 'node', 'lp', 'lp.client', 'escape', 'event',
201- 'event-focus', 'event-simulate', 'lazr.picker','lp.app.picker',
202- 'node-event-simulate',
203+ 'event-focus', 'event-simulate', 'lazr.picker', 'lp.app.widgets',
204+ 'lp.app.picker', 'node-event-simulate',
205 function(Y) {
206
207 var Assert = Y.Assert;
208@@ -86,7 +86,8 @@
209 // The picker can be instantiated.
210 this.create_picker();
211 Assert.isInstanceOf(
212- Y.lazr.Picker, this.picker, "Picker failed to be instantiated");
213+ Y.lp.app.widgets.Picker, this.picker,
214+ "Picker failed to be instantiated");
215 },
216
217 // Called when the picker saves it's data. Sets a flag for checking.
218@@ -243,17 +244,32 @@
219 },
220
221 create_picker: function(show_search_box) {
222+ var config = {
223+ "step_title": "Choose someone",
224+ "header": "Pick Someone",
225+ "validate_callback": null
226+ };
227+ if (show_search_box !== undefined) {
228+ config.show_search_box = show_search_box;
229+ }
230 this.picker = Y.lp.app.picker.addPickerPatcher(
231 this.vocabulary,
232 "foo/bar",
233 "test_link",
234 "picker_id",
235- {
236- "step_title": "Choose someone",
237- "header": "Pick Someone",
238- "show_search_box": show_search_box,
239- "validate_callback": null
240- });
241+ config
242+ );
243+ },
244+
245+ test_picker_displays_empty_list: function() {
246+ // With too many results, the results will be empty.
247+ this.create_picker(true);
248+ this.picker.render();
249+ this.picker.set('min_search_chars', 0);
250+ this.picker.fire('search', '');
251+ var result_text = this.picker.get('contentBox')
252+ .one('.yui3-picker-results').get('text');
253+ Assert.areEqual('', result_text);
254 },
255
256 test_picker_displays_warning: function() {
257@@ -267,6 +283,17 @@
258 this.picker.get('error'));
259 },
260
261+ test_picker_displays_warning_by_default: function() {
262+ // If show_search_box is not supplied in config, it defaults to true.
263+ // Thus the picker will refuse to display more than 120 values.
264+ this.create_picker();
265+ this.picker.set('min_search_chars', 0);
266+ this.picker.fire('search', '');
267+ Assert.areEqual(
268+ 'Too many matches. Please try to narrow your search.',
269+ this.picker.get('error'));
270+ },
271+
272 test_picker_no_warning: function() {
273 // Without a search box the picker will also display more than
274 // 120 values.
275
276=== renamed file 'lib/lp/registry/javascript/personpicker.js' => 'lib/lp/app/javascript/widgets.js'
277--- lib/lp/registry/javascript/personpicker.js 2011-05-31 18:27:50 +0000
278+++ lib/lp/app/javascript/widgets.js 2011-06-09 08:46:18 +0000
279@@ -1,21 +1,69 @@
280 /* Copyright 2011 Canonical Ltd. This software is licensed under the
281 * GNU Affero General Public License version 3 (see the file LICENSE).
282 *
283- * @namespace Y.lp.registry.personpicker
284- * @requires lazr.picker
285- */
286-YUI.add('lp.registry.personpicker', function(Y) {
287-var namespace = Y.namespace('lp.registry.personpicker');
288-
289-var footer_label = ".yui3-picker-footer-slot"
290+ * @namespace Y.lp.app.widgets
291+ * @requires Y.lazr.picker
292+ */
293+YUI.add('lp.app.widgets', function(Y) {
294+var namespace = Y.namespace('lp.app.widgets');
295+
296+/**
297+ * Extend the lazr-js Picker.
298+ */
299+var Picker = function() {
300+ Picker.superclass.constructor.apply(this, arguments);
301+};
302+
303+Y.extend(Picker, Y.lazr.Picker, {
304+ // We want to render alt title slightly differently.
305+ _renderTitleUI: function(data) {
306+ var li_title = Y.Node.create(
307+ '<span></span>').addClass(Y.lazr.Picker.C_RESULT_TITLE);
308+ if (data.title === undefined) {
309+ // Display an empty element if data is empty.
310+ return li_title;
311+ }
312+ var title = this._text_or_link(
313+ data.title, data.title_link, data.link_css);
314+ li_title.appendChild(title);
315+ if (data.alt_title) {
316+ var alt_link = null;
317+ if (data.alt_title_link) {
318+ alt_link =Y.Node.create('<a></a>')
319+ .addClass(data.link_css)
320+ .addClass('discreet');
321+ alt_link.set('text', " Details...")
322+ .set('href', data.alt_title_link);
323+ Y.on('click', function(e) {
324+ e.halt();
325+ window.open(data.alt_title_link);
326+ }, alt_link);
327+ }
328+ li_title.appendChild('&nbsp;(');
329+ li_title.appendChild(document.createTextNode(data.alt_title));
330+ li_title.appendChild(')');
331+ if (alt_link !== null) {
332+ li_title.appendChild(alt_link);
333+ }
334+ }
335+ return li_title;
336+ }
337+});
338+
339+Picker.NAME = 'picker';
340+namespace.Picker = Picker;
341+
342+/*
343+ * Extend the picker into the PersonPicker
344+ */
345+var footer_label = ".yui3-picker-footer-slot";
346
347 var PersonPicker = function() {
348 PersonPicker.superclass.constructor.apply(this, arguments);
349-
350 this._extra_buttons = Y.Node.create('<div class="extra-form-buttons"/>');
351 };
352
353-Y.extend(PersonPicker, Y.lazr.Picker, {
354+Y.extend(PersonPicker, namespace.Picker, {
355 hide: function() {
356 this.get('boundingBox').setStyle('display', 'none');
357 },
358@@ -36,29 +84,24 @@
359 renderUI: function() {
360 this.constructor.superclass.renderUI.call(this);
361 var remove_button, assign_me_button;
362- //# TODO config should set extrabuttons
363- var show_remove_button = true;
364- var show_assign_me_button = true;
365 var remove_button_text = "Remove assignee";
366 var assign_me_button_text = "Assign me";
367- if (show_remove_button) {
368- remove_button = Y.Node.create(
369- '<a class="yui-picker-remove-button bg-image" ' +
370- 'href="javascript:void(0)" ' +
371- 'style="background-image: url(/@@/remove); padding-right: 1em">' +
372- remove_button_text + '</a>');
373- remove_button.on('click', this.remove, this);
374- this._extra_buttons.appendChild(remove_button);
375- }
376- if (show_assign_me_button) {
377- assign_me_button = Y.Node.create(
378- '<a class="yui-picker-assign-me-button bg-image" ' +
379- 'href="javascript:void(0)" ' +
380- 'style="background-image: url(/@@/person)">' +
381- assign_me_button_text + '</a>');
382- assign_me_button.on('click', this.assign_me, this);
383- this._extra_buttons.appendChild(assign_me_button);
384- }
385+
386+ remove_button = Y.Node.create(
387+ '<a class="yui-picker-remove-button bg-image" ' +
388+ 'href="javascript:void(0)" ' +
389+ 'style="background-image: url(/@@/remove); padding-right: 1em">' +
390+ remove_button_text + '</a>');
391+ remove_button.on('click', this.remove, this);
392+ this._extra_buttons.appendChild(remove_button);
393+
394+ assign_me_button = Y.Node.create(
395+ '<a class="yui-picker-assign-me-button bg-image" ' +
396+ 'href="javascript:void(0)" ' +
397+ 'style="background-image: url(/@@/person)">' +
398+ assign_me_button_text + '</a>');
399+ assign_me_button.on('click', this.assign_me, this);
400+ this._extra_buttons.appendChild(assign_me_button);
401 },
402
403 syncUI: function() {
404@@ -69,6 +112,6 @@
405 }
406 });
407 PersonPicker.NAME = 'person-picker';
408-namespace.PersonPicker = PersonPicker
409+namespace.PersonPicker = PersonPicker;
410
411-}, "0.1", {"requires": ['lazr.picker']});
412+}, "0.1", {"requires": ["lazr.picker"]});
413
414=== modified file 'lib/lp/app/widgets/popup.py'
415--- lib/lp/app/widgets/popup.py 2011-06-01 13:59:43 +0000
416+++ lib/lp/app/widgets/popup.py 2011-06-09 08:46:18 +0000
417@@ -18,6 +18,7 @@
418
419 from canonical.launchpad.webapp import canonical_url
420 from lp.app.browser.stringformatter import FormattersAPI
421+from lp.services.features import getFeatureFlag
422 from lp.services.propertycache import cachedproperty
423
424
425@@ -26,6 +27,8 @@
426
427 __call__ = ViewPageTemplateFile('templates/form-picker.pt')
428
429+ picker_type = 'default'
430+
431 popup_name = 'popup-vocabulary-picker'
432
433 # Override inherited attributes for the form field.
434@@ -157,6 +160,16 @@
435
436 include_create_team_link = False
437
438+ @property
439+ def picker_type(self):
440+ # This is a method for now so we can block the use of the new
441+ # person picker js behind our picker_enhancments feature flag.
442+ if bool(getFeatureFlag('disclosure.picker_enhancements.enabled')):
443+ picker_type = 'person'
444+ else:
445+ picker_type = 'default'
446+ return picker_type
447+
448 def chooseLink(self):
449 link = super(PersonPickerWidget, self).chooseLink()
450 if self.include_create_team_link:
451@@ -172,7 +185,6 @@
452 class BugTrackerPickerWidget(VocabularyPickerWidget):
453
454 __call__ = ViewPageTemplateFile('templates/bugtracker-picker.pt')
455-
456 link_template = """
457 or (<a id="create-bugtracker-link"
458 href="/bugs/bugtrackers/+newbugtracker"
459
460=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
461--- lib/lp/app/widgets/templates/form-picker-macros.pt 2011-06-01 20:55:24 +0000
462+++ lib/lp/app/widgets/templates/form-picker-macros.pt 2011-06-09 08:46:18 +0000
463@@ -40,7 +40,8 @@
464 var config = {
465 header: ${view/header_text},
466 step_title: ${view/step_title_text},
467- extra_no_results_message: ${view/extra_no_results_message}
468+ extra_no_results_message: ${view/extra_no_results_message},
469+ picker_type: '${view/picker_type}'
470 };
471 var picker = Y.lp.app.picker.create('${view/vocabulary_name}', config);
472 if (config.extra_no_results_message !== null) {