From faa3b9ca7c6eb8fea579c85a21159388712f8080 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 Jan 2016 18:10:16 +0000 Subject: [PATCH 1/3] Add form field for a UK mobile phone number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This field does two things: - validates the format of the phone number - outputs a consistent representation of the phone number Because of this I think it’s better represented as a new field type, rather than individual validators. I also think that it’s better to do this without regular expression(s), because it makes returning the specific error easier. This commit also adds basic pass/fail test for a series of valid/invalid phone numbers. --- app/main/forms.py | 40 +++++++++++ .../app/main/test_phone_number_form_field.py | 68 +++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 tests/app/main/test_phone_number_form_field.py diff --git a/app/main/forms.py b/app/main/forms.py index e4559a164..9259f5c6f 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1,3 +1,4 @@ +import re from flask_wtf import Form from wtforms import ( @@ -24,6 +25,45 @@ def email_address(): Regexp(regex=gov_uk_email, message='Enter a gov.uk email address')]) +class UKMobileNumber(StringField): + + def pre_validate(self, form): + + self.data = self.data.replace('(', '') + self.data = self.data.replace(')', '') + self.data = self.data.replace(' ', '') + self.data = self.data.replace('-', '') + + if self.data.startswith('+'): + self.data = self.data[1:] + + if not sum([ + self.data.startswith(prefix) for prefix in ['07', '447', '4407', '00447'] + ]): + raise ValidationError('Must be a mobile number') + + for digit in self.data: + try: + int(digit) + except(ValueError): + raise ValidationError('Must not contain letters or symbols') + + self.data = self.data.split('7', 1)[1] + + if len(self.data) > 9: + raise ValidationError('Too many digits') + + if len(self.data) < 9: + raise ValidationError('Too few digits') + + def post_validate(self, form, validation_stopped): + + if len(self.data) != 9: + return + + self.data = '+44 7{} {} {}'.format(*re.findall('...', self.data)) + + def mobile_number(): mobile_number_regex = "^\\+44[\\d]{10}$" return StringField('Mobile phone number', diff --git a/tests/app/main/test_phone_number_form_field.py b/tests/app/main/test_phone_number_form_field.py new file mode 100644 index 000000000..a6994fce5 --- /dev/null +++ b/tests/app/main/test_phone_number_form_field.py @@ -0,0 +1,68 @@ +import pytest +from wtforms import Form +from app.main.forms import UKMobileNumber + + +class FormExample(Form): + phone_number = UKMobileNumber() + +phone_numbers = { + 'invalid': [ + # Too long + '0712345678910', + '0044712345678910', + '0044712345678910', + '+44 (0)7123 456 789 10', + # Too short + '0712345678', + '004471234567', + '00447123456', + '+44 (0)7123 456 78', + # Not mobile (from https://fakenumber.org/generator/freephone) + '08081 570364', + '+44 8081 570364', + '0117 496 0860', + '+44 117 496 0860', + '020 7946 0991', + '+44 20 7946 0991', + # Contains non-numbers + '07890x32109', + '07123 456789...', + '07123 ☟☜⬇⬆☞☝', + '07123☟☜⬇⬆☞☝', + '07";DROP TABLE;"', + '+44 07ab cde fgh', + ], + 'valid': [ + '07123456789', + '07123 456789', + '07123-456-789', + '00447123456789', + '00 44 7123456789', + '+447123456789', + '+44 7123 456 789', + '+44 (0)7123 456 789' + ] +} + + +@pytest.mark.parametrize("phone_number", phone_numbers['valid']) +def test_phone_number_accepts_valid_values(phone_number): + form = FormExample(phone_number=phone_number) + form.validate() + assert form.errors == {} + + +@pytest.mark.parametrize("phone_number", phone_numbers['invalid']) +def test_phone_number_rejects_invalid_values(phone_number): + form = FormExample(phone_number=phone_number) + form.validate() + print(phone_number) + assert form.errors != {} + + +@pytest.mark.parametrize("phone_number", phone_numbers['valid']) +def test_phone_number_outputs_in_correct_format(phone_number): + form = FormExample(phone_number=phone_number) + form.validate() + assert form.phone_number.data == '+44 7123 456 789' From 791324588bd4c763c87c937f3e1a5ee1ad3f866f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 12 Jan 2016 20:55:46 +0000 Subject: [PATCH 2/3] Test for specific error messages This commit: - improves the tests to check for specific error messages, rather than just pass/fail - makes the error messages more human, and more suggestive of what the user needs to do to fix the error --- app/main/forms.py | 8 +- .../app/main/test_phone_number_form_field.py | 93 ++++++++++--------- 2 files changed, 54 insertions(+), 47 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 9259f5c6f..1229b61e4 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -37,10 +37,10 @@ class UKMobileNumber(StringField): if self.data.startswith('+'): self.data = self.data[1:] - if not sum([ + if not sum( self.data.startswith(prefix) for prefix in ['07', '447', '4407', '00447'] - ]): - raise ValidationError('Must be a mobile number') + ): + raise ValidationError('Must be a UK mobile number (eg 07700 900460)') for digit in self.data: try: @@ -54,7 +54,7 @@ class UKMobileNumber(StringField): raise ValidationError('Too many digits') if len(self.data) < 9: - raise ValidationError('Too few digits') + raise ValidationError('Not enough digits') def post_validate(self, form, validation_stopped): diff --git a/tests/app/main/test_phone_number_form_field.py b/tests/app/main/test_phone_number_form_field.py index a6994fce5..c7a10583a 100644 --- a/tests/app/main/test_phone_number_form_field.py +++ b/tests/app/main/test_phone_number_form_field.py @@ -6,62 +6,69 @@ from app.main.forms import UKMobileNumber class FormExample(Form): phone_number = UKMobileNumber() -phone_numbers = { - 'invalid': [ - # Too long - '0712345678910', - '0044712345678910', - '0044712345678910', - '+44 (0)7123 456 789 10', - # Too short - '0712345678', - '004471234567', - '00447123456', - '+44 (0)7123 456 78', - # Not mobile (from https://fakenumber.org/generator/freephone) - '08081 570364', - '+44 8081 570364', - '0117 496 0860', - '+44 117 496 0860', - '020 7946 0991', - '+44 20 7946 0991', - # Contains non-numbers - '07890x32109', - '07123 456789...', - '07123 ☟☜⬇⬆☞☝', - '07123☟☜⬇⬆☞☝', - '07";DROP TABLE;"', - '+44 07ab cde fgh', - ], - 'valid': [ - '07123456789', - '07123 456789', - '07123-456-789', - '00447123456789', - '00 44 7123456789', - '+447123456789', - '+44 7123 456 789', - '+44 (0)7123 456 789' +invalid_phone_numbers = sum([ + [ + (phone_number, error) for phone_number in group + ] for error, group in [ + ('Too many digits', ( + '0712345678910', + '0044712345678910', + '0044712345678910', + '+44 (0)7123 456 789 10', + )), + ('Not enough digits', ( + '0712345678', + '004471234567', + '00447123456', + '+44 (0)7123 456 78', + )), + ('Must be a UK mobile number (eg 07700 900460)', ( + '08081 570364', + '+44 8081 570364', + '0117 496 0860', + '+44 117 496 0860', + '020 7946 0991', + '+44 20 7946 0991', + '71234567890', + )), + ('Must not contain letters or symbols', ( + '07890x32109', + '07123 456789...', + '07123 ☟☜⬇⬆☞☝', + '07123☟☜⬇⬆☞☝', + '07";DROP TABLE;"', + '+44 07ab cde fgh', + )) ] -} +], []) + +valid_phone_numbers = [ + '07123456789', + '07123 456789', + '07123-456-789', + '00447123456789', + '00 44 7123456789', + '+447123456789', + '+44 7123 456 789', + '+44 (0)7123 456 789' +] -@pytest.mark.parametrize("phone_number", phone_numbers['valid']) +@pytest.mark.parametrize("phone_number", valid_phone_numbers) def test_phone_number_accepts_valid_values(phone_number): form = FormExample(phone_number=phone_number) form.validate() assert form.errors == {} -@pytest.mark.parametrize("phone_number", phone_numbers['invalid']) -def test_phone_number_rejects_invalid_values(phone_number): +@pytest.mark.parametrize("phone_number, error_message", invalid_phone_numbers) +def test_phone_number_rejects_invalid_values(phone_number, error_message): form = FormExample(phone_number=phone_number) form.validate() - print(phone_number) - assert form.errors != {} + assert form.phone_number.errors[0] == error_message -@pytest.mark.parametrize("phone_number", phone_numbers['valid']) +@pytest.mark.parametrize("phone_number", valid_phone_numbers) def test_phone_number_outputs_in_correct_format(phone_number): form = FormExample(phone_number=phone_number) form.validate() From aa43bd9e752f40e9523a13cdc065f399284639eb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 13 Jan 2016 09:26:38 +0000 Subject: [PATCH 3/3] Add the new field to the application This commit replaces the previous `StringField` used for collecting mobile phone numbers with the `UKMobileNumber` field. This means changing a few of the preexisting tests to have more realistic mobile numbers so that they still pass. --- app/main/forms.py | 6 ++---- tests/app/main/views/test_code_not_received.py | 6 +++--- tests/app/main/views/test_register.py | 6 +++--- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 1229b61e4..0b18751c2 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -65,10 +65,8 @@ class UKMobileNumber(StringField): def mobile_number(): - mobile_number_regex = "^\\+44[\\d]{10}$" - return StringField('Mobile phone number', - validators=[DataRequired(message='Mobile number can not be empty'), - Regexp(regex=mobile_number_regex, message='Enter a +44 mobile number')]) + return UKMobileNumber('Mobile phone number', + validators=[DataRequired(message='Cannot be empty')]) def password(): diff --git a/tests/app/main/views/test_code_not_received.py b/tests/app/main/views/test_code_not_received.py index 367d138b4..af1081af4 100644 --- a/tests/app/main/views/test_code_not_received.py +++ b/tests/app/main/views/test_code_not_received.py @@ -67,7 +67,7 @@ def test_should_check_and_redirect_to_verify(notifications_admin, session['user_email'] = user.email_address verify_codes_dao.add_code(user.id, code='12345', code_type='sms') response = client.post(url_for('main.check_and_resend_text_code'), - data={'mobile_number': '+441234123412'}) + data={'mobile_number': '+447700900460'}) assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) @@ -103,11 +103,11 @@ def test_should_update_mobile_number_resend_code(notifications_admin, session['user_email'] = user.email_address verify_codes_dao.add_code(user_id=user.id, code='12345', code_type='sms') response = client.post(url_for('main.check_and_resend_text_code'), - data={'mobile_number': '+443456789012'}) + data={'mobile_number': '+447700900460'}) assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) updated_user = users_dao.get_user_by_id(user.id) - assert updated_user.mobile_number == '+443456789012' + assert updated_user.mobile_number == '+44 7700 900 460' def test_should_render_verification_code_not_received(notifications_admin, diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index cdf021adc..c779b4c22 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -13,7 +13,7 @@ def test_process_register_creates_new_user(notifications_admin, notifications_ad response = notifications_admin.test_client().post('/register', data={'name': 'Some One Valid', 'email_address': 'someone@example.gov.uk', - 'mobile_number': '+441231231231', + 'mobile_number': '+4407700900460', 'password': 'validPassword!'}) assert response.status_code == 302 assert response.location == 'http://localhost/verify' @@ -31,7 +31,7 @@ def test_process_register_returns_400_when_mobile_number_is_invalid(notification 'password': 'validPassword!'}) assert response.status_code == 200 - assert 'Enter a +44 mobile number' in response.get_data(as_text=True) + assert 'Must be a UK mobile number (eg 07700 900460)' in response.get_data(as_text=True) def test_should_return_400_when_email_is_not_gov_uk(notifications_admin, @@ -55,7 +55,7 @@ def test_should_add_verify_codes_on_session(notifications_admin, notifications_a response = client.post('/register', data={'name': 'Test Codes', 'email_address': 'test_codes@example.gov.uk', - 'mobile_number': '+441234567890', + 'mobile_number': '+4407700900460', 'password': 'validPassword!'}) assert response.status_code == 302 assert 'notify_admin_session' in response.headers.get('Set-Cookie')