From 27c14632130d27b199488ffb91199c2565fca3dd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 09:40:37 +0100 Subject: [PATCH] Validate recipients in send a one-off message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It would be annoying to get all the way to the end of the flow and get told that the phone number or email address you entered isn’t valid. So this commit reuses the existing WTForms objects that we have to do some extra validation on the first step in the send one-off message flow. It also accounts for international phone numbers, if the service is allowed to send them. It doesn’t reject other people’s phone numbers if your service is restricted, because I think it’s better to let users play with the feature – it’s good for learning. --- app/main/forms.py | 42 ++++++++++++++++++----- app/main/views/send.py | 1 + tests/app/main/test_placeholder_form.py | 44 +++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 8 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index eb60cc8f4..bf95901b3 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -7,6 +7,7 @@ from notifications_utils.recipients import ( validate_phone_number, InvalidPhoneError ) +from notifications_utils.columns import Columns from wtforms import ( validators, StringField, @@ -102,11 +103,26 @@ class UKMobileNumber(TelField): raise ValidationError(str(e)) -def mobile_number(): - return UKMobileNumber('Mobile number', +class InternationalPhoneNumber(TelField): + def pre_validate(self, form): + try: + validate_phone_number(self.data, international=True) + except InvalidPhoneError as e: + raise ValidationError(str(e)) + + +def mobile_number(label='Mobile number'): + return UKMobileNumber(label, validators=[DataRequired(message='Can’t be empty')]) +def international_phone_number(label='Mobile number'): + return InternationalPhoneNumber( + label, + validators=[DataRequired(message='Can’t be empty')] + ) + + def password(label='Password'): return PasswordField(label, validators=[DataRequired(message='Can’t be empty'), @@ -633,15 +649,25 @@ class PlaceholderForm(Form): def get_placeholder_form_instance( placeholder_name, dict_to_populate_from, - optional_placeholder=False + optional_placeholder=False, + allow_international_phone_numbers=False, ): - PlaceholderForm.placeholder_value = StringField( - placeholder_name, - validators=[ + if Columns.make_key(placeholder_name) == 'emailaddress': + field = email_address(label=placeholder_name, gov_user=False) + elif Columns.make_key(placeholder_name) == 'phonenumber': + if allow_international_phone_numbers: + field = international_phone_number(label=placeholder_name) + else: + field = mobile_number(label=placeholder_name) + elif optional_placeholder: + field = StringField(placeholder_name) + else: + field = StringField(placeholder_name, validators=[ DataRequired(message='Can’t be empty') - ] if not optional_placeholder else [] - ) + ]) + + PlaceholderForm.placeholder_value = field return PlaceholderForm( placeholder_value=dict_to_populate_from.get(placeholder_name, '') diff --git a/app/main/views/send.py b/app/main/views/send.py index b874bc07c..7051094b2 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -245,6 +245,7 @@ def send_test_step(service_id, template_id, step_index): current_placeholder, dict_to_populate_from=get_normalised_send_test_values_from_session(), optional_placeholder=optional_placeholder, + allow_international_phone_numbers=current_service['can_send_international_sms'], ) if form.validate_on_submit(): diff --git a/tests/app/main/test_placeholder_form.py b/tests/app/main/test_placeholder_form.py index 07fac6675..4c6093944 100644 --- a/tests/app/main/test_placeholder_form.py +++ b/tests/app/main/test_placeholder_form.py @@ -1,3 +1,4 @@ +import pytest from app.main.forms import get_placeholder_form_instance from wtforms import Label @@ -16,3 +17,46 @@ def test_form_class_not_mutated(app_): assert str(form1.placeholder_value.label) == '' assert str(form2.placeholder_value.label) == '' + + +@pytest.mark.parametrize('service_can_send_international_sms, placeholder_name, value, expected_error', [ + + (False, 'email address', '', 'Can’t be empty'), + (False, 'email address', '12345', 'Enter a valid email address'), + (False, 'email address', 'test@example.com', None), + (False, 'email address', 'test@example.gov.uk', None), + + (False, 'phone number', '', 'Can’t be empty'), + (False, 'phone number', '+1-2345-678890', 'Not a UK mobile number'), + (False, 'phone number', '07900900123', None), + (False, 'phone number', '+44(0)7900 900-123', None), + + (True, 'phone number', '+123', 'Not enough digits'), + (True, 'phone number', '+44(0)7900 900-123', None), + (True, 'phone number', '+1-2345-678890', None), + + (False, 'anything else', '', 'Can’t be empty'), + +]) +def test_validates_recipients( + app_, + placeholder_name, + value, + service_can_send_international_sms, + expected_error, +): + with app_.test_request_context( + method='POST', + data={'placeholder_value': value} + ): + form = get_placeholder_form_instance( + placeholder_name, + {}, + allow_international_phone_numbers=service_can_send_international_sms, + ) + + if expected_error: + assert not form.validate_on_submit() + assert form.placeholder_value.errors[0] == expected_error + else: + assert form.validate_on_submit()