launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/form-picker-macros-also-irritate-me/+merge/68862

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.

A previous branch was landed (see https://code.launchpad.net/~jcsackett/launchpad/form-macros-frighten-me/+merge/68277), but resolution of a conflict in merging broke some functionality and the branch was rolled back. This branch undoes the rollback and fixes the bad conflict resolution.

The diff for the conflict resolution is here: http://pastebin.ubuntu.com/650048/

Pre-implementation notes
========================
Benji York identified the problem in an earlier review; Aaron Bentley expanded on it in another pickers review, pointing out that the means of passing in all the various config data was likely more error prone since we were setting "booleans" in python code that were actually strings to be evaluated as javascript booleans.

Ian helped flesh out the final config ideas.

Proposed fix
============
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-picker-macros-also-irritate-me/+merge/68862
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/form-picker-macros-also-irritate-me into lp:launchpad.

=== modified file 'lib/lp/answers/browser/tests/test_questiontarget.py'
--- lib/lp/answers/browser/tests/test_questiontarget.py	2011-07-22 05:50:38 +0000
+++ lib/lp/answers/browser/tests/test_questiontarget.py	2011-07-22 15:31:28 +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/tests/test_picker_patcher.js'
--- lib/lp/app/javascript/picker/tests/test_picker_patcher.js	2011-07-22 05:50:38 +0000
+++ lib/lp/app/javascript/picker/tests/test_picker_patcher.js	2011-07-22 15:31:28 +0000
@@ -313,8 +313,10 @@
             "selected_value_metadata": params.selected_value_metadata,
             "assign_me_text": "Assign Moi",
             "remove_person_text": "Remove someone",
-            "remove_team_text": "Remove some team"
+            "remove_team_text": "Remove some team",
+            "vocabular_name": this.vocabulary
             };
+
         this.picker = Y.lp.app.picker.addPickerPatcher(
                 this.vocabulary,
                 "foo/bar",

=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py	2011-07-22 05:50:38 +0000
+++ lib/lp/app/widgets/popup.py	2011-07-22 15:31:28 +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-22 05:50:38 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt	2011-07-22 15:31:28 +0000
@@ -35,19 +35,10 @@
             return;
         }
 
-        // The vocabulary picker, created when used for the first time.
-        function make_picker() {
-            var config = ${view/json_config};
-            if (config.picker_type == 'person') {
-                config.show_assign_me_button = ${view/show_assign_me_button}
-                config.show_remove_button = ${view/show_remove_button}
-            }
-            var picker = Y.lp.app.picker.create('${view/vocabulary_name}',
-                                config,
-                                '${view/input_id}');
-            return picker;
-        }
         var picker = null;
+        var config = ${view/json_config};
+        var vocabulary = config.vocabulary_name;
+        var input_element = config.input_element;
         Y.on('domready', function(e) {
             // Sort out the Choose... link.
             var show_widget_node = Y.one('#${view/show_widget_id}');
@@ -57,7 +48,8 @@
             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.create(
+                        vocabulary, config, input_element);
                 }
                 picker.show();
                 e.preventDefault();

=== modified file 'lib/lp/app/widgets/tests/test_popup.py'
--- lib/lp/app/widgets/tests/test_popup.py	2011-07-22 05:50:38 +0000
+++ lib/lp/app/widgets/tests/test_popup.py	2011-07-22 15:31:28 +0000
@@ -71,13 +71,8 @@
             'field.test_valid.item', picker_widget.input_id)
         self.assertIsNone(picker_widget.extra_no_results_message)
         markup = picker_widget()
-        self.assertTextMatchesExpressionIgnoreWhitespace("""\
-            .*
-            var picker = Y\\.lp\\.app\\.picker\\.create\\('ValidTeamOwner',
-                config,
-                'field\\.test_valid.item'\\);
-            .*
-            """, markup)
+        self.assertIn("Y.lp.app.picker.create", markup)
+        self.assertIn('ValidTeamOwner', markup)
 
     def test_widget_fieldname_with_invalid_html_chars(self):
         # Check the picker widget is correctly set up for a field which has a
@@ -126,18 +121,14 @@
         # A vocabulary widget does not show the extra buttons by default.
         picker_widget = VocabularyPickerWidget(
             bound_field, self.vocabulary, self.request)
-        self.assertEqual('false',
-            picker_widget.config['show_assign_me_button'])
-        self.assertEqual('false',
-            picker_widget.config['show_remove_button'])
+        self.assertFalse(picker_widget.config['show_assign_me_button'])
+        self.assertFalse(picker_widget.config['show_remove_button'])
 
         # A person picker widget does show them by default.
         person_picker_widget = PersonPickerWidget(
             bound_field, self.vocabulary, self.request)
-        self.assertEqual('true',
-            person_picker_widget.config['show_assign_me_button'])
-        self.assertEqual('true',
-            person_picker_widget.config['show_remove_button'])
+        self.assertTrue(person_picker_widget.config['show_assign_me_button'])
+        self.assertTrue(person_picker_widget.config['show_remove_button'])
 
     def test_widget_personvalue_meta(self):
         # The person picker has the correct meta value for a person value.

=== modified file 'lib/lp/blueprints/browser/tests/test_specificationtarget.py'
--- lib/lp/blueprints/browser/tests/test_specificationtarget.py	2011-07-22 05:50:38 +0000
+++ lib/lp/blueprints/browser/tests/test_specificationtarget.py	2011-07-22 15:31:28 +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-07-22 05:50:38 +0000
+++ lib/lp/bugs/browser/tests/test_bugs.py	2011-07-22 15:31:28 +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)