launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/rollback-pickers-pickers-everywhere into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #707234 in Launchpad itself: "Ajax controls disabled on bugs with many tasks due to multiple redundant copies of person picker make_picker function in view-source:https://bugs.launchpad.net/launchpad-project/+bugs?advanced=1";
  https://bugs.launchpad.net/launchpad/+bug/707234

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/rollback-pickers-pickers-everywhere/+merge/65013

Removes the changes to the make_picker and form_picker macros made in 13521 that broke pickers in several places via rollback.
-- 
https://code.launchpad.net/~jcsackett/launchpad/rollback-pickers-pickers-everywhere/+merge/65013
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/rollback-pickers-pickers-everywhere into lp:launchpad.

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

=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py	2011-06-14 20:00:19 +0000
+++ lib/lp/app/widgets/popup.py	2011-06-17 15:18:35 +0000
@@ -43,28 +43,6 @@
     # Defaults to self.vocabulary.displayname.
     header = None
 
-    @property
-    def widget_rendered(self):
-        # We manually create the widget name since we don't want different
-        # popups colliding, and we have to put the attr on the request so all
-        # instances of the popup on this request can check the attr.
-        attr = self.__class__.__name__ + '_rendered'
-        if hasattr(self.request, attr):
-            return getattr(self.request, attr)
-        else:
-            # The widget has not rendered before this. We return False, but
-            # set the attribute to true, so future checks will see that it
-            # has rendered. It would be better to set this in __init__ or
-            # similar, but then this first check also sees True, and we never
-            # render the part of the template this is used to guard.
-            setattr(self.request, attr, True)
-            return False
-
-    @widget_rendered.setter
-    def widget_rendered(self, val):
-        attr = self.__class__.__name__ + '_rendered'
-        setattr(self.request, attr, val)
-
     @cachedproperty
     def matches(self):
         """Return a list of matches (as ITokenizedTerm) to whatever the

=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt	2011-06-14 19:30:29 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt	2011-06-17 15:18:35 +0000
@@ -29,19 +29,21 @@
     </tal:search_results>
 
     <tal:chooseLink replace="structure view/chooseLink" />
-
-    <script
-      tal:condition="not: view/widget_rendered"
-      tal:content="structure string:
+    <script tal:content="structure string:
     LPS.use('node', 'lp.app.picker', 'plugin', function(Y) {
         if (Y.UA.ie) {
             return;
         }
-        namespace = Y.namespace('lp.macros');
+
         // The vocabulary picker, created when used for the first time.
-        function make_picker(config) {
-            var vocab_name = config.vocabulary_name;
-            var picker = Y.lp.app.picker.create(vocab_name, config);
+        function make_picker() {
+            var config = {
+                header: ${view/header_text},
+                step_title: ${view/step_title_text},
+                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) {
                 picker.before('resultsChange', function (e) {
                     var new_results = e.details[0].newVal;
@@ -55,27 +57,10 @@
                 });
             }
             picker.plug(Y.lazr.TextFieldPickerPlugin,
-                        {input_element: '[id=\x22' + config.input_id + '\x22]'});
+                        {input_element: '[id=\x22${view/input_id}\x22]'});
             return picker;
         }
-        namespace.make_picker = make_picker;
-    });
-    "/>
-
-    <script tal:content="structure string:
-    LPS.use('node', 'lp.app.picker', 'plugin', function(Y) {
-        if (Y.UA.ie) {
-            return;
-        }
         var picker = null;
-        var config = {
-            vocabulary_name: '${view/vocabulary_name}',
-            header: ${view/header_text},
-            step_title: ${view/step_title_text},
-            extra_no_results_message: ${view/extra_no_results_message},
-            picker_type: '${view/picker_type}',
-            input_id: '${view/input_id}'
-        };
         Y.on('domready', function(e) {
             // Sort out the Choose... link.
             var show_widget_node = Y.one('#${view/show_widget_id}');
@@ -85,7 +70,7 @@
             show_widget_node.get('parentNode').removeClass('unseen');
             show_widget_node.on('click', function (e) {
                 if (picker === null) {
-                    picker = Y.lp.macros.make_picker(config);
+                    picker = make_picker();
                 }
                 picker.show();
                 e.preventDefault();

=== modified file 'lib/lp/app/widgets/tests/test_popup.py'
--- lib/lp/app/widgets/tests/test_popup.py	2011-06-15 17:16:40 +0000
+++ lib/lp/app/widgets/tests/test_popup.py	2011-06-17 15:18:35 +0000
@@ -24,8 +24,6 @@
             "test_invalid_chars+":
             Choice(vocabulary='ValidTeamOwner'),
             "test_valid.item":
-            Choice(vocabulary='ValidTeamOwner'),
-            "test_valid.second_item":
             Choice(vocabulary='ValidTeamOwner')}
         super(TestMetaClass, self).__init__(
             name, bases=bases, attrs=attrs, __doc__=__doc__,
@@ -49,24 +47,6 @@
             self.context, 'ValidTeamOwner')
         self.request = LaunchpadTestRequest()
 
-    def test_widget_rendered_one(self):
-        field = ITest['test_valid.item']
-        bound_field = field.bind(self.context)
-        picker_widget = VocabularyPickerWidget(
-            bound_field, self.vocabulary, self.request)
-        self.assertFalse(picker_widget.widget_rendered)
-        self.assertTrue(self.request.VocabularyPickerWidget_rendered)
-
-    def test_widget_rendered_multiple(self):
-        fields = (ITest['test_valid.item'], ITest['test_valid.second_item'])
-        bound_fields = [field.bind(self.context) for field in fields]
-        picker_widgets = [VocabularyPickerWidget(
-            bound_field, self.vocabulary, self.request)
-            for bound_field in bound_fields]
-        self.assertFalse(picker_widgets[0].widget_rendered)
-        self.assertTrue(self.request.VocabularyPickerWidget_rendered)
-        self.assertTrue(picker_widgets[1].widget_rendered)
-
     def test_widget_template_properties(self):
         # Check the picker widget is correctly set up for a field which has a
         # name containing only valid HTML ID characters.
@@ -91,8 +71,8 @@
         self.assertEqual(
             simplejson.dumps(None), picker_widget.extra_no_results_message)
         markup = picker_widget()
-        self.assertIn("vocabulary_name: 'ValidTeamOwner'", markup)
-        self.assertIn("Y.lp.app.picker.create(vocab_name, config);", markup)
+        self.assertIn(
+            "Y.lp.app.picker.create('ValidTeamOwner', config);", markup)
 
     def test_widget_fieldname_with_invalid_html_chars(self):
         # Check the picker widget is correctly set up for a field which has a

=== modified file 'lib/lp/blueprints/browser/tests/test_specificationtarget.py'
--- lib/lp/blueprints/browser/tests/test_specificationtarget.py	2011-06-15 17:16:40 +0000
+++ lib/lp/blueprints/browser/tests/test_specificationtarget.py	2011-06-17 15:18:35 +0000
@@ -300,7 +300,7 @@
             None, content.find(True, id=target_widget.show_widget_id))
         text = str(content)
         picker_script = (
-            "vocabulary_name: 'DistributionOrProductOrProjectGroup'")
+            "Y.lp.app.picker.create('DistributionOrProductOrProjectGroup'")
         self.assertIn(picker_script, 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-15 17:16:40 +0000
+++ lib/lp/bugs/browser/tests/test_bugs.py	2011-06-17 15:18:35 +0000
@@ -87,7 +87,7 @@
             None, content.find(True, id=target_widget.show_widget_id))
         text = str(content)
         picker_script = (
-            "vocabulary_name: 'DistributionOrProductOrProjectGroup'")
+            "Y.lp.app.picker.create('DistributionOrProductOrProjectGroup'")
         self.assertIn(picker_script, text)
         focus_script = "setFocusByName('field.searchtext')"
         self.assertIn(focus_script, text)