Support reporting top-level form validation errors

When we call `form.validate()` and the validation fails, the raised
exception is a `deform.ValidationFailure`. This object has an `error`
attribute which represents the underlying validation error object for
the entire form.

Previously, if the validation failure were a result of top-level
validation errors (such as the fact that an unactivated user is trying
to log in), this error would be lost, because we only reported errors
for the forms fields, or "children".

This commit changes `h.accounts.views.validate_form` so that it converts
the entire `colander.Invalid` object into a dictionary using its
`asdict()` instance method. By doing this, we get two immediate
benefits:

- top-level validation errors are reported in the '' (empty string)
  field
- we avoid the need to aggregate form field errors by hand in ajax_form

In addition, we need to deal with this case on the frontend, so this
commit also changes the formRespond directive so that if no overall
"reason" is provided for failure, then an empty-string member of the
"errors" object can set the overall validation status (and error
message) for the form.

h/accounts/test/views_test.py

@@ -45,7 +45,12 @@ def configure(config):
 
 
 # A fake version of colander.Invalid for use when testing validate_form
-FakeInvalid = namedtuple('FakeInvalid', 'children')
+class FakeInvalid(object):
+    def __init__(self, errors):
+        self.errors = errors
+
+    def asdict(self):
+        return self.errors
 
 
 def test_ajax_form_handles_http_redirect_as_success():
@@ -68,34 +73,36 @@ def test_ajax_form_handles_http_error_as_error():
 
 def test_ajax_form_sets_failure_status_on_errors():
     request = DummyRequest()
-    result = ajax_form(request, {'errors': 'data'})
+    result = ajax_form(request, {'errors': {}})
 
     assert result['status'] == 'failure'
 
 
 def test_ajax_form_sets_status_code_400_on_errors():
     request = DummyRequest()
-    _ = ajax_form(request, {'errors': 'data'})
+    _ = ajax_form(request, {'errors': {}})
 
     assert request.response.status_code == 400
 
 
 def test_ajax_form_sets_status_code_from_input_on_errors():
     request = DummyRequest()
-    _ = ajax_form(request, {'errors': 'data', 'code': 418})
+    _ = ajax_form(request, {'errors': {}, 'code': 418})
 
     assert request.response.status_code == 418
 
 
-def test_ajax_form_aggregates_errors_on_success():
+def test_ajax_form_passes_errors_through_on_errors():
     request = DummyRequest()
-    errors = [
-        {'name': 'Name is too weird'},
-        {'email': 'Email must be @hotmail.com'},
-    ]
+    errors = {
+        '': 'Top level error',
+        'name': 'Name is too weird',
+        'email': 'Email must be @hotmail.com',
+    }
     result = ajax_form(request, {'errors': errors})
 
-    assert result['errors'] == {'name': 'Name is too weird',
+    assert result['errors'] == {'': 'Top level error',
+                                'name': 'Name is too weird',
                                 'email': 'Email must be @hotmail.com'}
 
 
@@ -132,13 +139,13 @@ def test_validate_form_passes_data_to_validate():
 
 
 def test_validate_form_failure():
-    invalid = FakeInvalid(children=object())
+    invalid = FakeInvalid({'': 'Asplode!', 'email': 'No @ sign!'})
     form = MagicMock()
     form.validate.side_effect = deform.ValidationFailure(None, None, invalid)
 
     err, data = validate_form(form, {})
 
-    assert err == {'errors': invalid.children}
+    assert err == {'errors': {'': 'Asplode!', 'email': 'No @ sign!'}}
     assert data is None
 
 

h/accounts/views.py

@@ -1,7 +1,6 @@
 # -*- coding: utf-8 -*-
 import json
 
-import colander
 import deform
 import horus.events
 import horus.views
@@ -33,19 +32,9 @@ def ajax_form(request, result):
     elif isinstance(result, httpexceptions.HTTPError):
         request.response.status_code = result.code
         result = {'status': 'failure', 'reason': str(result)}
-    else:
-        errors = result.pop('errors', None)
-        if errors is not None:
-            status_code = result.pop('code', 400)
-            request.response.status_code = status_code
-            result['status'] = 'failure'
-
-            result.setdefault('errors', {})
-            for e in errors:
-                if isinstance(e, colander.Invalid):
-                    result['errors'].update(e.asdict())
-                elif isinstance(e, dict):
-                    result['errors'].update(e)
+    elif 'errors' in result:
+        request.response.status_code = result.pop('code', 400)
+        result['status'] = 'failure'
 
     result['flash'] = session.pop_flash(request)
 
@@ -57,7 +46,7 @@ def validate_form(form, data):
     try:
         appstruct = form.validate(data)
     except deform.ValidationFailure as err:
-        return {'errors': err.error.children}, None
+        return {'errors': err.error.asdict()}, None
     else:
         return None, appstruct
 

h/static/scripts/form-respond.coffee

@@ -4,9 +4,17 @@
 # will contain the API error message.
 module.exports = ->
   (form, errors, reason) ->
+    form.$setValidity('response', !reason)
+    form.responseErrorMessage = reason
+
     for own field, error of errors
+      # If there's an empty-string field, it's a top-level form error. Set the
+      # overall form validity from this field, but only if there wasn't already
+      # a reason.
+      if !reason and field == ''
+        form.$setValidity('response', false)
+        form.responseErrorMessage = error
+        continue
+
       form[field].$setValidity('response', false)
       form[field].responseErrorMessage = error
-
-    form.$setValidity('response', !reason)
-    form.responseErrorMessage = reason

h/static/scripts/test/form-respond-test.coffee

@@ -38,6 +38,14 @@ describe 'form-respond', ->
     assert.equal(form.username.responseErrorMessage, 'must be at least 3 characters')
     assert.equal(form.password.responseErrorMessage, 'must be present')
 
+  it 'sets the "response" error key if the form has a top-level error', ->
+    formRespond form, {'': 'Explosions!'}
+    assert.calledWith(form.$setValidity, 'response', false)
+
+  it 'adds an error message if the form has a top-level error', ->
+    formRespond form, {'': 'Explosions!'}
+    assert.equal(form.responseErrorMessage, 'Explosions!')
+
   it 'sets the "response" error key if the form has a failure reason', ->
     formRespond form, null, 'fail'
     assert.calledWith(form.$setValidity, 'response', false)