Give better error messages for incorrect code

If we know the code won’t pass the validation on the API side, we might
as well tell the user before even passing it to the API.

So this commit:
- adds some more validators to the field
- rewrites the validation function on the form to actually call the
  field-level validators before hitting the API 🤦‍♂️
- refactors the tests to be parametrize, which means they can be
  shorter, easier to read, and more comprehensive

app/main/forms.py

@@ -161,11 +161,12 @@ def password(label='Password'):
 
 
 def sms_code():
-    verify_code = '^\d{5}$'
-    return StringField('Text message code',
-                       validators=[DataRequired(message='Can’t be empty'),
-                                   Regexp(regex=verify_code,
-                                          message='Code not found')])
+    return StringField('Text message code', validators=[
+        DataRequired(message='Can’t be empty'),
+        Regexp(regex='^\d+$', message='Numbers only'),
+        Length(min=5, message='Not enough numbers'),
+        Length(max=5, message='Too many numbers'),
+    ])
 
 
 def organisation_type():
@@ -317,10 +318,18 @@ class TwoFactorForm(StripWhitespaceForm):
 
     sms_code = sms_code()
 
-    def validate_sms_code(self, field):
-        is_valid, reason = self.validate_code_func(field.data)
+    def validate(self):
+
+        if not self.sms_code.validate(self):
+            return False
+
+        is_valid, reason = self.validate_code_func(self.sms_code.data)
+
         if not is_valid:
-            raise ValidationError(reason)
+            self.sms_code.errors.append(reason)
+            return False
+
+        return True
 
 
 class EmailNotReceivedForm(StripWhitespaceForm):

tests/app/main/test_two_factor_form.py

@@ -1,82 +1,74 @@
+import pytest
+
 from app import user_api_client
 from app.main.forms import TwoFactorForm
+from tests.conftest import (
+    mock_check_verify_code,
+    mock_check_verify_code_code_expired,
+    mock_check_verify_code_code_not_found,
+)
 
 
+def _check_code(code):
+    return user_api_client.check_verify_code('1', code, "sms")
+
+
+@pytest.mark.parametrize('post_data', [
+    {'sms_code': '12345'},
+    {'sms_code': ' 12345 '},
+])
 def test_form_is_valid_returns_no_errors(
     app_,
     mock_check_verify_code,
+    post_data,
 ):
-    with app_.test_request_context(
-        method='POST',
-        data={'sms_code': '12345'}
-    ):
-        def _check_code(code):
-            return user_api_client.check_verify_code('1', code, "sms")
+    with app_.test_request_context(method='POST', data=post_data):
         form = TwoFactorForm(_check_code)
         assert form.validate() is True
-        assert len(form.errors) == 0
+        assert form.errors == {}
 
 
+@pytest.mark.parametrize('mock, post_data, expected_error', (
+    (
+        mock_check_verify_code,
+        {'sms_code': '1234'},
+        'Not enough numbers',
+    ),
+    (
+        mock_check_verify_code,
+        {'sms_code': '123456'},
+        'Too many numbers',
+    ),
+    (
+        mock_check_verify_code,
+        {},
+        'Can’t be empty',
+    ),
+    (
+        mock_check_verify_code,
+        {'sms_code': '12E45'},
+        'Numbers only',
+    ),
+    (
+        mock_check_verify_code_code_expired,
+        {'sms_code': '99999'},
+        'Code has expired',
+    ),
+    (
+        mock_check_verify_code_code_not_found,
+        {'sms_code': '99999'},
+        'Code not found',
+    ),
+))
 def test_returns_errors_when_code_is_too_short(
     app_,
-    mock_check_verify_code,
+    mocker,
+    mock,
+    post_data,
+    expected_error,
 ):
-    with app_.test_request_context(
-        method='POST',
-        data={'sms_code': '145'}
-    ):
-        def _check_code(code):
-            return user_api_client.check_verify_code('1', code, "sms")
+    mock(mocker)
+    with app_.test_request_context(method='POST', data=post_data):
         form = TwoFactorForm(_check_code)
         assert form.validate() is False
-        assert len(form.errors) == 1
-        assert set(form.errors) == set({'sms_code': ['Code not found', 'Code does not match']})
-
-
-def test_returns_errors_when_code_is_missing(
-    app_,
-    mock_check_verify_code,
-):
-    with app_.test_request_context(
-        method='POST',
-        data={}
-    ):
-        def _check_code(code):
-            return user_api_client.check_verify_code('1', code, "sms")
-        form = TwoFactorForm(_check_code)
-        assert form.validate() is False
-        assert len(form.errors) == 1
-        assert set(form.errors) == set({'sms_code': ['Code must not be empty']})
-
-
-def test_returns_errors_when_code_contains_letters(
-    app_,
-    mock_check_verify_code,
-):
-    with app_.test_request_context(
-        method='POST',
-        data={'sms_code': 'asdfg'}
-    ):
-        def _check_code(code):
-            return user_api_client.check_verify_code('1', code, "sms")
-        form = TwoFactorForm(_check_code)
-        assert form.validate() is False
-        assert len(form.errors) == 1
-        assert set(form.errors) == set({'sms_code': ['Code not found', 'Code does not match']})
-
-
-def test_should_return_errors_when_code_is_expired(
-    app_,
-    mock_check_verify_code_code_expired,
-):
-    with app_.test_request_context(
-        method='POST',
-        data={'sms_code': '23456'}
-    ):
-        def _check_code(code):
-            return user_api_client.check_verify_code('1', code, "sms")
-        form = TwoFactorForm(_check_code)
-        assert form.validate() is False
-        errors = form.errors
-        assert len(errors) == 1
-        assert errors == {'sms_code': ['Code has expired']}
+        assert form.errors == {'sms_code': [expected_error]}