From 188ce5e5a77721bcefcfb5acf7fe897b096e571e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 29 Aug 2017 14:52:24 +0100 Subject: [PATCH] =?UTF-8?q?Let=20users=20register=20with=20int=E2=80=99nat?= =?UTF-8?q?ional=20phone=20numbers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Right now Notify restricts you to registering with a UK mobile number. This is because when we built the user registration stuff we couldn’t send to international mobiles. However we can send to international mobile numbers, and it’s totally reasonable to expect employees of the UK government to be working abroad, and have a foreign mobile phone – we’ve heard from one such user. So this commit: - changes all places where users enter their own phone number to use the validation function which allows international phone numbers - renames the `mobile_number` validation to `uk_mobile_number` to make it explicit, and force it to break the tests if there’s somewhere it’s being used that I haven’t thought of --- app/main/forms.py | 14 +++++++------- tests/app/main/views/test_api_keys.py | 5 +++-- .../app/main/views/test_code_not_received.py | 9 +++++++-- tests/app/main/views/test_register.py | 9 ++++++++- tests/app/main/views/test_user_profile.py | 19 ++++++++++++++++--- 5 files changed, 41 insertions(+), 15 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index acdfb5722..1bdbb2ba6 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -114,7 +114,7 @@ class InternationalPhoneNumber(TelField): raise ValidationError(str(e)) -def mobile_number(label='Mobile number'): +def uk_mobile_number(label='Mobile number'): return UKMobileNumber(label, validators=[DataRequired(message='Can’t be empty')]) @@ -156,14 +156,14 @@ class RegisterUserForm(Form): name = StringField('Full name', validators=[DataRequired(message='Can’t be empty')]) email_address = email_address() - mobile_number = mobile_number() + mobile_number = international_phone_number() password = password() class RegisterUserFromInviteForm(Form): name = StringField('Full name', validators=[DataRequired(message='Can’t be empty')]) - mobile_number = mobile_number() + mobile_number = international_phone_number() password = password() service = HiddenField('service') email_address = HiddenField('email_address') @@ -210,7 +210,7 @@ class EmailNotReceivedForm(Form): class TextNotReceivedForm(Form): - mobile_number = mobile_number() + mobile_number = international_phone_number() class ServiceNameForm(Form): @@ -329,7 +329,7 @@ class ChangeEmailForm(Form): class ChangeMobileNumberForm(Form): - mobile_number = mobile_number() + mobile_number = international_phone_number() class ConfirmMobileNumberForm(Form): @@ -587,7 +587,7 @@ class Whitelist(Form): ) phone_numbers = FieldList( - UKMobileNumber( + InternationalPhoneNumber( '', validators=[ Optional() @@ -669,7 +669,7 @@ def get_placeholder_form_instance( if allow_international_phone_numbers: field = international_phone_number(label=placeholder_name) else: - field = mobile_number(label=placeholder_name) + field = uk_mobile_number(label=placeholder_name) elif optional_placeholder: field = StringField(placeholder_name) else: diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index caceb457c..b7a42be59 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -318,7 +318,8 @@ def test_should_update_whitelist( data = OrderedDict([ ('email_addresses-1', 'test@example.com'), ('email_addresses-3', 'test@example.com'), - ('phone_numbers-0', '07900900000') + ('phone_numbers-0', '07900900000'), + ('phone_numbers-2', '+1800-555-555'), ]) response = logged_in_client.post( @@ -328,7 +329,7 @@ def test_should_update_whitelist( mock_update_whitelist.assert_called_once_with(service_id, { 'email_addresses': ['test@example.com', 'test@example.com'], - 'phone_numbers': ['07900900000']}) + 'phone_numbers': ['07900900000', '+1800-555-555']}) def test_should_validate_whitelist_items( diff --git a/tests/app/main/views/test_code_not_received.py b/tests/app/main/views/test_code_not_received.py index eba85d80a..6f7ad700e 100644 --- a/tests/app/main/views/test_code_not_received.py +++ b/tests/app/main/views/test_code_not_received.py @@ -71,12 +71,17 @@ def test_should_render_correct_resend_template_for_pending_user( assert page.find('form').input['value'] == api_user_pending.mobile_number +@pytest.mark.parametrize('phone_number_to_register_with', [ + '+447700900460', + '+1800-555-555', +]) def test_should_resend_verify_code_and_update_mobile_for_pending_user( client, mocker, api_user_pending, mock_update_user, mock_send_verify_code, + phone_number_to_register_with, ): mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_pending) @@ -86,12 +91,12 @@ def test_should_resend_verify_code_and_update_mobile_for_pending_user( 'id': api_user_pending.id, 'email': api_user_pending.email_address} response = client.post(url_for('main.check_and_resend_text_code'), - data={'mobile_number': '+447700900460'}) + data={'mobile_number': phone_number_to_register_with}) assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) mock_update_user.assert_called_once_with(api_user_pending) - mock_send_verify_code.assert_called_once_with(api_user_pending.id, 'sms', to='+447700900460') + mock_send_verify_code.assert_called_once_with(api_user_pending.id, 'sms', to=phone_number_to_register_with) def test_check_and_redirect_to_two_factor_if_user_active( diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index e2f9c7aa3..9c6fd0e25 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -1,3 +1,5 @@ +import pytest + from datetime import datetime from bs4 import BeautifulSoup from unittest.mock import ANY @@ -30,6 +32,10 @@ def test_logged_in_user_redirects_to_choose_service( assert response.location == url_for('main.choose_service', _external=True) +@pytest.mark.parametrize('phone_number_to_register_with', [ + '+4407700900460', + '+1800-555-555', +]) def test_register_creates_new_user_and_redirects_to_continue_page( client, mock_send_verify_code, @@ -38,10 +44,11 @@ def test_register_creates_new_user_and_redirects_to_continue_page( mock_is_email_unique, mock_send_verify_email, mock_login, + phone_number_to_register_with, ): user_data = {'name': 'Some One Valid', 'email_address': 'notfound@example.gov.uk', - 'mobile_number': '+4407700900460', + 'mobile_number': phone_number_to_register_with, 'password': 'validPassword!' } diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index 34d551311..30b1e07c4 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -1,3 +1,4 @@ +import pytest import uuid import json @@ -117,16 +118,23 @@ def test_should_show_mobile_number_page( assert response.status_code == 200 +@pytest.mark.parametrize('phone_number_to_register_with', [ + '+4407700900460', + '+1800-555-555', +]) def test_should_redirect_after_mobile_number_change( logged_in_client, + phone_number_to_register_with, ): - data = {'mobile_number': '07121231234'} + data = {'mobile_number': phone_number_to_register_with} response = logged_in_client.post( url_for('main.user_profile_mobile_number'), data=data) assert response.status_code == 302 assert response.location == url_for( 'main.user_profile_mobile_number_authenticate', _external=True) + with logged_in_client.session_transaction() as session: + assert session['new-mob'] == phone_number_to_register_with def test_should_show_authenticate_after_mobile_number_change( @@ -172,12 +180,17 @@ def test_should_show_confirm_after_mobile_number_change( assert response.status_code == 200 +@pytest.mark.parametrize('phone_number_to_register_with', [ + '+4407700900460', + '+1800-555-555', +]) def test_should_redirect_after_mobile_number_confirm( logged_in_client, mocker, mock_update_user_attribute, mock_check_verify_code, - fake_uuid + fake_uuid, + phone_number_to_register_with, ): user_before = create_user(fake_uuid) user_after = create_user(fake_uuid) @@ -189,7 +202,7 @@ def test_should_redirect_after_mobile_number_confirm( with logged_in_client.session_transaction() as session: session['new-mob-password-confirmed'] = True - session['new-mob'] = '+441234123123' + session['new-mob'] = phone_number_to_register_with session['current_session_id'] = user_before.current_session_id data = {'sms_code': '12345'} response = logged_in_client.post(