launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06867
lp:~wallyworld/launchpad/register-merge-validation-errors-961126 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/register-merge-validation-errors-961126 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #961126 in Launchpad itself: "+register-merge JS doesn't handle form validation errors"
https://bugs.launchpad.net/launchpad/+bug/961126
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/register-merge-validation-errors-961126/+merge/99009
== Implementation ==
Add javascript form error handling which mimics what is done for server side rendering using TAL templates. There was no infrastructure to do this so I invented some. This allows new forms which submit their requests to the server using XHR (like the merge proposal page) to correctly highlight field errors etc if the server says there is an issue.
The LaunchpadFormView class has a new ajax_failure_handler method. This can be plugged in as required for each action:
@action('Propose Merge', name='register',
failure=LaunchpadFormView.ajax_failure_handler)
def register_action(self, action, data):
xxxxx
If the standard form validation checks result in any errors (form wide or field related), this information is packaged up into a json data structure and assigned to form_result. This cases the form to send the error data to the client (with a status code of 400 and status reason of 'Validation') instead of doing the rendering. The XHR caller then uses a FormErrorHandler class to render the error info.
Form actions can also return their own bespoke error info as is done for the Propose Merge action in RegisterBranchMergeProposalView.
On the client side of things, the existing javascript ErrorHandler and FormErrorHandler classes have been extended. They are now proper YUI objects extending Y.Base (to allow easier instantiation with specified attributes). And the form error rendering code has been added.
== Demo and QA ==
Open a register merge proposal page. Hit Propose Merge without typing anything into the form. A "Required Input Missing" field should be displayed.
== Tests ==
Add test to test_launchpadform to check the basic infrastructure.
Add tests to TestRegisterBranchMergeProposalView to ensure correct errors are returned.
Add tests to test_lp_client to test the javascript side of things.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/browser/launchpadform.py
lib/lp/app/browser/tests/test_launchpadform.py
lib/lp/app/javascript/client.js
lib/lp/app/javascript/tests/test_lp_client.html
lib/lp/app/javascript/tests/test_lp_client.js
lib/lp/code/browser/branch.py
lib/lp/code/browser/tests/test_branchmergeproposal.py
lib/lp/code/javascript/branchmergeproposal.nominate.js
lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js
--
https://code.launchpad.net/~wallyworld/launchpad/register-merge-validation-errors-961126/+merge/99009
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/register-merge-validation-errors-961126 into lp:launchpad.
=== modified file 'lib/lp/app/browser/launchpadform.py'
--- lib/lp/app/browser/launchpadform.py 2011-12-24 17:49:30 +0000
+++ lib/lp/app/browser/launchpadform.py 2012-03-23 13:05:23 +0000
@@ -345,6 +345,29 @@
else:
return 'There are %d errors.' % count
+ def ajax_failure_handler(self, action, data, errors):
+ """Called by the form if validate() finds any errors.
+
+ For ajax requests the standard Launchpad form template is not available
+ to render any errors. We simply convert the errors to json and return
+ that data to the caller so the errors can be rendered.
+ """
+
+ if not self.request.is_ajax:
+ return
+ self.request.response.setStatus(400, "Validation")
+ self.request.response.setHeader('Content-type', 'application/json')
+ errors = {}
+ for widget in self.widgets:
+ widget_error = self.getFieldError(widget.context.getName())
+ if widget_error:
+ errors[widget.name] = widget_error
+ return_data = dict(
+ form_wide_errors=self.form_wide_errors,
+ errors=errors,
+ error_summary=self.error_count)
+ return simplejson.dumps(return_data)
+
def validate(self, data):
"""Validate the form.
=== modified file 'lib/lp/app/browser/tests/test_launchpadform.py'
--- lib/lp/app/browser/tests/test_launchpadform.py 2012-01-01 02:58:52 +0000
+++ lib/lp/app/browser/tests/test_launchpadform.py 2012-03-23 13:05:23 +0000
@@ -11,8 +11,10 @@
)
from lxml import html
+import simplejson
from testtools.content import text_content
from z3c.ptcompat import ViewPageTemplateFile
+from zope.formlib.form import action
from zope.interface import Interface
from zope.schema import (
Choice,
@@ -198,3 +200,56 @@
"field.multi_line", "hidden",
"field.checkbox", "hidden"],
root.xpath("//input/@id | //input/@type"))
+
+
+class TestFormView(TestWidgetDivView):
+ """A trivial view with an action and a validator which sets errors."""
+ @action('Test', name='test',
+ failure=LaunchpadFormView.ajax_failure_handler)
+ def test_action(self, action, data):
+ pass
+
+ def validate(self, data):
+ single_line_value = data['single_line']
+ if single_line_value == 'success':
+ return
+ self.setFieldError('single_line', 'An error occurred')
+ self.addError('A form error')
+
+
+class TestAjaxValidator(TestCase):
+ # For ajax requests to form views, when the validators record errors as
+ # having occurred, the form returns json data which contains information
+ # about the errors.
+
+ layer = FunctionalLayer
+
+ def test_ajax_failure_handler(self):
+ # Validation errors are recorded properly.
+ extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
+ request = LaunchpadTestRequest(
+ method='POST',
+ form={
+ 'field.actions.test': 'Test',
+ 'field.single_line': 'error'},
+ **extra)
+ view = TestFormView({}, request)
+ view.initialize()
+ self.assertEqual(
+ {"error_summary": "There are 2 errors.",
+ "errors": {"field.single_line": "An error occurred"},
+ "form_wide_errors": ["A form error"]},
+ simplejson.loads(view.form_result))
+
+ def test_ajax_action_success(self):
+ # When there are no errors, form_result is None.
+ extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
+ request = LaunchpadTestRequest(
+ method='POST',
+ form={
+ 'field.actions.test': 'Test',
+ 'field.single_line': 'success'},
+ **extra)
+ view = TestFormView({}, request)
+ view.initialize()
+ self.assertIsNone(view.form_result)
=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js 2012-03-21 01:18:06 +0000
+++ lib/lp/app/javascript/client.js 2012-03-23 13:05:23 +0000
@@ -859,10 +859,11 @@
*
* @class ErrorHandler
*/
-var ErrorHandler = function () {
-};
+function ErrorHandler(config) {
+ ErrorHandler.superclass.constructor.apply(this, arguments);
+}
-ErrorHandler.prototype = {
+Y.extend(ErrorHandler, Y.Base, {
/**
* Clear the progress indicator.
*
@@ -947,36 +948,109 @@
get_generic_error: function(response) {
return response.responseText;
}
-};
+});
module.ErrorHandler = ErrorHandler;
-var FormErrorHandler = function() {};
-
-
-FormErrorHandler.prototype = new ErrorHandler();
-
-
-FormErrorHandler.prototype.get_oops_id = function(response) {
- var oops_re = /code class\="oopsid">(OOPS-[^<]*)/;
- var result = response.responseText.match(oops_re);
- if (result === null) {
- return null;
- }
- return result[1];
-};
-
-
-FormErrorHandler.prototype.get_generic_error = function(response) {
- if (response.status !== 403){
- return "Sorry, you don't have permission to make this change.";
- }
- else {
- return response.status + ' ' + response.statusText;
- }
-};
-
+function FormErrorHandler(config) {
+ FormErrorHandler.superclass.constructor.apply(this, arguments);
+}
+
+FormErrorHandler.ATTRS = {
+ form: {
+ value: null
+ }
+};
+
+Y.extend(FormErrorHandler, ErrorHandler, {
+
+ // Clear any errors on the form.
+ clearFormErrors: function() {
+ Y.all('.error.message').remove(true);
+ Y.all('.error .message').remove(true);
+ Y.all('div.error').removeClass('error');
+ },
+
+ // If the XHR call returns a form validation error, we display the errors
+ // on the form.
+ handleError: function(ioId, response) {
+ if (response.status === 400
+ && response.statusText === 'Validation') {
+ var response_info = Y.JSON.parse(response.responseText);
+ var error_summary = response_info.error_summary;
+ var form_wide_errors = response_info.form_wide_errors;
+ var errors = response_info.errors;
+ this.handleFormValidationError(
+ error_summary, form_wide_errors, errors);
+ return true;
+ }
+ return false;
+ },
+
+ // Display the specified errors on the form. The errors are displayed in
+ // the same way as is done by the Launchpad HTML form rendering
+ // infrastructure using TAL templates.
+ handleFormValidationError: function(error_summary,
+ form_wide_errors, errors) {
+ var form = this.get('form');
+ if (!Y.Lang.isValue(form)) {
+ form = Y.one("[name='launchpadform']");
+ }
+ if (!Y.Lang.isValue(form)) {
+ return;
+ }
+ var form_content = form.one('table.form');
+ if (!Y.Lang.isValue(form_content)) {
+ form_content = form;
+ }
+ // Display the error summary information.
+ var error_summary_node =
+ Y.Node.create('<p class="error message"></p>')
+ .setContent(error_summary);
+ form_content.insertBefore(error_summary_node, form_content);
+ // Display the form wide errors.
+ if (form_wide_errors.length > 0) {
+ var form_error_node =
+ Y.Node.create('<p class="error message"></p>');
+ Y.Array.each(form_wide_errors, function(message) {
+ form_error_node.appendChild(Y.Node.create('<p></p>')
+ .setContent(message));
+ });
+ form_content.insertBefore(form_error_node, form_content);
+ }
+ // Display the field specific errors.
+ Y.each(errors, function(message, field_name) {
+ var label = Y.one('label[for=' + field_name + ']');
+ if (Y.Lang.isValue(label)) {
+ label.ancestor('div').addClass('error');
+ var field = label.next('div');
+ var error_node =
+ Y.Node.create('<div class="message"></div>')
+ .setContent(message);
+ field.insert(error_node, 'after');
+ }
+ });
+ },
+
+ get_oops_id: function(response) {
+ var oops_re = /code class\="oopsid">(OOPS-[^<]*)/;
+ var result = response.responseText.match(oops_re);
+ if (result === null) {
+ return null;
+ }
+ return result[1];
+ },
+
+ get_generic_error: function(response) {
+ if (response.status !== 403){
+ return "Sorry, you don't have permission to make this change.";
+ }
+ else {
+ return response.status + ' ' + response.statusText;
+ }
+ }
+});
module.FormErrorHandler = FormErrorHandler;
=== modified file 'lib/lp/app/javascript/tests/test_lp_client.html'
--- lib/lp/app/javascript/tests/test_lp_client.html 2012-03-14 04:41:36 +0000
+++ lib/lp/app/javascript/tests/test_lp_client.html 2012-03-23 13:05:23 +0000
@@ -52,5 +52,18 @@
<div id="container-of-stuff">
</div>
</div>
+ <form id="testform">
+ <table class="form">
+ <tbody><tr><td colspan="2">
+ <div id="field_div">
+ <label for="field.test">Field:</label>
+ <div>
+ <input id="field.test" type="text"/>
+ </div>
+ <p class="formHelp">Help.</p>
+ </div>
+ </td>
+ </tr></tbody></table>
+ </form>
</body>
</html>
=== modified file 'lib/lp/app/javascript/tests/test_lp_client.js'
--- lib/lp/app/javascript/tests/test_lp_client.js 2011-11-17 21:24:10 +0000
+++ lib/lp/app/javascript/tests/test_lp_client.js 2012-03-23 13:05:23 +0000
@@ -450,7 +450,67 @@
this._checkNotificationNode('.debug.message', 'A debug');
this._checkNotificationNode('.informational.message', 'An info');
}
-
+}));
+
+suite.add(new Y.Test.Case({
+ name: "lp.client.forms",
+
+ setUp: function() {
+ var form = Y.one("#testform");
+ this.error_handler = new Y.lp.client.FormErrorHandler({
+ form: form
+ });
+ },
+
+ tearDown: function() {
+ this.error_handler.clearFormErrors();
+ },
+
+ test_form_error_handler_ignores_other_responses: function() {
+ // Only XHR responses not containing validation data are ignored.
+ var result = this.error_handler.handleError(0, {
+ status: 400,
+ statusText: 'Not Validation'
+ });
+ Y.Assert.isFalse(result);
+ },
+
+ test_form_error_handler_handles_responses: function() {
+ // XHR responses containing validation data are processed.
+ var error_data = {
+ 'error_summary': 'Some errors',
+ 'form_wide_errors': ['Form error'],
+ errors: {'field.test': 'Field error'}
+ };
+ var result = this.error_handler.handleError(0, {
+ status: 400,
+ statusText: 'Validation',
+ responseText: Y.JSON.stringify(error_data)
+ });
+ Y.Assert.isTrue(result);
+ this._assert_error_rendering();
+ },
+
+ _assert_error_rendering: function() {
+ var label = Y.one('label[for="field.test"]');
+ var field_error = label.next('div').next('.message');
+ Y.Assert.isTrue(Y.one('#field_div').hasClass('error'));
+ Y.Assert.areEqual('Field error', field_error.getContent());
+ Y.all('p.error.message').each(function(error_node) {
+ var error_message = error_node.getContent();
+ Y.Assert.isTrue(
+ error_message === '<p>Form error</p>' ||
+ error_message === 'Some errors');
+ });
+ },
+
+ test_form_error_handler_renders_errors: function() {
+ // Form errors are rendered correctly.
+ this.error_handler.handleFormValidationError(
+ "Some errors", ["Form error"],
+ {'field.test': "Field error"});
+ this._assert_error_rendering();
+ }
}));
Y.lp.testing.Runner.run(suite);
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py 2012-03-22 23:21:24 +0000
+++ lib/lp/code/browser/branch.py 2012-03-23 13:05:23 +0000
@@ -1256,7 +1256,8 @@
raise NotFound(self.context, '+register-merge')
LaunchpadFormView.initialize(self)
- @action('Propose Merge', name='register')
+ @action('Propose Merge', name='register',
+ failure=LaunchpadFormView.ajax_failure_handler)
def register_action(self, action, data):
"""Register the new branch merge proposal."""
@@ -1279,6 +1280,7 @@
self.user, reviewer, branch_names)
visible_branches = list(visibility_info['visible_branches'])
if self.request.is_ajax and len(visible_branches) < 2:
+ self.request.response.setStatus(400, "Branch Visibility")
self.request.response.setHeader(
'Content-Type', 'application/json')
return simplejson.dumps({
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2012-02-14 03:49:35 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2012-03-23 13:05:23 +0000
@@ -488,8 +488,40 @@
{'target_branch': target_branch,
'reviewer': reviewer,
'needs_review': True})
+ self.assertEqual(
+ '400 Branch Visibility',
+ view.request.response.getStatusString())
self.assertEqual(expected_data, simplejson.loads(result_data))
+ def test_register_ajax_request_with_validation_errors(self):
+ # Ajax submits where there is a validation error in the submitted data
+ # return the expected json response containing the error info.
+ owner = self.factory.makePerson()
+ target_branch = self._makeTargetBranch(owner=owner, private=True)
+ extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
+ with person_logged_in(owner):
+ request = LaunchpadTestRequest(
+ method='POST', principal=owner,
+ form={
+ 'field.actions.register': 'Propose Merge',
+ 'field.target_branch.target_branch':
+ target_branch.unique_name},
+ **extra)
+ view = create_initialized_view(
+ target_branch,
+ name='+register-merge',
+ request=request)
+ self.assertEqual(
+ '400 Validation', view.request.response.getStatusString())
+ self.assertEqual(
+ {'error_summary': 'There is 1 error.',
+ 'errors': {
+ 'field.target_branch':
+ ('The target branch cannot be the same as the '
+ 'source branch.')},
+ 'form_wide_errors': []},
+ simplejson.loads(view.form_result))
+
def test_register_ajax_request_with_no_confirmation(self):
# Ajax submits where there is no confirmation required return a 201
# with the new location.
=== modified file 'lib/lp/code/javascript/branchmergeproposal.nominate.js'
--- lib/lp/code/javascript/branchmergeproposal.nominate.js 2012-03-15 05:40:30 +0000
+++ lib/lp/code/javascript/branchmergeproposal.nominate.js 2012-03-23 13:05:23 +0000
@@ -144,7 +144,7 @@
*/
var _showSubmitSpinner = function(submit_link) {
var spinner_node = Y.Node.create(
- '<img class="spinner" src="/@@/spinner" alt="Deleting..." />');
+ '<img class="spinner" src="/@@/spinner" alt="Submitting..." />');
submit_link.insert(spinner_node, 'after');
};
@@ -176,24 +176,38 @@
*/
var setup_nominate_submit = function(io_provider) {
Y.lp.client.remove_notifications();
- var error_handler = new Y.lp.client.ErrorHandler();
+ var form = Y.one("[name='launchpadform']");
+ var error_handler = new Y.lp.client.FormErrorHandler({
+ form: form
+ });
var submit_link = Y.one("[name='field.actions.register']");
error_handler.showError = Y.bind(function (error_msg) {
_hideSubmitSpinner(submit_link);
Y.lp.app.errors.display_error(undefined, error_msg);
}, this);
+ var self = this;
+ error_handler.handleError = Y.bind(function(id, response) {
+ if( response.status === 400
+ && response.statusText === 'Branch Visibility') {
+ var response_info = Y.JSON.parse(response.responseText);
+ namespace.confirm_reviewer_nomination(response_info);
+ return true;
+ }
+ return error_handler.constructor.prototype.handleError(id, response);
+ }, this);
var base_url = LP.cache.context.web_link;
var submit_url = base_url + "/+register-merge";
- var form = Y.one("[name='launchpadform']");
form.on('submit', function(e) {
e.preventDefault();
var y_config = {
method: "POST",
headers: {'Accept': 'application/json;'},
on: {
- start:
- Y.bind(_showSubmitSpinner, namespace, submit_link),
+ start: Y.bind(function() {
+ error_handler.clearFormErrors();
+ _showSubmitSpinner(submit_link);
+ }),
end:
Y.bind(_hideSubmitSpinner, namespace, submit_link),
failure: error_handler.getFailureHandler(),
@@ -202,10 +216,6 @@
if (response.status === 201) {
namespace._redirect(
response.getResponseHeader("Location"));
- } else {
- var branch_info =
- Y.JSON.parse(response.responseText);
- namespace.confirm_reviewer_nomination(branch_info);
}
}
}
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js 2012-02-14 20:44:01 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js 2012-03-23 13:05:23 +0000
@@ -191,7 +191,7 @@
// Test that the Propose Merge submit button is re-wired to perform an XHR
// call and that the correct data is passed and the expected callback is
- // invoked wit hthe returned data.
+ // invoked with the returned data.
test_setup_nominate_submit: function() {
var orig_confirm_reviewer_nomination
= module.confirm_reviewer_nomination;
@@ -219,7 +219,9 @@
field.get('value'),
self.mockio.last_request.config.form[field.get('name')]);
});
- this.mockio.success({
+ this.mockio.respond({
+ status: 400,
+ statusText: 'Branch Visibility',
responseText: '{"person_name": "Fred"}',
responseHeaders: {'Content-Type': 'application/json'}});
Y.Assert.isTrue(confirm_reviewer_nomination_called);
