launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06868
[Merge] lp:~jtv/maas/bug-954653 into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-954653 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #954653 in MAAS: ""Unable to create node" is a useless error message"
https://bugs.launchpad.net/maas/+bug/954653
For more details, see:
https://code.launchpad.net/~jtv/maas/bug-954653/+merge/99029
Actually by far most of the time went into figuring out why I couldn't make the test break before starting work (it used the wrong variable in its assertion); figuring out what some of the code was meant to do exactly (so you'll see some documentation for pre-existing code show up here); and testing previously untested code (for distributing field errors among form fields).
I also factored out some tedious commonality in the tests. They're a bit easier to follow now, as well as easier to write. Nor did I see any tests for the HTML-escaping of the error strings, so I added one.
Jeroen
--
https://code.launchpad.net/~jtv/maas/bug-954653/+merge/99029
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-954653 into lp:maas.
=== modified file 'src/maasserver/static/js/node_add.js'
--- src/maasserver/static/js/node_add.js 2012-03-15 13:58:32 +0000
+++ src/maasserver/static/js/node_add.js 2012-03-23 14:26:19 +0000
@@ -87,6 +87,12 @@
this.get('srcNode').all('div.field-error').remove();
},
+ /* Display validation errors on their respective fields.
+ *
+ * The "errors" argument is an object. If a field has validation errors,
+ * this object will map the field's name to a list of error strings. Each
+ * field's errors will be shown with the label for that field.
+ */
displayFieldErrors: function(errors) {
this.cleanFormErrors();
var key;
@@ -227,15 +233,21 @@
self.hidePanel();
},
failure: function(id, out) {
+ Y.log("Adding a node failed. Response object follows.")
Y.log(out);
if (out.status === 400) {
try {
- // Validation error: display the errors in the
- // form.
+ /* Validation error: display the errors in the
+ * form next to their respective fields.
+ */
self.displayFieldErrors(JSON.parse(out.response));
}
catch (e) {
- self.displayFormError("Unable to create Node.");
+ Y.log(
+ "Exception while decoding error JSON: " +
+ e.message);
+ self.displayFormError(
+ "Unable to create Node: " + out.responseText);
}
}
else if (out.status === 401) {
@@ -244,7 +256,8 @@
}
else {
// Unexpected error.
- self.displayFormError("Unable to create Node.");
+ self.displayFormError(
+ "Unable to create Node: " + out.responseText);
}
},
end: Y.bind(self.hideSpinner, self)
=== modified file 'src/maasserver/static/js/tests/test_node_add.js'
--- src/maasserver/static/js/tests/test_node_add.js 2012-03-15 13:58:32 +0000
+++ src/maasserver/static/js/tests/test_node_add.js 2012-03-23 14:26:19 +0000
@@ -50,14 +50,72 @@
}));
+
+/* Find the add-node widget.
+ */
+function find_widget() {
+ return module._add_node_singleton.get('srcNode');
+}
+
+
+/* Find the add-node form.
+ */
+function find_form() {
+ return find_widget().one('form');
+}
+
+
+/* Find the hostname input field in the add-node form.
+ */
+function find_hostname_input() {
+ return find_widget().one('#id_hostname');
+}
+
+
+/* Find the "Add node" button at the bottom of the add-node form.
+ */
+function find_add_button() {
+ return find_widget().one('.yui3-button');
+}
+
+
+/* Find the global errors panel at the top of the add-node form.
+ */
+function find_global_errors() {
+ return find_widget().one('.form-global-errors');
+}
+
+
+/* Set up and submit the add-node form.
+ */
+function submit_add_node() {
+ module.showAddNodeWidget();
+ find_hostname_input().set('value', 'host');
+ find_add_button().simulate('click');
+}
+
+
suite.add(new Y.maas.testing.TestCase({
name: 'test-add-node-widget-add-node',
+ /* Set up a mock failure for adding a node.
+ *
+ * When the form tries to send an add-node request, the request's failure
+ * handler will be called with the fake response you provide.
+ */
+ mock_failure: function(fake_response) {
+ var mockXhr = new Y.Base();
+ mockXhr.send = function(url, cfg) {
+ /* The 3 is a bogus transaction id. */
+ cfg.on.failure(3, fake_response);
+ };
+ this.mockIO(mockXhr, module);
+ },
+
testFormContainsArchitectureChoice: function() {
// The generated form contains an 'architecture' field.
module.showAddNodeWidget();
- var panel = module._add_node_singleton;
- var arch = panel.get('srcNode').one('form').one('#id_architecture');
+ var arch = find_form().one('#id_architecture');
Y.Assert.isNotNull(arch);
var arch_options = arch.all('option');
Y.Assert.areEqual(2, arch_options.size());
@@ -67,18 +125,16 @@
// The call to the API triggered by clicking on 'Add a node'
// submits (via an API call) the panel's form.
module.showAddNodeWidget();
- var panel = module._add_node_singleton;
var mockXhr = new Y.Base();
var fired = false;
- var form = panel.get('srcNode').one('form');
+ var form = find_form();
mockXhr.send = function(uri, cfg) {
fired = true;
Y.Assert.areEqual(form, cfg.form);
};
this.mockIO(mockXhr, module);
- panel.get('srcNode').one('#id_hostname').set('value', 'host');
- var button = panel.get('srcNode').one('.yui3-button');
- button.simulate('click');
+ find_hostname_input().set('value', 'host');
+ find_add_button().simulate('click');
Y.Assert.isTrue(fired);
},
@@ -90,10 +146,8 @@
});
this.mockIO(mockXhr, module);
module.showAddNodeWidget();
- var panel = module._add_node_singleton;
- panel.get('srcNode').one('#id_hostname').set('value', 'host');
- var button = panel.get('srcNode').one('.yui3-button');
- button.simulate('click');
+ find_hostname_input().set('value', 'host');
+ find_add_button().simulate('click');
Y.Mock.verify(mockXhr);
},
@@ -105,11 +159,9 @@
});
this.mockIO(mockXhr, module);
module.showAddNodeWidget();
- var panel = module._add_node_singleton;
- panel.get('srcNode').one('#id_hostname').set('value', 'host');
- var form = panel.get('srcNode').one('form');
+ find_hostname_input().set('value', 'host');
// Simulate 'Enter' being pressed.
- form.simulate("keypress", { keyCode: 13 });
+ find_form().simulate("keypress", { keyCode: 13 });
Y.Mock.verify(mockXhr);
},
@@ -124,9 +176,8 @@
Y.bind(
module._add_node_singleton.destroy,
module._add_node_singleton));
- var panel = module._add_node_singleton;
- panel.get('srcNode').one('#id_hostname').set('value', 'host');
- var button = panel.get('srcNode').one('.yui3-button');
+ find_hostname_input().set('value', 'host');
+ var button = find_add_button();
var fired = false;
this.registerListener(
@@ -140,39 +191,48 @@
Y.Assert.isTrue(fired);
},
- testGenericErrorMessage: function() {
- var mockXhr = new Y.Base();
- mockXhr.send = function(url, cfg) {
- cfg.on.failure(3, {status: 500});
- };
- this.mockIO(mockXhr, module);
- module.showAddNodeWidget();
- var panel = module._add_node_singleton;
- panel.get('srcNode').one('#id_hostname').set('value', 'host');
- var button = panel.get('srcNode').one('.yui3-button');
- button.simulate('click');
- var error_message = panel.get(
- 'srcNode').one('.form-global-errors').get('innerHTML');
- var message_position = error_message.search("Unable to create Node.");
- Y.Assert.areNotEqual(-1, error_message);
+ testValidationErrorInJSONGoesToFieldsNotGlobalErrors: function() {
+ this.mock_failure({
+ status: 400,
+ response: '{"architecture": ["Xur."]}'
+ });
+ submit_add_node();
+ Y.Assert.areEqual(
+ -1, find_global_errors().get('innerHTML').search("Xur."));
+ var field_label = find_widget().one('label[for="id_architecture"]');
+ var error_node = field_label.next();
+ Y.Assert.areNotEqual(-1, error_node.get('innerHTML').search("Xur."));
+ },
+
+ test400ErrorMessageWithPlainText: function() {
+ this.mock_failure({status: 400, responseText: "Blergh."});
+ submit_add_node();
+ var error_message = find_global_errors().get('innerHTML');
+ Y.Assert.areNotEqual(-1, error_message.search("Blergh."));
},
testLoggedOffErrorMessage: function() {
- var mockXhr = new Y.Base();
- mockXhr.send = function(url, cfg) {
- cfg.on.failure(3, {status: 401});
- };
- this.mockIO(mockXhr, module);
- module.showAddNodeWidget();
- var panel = module._add_node_singleton;
- panel.get('srcNode').one('#id_hostname').set('value', 'host');
- var button = panel.get('srcNode').one('.yui3-button');
- button.simulate('click');
- var error_message = panel.get(
- 'srcNode').one('.form-global-errors').get('innerHTML');
+ this.mock_failure({status: 401});
+ submit_add_node();
+ var error_message = find_global_errors().get('innerHTML');
// The link to the login page is present in the error message.
var link_position = error_message.search(MAAS_config.uris.login);
Y.Assert.areNotEqual(-1, link_position);
+ },
+
+ testGenericErrorMessage: function() {
+ this.mock_failure({status: 500, responseText: "Internal error."});
+ submit_add_node();
+ var error_message = find_global_errors().get('innerHTML');
+ Y.Assert.areNotEqual(-1, error_message.search("Internal error."));
+ },
+
+ testErrorsAreEscaped: function() {
+ this.mock_failure({status: 400, responseText: "<huh>"});
+ submit_add_node();
+ var error_message = find_global_errors().get('innerHTML');
+ Y.Assert.areEqual(-1, error_message.search("<huh>"));
+ Y.Assert.areNotEqual(-1, error_message.search("<huh>"));
}
}));
