← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~gary/launchpad/bug750561 into lp:launchpad

 

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #750561 in Launchpad itself: "Spinner for adding/editing/deleting/muting subscriptions"
  https://bugs.launchpad.net/launchpad/+bug/750561

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

This branch adds a spinner when you delete, edit, or add a structural subscription.

It does this with some simple CSS changes.  What I've done seems like a pretty good pattern to follow for how to do this, because it seems very simple: just change classes around to spinner while you are waiting for the work to be done.

I added a couple of JS tests for this.  They should be relatively straightforward to read.  I added the ability of our LPClient stub to halt execution of the callback so you can examine the intermediate state.  In order for the test to pass I had to add a dependency for lp.app.errors to work, and then I had to add some code to clean up the overlay that lp.app.errors adds.

While working on the tests, which involved a fair amount of debugging, I became annoyed and distracted by five errors I was seeing in the Chrome error log (but not in the test results).  I traced the problem down to the fact that we were reusing the link for every test, which caused the errors as unpleasant side effects.  Therefore, I made our setup code set up the link as well.  This seemed to work nicely.

I also needed to simulate a click, and I noticed that there were two approaches to simulating a click in our tests: "Y.Event.simulate(Y.Node.getDOMNode(select_none), 'click');" and "select_none.simulate('click');".  The second looked wildly nicer and less error-prone (like forgetting to get the DOM node) so I converted all of the code to use it.  FF4 newest, Chrome newest and Safari newest all passed with this clean-up.

This is an incremental branch. In order to completely fix this bug, I will need to also add a spinner for the "muting" code in db-devel.  I will do that next.

For QA, you should see the "remove" icon turn into a spinner when you click "Unsubscribe" on [structural subscription target]/+subscriptions (when you are in the proper feature-flagged group); and you should see the "submit" button turn into a spinner when you edit or add a subscription on that page or on the structural subscription target overview or bug pages.

Thank you

Gary
-- 
https://code.launchpad.net/~gary/launchpad/bug750561/+merge/57195
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug750561 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2011-03-08 15:05:21 +0000
+++ lib/canonical/launchpad/icing/style.css	2011-04-11 16:26:14 +0000
@@ -538,6 +538,17 @@
   text-align: left;
 }
 
+/* Intended to be used as temporary replacement for a sprite class when the
+   associated link's action is in-progress. */
+.spinner {
+  background: url(/@@/spinner) center left no-repeat;
+}
+
+/* Intended to be used as temporary replacement for a button class like
+   lazr-pos when the associated button's action is in-progress. */
+button.spinner {
+  background-image: url(/@@/spinner) !important;
+}
 
 
 /* === Translations === */

=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js	2011-04-09 00:52:33 +0000
+++ lib/lp/registry/javascript/structural-subscription.js	2011-04-11 16:26:14 +0000
@@ -43,7 +43,11 @@
 
 var overlay_error_handler = new Y.lp.client.ErrorHandler();
 overlay_error_handler.showError = function(error_msg) {
-  add_subscription_overlay.showError(error_msg);
+    add_subscription_overlay.showError(error_msg);
+};
+overlay_error_handler.clearProgressUI = function() {
+    add_subscription_overlay.bodyNode.one('[name="field.actions.create"]')
+        .replaceClass('spinner', 'lazr-pos');
 };
 
 /**
@@ -167,6 +171,9 @@
  * @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(
+        '[name="field.actions.create"]');
+    submit_button.replaceClass('lazr-pos', 'spinner');
     var config = {
         on: {success: function (bug_filter) {
                 // If we fail to PATCH the new bug filter, DELETE it.
@@ -183,6 +190,7 @@
                     success: function (bug_filter) {
                         success_callback(form_data, bug_filter);
                         subscription_success(bug_filter);
+                        submit_button.replaceClass('spinner', 'lazr-pos');
                     }
                 };
                 patch_bug_filter(bug_filter.getAttrs(), form_data, on);
@@ -273,10 +281,14 @@
 function edit_subscription_handler(context, form_data) {
     var has_errors = check_for_errors_in_overlay(add_subscription_overlay);
     var filter_id = '#filter-description-'+context.filter_id.toString();
+    var submit_button = add_subscription_overlay.bodyNode.one(
+        '[name="field.actions.create"]');
+    submit_button.replaceClass('lazr-pos', 'spinner');
     if (has_errors) {
         return false;
     }
     var on = {success: function (new_data) {
+        submit_button.replaceClass('spinner', 'lazr-pos');
         var description_node = Y.one(filter_id);
         var filter = new_data.getAttrs();
         fill_filter_description(
@@ -1082,11 +1094,14 @@
 /**
  * Construct a handler for an unsubscribe link.
  */
-function make_delete_handler(filter, filter_id, subscriber_id) {
+function make_delete_handler(filter, filter_id, node, subscriber_id) {
     var error_handler = new Y.lp.client.ErrorHandler();
+    var unsubscribe_node = node.one('a.delete-subscription');
     error_handler.showError = function(error_msg) {
-      var unsubscribe_node = Y.one('#unsubscribe-'+filter_id.toString());
-      Y.lp.app.errors.display_error(unsubscribe_node, error_msg);
+        Y.lp.app.errors.display_error(unsubscribe_node, error_msg);
+    };
+    error_handler.clearProgressUI = function () {
+        unsubscribe_node.replaceClass('spinner', 'remove');
     };
     return function() {
         var y_config = {
@@ -1094,6 +1109,7 @@
             headers: {'X-HTTP-Method-Override': 'DELETE'},
             on: {
                 success: function(transactionid, response, args){
+                    unsubscribe_node.replaceClass('spinner', 'remove');
                     var filter_node = Y.one(
                         '#subscription-filter-'+filter_id.toString());
                     filter_node.setStyle("margin-top", "0");
@@ -1111,6 +1127,7 @@
                 failure: error_handler.getFailureHandler()
             }
         };
+        unsubscribe_node.replaceClass('remove', 'spinner');
         do_io(filter.self_link, y_config);
     };
 }
@@ -1131,7 +1148,7 @@
         edit_link.on('click', edit_handler);
         var delete_link = node.one('a.delete-subscription');
         var delete_handler = make_delete_handler(
-            filter_info.filter, filter_id, subscription_id);
+            filter_info.filter, filter_id, node, subscription_id);
         delete_link.on('click', delete_handler);
     }
 }

=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.html'
--- lib/lp/registry/javascript/tests/test_structural_subscription.html	2011-03-21 15:25:10 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.html	2011-04-11 16:26:14 +0000
@@ -30,6 +30,8 @@
   <script type="text/javascript"
     src="../../../app/javascript/client.js"></script>
   <script type="text/javascript"
+    src="../../../app/javascript/errors.js"></script>
+  <script type="text/javascript"
     src="../../../contrib/javascript/yui3-gallery/gallery-accordion/gallery-accordion.js">
   </script>
   <script type="text/javascript"
@@ -53,11 +55,6 @@
   </style>
 </head>
 <body class="yui3-skin-sam">
-  <div>
-    This exists to stop tests from breaking.
-    <a href="#" class="menu-link-subscribe_to_bug_mail"
-    >A link, a link, my kingdom for a link</a>
-  </div>
   <div id="log"></div>
 </body>
 </html>

=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
--- lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-04-09 00:49:32 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-04-11 16:26:14 +0000
@@ -47,6 +47,9 @@
         var test_node = Y.Node.create('<div id="test-content">')
             .append(Y.Node.create('<div></div>')
                 .set('id', content_box_name));
+        test_node.append(Y.Node.create(
+            '<a href="#" class="menu-link-subscribe_to_bug_mail">'+
+            'A link, a link, my kingdom for a link</a>'));
 
         if (include_listing) {
             test_node.append(Y.Node.create('<div style="width: 50%"></div>')
@@ -58,6 +61,10 @@
 
     function remove_test_node() {
         Y.one('body').removeChild(Y.one('#test-content'));
+        var error_overlay = Y.one('.yui3-lazr-formoverlay');
+        if (Y.Lang.isValue(error_overlay)) {
+            Y.one('body').removeChild(error_overlay);
+        }
     }
 
     function test_checked(list, expected) {
@@ -117,10 +124,17 @@
         if (!Y.Lang.isValue(args.callee.args)) {
             throw new Error("Set call_args on "+name);
         }
-        if (Y.Lang.isValue(args.callee.fail) && args.callee.fail) {
-            config.on.failure.apply(undefined, args.callee.args);
+        var do_action = function () {
+            if (Y.Lang.isValue(args.callee.fail) && args.callee.fail) {
+                config.on.failure.apply(undefined, args.callee.args);
+            } else {
+                config.on.success.apply(undefined, args.callee.args);
+            }
+        };
+        if (Y.Lang.isValue(args.callee.halt) && args.callee.halt) {
+            args.callee.resume = do_action;
         } else {
-            config.on.success.apply(undefined, args.callee.args);
+            do_action();
         }
     };
     // DELETE uses Y.io directly as of this writing, so we cannot stub it
@@ -530,7 +544,7 @@
             select_none.simulate('click');
             Assert.isTrue(test_checked(checkboxes, false));
             // Simulate a click on the select_all control.
-            Y.Event.simulate(Y.Node.getDOMNode(select_all), 'click');
+            select_all.simulate('click');
             Assert.isTrue(test_checked(checkboxes, true));
         },
 
@@ -544,10 +558,10 @@
             var select_none = Y.one('#statuses-selectors > a.select-none');
             Assert.isTrue(test_checked(checkboxes, true));
             // Simulate a click on the select_none control.
-            Y.Event.simulate(Y.Node.getDOMNode(select_none), 'click');
+            select_none.simulate('click');
             Assert.isTrue(test_checked(checkboxes, false));
             // Simulate a click on the select_all control.
-            Y.Event.simulate(Y.Node.getDOMNode(select_all), 'click');
+            select_all.simulate('click');
             Assert.isTrue(test_checked(checkboxes, true));
         }
 
@@ -594,7 +608,7 @@
             overlay = Y.one('#accordion-overlay');
             Assert.isNotNull(overlay);
             submit_button = Y.one('.yui3-lazr-formoverlay-actions button');
-            Y.Event.simulate(Y.Node.getDOMNode(submit_button), 'click');
+            submit_button.simulate('click');
 
             var error_box = Y.one('.yui3-lazr-formoverlay-errors');
             Assert.areEqual(
@@ -602,6 +616,29 @@
                 error_box.get('text'));
         },
 
+        test_spinner_removed_on_error: function() {
+            // The spinner is removed from the submit button after a failure.
+            this.configuration.lp_client.named_post.fail = true;
+            this.configuration.lp_client.named_post.halt = true;
+            this.configuration.lp_client.named_post.args = [true, true];
+            module.setup(this.configuration);
+            module._show_add_overlay(this.configuration);
+            // After the setup the overlay should be in the DOM.
+            overlay = Y.one('#accordion-overlay');
+            Assert.isNotNull(overlay);
+            submit_button = Y.one('.yui3-lazr-formoverlay-actions button');
+            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.
+            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()
+
+            Assert.isTrue(submit_button.hasClass('lazr-pos'));
+            Assert.isFalse(submit_button.hasClass('spinner'));
+        },
+
         test_overlay_error_handling_patching: function() {
             // Verify that errors generated during patching of a filter are
             // displayed to the user.
@@ -617,7 +654,7 @@
             overlay = Y.one('#accordion-overlay');
             Assert.isNotNull(overlay);
             submit_button = Y.one('.yui3-lazr-formoverlay-actions button');
-            Y.Event.simulate(Y.Node.getDOMNode(submit_button), 'click');
+            submit_button.simulate('click');
 
             // Put this stubbed function back.
             module._delete_filter = original_delete_filter;
@@ -927,7 +964,7 @@
             Assert.isTrue(module.lp_client.patch_called);
         },
 
-        test_simple_add_workflow_cancled: function() {
+        test_simple_add_workflow_canceled: function() {
             // Clicking on the "Subscribe to bug mail" link and then clicking
             // on the overlay form's cancel button results in no filter being
             // created or PATCHed.
@@ -1107,6 +1144,26 @@
             module.setup_bug_subscriptions(this.configuration);
             Y.one('a.delete-subscription').simulate('click');
             Assert.isTrue(DELETE_performed);
+        },
+
+        test_unsubscribe_spinner: function () {
+            // The delete link shows a spinner while a deletion is requested.
+            // if the deletion fails, the spinner is removed.
+            var resume;
+            module._Y_io_hook = function (link, config) {
+                resume = function () {
+                    config.on.failure(true, true);
+                };
+            };
+
+            module.setup_bug_subscriptions(this.configuration);
+            var delete_link = Y.one('a.delete-subscription');
+            delete_link.simulate('click');
+            Assert.isTrue(delete_link.hasClass('spinner'));
+            Assert.isFalse(delete_link.hasClass('remove'));
+            resume();
+            Assert.isTrue(delete_link.hasClass('remove'));
+            Assert.isFalse(delete_link.hasClass('spinner'));
         }
 
     }));
@@ -1172,7 +1229,7 @@
                 called_method = true;
             };
             module.setup_subscription_link(this.config, "#link");
-            Y.Event.simulate(Y.Node.getDOMNode(link), 'click');
+            link.simulate('click');
 
             this.wait(function() {
                 Assert.isTrue(called_method);