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

[Henning Eggers]

[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.

= 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.

[Curtis Hovey (sinzui)]

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)

=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
--- lib/canonical/launchpad/webapp/configure.zcml	2011-05-27 21:03:22 +0000
+++ lib/canonical/launchpad/webapp/configure.zcml	2011-06-09 08:46:18 +0000
@@ -396,10 +396,10 @@
       permission="zope.Public"
       />
 
-    <!-- Define the widget used by PublicPersonChoice fields. -->
+    <!-- Define the widget used by PersonChoice fields. -->
     <view
       type="zope.publisher.interfaces.browser.IBrowserRequest"
-      for="lp.services.fields.PublicPersonChoice
+      for="lp.services.fields.PersonChoice
         canonical.launchpad.webapp.vocabulary.IHugeVocabulary"
       provides="zope.app.form.interfaces.IInputWidget"
       factory="lp.app.widgets.popup.PersonPickerWidget"

=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js	2011-06-06 03:24:06 +0000
+++ lib/lp/app/javascript/picker.js	2011-06-09 08:46:18 +0000
@@ -328,47 +328,6 @@
     Y.on('change', copy_selected_value, select_menu);
     };
 
-
-/**
- * Extend the lazr-js Picker.
- */
-var Picker = function() {
-    Picker.superclass.constructor.apply(this, arguments);
-};
-
-Y.extend(Picker, Y.lazr.Picker, {
-    // We want to render alt title slightly differently.
-    _renderTitleUI: function(data) {
-        var li_title = Y.Node.create(
-            '<span></span>').addClass(Y.lazr.Picker.C_RESULT_TITLE);
-        var title = this._text_or_link(
-            data.title, data.title_link, data.link_css);
-        li_title.appendChild(title);
-        if (data.alt_title) {
-            var alt_link = null;
-            if (data.alt_title_link) {
-                alt_link =Y.Node.create('<a></a>')
-                    .addClass(data.link_css)
-                    .addClass('discreet');
-                alt_link.set('text', " Details...")
-                    .set('href', data.alt_title_link);
-                Y.on('click', function(e) {
-                    e.halt();
-                    window.open(data.alt_title_link);
-                }, alt_link);
-            }
-            li_title.appendChild('&nbsp;(');
-            li_title.appendChild(document.createTextNode(data.alt_title));
-            if (alt_link !== null)
-                li_title.appendChild(alt_link);
-            li_title.appendChild(')');
-        }
-        return li_title;
-    }
-});
-Picker.NAME = 'picker';
-namespace.Picker = Picker;
-
 /**
   * Creates a picker widget that has already been rendered and hidden.
   *
@@ -381,6 +340,8 @@
   *                        config.step_title overrides the subtitle.
   *                        config.save is a Function (optional) which takes
   *                        a single string argument.
+  *                        config.show_search_box: Should the search box be
+  *                        shown.
   */
 namespace.create = function (vocabulary, config, activator) {
     if (Y.UA.ie) {
@@ -389,6 +350,8 @@
 
     var header = 'Choose an item.';
     var step_title = "Enter search terms";
+    var show_search_box = true;
+    var picker_type = "default";
     if (config !== undefined) {
         if (config.header !== undefined) {
             header = config.header;
@@ -397,6 +360,14 @@
         if (config.step_title !== undefined) {
             step_title = config.step_title;
         }
+
+        if (config.show_search_box !== undefined) {
+            show_search_box = config.show_search_box;
+        }
+
+        if (config.picker_type !== undefined) {
+            picker_type = config.picker_type;
+        }
     }
 
     if (typeof vocabulary !== 'string' && typeof vocabulary !== 'object') {
@@ -417,7 +388,13 @@
         zIndex: 1000,
         visible: false
         });
-    var picker = new Picker(new_config);
+
+    var picker = null;
+    if (picker_type === 'person') {
+        picker = new Y.lp.app.widgets.PersonPicker(new_config);
+    } else {
+        picker = new Y.lp.app.widgets.Picker(new_config);
+    }
 
     // We don't want the Y.lazr.Picker default save to fire since this hides
     // the form. We want to do this ourselves after any validation has had a
@@ -457,7 +434,7 @@
         var batch = 0;
         var display_vocabulary = function(results, total_size, start) {
             var max_size = MAX_BATCHES * BATCH_SIZE;
-            if (config.show_search_box && total_size > max_size)  {
+            if (show_search_box && total_size > max_size)  {
                 picker.set('error',
                     'Too many matches. Please try to narrow your search.');
                 // Display a single empty result item so that the picker
@@ -540,5 +517,5 @@
 
 }, "0.1", {"requires": [
     "io", "dom", "dump", "event", "lazr.picker", "lazr.activator",
-    "json-parse", "lp.client"
+    "json-parse", "lp.client", "lp.app.widgets"
     ]});

=== renamed file 'lib/lp/registry/javascript/tests/test_personpicker.html' => 'lib/lp/app/javascript/tests/test_personpicker.html'
--- lib/lp/registry/javascript/tests/test_personpicker.html	2011-06-01 13:59:43 +0000
+++ lib/lp/app/javascript/tests/test_personpicker.html	2011-06-09 08:46:18 +0000
@@ -17,7 +17,7 @@
     <script type="text/javascript" src="../../../app/javascript/lp.js"></script>
 
     <!-- The module under test -->
-    <script type="text/javascript" src="../personpicker.js"></script>
+    <script type="text/javascript" src="../widgets.js"></script>
 
     <!-- The test suite -->
     <script type="text/javascript" src="test_personpicker.js"></script>

=== renamed file 'lib/lp/registry/javascript/tests/test_personpicker.js' => 'lib/lp/app/javascript/tests/test_personpicker.js'
--- lib/lp/registry/javascript/tests/test_personpicker.js	2011-06-06 17:43:55 +0000
+++ lib/lp/app/javascript/tests/test_personpicker.js	2011-06-09 08:46:18 +0000
@@ -6,10 +6,10 @@
     base: '../../../../canonical/launchpad/icing/yui/',
     filter: 'raw', combine: false,
     fetchCSS: false
-    }).use('test', 'console', 'plugin', 'lp.registry.personpicker',
+    }).use('test', 'console', 'plugin', 'lp.app.widgets',
            'node-event-simulate', function(Y) {
 
-    var suite = new Y.Test.Suite("lp.registry.personpicker Tests");
+    var suite = new Y.Test.Suite("lp.app.widgets.PersonPicker Tests");
 
     suite.add(new Y.Test.Case({
         name: 'personpicker',
@@ -22,7 +22,7 @@
         },
 
         test_render: function () {
-            var personpicker = new Y.lp.registry.personpicker.PersonPicker();
+            var personpicker = new Y.lp.app.widgets.PersonPicker();
             personpicker.render();
             personpicker.show();
 
@@ -33,7 +33,7 @@
         },
 
         test_buttons: function () {
-            var personpicker = new Y.lp.registry.personpicker.PersonPicker();
+            var personpicker = new Y.lp.app.widgets.PersonPicker();
             personpicker.render();
             personpicker.show();
 

=== modified file 'lib/lp/app/javascript/tests/test_picker.html'
--- lib/lp/app/javascript/tests/test_picker.html	2011-04-19 11:27:26 +0000
+++ lib/lp/app/javascript/tests/test_picker.html	2011-06-09 08:46:18 +0000
@@ -19,6 +19,7 @@
   <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/anim/anim.js"></script>
   <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/lazr/lazr.js"></script>
   <script type="text/javascript" src="../client.js"></script>
+  <script type="text/javascript" src="../widgets.js"></script>
 
   <!-- The module under test -->
   <script type="text/javascript" src="../picker.js"></script>

=== modified file 'lib/lp/app/javascript/tests/test_picker.js'
--- lib/lp/app/javascript/tests/test_picker.js	2011-06-03 22:46:35 +0000
+++ lib/lp/app/javascript/tests/test_picker.js	2011-06-09 08:46:18 +0000
@@ -6,8 +6,8 @@
     combine: false,
     fetchCSS: false
     }).use('test', 'console', 'node', 'lp', 'lp.client', 'escape', 'event',
-        'event-focus', 'event-simulate', 'lazr.picker','lp.app.picker',
-        'node-event-simulate',
+        'event-focus', 'event-simulate', 'lazr.picker', 'lp.app.widgets',
+        'lp.app.picker', 'node-event-simulate',
         function(Y) {
 
 var Assert = Y.Assert;
@@ -86,7 +86,8 @@
         // The picker can be instantiated.
         this.create_picker();
         Assert.isInstanceOf(
-            Y.lazr.Picker, this.picker, "Picker failed to be instantiated");
+            Y.lp.app.widgets.Picker, this.picker,
+            "Picker failed to be instantiated");
     },
 
     // Called when the picker saves it's data. Sets a flag for checking.
@@ -243,17 +244,32 @@
     },
 
     create_picker: function(show_search_box) {
+        var config = {
+            "step_title": "Choose someone",
+            "header": "Pick Someone",
+            "validate_callback": null
+            };
+        if (show_search_box !== undefined) {
+            config.show_search_box = show_search_box;
+        }
         this.picker = Y.lp.app.picker.addPickerPatcher(
                 this.vocabulary,
                 "foo/bar",
                 "test_link",
                 "picker_id",
-                {
-                    "step_title": "Choose someone",
-                    "header": "Pick Someone",
-                    "show_search_box": show_search_box,
-                    "validate_callback": null
-                });
+                config
+                );
+    },
+
+    test_picker_displays_empty_list: function() {
+        // With too many results, the results will be empty.
+        this.create_picker(true);
+        this.picker.render();
+        this.picker.set('min_search_chars', 0);
+        this.picker.fire('search', '');
+        var result_text = this.picker.get('contentBox')
+            .one('.yui3-picker-results').get('text');
+        Assert.areEqual('', result_text);
     },
 
     test_picker_displays_warning: function() {
@@ -267,6 +283,17 @@
             this.picker.get('error'));
     },
 
+    test_picker_displays_warning_by_default: function() {
+        // If show_search_box is not supplied in config, it defaults to true.
+        // Thus the picker will refuse to display more than 120 values.
+        this.create_picker();
+        this.picker.set('min_search_chars', 0);
+        this.picker.fire('search', '');
+        Assert.areEqual(
+            'Too many matches. Please try to narrow your search.',
+            this.picker.get('error'));
+    },
+
     test_picker_no_warning: function() {
         // Without a search box the picker will also display more than
         // 120 values.

=== renamed file 'lib/lp/registry/javascript/personpicker.js' => 'lib/lp/app/javascript/widgets.js'
--- lib/lp/registry/javascript/personpicker.js	2011-05-31 18:27:50 +0000
+++ lib/lp/app/javascript/widgets.js	2011-06-09 08:46:18 +0000
@@ -1,21 +1,69 @@
 /* Copyright 2011 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
- * @namespace Y.lp.registry.personpicker
- * @requires lazr.picker
- */
-YUI.add('lp.registry.personpicker', function(Y) {
-var namespace = Y.namespace('lp.registry.personpicker');
-
-var footer_label = ".yui3-picker-footer-slot"
+ * @namespace Y.lp.app.widgets
+ * @requires Y.lazr.picker
+ */
+YUI.add('lp.app.widgets', function(Y) {
+var namespace = Y.namespace('lp.app.widgets');
+
+/**
+ * Extend the lazr-js Picker.
+ */
+var Picker = function() {
+    Picker.superclass.constructor.apply(this, arguments);
+};
+
+Y.extend(Picker, Y.lazr.Picker, {
+    // We want to render alt title slightly differently.
+    _renderTitleUI: function(data) {
+        var li_title = Y.Node.create(
+            '<span></span>').addClass(Y.lazr.Picker.C_RESULT_TITLE);
+        if (data.title === undefined) {
+            // Display an empty element if data is empty.
+            return li_title;
+        }
+        var title = this._text_or_link(
+            data.title, data.title_link, data.link_css);
+        li_title.appendChild(title);
+        if (data.alt_title) {
+            var alt_link = null;
+            if (data.alt_title_link) {
+                alt_link =Y.Node.create('<a></a>')
+                    .addClass(data.link_css)
+                    .addClass('discreet');
+                alt_link.set('text', " Details...")
+                    .set('href', data.alt_title_link);
+                Y.on('click', function(e) {
+                    e.halt();
+                    window.open(data.alt_title_link);
+                }, alt_link);
+            }
+            li_title.appendChild('&nbsp;(');
+            li_title.appendChild(document.createTextNode(data.alt_title));
+            li_title.appendChild(')');
+            if (alt_link !== null) {
+                li_title.appendChild(alt_link);
+            }
+        }
+        return li_title;
+    }
+});
+
+Picker.NAME = 'picker';
+namespace.Picker = Picker;
+
+/*
+ * Extend the picker into the PersonPicker
+ */
+var footer_label = ".yui3-picker-footer-slot";
 
 var PersonPicker = function() {
     PersonPicker.superclass.constructor.apply(this, arguments);
-
     this._extra_buttons = Y.Node.create('<div class="extra-form-buttons"/>');
 };
 
-Y.extend(PersonPicker, Y.lazr.Picker, {
+Y.extend(PersonPicker, namespace.Picker, {
     hide: function() {
         this.get('boundingBox').setStyle('display', 'none');
     },
@@ -36,29 +84,24 @@
     renderUI: function() {
         this.constructor.superclass.renderUI.call(this);
         var remove_button, assign_me_button;
-        //# TODO config should set extrabuttons
-        var show_remove_button = true;
-        var show_assign_me_button = true;
         var remove_button_text = "Remove assignee";
         var assign_me_button_text = "Assign me";
-        if (show_remove_button) {
-            remove_button = Y.Node.create(
-                '<a class="yui-picker-remove-button bg-image" ' +
-                'href="javascript:void(0)" ' +
-                'style="background-image: url(/@@/remove); padding-right: 1em">' +
-                remove_button_text + '</a>');
-            remove_button.on('click', this.remove, this);
-            this._extra_buttons.appendChild(remove_button);
-        }
-        if (show_assign_me_button) {
-            assign_me_button = Y.Node.create(
-                '<a class="yui-picker-assign-me-button bg-image" ' +
-                'href="javascript:void(0)" ' +
-                'style="background-image: url(/@@/person)">' +
-                assign_me_button_text + '</a>');
-            assign_me_button.on('click', this.assign_me, this);
-            this._extra_buttons.appendChild(assign_me_button);
-        }
+
+        remove_button = Y.Node.create(
+            '<a class="yui-picker-remove-button bg-image" ' +
+            'href="javascript:void(0)" ' +
+            'style="background-image: url(/@@/remove); padding-right: 1em">' +
+            remove_button_text + '</a>');
+        remove_button.on('click', this.remove, this);
+        this._extra_buttons.appendChild(remove_button);
+
+        assign_me_button = Y.Node.create(
+            '<a class="yui-picker-assign-me-button bg-image" ' +
+            'href="javascript:void(0)" ' +
+            'style="background-image: url(/@@/person)">' +
+            assign_me_button_text + '</a>');
+        assign_me_button.on('click', this.assign_me, this);
+        this._extra_buttons.appendChild(assign_me_button);
     },
 
     syncUI: function() {
@@ -69,6 +112,6 @@
     }
 });
 PersonPicker.NAME = 'person-picker';
-namespace.PersonPicker = PersonPicker
+namespace.PersonPicker = PersonPicker;
 
-}, "0.1", {"requires": ['lazr.picker']});
+}, "0.1", {"requires": ["lazr.picker"]});

=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py	2011-06-01 13:59:43 +0000
+++ lib/lp/app/widgets/popup.py	2011-06-09 08:46:18 +0000
@@ -18,6 +18,7 @@
 
 from canonical.launchpad.webapp import canonical_url
 from lp.app.browser.stringformatter import FormattersAPI
+from lp.services.features import getFeatureFlag
 from lp.services.propertycache import cachedproperty
 
 
@@ -26,6 +27,8 @@
 
     __call__ = ViewPageTemplateFile('templates/form-picker.pt')
 
+    picker_type = 'default'
+
     popup_name = 'popup-vocabulary-picker'
 
     # Override inherited attributes for the form field.
@@ -157,6 +160,16 @@
 
     include_create_team_link = False
 
+    @property
+    def picker_type(self):
+        # This is a method for now so we can block the use of the new
+        # person picker js behind our picker_enhancments feature flag.
+        if bool(getFeatureFlag('disclosure.picker_enhancements.enabled')):
+            picker_type = 'person'
+        else:
+            picker_type = 'default'
+        return picker_type
+
     def chooseLink(self):
         link = super(PersonPickerWidget, self).chooseLink()
         if self.include_create_team_link:
@@ -172,7 +185,6 @@
 class BugTrackerPickerWidget(VocabularyPickerWidget):
 
     __call__ = ViewPageTemplateFile('templates/bugtracker-picker.pt')
-
     link_template = """
         or (<a id="create-bugtracker-link"
         href="/bugs/bugtrackers/+newbugtracker"

=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt	2011-06-01 20:55:24 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt	2011-06-09 08:46:18 +0000
@@ -40,7 +40,8 @@
             var config = {
                 header: ${view/header_text},
                 step_title: ${view/step_title_text},
-                extra_no_results_message: ${view/extra_no_results_message}
+                extra_no_results_message: ${view/extra_no_results_message},
+                picker_type: '${view/picker_type}'
             };
             var picker = Y.lp.app.picker.create('${view/vocabulary_name}', config);
             if (config.extra_no_results_message !== null) {