From 5f3c72729ef247c240a8883437319dc6da3d21e5 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 29 Feb 2016 17:35:21 +0000 Subject: [PATCH 1/5] [WIP] Start of user accepting invite. This commit only deals with acceptance by users who are already in system. Changed invite client to return invited user objects instead of dictionaries. Added commented out test. fixed up fixtures to return invited user object for invites --- app/main/__init__.py | 23 ++++- app/main/views/invites.py | 34 +++++++ app/main/views/manage_users.py | 4 +- app/notify_client/invite_api_client.py | 21 ++++- app/notify_client/models.py | 15 ++++ app/notify_client/user_api_client.py | 5 ++ app/templates/views/manage-users.html | 6 +- tests/__init__.py | 7 +- tests/app/main/views/test_accept_invite.py | 25 ++++++ tests/app/main/views/test_manage_users.py | 100 ++++++++++----------- tests/conftest.py | 59 ++++++++---- 11 files changed, 220 insertions(+), 79 deletions(-) create mode 100644 app/main/views/invites.py create mode 100644 tests/app/main/views/test_accept_invite.py diff --git a/app/main/__init__.py b/app/main/__init__.py index ec9691d24..39f95b71d 100644 --- a/app/main/__init__.py +++ b/app/main/__init__.py @@ -3,7 +3,24 @@ from flask import Blueprint main = Blueprint('main', __name__) from app.main.views import ( - index, sign_in, sign_out, register, two_factor, verify, send, add_service, - code_not_received, jobs, dashboard, templates, service_settings, forgot_password, - new_password, styleguide, user_profile, choose_service, api_keys, manage_users + index, + sign_in, + sign_out, + register, + two_factor, + verify, + send, + add_service, + code_not_received, + jobs, dashboard, + templates, + service_settings, + forgot_password, + new_password, + styleguide, + user_profile, + choose_service, + api_keys, + manage_users, + invites ) diff --git a/app/main/views/invites.py b/app/main/views/invites.py new file mode 100644 index 000000000..4b271e29c --- /dev/null +++ b/app/main/views/invites.py @@ -0,0 +1,34 @@ +from flask import ( + redirect, + url_for, + abort +) + +from notifications_python_client.errors import HTTPError + +from app.main import main +from app import ( + invite_api_client, + user_api_client +) + + +@main.route("/invitation/") +def accept_invite(token): + try: + invited_user = invite_api_client.accept_invite(token) + existing_user = user_api_client.get_user_by_email(invited_user['email_address']) + + if existing_user: + user_api_client.add_user_to_service(invited_user['service'], + existing_user.id, + invited_user) + return redirect(url_for('main.service_dashboard', service_id=invited_user['service'])) + else: + # TODO implement registration flow for new users + abort(404) + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 0b5f0e7f0..f5d83bfd6 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -58,8 +58,8 @@ def invite_user(service_id): email_address = form.email_address.data permissions = _get_permissions(request.form) try: - resp = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions) - flash('Invite sent to {}'.format(resp['email_address']), 'default_with_tick') + invited_user = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions) + flash('Invite sent to {}'.format(invited_user.email_address), 'default_with_tick') return redirect(url_for('.manage_users', service_id=service_id)) except HTTPError as e: diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 1dffed425..d1acd5f6d 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -1,6 +1,5 @@ - from notifications_python_client.base import BaseAPIClient -from app.notify_client.models import User +from app.notify_client.models import InvitedUser class InviteApiClient(BaseAPIClient): @@ -22,9 +21,23 @@ class InviteApiClient(BaseAPIClient): 'permissions': permissions } resp = self.post(url='/service/{}/invite'.format(service_id), data=data) - return resp['data'] + return InvitedUser(**resp['data']) def get_invites_for_service(self, service_id): endpoint = '/service/{}/invite'.format(service_id) resp = self.get(endpoint) - return [User(data) for data in resp['data']] + invites = resp['data'] + invited_users = _get_invited_users(invites) + return invited_users + + def accept_invite(self, token): + resp = self.get(url='/invite/{}'.format(token)) + return InvitedUser(**resp['data']) + + +def _get_invited_users(invites): + invited_users = [] + for invite in invites: + invited_user = InvitedUser(**invite) + invited_users.append(invited_user) + return invited_users diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 986caf570..d5caf5b33 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -110,3 +110,18 @@ class User(UserMixin): def set_password(self, pwd): self._password = pwd + + +class InvitedUser(object): + + def __init__(self, id, service, from_user, email_address, permissions, status, created_at): + self.id = id + self.service = str(service) + self.from_user = from_user + self.email_address = email_address + self.permissions = permissions.split(',') + self.status = status + self.created_at = created_at + + def has_permissions(self, permission): + return permission in self.permissions diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 179a7d4f8..5bb744a91 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -85,3 +85,8 @@ class UserApiClient(BaseAPIClient): endpoint = '/service/{}/users'.format(service_id) resp = self.get(endpoint) return [User(data) for data in resp['data']] + + def add_user_to_service(self, service_id, user_id, invited_user): + endpoint = '/service/{}/users/{}'.format(service_id, user_id) + resp = self.post(endpoint, data=invited_user) + return User(resp['data'], max_failed_login_count=self.max_failed_login_count) diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 02d3d817e..b3adee4be 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -43,9 +43,9 @@ Manage users – GOV.UK Notify {% call field() %} {{ item.email_address }} {% endcall %} - {{ boolean_field(item.has_permissions(service_id, 'send_messages')) }} - {{ boolean_field(item.has_permissions(service_id, 'manage_service')) }} - {{ boolean_field(item.has_permissions(service_id, 'api_keys')) }} + {{ boolean_field(item.has_permissions('send_messages')) }} + {{ boolean_field(item.has_permissions('manage_service')) }} + {{ boolean_field(item.has_permissions('api_keys')) }} {% call field(align='right') %} Change {% endcall %} diff --git a/tests/__init__.py b/tests/__init__.py index 4792e8c7e..57e3984fb 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -48,12 +48,15 @@ def api_key_json(id_, name, expiry_date=None): } -def invite_json(id, from_user, service_id, email_address): +def invite_json(id, from_user, service_id, email_address, permissions, created_at): return {'id': id, 'from_user': from_user, 'service': service_id, 'email_address': email_address, - 'status': 'pending'} + 'status': 'pending', + 'permissions': permissions, + 'created_at': created_at + } TEST_USER_EMAIL = 'test@user.gov.uk' diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py new file mode 100644 index 000000000..8c52400c4 --- /dev/null +++ b/tests/app/main/views/test_accept_invite.py @@ -0,0 +1,25 @@ +from flask import url_for + + +def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard(app_, + service_one, + sample_invite, + mock_accept_invite, + mock_get_user_by_email, + mock_add_user_to_service): + + expected_service = service_one['id'] + expected_from_user = service_one['users'][0] + expected_redirect_location = 'http://localhost/services/{}/dashboard'.format(expected_service) + + with app_.test_request_context(): + with app_.test_client() as client: + + response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) + + mock_accept_invite.assert_called_with('thisisnotarealtoken') + mock_get_user_by_email.assert_called_with('invited_user@test.gov.uk') + mock_add_user_to_service.assert_called_with(expected_service, expected_from_user, sample_invite) + + assert response.status_code == 302 + assert response.location == expected_redirect_location diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 4af60ee7c..438e3060e 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -3,22 +3,22 @@ from flask import url_for from bs4 import BeautifulSoup -# def test_should_show_overview_page( -# app_, -# api_user_active, -# mock_login, -# mock_get_service, -# mock_get_users_by_service, -# mock_get_invites_for_service -# ): -# with app_.test_request_context(): -# with app_.test_client() as client: -# client.login(api_user_active) -# response = client.get(url_for('main.manage_users', service_id=55555)) -# -# assert 'Manage team' in response.get_data(as_text=True) -# assert response.status_code == 200 -# mock_get_users_by_service.assert_called_once_with(service_id='55555') +def test_should_show_overview_page( + app_, + api_user_active, + mock_login, + mock_get_service, + mock_get_users_by_service, + mock_get_invites_for_service +): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + response = client.get(url_for('main.manage_users', service_id=55555)) + + assert 'Manage team' in response.get_data(as_text=True) + assert response.status_code == 200 + mock_get_users_by_service.assert_called_once_with(service_id='55555') def test_should_show_page_for_one_user( @@ -70,37 +70,37 @@ def test_should_show_page_for_inviting_user( assert 'Add a new team member' in response.get_data(as_text=True) assert response.status_code == 200 -# -# def test_invite_user( -# app_, -# service_one, -# api_user_active, -# mock_login, -# mock_get_users_by_service, -# mock_create_invite, -# mock_get_invites_for_service -# ): -# from_user = api_user_active.id -# service_id = service_one['id'] -# email_address = 'test@example.gov.uk' -# permissions = 'send_messages,manage_service,manage_api_keys' -# -# with app_.test_request_context(): -# with app_.test_client() as client: -# client.login(api_user_active) -# response = client.post( -# url_for('main.invite_user', service_id=service_id), -# data={'email_address': email_address, -# 'send_messages': 'yes', -# 'manage_service': 'yes', -# 'manage_api_keys': 'yes'}, -# follow_redirects=True -# ) -# -# assert response.status_code == 200 -# mock_create_invite.assert_called_with(from_user, service_id, email_address, permissions) -# mock_get_invites_for_service.assert_called_with(service_id=service_id) -# page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') -# assert page.h1.string.strip() == 'Manage team' -# flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() -# assert flash_banner == 'Invite sent to test@example.gov.uk' + +def test_invite_user( + app_, + service_one, + api_user_active, + mock_login, + mock_get_users_by_service, + mock_create_invite, + mock_get_invites_for_service +): + from_user = api_user_active.id + service_id = service_one['id'] + email_address = 'test@example.gov.uk' + permissions = 'send_messages,manage_service,manage_api_keys' + + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + response = client.post( + url_for('main.invite_user', service_id=service_id), + data={'email_address': email_address, + 'send_messages': 'yes', + 'manage_service': 'yes', + 'manage_api_keys': 'yes'}, + follow_redirects=True + ) + + assert response.status_code == 200 + mock_create_invite.assert_called_with(from_user, service_id, email_address, permissions) + mock_get_invites_for_service.assert_called_with(service_id=service_id) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Manage team' + flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() + assert flash_banner == 'Invite sent to test@example.gov.uk' diff --git a/tests/conftest.py b/tests/conftest.py index db5bdfa2c..b04b81e14 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,7 +12,10 @@ from . import ( job_json, invite_json ) -from app.notify_client.models import User +from app.notify_client.models import ( + User, + InvitedUser +) @pytest.fixture(scope='session') @@ -551,27 +554,53 @@ def mock_s3_upload(mocker): @pytest.fixture(scope='function') -def mock_create_invite(mocker): +def sample_invite(mocker, service_one): + import datetime + id = str(uuid.uuid4()) + from_user = service_one['users'][0] + email_address = 'invited_user@test.gov.uk' + service_id = service_one['id'] + permissions = 'send_messages,manage_service,manage_api_keys' + created_at = datetime.datetime.now() + return invite_json(id, from_user, service_id, email_address, permissions, created_at) + + +@pytest.fixture(scope='function') +def mock_create_invite(mocker, sample_invite): + def _create_invite(from_user, service_id, email_address, permissions): - data = {'id': uuid.uuid4(), - 'from_user': from_user, - 'service': service_id, - 'email_address': email_address, - 'status': 'pending', - 'permissions': permissions} - return data + sample_invite['from_user'] = from_user + sample_invite['service'] = service_id + sample_invite['email_address'] = email_address + sample_invite['status'] = 'pending' + sample_invite['permissions'] = permissions + return InvitedUser(**sample_invite) return mocker.patch('app.invite_api_client.create_invite', side_effect=_create_invite) @pytest.fixture(scope='function') -def mock_get_invites_for_service(mocker, service_one): +def mock_get_invites_for_service(mocker, service_one, sample_invite): + import copy + def _get_invites(service_id): data = [] - from_user = service_one['users'][0] - service_id = service_one['id'] for i in range(0, 5): - email_address = 'user_{}@testnotify.gov.uk'.format(i) - invite = invite_json(uuid.uuid4(), from_user, service_id, email_address) - data.append(invite) + invite = copy.copy(sample_invite) + invite['email_address'] = 'user_{}@testnotify.gov.uk'.format(i) + data.append(InvitedUser(**invite)) return data return mocker.patch('app.invite_api_client.get_invites_for_service', side_effect=_get_invites) + + +@pytest.fixture(scope='function') +def mock_accept_invite(mocker, sample_invite): + def _accept_token(token): + return sample_invite + return mocker.patch('app.invite_api_client.accept_invite', side_effect=_accept_token) + + +@pytest.fixture(scope='function') +def mock_add_user_to_service(mocker, service_one): + def _add_user(service_id, user_id, invitation): + return api_user_active + return mocker.patch('app.user_api_client.add_user_to_service', side_effect=_add_user) From 5f02d4cefeb84f469b095eb9352fef9a45b525cb Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 1 Mar 2016 17:23:23 +0000 Subject: [PATCH 2/5] [WIP] Post does not need any data Invites rest module can use invited user object instead of dict. --- app/main/views/invites.py | 10 +++++----- app/notify_client/user_api_client.py | 6 ++++-- app/templates/views/manage-users.html | 2 +- tests/app/main/views/test_accept_invite.py | 9 +++++++-- tests/conftest.py | 6 +++--- 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 4b271e29c..c9052303d 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -17,13 +17,13 @@ from app import ( def accept_invite(token): try: invited_user = invite_api_client.accept_invite(token) - existing_user = user_api_client.get_user_by_email(invited_user['email_address']) + existing_user = user_api_client.get_user_by_email(invited_user.email_address) if existing_user: - user_api_client.add_user_to_service(invited_user['service'], - existing_user.id, - invited_user) - return redirect(url_for('main.service_dashboard', service_id=invited_user['service'])) + user_api_client.add_user_to_service(invited_user.service, + existing_user.id) + + return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) else: # TODO implement registration flow for new users abort(404) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 5bb744a91..b1054f769 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -1,3 +1,5 @@ +import json + from notifications_python_client.notifications import BaseAPIClient from notifications_python_client.errors import HTTPError @@ -86,7 +88,7 @@ class UserApiClient(BaseAPIClient): resp = self.get(endpoint) return [User(data) for data in resp['data']] - def add_user_to_service(self, service_id, user_id, invited_user): + def add_user_to_service(self, service_id, user_id): endpoint = '/service/{}/users/{}'.format(service_id, user_id) - resp = self.post(endpoint, data=invited_user) + resp = self.post(endpoint) return User(resp['data'], max_failed_login_count=self.max_failed_login_count) diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index b3adee4be..44a0b39e5 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -45,7 +45,7 @@ Manage users – GOV.UK Notify {% endcall %} {{ boolean_field(item.has_permissions('send_messages')) }} {{ boolean_field(item.has_permissions('manage_service')) }} - {{ boolean_field(item.has_permissions('api_keys')) }} + {{ boolean_field(item.has_permissions('manage_api_keys')) }} {% call field(align='right') %} Change {% endcall %} diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 8c52400c4..54b527d93 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -1,15 +1,20 @@ from flask import url_for +from app.notify_client.models import InvitedUser +from notifications_python_client.errors import HTTPError + +import pytest + def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard(app_, service_one, + api_user_active, sample_invite, mock_accept_invite, mock_get_user_by_email, mock_add_user_to_service): expected_service = service_one['id'] - expected_from_user = service_one['users'][0] expected_redirect_location = 'http://localhost/services/{}/dashboard'.format(expected_service) with app_.test_request_context(): @@ -19,7 +24,7 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard(app_, mock_accept_invite.assert_called_with('thisisnotarealtoken') mock_get_user_by_email.assert_called_with('invited_user@test.gov.uk') - mock_add_user_to_service.assert_called_with(expected_service, expected_from_user, sample_invite) + mock_add_user_to_service.assert_called_with(expected_service, api_user_active.id) assert response.status_code == 302 assert response.location == expected_redirect_location diff --git a/tests/conftest.py b/tests/conftest.py index b04b81e14..acd163be9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -595,12 +595,12 @@ def mock_get_invites_for_service(mocker, service_one, sample_invite): @pytest.fixture(scope='function') def mock_accept_invite(mocker, sample_invite): def _accept_token(token): - return sample_invite + return InvitedUser(**sample_invite) return mocker.patch('app.invite_api_client.accept_invite', side_effect=_accept_token) @pytest.fixture(scope='function') -def mock_add_user_to_service(mocker, service_one): - def _add_user(service_id, user_id, invitation): +def mock_add_user_to_service(mocker, service_one, api_user_active): + def _add_user(service_id, user_id): return api_user_active return mocker.patch('app.user_api_client.add_user_to_service', side_effect=_add_user) From 98169550d8b714fd7d6056106f128d59a4f244db Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 1 Mar 2016 17:32:04 +0000 Subject: [PATCH 3/5] [WIP] no data needed for post to create add user to service --- app/notify_client/user_api_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index b1054f769..d6a4ad574 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -90,5 +90,5 @@ class UserApiClient(BaseAPIClient): def add_user_to_service(self, service_id, user_id): endpoint = '/service/{}/users/{}'.format(service_id, user_id) - resp = self.post(endpoint) + resp = self.post(endpoint, data={}) return User(resp['data'], max_failed_login_count=self.max_failed_login_count) From 0b8b41979840552c439ee5fbc84fa8829e69f866 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 2 Mar 2016 09:55:59 +0000 Subject: [PATCH 4/5] Fix missing import on template for email_message. https://www.pivotaltracker.com/story/show/113991355 --- app/templates/views/job.html | 1 + 1 file changed, 1 insertion(+) diff --git a/app/templates/views/job.html b/app/templates/views/job.html index ae60fed13..416da439e 100644 --- a/app/templates/views/job.html +++ b/app/templates/views/job.html @@ -3,6 +3,7 @@ {% from "components/big-number.html" import big_number %} {% from "components/banner.html" import banner %} {% from "components/sms-message.html" import sms_message %} +{% from "components/email-message.html" import email_message %} {% block page_title %} {{ uploaded_file_name }} – GOV.UK Notify From b57ac2f7e5f3090648b92f98e20501dee6765bcf Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 1 Mar 2016 09:41:08 +0000 Subject: [PATCH 5/5] Accept CSVs with 'email address' or 'phone number' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CSV files currently have ‘to’ as the recipient column. This is changing in https://github.com/alphagov/notifications-api/pull/109 The admin app also has to validate that the CSV files have the right columns, because the API expects any CSV that it’s given to have been checked (also we want things to actually work). This commit is the minimum code change needed. In the future it should reuse the same code as the API for processing CSV files. This will need more thinking. --- app/main/views/send.py | 42 ++++++++++++++++++++----------- app/templates/views/check.html | 10 +++++--- app/templates/views/send.html | 2 +- app/utils.py | 6 +++-- requirements.txt | 2 +- tests/app/main/views/test_send.py | 16 ++++++------ 6 files changed, 48 insertions(+), 30 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 856403105..2fdb5d620 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -27,6 +27,7 @@ from app.main.dao import templates_dao from app.main.dao import services_dao from app import job_api_client from app.utils import validate_recipient, InvalidPhoneError, InvalidEmailError +from utils.process_csv import first_column_heading page_headings = { 'email': 'Send emails', @@ -97,11 +98,12 @@ def send_messages(service_id, template_id): templates_dao.get_service_template_or_404(service_id, template_id)['data'], prefix=service['name'] ) + recipient_column = first_column_heading[template.template_type] return render_template( 'views/send.html', template=template, - column_headers=['to'] + template.placeholders_as_markup, + recipient_column=first_column_heading[template.template_type], form=form, service=service, service_id=service_id @@ -111,29 +113,36 @@ def send_messages(service_id, template_id): @main.route("/services//send/.csv", methods=['GET']) @login_required def get_example_csv(service_id, template_id): - template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] - placeholders = list(Template(template).placeholders) + template = Template(templates_dao.get_service_template_or_404(service_id, template_id)['data']) output = io.StringIO() writer = csv.writer(output) - writer.writerow(['to'] + placeholders) + writer.writerow( + [first_column_heading[template.template_type]] + + list(template.placeholders) + ) writer.writerow([ { 'email': current_user.email_address, 'sms': current_user.mobile_number - }[template['template_type']] - ] + ["test {}".format(header) for header in placeholders]) + }[template.template_type] + ] + ["test {}".format(header) for header in template.placeholders]) return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} @main.route("/services//send//to-self", methods=['GET']) @login_required def send_message_to_self(service_id, template_id): - template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] - placeholders = list(Template(template).placeholders) + template = Template(templates_dao.get_service_template_or_404(service_id, template_id)['data']) output = io.StringIO() writer = csv.writer(output) - writer.writerow(['to'] + placeholders) - writer.writerow([current_user.mobile_number] + ["test {}".format(header) for header in placeholders]) + writer.writerow( + [first_column_heading[template.template_type]] + + list(template.placeholders) + ) + writer.writerow( + [current_user.mobile_number] + + ["test {}".format(header) for header in template.placeholders] + ) filedata = { 'file_name': 'Test run', 'data': output.getvalue().splitlines() @@ -166,7 +175,7 @@ def check_messages(service_id, upload_id): template = Template( raw_template, values=upload_result['rows'][0] if upload_result['valid'] else {}, - drop_values={'to'}, + drop_values={first_column_heading[raw_template['template_type']]}, prefix=service['name'] ) return render_template( @@ -174,7 +183,7 @@ def check_messages(service_id, upload_id): upload_result=upload_result, template=template, page_heading=page_headings[template.template_type], - column_headers=['to'] + list(template.placeholders_as_markup), + column_headers=[first_column_heading[template.template_type]] + list(template.placeholders_as_markup), original_file_name=upload_data.get('original_file_name'), service_id=service_id, service=service, @@ -236,10 +245,13 @@ def _get_rows(contents, raw_template): rows.append(row) try: validate_recipient( - row.get('to', ''), - template_type=raw_template['template_type'] + row, template_type=raw_template['template_type'] ) - Template(raw_template, values=row, drop_values={'to'}).replaced + Template( + raw_template, + values=row, + drop_values={first_column_heading[raw_template['template_type']]} + ).replaced except (InvalidEmailError, InvalidPhoneError, NeededByTemplateError, NoPlaceholderForDataError): valid = False return {"valid": valid, "rows": rows} diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 315bc32eb..d60bef88f 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -61,13 +61,17 @@ caption=original_file_name, field_headings=column_headers ) %} - {% if item.to or ''|valid_phone_number %} + {% if item.get('phone number', '')|valid_phone_number %} {% call field() %} - {{ item.to }} + {{ item['phone number'] }} + {% endcall %} + {% elif item.get('email address') %} + {% call field() %} + {{ item['email address'] }} {% endcall %} {% else %} {% call field(status='missing') %} - {{ item.to }} + {{ item['phone number'] }} {% endcall %} {% endif %} {% for column in template.placeholders %} diff --git a/app/templates/views/send.html b/app/templates/views/send.html index 7b0dcf027..2d62b9f2a 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -40,7 +40,7 @@ {% endif %}

- to + {{ recipient_column }} {{ template.placeholders_as_markup|join(" ") }}

diff --git a/app/utils.py b/app/utils.py index 176ff26cb..0fd38f808 100644 --- a/app/utils.py +++ b/app/utils.py @@ -3,6 +3,8 @@ import re from functools import wraps from flask import (abort, session) +from utils.process_csv import get_recipient_from_row + class BrowsableItem(object): """ @@ -87,11 +89,11 @@ def validate_email_address(email_address): raise InvalidEmailError('Not a valid email address') -def validate_recipient(recipient, template_type): +def validate_recipient(row, template_type): return { 'email': validate_email_address, 'sms': validate_phone_number - }[template_type](recipient) + }[template_type](get_recipient_from_row(row, template_type)) def user_has_permissions(*permissions): diff --git a/requirements.txt b/requirements.txt index 2c746a497..45cd1b2d5 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.2.8#egg=notifications-python-client==0.2.8 -git+https://github.com/alphagov/notifications-utils.git@0.1.2#egg=notifications-utils==0.1.2 +git+https://github.com/alphagov/notifications-utils.git@1.0.0#egg=notifications-utils==1.0.0 diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e638a5481..7371318d4 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -67,7 +67,7 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( mock_s3_upload ): - contents = 'to,name\n+44 123,test1\n+44 456,test2' + contents = 'phone number,name\n+44 123,test1\n+44 456,test2' file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv') mocker.patch('app.main.views.send.s3download', return_value=contents) @@ -98,14 +98,14 @@ def test_upload_csvfile_removes_empty_lines_and_trailing_commas( mock_s3_upload ): - contents = 'to,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n' + contents = 'phone number,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n' file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv') - expected_data = {'data': ['to,name', '++44 7700 900981,test1', '+44 7700 900981,test2'], + expected_data = {'data': ['phone number,name', '++44 7700 900981,test1', '+44 7700 900981,test2'], 'file_name': 'invalid.csv'} mocker.patch('app.main.views.send.s3download', - return_value='to,name\n++44 7700 900981,test1\n+44 7700 900981,test2') + return_value='phone number,name\n++44 7700 900981,test1\n+44 7700 900981,test2') with app_.test_request_context(): with app_.test_client() as client: @@ -130,8 +130,8 @@ def test_send_test_message_to_self( mock_s3_upload ): - expected_data = {'data': ['to', '+4412341234'], 'file_name': 'Test run'} - mocker.patch('app.main.views.send.s3download', return_value='to\r\n+4412341234') + expected_data = {'data': ['phone number', '+4412341234'], 'file_name': 'Test run'} + mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') with app_.test_request_context(): with app_.test_client() as client: @@ -161,7 +161,7 @@ def test_download_example_csv( follow_redirects=True ) assert response.status_code == 200 - assert response.get_data(as_text=True) == 'to\r\n+4412341234\r\n' + assert response.get_data(as_text=True) == 'phone number\r\n+4412341234\r\n' assert 'text/csv' in response.headers['Content-Type'] @@ -175,7 +175,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_s3_upload ): - contents = 'to\n+44 7700 900981\n+44 7700 900982\n+44 7700 900983\n+44 7700 900984\n+44 7700 900985\n+44 7700 900986' # noqa + contents = 'phone number\n+44 7700 900981\n+44 7700 900982\n+44 7700 900983\n+44 7700 900984\n+44 7700 900985\n+44 7700 900986' # noqa file_data = (BytesIO(contents.encode('utf-8')), 'valid.csv') mocker.patch('app.main.views.send.s3download', return_value=contents)