launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug761798 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #761798 in Launchpad itself: "structural subscription add overlay closes before we get a (success or failure) response from server"
  https://bugs.launchpad.net/launchpad/+bug/761798

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug761798/+merge/57922

This branch introduces a small test for bug 761798 (the structural subscription add/edit overlay would close before hearing a response from the server) and fixes it.  

Along the way, I discovered an integration issue, that our delete-on-error handler used the wrong value for deleting.  I fixed this as well, but I did not have a test for it because our integration test story is not very good.

I exposed the overlay on the namespace because this allowed me to look at it for a test.  I tried looking at values on other objects, but this was the one I could get to work as I needed.  It doesn't seem unreasonable for us to expose it to test machinery anyway.

Thank you!

Gary
-- 
https://code.launchpad.net/~gary/launchpad/bug761798/+merge/57922
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug761798 into lp:launchpad.

=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js	2011-04-14 20:03:32 +0000
+++ lib/lp/registry/javascript/structural-subscription.js	2011-04-15 17:59:23 +0000
@@ -21,7 +21,6 @@
     MATCH_ANY = 'match-any'
     ;
 
-var add_subscription_overlay;
 var cancel_button_html =
     '<button type="button" name="field.actions.cancel" ' +
     'class="lazr-neg lazr-btn" >Cancel</button>';
@@ -38,16 +37,17 @@
 
 function subscription_success() {
     // TODO Should there be some success notification?
-    add_subscription_overlay.hide();
+    namespace._add_subscription_overlay.hide();
 }
 
 var overlay_error_handler = new Y.lp.client.ErrorHandler();
 overlay_error_handler.showError = function(error_msg) {
-    add_subscription_overlay.showError(error_msg);
+    namespace._add_subscription_overlay.showError(error_msg);
 };
 overlay_error_handler.clearProgressUI = function() {
-    add_subscription_overlay.bodyNode.one('[name="field.actions.create"]')
-        .replaceClass('spinner', 'lazr-pos');
+    var submit_button = namespace._add_subscription_overlay.bodyNode.one(
+        '[name="field.actions.create"]');
+    submit_button.replaceClass('spinner', 'lazr-pos');
 };
 
 /**
@@ -156,7 +156,9 @@
         headers: {'X-HTTP-Method-Override': 'DELETE'},
         on: {failure: overlay_error_handler.getFailureHandler()}
     };
-    Y.io(filter.self_link, y_config);
+    // This is called with a YUI-proxied version of the filter, so we need
+    // to use the YUI getter for the attribute.
+    Y.io(filter.get('self_link'), y_config);
 }
 
 // Exported for testing.
@@ -170,8 +172,8 @@
  * @param {Object} form_data The data returned from the form submission.
  * @param {Object} success_callback Function to execute when filter is added.
  */
-function add_bug_filter(who, form_data, success_callback) {
-    var submit_button = add_subscription_overlay.bodyNode.one(
+function add_bug_filter(who, form_data, success_callback, clean_up) {
+    var submit_button = namespace._add_subscription_overlay.bodyNode.one(
         '[name="field.actions.create"]');
     submit_button.replaceClass('lazr-pos', 'spinner');
     var config = {
@@ -188,6 +190,7 @@
                             .apply(this, arguments);
                     },
                     success: function (bug_filter) {
+                        clean_up();
                         success_callback(form_data, bug_filter);
                         subscription_success(bug_filter);
                         submit_button.replaceClass('spinner', 'lazr-pos');
@@ -218,12 +221,12 @@
  *        addition.
  */
 function make_add_subscription_handler(success_callback) {
-    var save_subscription = function(form_data) {
+    var save_subscription = function(form_data, clean_up) {
         var who;
         var has_errors = check_for_errors_in_overlay(
-            add_subscription_overlay);
+            namespace._add_subscription_overlay);
         if (has_errors) {
-            return false;
+            return;
         }
         if (form_data.recipient[0] === 'user') {
             who = LP.links.me;
@@ -231,7 +234,7 @@
             // There can be only one.
             who = form_data.team[0];
         }
-        return add_bug_filter(who, form_data, success_callback);
+        return add_bug_filter(who, form_data, success_callback, clean_up);
     };
     return save_subscription;
 }
@@ -278,14 +281,14 @@
 /**
  * Handle the activation of the edit subscription link.
  */
-function edit_subscription_handler(context, form_data) {
-    var has_errors = check_for_errors_in_overlay(add_subscription_overlay);
+function edit_subscription_handler(context, form_data, clean_up) {
+    var has_errors = check_for_errors_in_overlay(
+        namespace._add_subscription_overlay);
     var filter_id = '#filter-description-'+context.filter_id.toString();
-    var submit_button = add_subscription_overlay.bodyNode.one(
+    var submit_button = namespace._add_subscription_overlay.bodyNode.one(
         '[name="field.actions.create"]');
-    submit_button.replaceClass('lazr-pos', 'spinner');
     if (has_errors) {
-        return false;
+        return;
     }
     var on = {success: function (new_data) {
         submit_button.replaceClass('spinner', 'lazr-pos');
@@ -293,8 +296,9 @@
         var filter = new_data.getAttrs();
         fill_filter_description(
             description_node, context.filter_info, filter);
-        add_subscription_overlay.hide();
+        clean_up();
     }};
+    submit_button.replaceClass('lazr-pos', 'spinner');
     patch_bug_filter(context.filter_info.filter, form_data, on);
 }
 
@@ -312,7 +316,7 @@
 function create_overlay(content_box_id, overlay_id, submit_button,
                         submit_callback, success_callback) {
     // Create the overlay.
-    add_subscription_overlay = new Y.lazr.FormOverlay({
+    namespace._add_subscription_overlay = new Y.lazr.FormOverlay({
         headerContent:
             '<h2 id="subscription-overlay-title">Add a mail subscription '+
             'for '+LP.cache.context.title + ' bugs</h2>',
@@ -322,40 +326,37 @@
         form_cancel_button: Y.Node.create(cancel_button_html),
         form_submit_callback: function(formdata) {
             // Do not clean up if saving was not successful.
-            var save_succeeded = submit_callback(formdata);
-            if (save_succeeded !== false) {
-                clean_up();
-            }
+            submit_callback(formdata, clean_up);
         }
     });
 
     var side_portlets = Y.one('#side-portlets');
     if (side_portlets) {
-        add_subscription_overlay.set('align', {
+        namespace._add_subscription_overlay.set('align', {
             node: side_portlets,
             points: [Y.WidgetPositionAlign.TR, Y.WidgetPositionAlign.TL]
         });
     } else {
-        add_subscription_overlay.set('centered', true);
+        namespace._add_subscription_overlay.set('centered', true);
     }
-    add_subscription_overlay.render(content_box_id);
+    namespace._add_subscription_overlay.render(content_box_id);
     if (side_portlets) {
         Y.one('#subscription-overlay-title').scrollIntoView();
     }
-    setup_overlay_validators(add_subscription_overlay, overlay_id);
+    setup_overlay_validators(namespace._add_subscription_overlay, overlay_id);
     // Prevent cruft from hanging around upon closing.
     function clean_up(e) {
-        add_subscription_overlay.hide();
+        namespace._add_subscription_overlay.hide();
         var filter_wrapper = Y.one('#' + FILTER_WRAPPER);
         filter_wrapper.hide();
         collapse_node(filter_wrapper);
     }
     // For some reason, clicking on the cancel button doesn't fire a cancel
     // event, so we have to wire up this event handler.
-    add_subscription_overlay.get('form_cancel_button').on(
+    namespace._add_subscription_overlay.get('form_cancel_button').on(
         'click', clean_up);
-    add_subscription_overlay.on('submit', clean_up);
-    add_subscription_overlay.on('cancel', clean_up);
+    namespace._add_subscription_overlay.on('submit', clean_up);
+    namespace._add_subscription_overlay.on('cancel', clean_up);
 }
 
 /**
@@ -1054,8 +1055,8 @@
     };
     create_overlay(
         config.content_box, overlay_id, submit_button,
-        function (form_data) {
-            return edit_subscription_handler(context, form_data);});
+        function (form_data, clean_up) {
+            return edit_subscription_handler(context, form_data, clean_up);});
 
     load_overlay_with_filter_data(content_node, filter_info);
     var title = subscription.target_title;
@@ -1068,7 +1069,7 @@
     // initialized except for the ones we added, so setupHelpTrigger
     // is idempotent.  Notice that this is old MochiKit code.
     forEach(findHelpLinks(), setupHelpTrigger);
-    add_subscription_overlay.show();
+    namespace._add_subscription_overlay.show();
 }
 
 /**
@@ -1565,7 +1566,7 @@
     // initialized except for the ones we added, so setupHelpTrigger
     // is idempotent.  Notice that this is old MochiKit code.
     forEach(findHelpLinks(), setupHelpTrigger);
-    add_subscription_overlay.show();
+    namespace._add_subscription_overlay.show();
     return overlay_id;
 }
 namespace._show_add_overlay = show_add_overlay;

=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
--- lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-04-12 00:14:41 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-04-15 17:59:23 +0000
@@ -630,11 +630,14 @@
             submit_button.simulate('click');
             // We are now looking at the state after the named post has been
             // called, but before it has returned with a failure.
+            // The overlay should still be visible.
+            Assert.isTrue(module._add_subscription_overlay.get('visible'));
+            // The spinner should be spinning.
             Assert.isTrue(submit_button.hasClass('spinner'));
             Assert.isFalse(submit_button.hasClass('lazr-pos'));
             // Now we resume the call to trigger the failure.
             this.configuration.lp_client.named_post.resume()
-
+            // The spinner is gone.
             Assert.isTrue(submit_button.hasClass('lazr-pos'));
             Assert.isFalse(submit_button.hasClass('spinner'));
         },