launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03281
[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);