launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/form-macros-frighten-me into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/form-macros-frighten-me/+merge/68277

Summary
=======
The form-picker-macros.pt template provides the means of wiring pickers into the launchpad webui. While it works, it's ugly--javascript is repeated for every picker element that could be extracted into our javascript libraries, and a lot of manual config is assembled from the view properties.

This branch cleans this up to the extent possible.

Pre-implementation notes
========================
Initially spoke with Benji York about the problem existing in the first place; he recommended trying to set things up so that the javascript used the JSONCache to get its configuration files.

Later, Aaron Bentley indicated that a part of the problem involved passing in javastyle booleans that looked like Python booleans (e.g. a popup with show_assign_button = 'false' in python code), which was likely to bite a developer at some point.

Proposed fix
============
Per Benji's suggestion, the intial idea involved moving the make_picker function into the existing javascript files, and using the JSONRequestCache to get config data. It turns out that the popup.py widgets can't put values into JSONRequestCache (or if they can, it's sufficiently roundabout as to constitute a bad hack).

Implementation details 
======================
The majority of the properties in the view are now assembled into a config property and a json_config propery, which uses simplejson to dump them to javascript. This approach was implemented simulataneously in this branch and in another that beat it to devel, so the majority of the work doesn't show up in the diff (being part of the tree). Please keep an eye open for bad conflict resolution artifacts on my part, since the collision was pretty massive. I believe I caught everything, and tests/UI all work as expected, but another set of eyes wouldn't hurt.

Because simplejson is now used to convert the variables into a javascript dictionary, the properties that were javascript style booleans can now be converted back into python booleans. They're unlikely to be used outside of the widget, but view code that does so will no longer be dangerous.

make_picker now exists in the picker files as initially planned.

Tests
=====

bin/test -vvct test_popup
bin/test -vvc --layer=YUI

QA
==

Checkout a personpicker instance (e.g. on +edit-people). Everything should work as before this branch.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/widgets/templates/form-picker-macros.pt
  lib/lp/answers/browser/tests/test_questiontarget.py
  lib/lp/app/javascript/picker/picker_patcher.js
  lib/lp/bugs/browser/tests/test_bugs.py
  lib/lp/blueprints/browser/tests/test_specificationtarget.py
  lib/lp/app/widgets/popup.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/form-macros-frighten-me/+merge/68277
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/form-macros-frighten-me into lp:launchpad.

=== modified file 'lib/lp/answers/browser/tests/test_questiontarget.py'
--- lib/lp/answers/browser/tests/test_questiontarget.py	2011-06-17 15:09:15 +0000
+++ lib/lp/answers/browser/tests/test_questiontarget.py	2011-07-18 16:27:49 +0000
@@ -218,8 +218,7 @@
         self.assertIsNot(
             None, content.find(True, id=target_widget.show_widget_id))
         text = str(content)
-        picker_script = (
-            "Y.lp.app.picker.create('DistributionOrProductOrProjectGroup'")
-        self.assertIn(picker_script, text)
+        picker_vocab = "DistributionOrProductOrProjectGroup"
+        self.assertIn(picker_vocab, text)
         focus_script = "setFocusByName('field.search_text')"
         self.assertIn(focus_script, text)

=== modified file 'lib/lp/app/javascript/picker/picker_patcher.js'
--- lib/lp/app/javascript/picker/picker_patcher.js	2011-07-18 05:32:40 +0000
+++ lib/lp/app/javascript/picker/picker_patcher.js	2011-07-18 16:27:49 +0000
@@ -535,6 +535,28 @@
     return picker;
 };
 
+namespace.make_picker = function make_picker(config) {
+    // We split out the vocabulary since config is passed in as one bundle
+    // from a template for convenience, but `create` needs it as a separate
+    // variable.
+    var vocabulary = config.vocabulary_name;
+    var picker = namespace.create(vocabulary, config);
+    if (config.extra_no_results_message !== null) {
+        picker.before('resultsChange', function (e) {
+            var new_results = e.details[0].newVal;
+            if (new_results.length === 0) {
+                picker.set('footer_slot',
+                           Y.Node.create(config.extra_no_results_message));
+            }
+            else {
+                picker.set('footer_slot', null);
+            }
+        });
+    }
+    picker.plug(Y.lazr.picker.TextFieldPickerPlugin,
+        {input_element: '[id=\x22' + config.input_element + '\x22]'});
+    return picker;
+}
 }, "0.1", {"requires": [
     "io", "dom", "dump", "event", "lazr.activator", "json-parse",
     "lp.client", "lazr.picker", "lazr.person-picker"

=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py	2011-07-18 00:45:28 +0000
+++ lib/lp/app/widgets/popup.py	2011-07-18 16:27:49 +0000
@@ -32,8 +32,8 @@
     # Provide default values for the following properties in case someone
     # creates a vocab picker for a person instead of using the derived
     # PersonPicker.
-    show_assign_me_button = 'false'
-    show_remove_button = 'false'
+    show_assign_me_button = False
+    show_remove_button = False
     assign_me_text = 'Pick me'
     remove_person_text = 'Remove person'
     remove_team_text = 'Remove team'
@@ -123,7 +123,9 @@
             remove_person_text=self.remove_person_text,
             remove_team_text=self.remove_team_text,
             show_remove_button=self.show_remove_button,
-            show_assign_me_button=self.show_assign_me_button)
+            show_assign_me_button=self.show_assign_me_button,
+            vocabulary_name=self.vocabulary_name,
+            input_element=self.input_id)
 
     @property
     def json_config(self):
@@ -189,8 +191,8 @@
 class PersonPickerWidget(VocabularyPickerWidget):
 
     include_create_team_link = False
-    show_assign_me_button = 'true'
-    show_remove_button = 'true'
+    show_assign_me_button = True
+    show_remove_button = True
 
     @property
     def picker_type(self):

=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt	2011-07-14 12:44:48 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt	2011-07-18 16:27:49 +0000
@@ -35,27 +35,8 @@
             return;
         }
 
-        // The vocabulary picker, created when used for the first time.
-        function make_picker() {
-            var config = ${view/json_config};
-            var picker = Y.lp.app.picker.create('${view/vocabulary_name}', config);
-            if (config.extra_no_results_message !== null) {
-                picker.before('resultsChange', function (e) {
-                    var new_results = e.details[0].newVal;
-                    if (new_results.length === 0) {
-                        picker.set('footer_slot',
-                                   Y.Node.create(config.extra_no_results_message));
-                    }
-                    else {
-                        picker.set('footer_slot', null);
-                    }
-                });
-            }
-            picker.plug(Y.lazr.picker.TextFieldPickerPlugin,
-                        {input_element: '[id=\x22${view/input_id}\x22]'});
-            return picker;
-        }
         var picker = null;
+        var config = ${view/json_config};
         Y.on('domready', function(e) {
             // Sort out the Choose... link.
             var show_widget_node = Y.one('#${view/show_widget_id}');
@@ -65,7 +46,7 @@
             show_widget_node.get('parentNode').removeClass('unseen');
             show_widget_node.on('click', function (e) {
                 if (picker === null) {
-                    picker = make_picker();
+                    picker = Y.lp.app.picker.make_picker(config);
                 }
                 picker.show();
                 e.preventDefault();

=== modified file 'lib/lp/blueprints/browser/tests/test_specificationtarget.py'
--- lib/lp/blueprints/browser/tests/test_specificationtarget.py	2011-06-17 15:09:15 +0000
+++ lib/lp/blueprints/browser/tests/test_specificationtarget.py	2011-07-18 16:27:49 +0000
@@ -299,8 +299,7 @@
         self.assertIsNot(
             None, content.find(True, id=target_widget.show_widget_id))
         text = str(content)
-        picker_script = (
-            "Y.lp.app.picker.create('DistributionOrProductOrProjectGroup'")
-        self.assertIn(picker_script, text)
+        picker_vocab = 'DistributionOrProductOrProjectGroup'
+        self.assertIn(picker_vocab, text)
         focus_script = "setFocusByName('field.search_text')"
         self.assertIn(focus_script, text)

=== modified file 'lib/lp/bugs/browser/tests/test_bugs.py'
--- lib/lp/bugs/browser/tests/test_bugs.py	2011-06-17 15:09:15 +0000
+++ lib/lp/bugs/browser/tests/test_bugs.py	2011-07-18 16:27:49 +0000
@@ -86,8 +86,7 @@
         self.assertIsNot(
             None, content.find(True, id=target_widget.show_widget_id))
         text = str(content)
-        picker_script = (
-            "Y.lp.app.picker.create('DistributionOrProductOrProjectGroup'")
-        self.assertIn(picker_script, text)
+        picker_vocab = "DistributionOrProductOrProjectGroup"
+        self.assertIn(picker_vocab, text)
         focus_script = "setFocusByName('field.searchtext')"
         self.assertIn(focus_script, text)