From 4d678aec9379c4cff801fc3d175775f568ef8c21 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 May 2018 21:24:23 +0100 Subject: [PATCH] Give better error messages for incorrect code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 25 +++-- tests/app/main/test_two_factor_form.py | 122 ++++++++++++------------- 2 files changed, 74 insertions(+), 73 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 342a2a6b1..b0d7f5afe 100644 --- a/app/main/forms.py +++ b/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): diff --git a/tests/app/main/test_two_factor_form.py b/tests/app/main/test_two_factor_form.py index 0c87befae..5d8ae0907 100644 --- a/tests/app/main/test_two_factor_form.py +++ b/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]}