launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/global-yui-namespaces-gone into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #922462 in Launchpad itself: "bugtask delete doesn't rewire pickers after table is updated"
  https://bugs.launchpad.net/launchpad/+bug/922462

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/global-yui-namespaces-gone/+merge/90385

The recent YUI changes for the combo loader functionality effectively killed global namespaces, since each YUI block is now sandboxed. A new way was needed to re-wire pickers after deleting a bugtask.

== Implementation ==

Fire an event to trigger the picker updates. Change the code to listen to the event and do the wiring.

== Tests ==

Update the test_setup_bugtask_table test in test_bugtask_delete

== Lint ==

Did a bit of drive-by lint fixing.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/widgets/templates/form-picker-macros.pt
  lib/lp/bugs/javascript/bugtask_index.js
  lib/lp/bugs/javascript/tests/test_bugtask_delete.js
-- 
https://code.launchpad.net/~wallyworld/launchpad/global-yui-namespaces-gone/+merge/90385
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/global-yui-namespaces-gone into lp:launchpad.

=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt	2012-01-19 22:14:01 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt	2012-01-27 08:05:27 +0000
@@ -41,11 +41,14 @@
         var vocabulary_filters = config.vocabulary_filters;
         var input_element = config.input_element;
         var show_widget_id = '${view/show_widget_id}';
-        var namespace = Y.namespace('lp.app.picker.connect');
-        namespace[show_widget_id] = function() {
+        var body = Y.one('body');
+        body.on('lp.widget.connect', function(ev) {
             // Sort out the Choose... link.
+            var show_widget_id = ev.details[0];
             var show_widget_node = Y.one('#'+show_widget_id);
-
+            if (show_widget_node === null ) {
+                return;
+            }
             show_widget_node.set('innerHTML', 'Choose…');
             show_widget_node.addClass('js-action');
             show_widget_node.get('parentNode').removeClass('unseen');
@@ -58,9 +61,9 @@
                 picker.show();
                 e.preventDefault();
             });
-        };
+        });
         Y.on('domready', function(e) {
-            namespace[show_widget_id]();
+            body.fire('lp.widget.connect', show_widget_id);
         });
     });
     "/>

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2012-01-19 04:50:47 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2012-01-27 08:05:27 +0000
@@ -147,7 +147,8 @@
                     // XXX Abel Deuring 2009-04-23, bug 365462
                     // Y.one('#field.private') returns null.
                     // Seems that YUI does not like IDs containing a '.'
-                    var field_private = document.getElementById('field.private');
+                    var field_private
+                        = document.getElementById('field.private');
                     if (field_private !== null) {
                         field_private.focus();
                     }
@@ -658,16 +659,14 @@
     if (!Y.Lang.isValue(bugtask_data)) {
         return;
     }
-    var picker_connect = Y.namespace('lp.app.picker.connect');
     var process_link = function(link) {
         // The link may already have been processed.
         if (link.hasClass('js-action')) {
             return;
         }
-        var func_name = link.get('id');
-        var connect_func = picker_connect[func_name];
-        if (Y.Lang.isFunction(connect_func)) {
-            connect_func();
+        var widget_id = link.get('id');
+        if (widget_id !== '') {
+            Y.one('body').fire('lp.widget.connect', widget_id);
         }
     };
     var id;
@@ -1508,7 +1507,7 @@
         expander.setUp();
     });
 
-}
+};
 
 
 }, "0.1", {"requires": ["base", "oop", "node", "event", "io-base",

=== modified file 'lib/lp/bugs/javascript/tests/test_bugtask_delete.js'
--- lib/lp/bugs/javascript/tests/test_bugtask_delete.js	2011-12-19 09:53:25 +0000
+++ lib/lp/bugs/javascript/tests/test_bugtask_delete.js	2012-01-27 08:05:27 +0000
@@ -117,11 +117,11 @@
         test_setup_bugtask_table: function() {
             // Test that the bugtask table is wired up, the pickers and the
             // delete links etc.
-            var namespace = Y.namespace('lp.app.picker.connect');
             var connect_picker_called = false;
-            namespace['show-widget-product'] = function() {
-                connect_picker_called = true;
-            };
+            Y.one('body').on('lp.widget.connect', function(ev) {
+                connect_picker_called = ev.details[0]
+                    === 'show-widget-product';
+            });
             var orig_confirm_bugtask_delete = module._confirm_bugtask_delete;
             var self = this;
             var confirm_delete_called = false;
@@ -136,8 +136,11 @@
 
             // Test wiring of delete link for row with an associated form.
             this.delete_link.simulate('click');
-            Y.Assert.isTrue(connect_picker_called);
-            Y.Assert.isTrue(confirm_delete_called);
+            this.wait(function() {
+                // Wait for the events to fire.
+                Y.Assert.isTrue(connect_picker_called);
+                Y.Assert.isTrue(confirm_delete_called);
+            }, 20);
 
             // Test wiring of delete link for row without an associated form.
             confirm_delete_called = false;