From 7a0a81c07c61c42d6661234f20b49ecc769c3da5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 4 Apr 2016 14:28:52 +0100 Subject: [PATCH] Show error if restricted service uploads real CSV Implements: https://github.com/alphagov/notifications-utils/pull/16 Also changes a bunch of the mocked phone numbers to be real-looking so that the tests are checking for the right kind of error. --- app/main/views/send.py | 12 ++++-- requirements.txt | 2 +- tests/app/main/views/test_send.py | 61 +++++++++++++++++-------------- tests/conftest.py | 14 +++---- 4 files changed, 50 insertions(+), 39 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index c0d0297d3..e6ae682fb 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -2,6 +2,7 @@ import csv import io import json import uuid +import itertools from contextlib import suppress from flask import ( @@ -25,8 +26,8 @@ from app.main.uploader import ( s3upload, s3download ) -from app import (job_api_client, service_api_client, current_service) -from app.utils import (user_has_permissions, get_errors_for_csv) +from app import job_api_client, service_api_client, current_service, user_api_client +from app.utils import user_has_permissions, get_errors_for_csv def get_send_button_text(template_type, number_of_messages): @@ -204,6 +205,8 @@ def check_messages(service_id, template_type, upload_id): if not session.get('upload_data'): return redirect(url_for('main.choose_template', service_id=service_id, template_type=template_type)) + users = user_api_client.get_users_for_service(service_id=service_id) + contents = s3download(service_id, upload_id) if not contents: flash('There was a problem reading your upload file') @@ -223,7 +226,10 @@ def check_messages(service_id, template_type, upload_id): template_type=template.template_type, placeholders=template.placeholders, max_initial_rows_shown=15, - max_errors_shown=15 + max_errors_shown=15, + whitelist=itertools.chain.from_iterable( + [user.mobile_number, user.email_address] for user in users + ) if current_service['restricted'] else None ) with suppress(StopIteration): diff --git a/requirements.txt b/requirements.txt index d37ae37df..424925db0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,4 +14,4 @@ Pygments==2.0.2 git+https://github.com/alphagov/notifications-python-client.git@0.3.1#egg=notifications-python-client==0.3.1 -git+https://github.com/alphagov/notifications-utils.git@3.1.3#egg=notifications-utils==3.1.3 +git+https://github.com/alphagov/notifications-utils.git@3.2.0#egg=notifications-utils==3.2.0 diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 12afc7884..d77776c72 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -16,7 +16,8 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( mock_get_service, mock_get_service_template, mock_s3_upload, - mock_has_permissions + mock_has_permissions, + mock_get_users_by_service ): contents = u'phone number,name\n+44 123,test1\n+44 456,test2' @@ -48,10 +49,11 @@ def test_send_test_sms_message_to_self( mock_get_service, mock_get_service_template, mock_s3_upload, - mock_has_permissions + mock_has_permissions, + mock_get_users_by_service ): - expected_data = {'data': 'phone number\r\n+4412341234\r\n', 'file_name': 'Test run'} + expected_data = {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'Test run'} mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') with app_.test_request_context(): @@ -73,7 +75,8 @@ def test_send_test_email_message_to_self( mock_get_service, mock_get_service_email_template, mock_s3_upload, - mock_has_permissions + mock_has_permissions, + mock_get_users_by_service ): expected_data = {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'Test run'} @@ -129,7 +132,7 @@ def test_download_example_csv( follow_redirects=True ) assert response.status_code == 200 - assert response.get_data(as_text=True) == 'phone number\r\n+4412341234\r\n+4412341234\r\n' + assert response.get_data(as_text=True) == 'phone number\r\n07700 900762\r\n07700 900762\r\n' assert 'text/csv' in response.headers['Content-Type'] @@ -141,31 +144,32 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_get_service, mock_get_service_template, mock_s3_upload, - mock_has_permissions + mock_has_permissions, + mock_get_users_by_service ): mocker.patch( 'app.main.views.send.s3download', return_value=""" phone number - +44 7700 9009 01 - +44 7700 9009 02 - +44 7700 9009 03 - +44 7700 9009 04 - +44 7700 9009 05 - +44 7700 9009 06 - +44 7700 9009 07 - +44 7700 9009 08 - +44 7700 9009 09 - +44 7700 9009 10 - +44 7700 9009 11 - +44 7700 9009 12 - +44 7700 9009 13 - +44 7700 9009 14 - +44 7700 9009 15 - +44 7700 9009 99 - +44 7700 9009 99 - +44 7700 9009 99 + 07700 900701 + 07700 900702 + 07700 900703 + 07700 900704 + 07700 900705 + 07700 900706 + 07700 900707 + 07700 900708 + 07700 900709 + 07700 900710 + 07700 900711 + 07700 900712 + 07700 900713 + 07700 900714 + 07700 900715 + 07700 900799 + 07700 900799 + 07700 900799 """ ) @@ -185,9 +189,9 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( content = response.get_data(as_text=True) assert response.status_code == 200 - assert '+44 7700 9009 01' in content - assert '+44 7700 9009 15' in content - assert '+44 7700 9009 16' not in content + assert '07700 900701' in content + assert '07700 900715' in content + assert '07700 900716' not in content assert '3 rows not shown' in content @@ -238,7 +242,8 @@ def test_check_messages_should_revalidate_file_when_uploading_file( mock_get_service_template, mock_s3_upload, mocker, - mock_has_permissions + mock_has_permissions, + mock_get_users_by_service ): service_id = service_one['id'] diff --git a/tests/conftest.py b/tests/conftest.py index 2a404feee..ed20ec89f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -245,7 +245,7 @@ def api_user_pending(): 'name': 'Test User', 'password': 'somepassword', 'email_address': 'test@user.gov.uk', - 'mobile_number': '+4412341234', + 'mobile_number': '07700 900762', 'state': 'pending', 'failed_login_count': 0, 'permissions': {} @@ -261,7 +261,7 @@ def platform_admin_user(): 'name': 'Platform admin user', 'password': 'somepassword', 'email_address': 'platform@admin.gov.uk', - 'mobile_number': '+4472341234', + 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 0, 'permissions': {}, @@ -278,7 +278,7 @@ def api_user_active(): 'name': 'Test User', 'password': 'somepassword', 'email_address': 'test@user.gov.uk', - 'mobile_number': '+4412341234', + 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 0, 'permissions': {}, @@ -296,7 +296,7 @@ def active_user_with_permissions(): 'name': 'Test User', 'password': 'somepassword', 'email_address': 'test@user.gov.uk', - 'mobile_number': '+4412341234', + 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 0, 'permissions': {SERVICE_ONE_ID: ['send_texts', @@ -320,7 +320,7 @@ def api_user_locked(): 'name': 'Test User', 'password': 'somepassword', 'email_address': 'test@user.gov.uk', - 'mobile_number': '+4412341234', + 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 5, 'permissions': {} @@ -336,7 +336,7 @@ def api_user_request_password_reset(): 'name': 'Test User', 'password': 'somepassword', 'email_address': 'test@user.gov.uk', - 'mobile_number': '+4412341234', + 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 5, 'permissions': {}, @@ -353,7 +353,7 @@ def api_user_changed_password(): 'name': 'Test User', 'password': 'somepassword', 'email_address': 'test@user.gov.uk', - 'mobile_number': '+4412341234', + 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 5, 'permissions': {},