From b57ac2f7e5f3090648b92f98e20501dee6765bcf Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 1 Mar 2016 09:41:08 +0000 Subject: [PATCH] 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)