From de2886dbaff60140d9909535f123cfcf624829e4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 26 Apr 2017 16:19:34 +0100 Subject: [PATCH 1/3] =?UTF-8?q?Remove=20=E2=80=98human-readable=E2=80=99?= =?UTF-8?q?=20phone=20number=20formatting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This has been removed from utils (so things will break if utils is upgraded without this change isn’t made). I think it’s friendlier to present the phone number as the user entered it anyway – because this is what they think a ‘correct’ phone number representation looks like anyway. --- app/main/views/send.py | 4 +--- tests/app/main/views/test_send.py | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index dd20ff41f..12a717a56 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -60,9 +60,7 @@ def get_example_csv_fields(column_headers, use_example_as_example, submitted_fie def get_example_csv_rows(template, use_example_as_example=True, submitted_fields=False): return { 'email': ['test@example.com'] if use_example_as_example else [current_user.email_address], - 'sms': ['07700 900321'] if use_example_as_example else [validate_and_format_phone_number( - current_user.mobile_number, human_readable=True - )], + 'sms': ['07700 900321'] if use_example_as_example else [current_user.mobile_number], 'letter': [ (submitted_fields or {}).get( key, get_example_letter_address(key) if use_example_as_example else key diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index fe443f138..6c6482d20 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -293,7 +293,7 @@ def test_send_test_sms_message( fake_uuid ): - expected_data = {'data': 'phone number\r\n07700 900 762\r\n', 'file_name': 'Test message'} + expected_data = {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'Test message'} mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') response = logged_in_client.get( @@ -344,7 +344,7 @@ def test_send_test_sms_message_with_placeholders( ): expected_data = { - 'data': 'phone number,name\r\n07700 900 762,Jo\r\n', + 'data': 'phone number,name\r\n07700 900762,Jo\r\n', 'file_name': 'Test message' } mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') From d62fbcb4cb2156ba9e298094a21d08f913d64c33 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Apr 2017 13:51:32 +0100 Subject: [PATCH 2/3] Update utils version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 57464df18..b0ac7ce73 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,4 +28,4 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@16.0.0#egg=notifications-utils==16.0.0 +git+https://github.com/alphagov/notifications-utils.git@16.2.0#egg=notifications-utils==16.2.0 From fd7a34f1e4834549d781e3e7b6e58edfcbf7adc5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 26 Apr 2017 16:19:49 +0100 Subject: [PATCH 3/3] Allow international phone numbers in spreadsheet If a service can send internationally, our CSV validation should not catch valid international phone numbers. This means calling through to code added to utils in: - [ ] https://github.com/alphagov/notifications-utils/pull/156 --- app/main/views/send.py | 4 ++-- tests/app/main/views/test_send.py | 39 +++++++++++++++++++++++++++++++ tests/conftest.py | 9 +++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 12a717a56..1ea5e8bf6 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -218,7 +218,6 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): upload_id=upload_id ) if not letters_as_pdf else None ) - recipients = RecipientCSV( contents, template_type=template.template_type, @@ -228,7 +227,8 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): whitelist=itertools.chain.from_iterable( [user.name, user.mobile_number, user.email_address] for user in users ) if current_service['restricted'] else None, - remaining_messages=remaining_messages + remaining_messages=remaining_messages, + international_sms=current_service['can_send_international_sms'], ) if request.args.get('from_test'): diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 6c6482d20..a4c063a1e 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -9,6 +9,7 @@ import pytest from bs4 import BeautifulSoup from flask import url_for from notifications_utils.template import LetterPreviewTemplate +from notifications_utils.recipients import RecipientCSV from app.main.views.send import get_check_messages_back_url @@ -18,6 +19,8 @@ from tests.conftest import ( mock_get_service_template, mock_get_service_template_with_placeholders, mock_get_service_letter_template, + mock_get_service, + mock_get_international_service, ) template_types = ['email', 'sms'] @@ -424,6 +427,42 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_get_detailed_service_for_today.assert_called_once_with(fake_uuid) +@pytest.mark.parametrize('service_mock, should_allow_international', [ + (mock_get_service, False), + (mock_get_international_service, True), +]) +def test_upload_csvfile_with_international_validates( + mocker, + api_user_active, + logged_in_client, + mock_get_service_template, + mock_s3_upload, + mock_has_permissions, + mock_get_users_by_service, + mock_get_detailed_service_for_today, + fake_uuid, + service_mock, + should_allow_international, +): + + service_mock(mocker, api_user_active) + mocker.patch('app.main.views.send.s3download', return_value='') + mock_recipients = mocker.patch( + 'app.main.views.send.RecipientCSV', + return_value=RecipientCSV("", template_type="sms"), + ) + + response = logged_in_client.post( + url_for('main.send_messages', service_id=fake_uuid, template_id=fake_uuid), + data={'file': (BytesIO(''.encode('utf-8')), 'valid.csv')}, + content_type='multipart/form-data', + follow_redirects=True, + ) + + assert response.status_code == 200 + assert mock_recipients.call_args[1]['international_sms'] == should_allow_international + + def test_test_message_can_only_be_sent_now( logged_in_client, mocker, diff --git a/tests/conftest.py b/tests/conftest.py index 07ae87bc8..8631b1061 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -77,6 +77,15 @@ def mock_get_service(mocker, api_user_active): return mocker.patch('app.service_api_client.get_service', side_effect=_get) +@pytest.fixture(scope='function') +def mock_get_international_service(mocker, api_user_active): + def _get(service_id): + service = service_json(service_id, users=[api_user_active.id], can_send_international_sms=True) + return {'data': service} + + return mocker.patch('app.service_api_client.get_service', side_effect=_get) + + @pytest.fixture(scope='function') def mock_get_detailed_service(mocker, api_user_active): def _get(service_id):