From 1e4692287668aa11ab1d1d825bc77754ca2f8810 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Feb 2016 17:17:18 +0000 Subject: [PATCH] Make send the send flow generic This commit parameterises all methods in the send view so that they can send either emails or SMS messages. It works out what kind of message it is sending from the `template_type` property of the template object. This means that the `Template` util class needs to know about these properties, which means that this commit depends on: https://github.com/alphagov/notifications-utils/pull/2 This commit does _not_ add tests for sending emails. The existing tests for sending SMS still pass, but actually sending emails is outside the scope of this story. --- app/assets/stylesheets/components/banner.scss | 2 +- app/main/dao/templates_dao.py | 8 +- app/main/views/send.py | 64 +++++---- app/main/views/templates.py | 7 +- app/templates/views/check-sms.html | 2 +- .../views/choose-email-template.html | 2 - app/templates/views/choose-sms-template.html | 4 +- .../views/{send-sms.html => send.html} | 19 ++- app/templates/views/service_dashboard.html | 4 +- app/utils.py | 20 +++ requirements.txt | 2 +- tests/app/main/views/test_send.py | 124 +++++++++++------- tests/conftest.py | 19 ++- 13 files changed, 174 insertions(+), 103 deletions(-) rename app/templates/views/{send-sms.html => send.html} (57%) diff --git a/app/assets/stylesheets/components/banner.scss b/app/assets/stylesheets/components/banner.scss index c62bfe2c6..77b706779 100644 --- a/app/assets/stylesheets/components/banner.scss +++ b/app/assets/stylesheets/components/banner.scss @@ -71,7 +71,7 @@ color: $text-colour; background-image: file-url('icon-important-2x.png'); background-size: 34px 34px; - background-position: 0 0px; + background-position: 0 0; background-repeat: no-repeat; padding: 7px 0 5px 50px; } diff --git a/app/main/dao/templates_dao.py b/app/main/dao/templates_dao.py index b7ce664f8..7d07b7d0e 100644 --- a/app/main/dao/templates_dao.py +++ b/app/main/dao/templates_dao.py @@ -4,14 +4,14 @@ from app.utils import BrowsableItem from notifications_python_client.errors import HTTPError -def insert_service_template(name, content, service_id, subject=None): +def insert_service_template(name, type_, content, service_id, subject=None): return notifications_api_client.create_service_template( - name, 'sms' if subject is None else 'email', content, service_id, subject) + name, type_, content, service_id, subject) -def update_service_template(id_, name, content, service_id, subject=None): +def update_service_template(id_, name, type_, content, service_id, subject=None): return notifications_api_client.update_service_template( - id_, name, 'sms', content, service_id) + id_, name, type_, content, service_id) def get_service_templates(service_id): diff --git a/app/main/views/send.py b/app/main/views/send.py index 63b947218..91a41fc5f 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -28,15 +28,21 @@ from app.main.uploader import ( s3download ) 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_phone_number, - InvalidPhoneError -) +from app.utils import validate_recipient, InvalidPhoneError, InvalidEmailError + +first_column_header = { + 'email': 'email', + 'sms': 'phone' +} @main.route("/services//send/", methods=['GET']) def choose_template(service_id, template_type): + + services_dao.get_service_by_id_or_404(service_id) + if template_type not in ['email', 'sms']: abort(404) try: @@ -59,7 +65,7 @@ def choose_template(service_id, template_type): @main.route("/services//send/", methods=['GET', 'POST']) @login_required -def send_sms(service_id, template_id): +def send_messages(service_id, template_id): form = CsvUploadForm() if form.validate_on_submit(): @@ -69,23 +75,25 @@ def send_sms(service_id, template_id): upload_id = str(uuid.uuid4()) s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION']) session['upload_data'] = {"template_id": template_id, "original_file_name": filedata['file_name']} - return redirect(url_for('.check_sms', + return redirect(url_for('.check_messages', service_id=service_id, upload_id=upload_id)) except ValueError as e: flash('There was a problem uploading: {}'.format(csv_file.filename)) flash(str(e)) - return redirect(url_for('.send_sms', service_id=service_id, template_id=template_id)) + return redirect(url_for('.send_messages', service_id=service_id, template_id=template_id)) + service = services_dao.get_service_by_id_or_404(service_id) template = Template( templates_dao.get_service_template_or_404(service_id, template_id)['data'] ) return render_template( - 'views/send-sms.html', + 'views/send.html', template=template, - column_headers=['phone'] + template.placeholders_as_markup, + column_headers=[first_column_header[template.template_type]] + template.placeholders_as_markup, form=form, + service=service, service_id=service_id ) @@ -97,7 +105,7 @@ def get_example_csv(service_id, template_id): placeholders = list(Template(template).placeholders) output = io.StringIO() writer = csv.writer(output) - writer.writerow(['phone'] + placeholders) + writer.writerow([first_column_header[template['template_type']]] + placeholders) writer.writerow([current_user.mobile_number] + ["test {}".format(header) for header in placeholders]) return(output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'}) @@ -105,12 +113,12 @@ def get_example_csv(service_id, template_id): @main.route("/services//send//to-self", methods=['GET']) @login_required -def send_sms_to_self(service_id, template_id): +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) output = io.StringIO() writer = csv.writer(output) - writer.writerow(['phone'] + placeholders) + writer.writerow([first_column_header[template['template_type']]] + placeholders) writer.writerow([current_user.mobile_number] + ["test {}".format(header) for header in placeholders]) filedata = { 'file_name': 'Test run', @@ -120,7 +128,7 @@ def send_sms_to_self(service_id, template_id): s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION']) session['upload_data'] = {"template_id": template_id, "original_file_name": filedata['file_name']} - return redirect(url_for('.check_sms', + return redirect(url_for('.check_messages', service_id=service_id, upload_id=upload_id)) @@ -128,27 +136,29 @@ def send_sms_to_self(service_id, template_id): @main.route("/services//check/", methods=['GET', 'POST']) @login_required -def check_sms(service_id, upload_id): +def check_messages(service_id, upload_id): + + upload_data = session['upload_data'] + template_id = upload_data.get('template_id') if request.method == 'GET': contents = s3download(service_id, upload_id) if not contents: flash('There was a problem reading your upload file') - upload_data = session['upload_data'] - template_id = upload_data.get('template_id') raw_template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] + recipient_type = first_column_header[raw_template['template_type']] upload_result = _get_rows(contents, raw_template) session['upload_data']['notification_count'] = len(upload_result['rows']) template = Template( raw_template, values=upload_result['rows'][0] if upload_result['valid'] else {}, - drop_values={'phone'} + drop_values={recipient_type} ) return render_template( 'views/check-sms.html', upload_result=upload_result, template=template, - column_headers=['phone number'] + list( + column_headers=[recipient_type] + list( template.placeholders if upload_result['valid'] else template.placeholders_as_markup ), original_file_name=upload_data.get('original_file_name'), @@ -156,9 +166,7 @@ def check_sms(service_id, upload_id): form=CsvUploadForm() ) elif request.method == 'POST': - upload_data = session['upload_data'] original_file_name = upload_data.get('original_file_name') - template_id = upload_data.get('template_id') notification_count = upload_data.get('notification_count') session.pop('upload_data') try: @@ -170,9 +178,9 @@ def check_sms(service_id, upload_id): raise e flash('We’ve started sending your messages', 'default_with_tick') - return redirect(url_for('main.view_job', - service_id=service_id, - job_id=upload_id)) + return redirect( + url_for('main.view_job', service_id=service_id, job_id=upload_id) + ) def _get_filedata(file): @@ -195,8 +203,12 @@ def _get_rows(contents, raw_template): for row in reader: rows.append(row) try: - validate_phone_number(row['phone']) - Template(raw_template, values=row, drop_values={'phone'}).replaced - except (InvalidPhoneError, NeededByTemplateError, NoPlaceholderForDataError): + recipient_column = first_column_header[raw_template['template_type']] + validate_recipient( + row[recipient_column], + template_type=raw_template['template_type'] + ) + Template(raw_template, values=row, drop_values={recipient_column}).replaced + except (InvalidEmailError, InvalidPhoneError, NeededByTemplateError, NoPlaceholderForDataError): valid = False return {"valid": valid, "rows": rows} diff --git a/app/main/views/templates.py b/app/main/views/templates.py index b291e1b8c..01f3711dd 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -31,7 +31,7 @@ def add_service_template(service_id, template_type): if form.validate_on_submit(): tdao.insert_service_template( - form.name.data, form.template_content.data, service_id, form.subject.data or None + form.name.data, template['template_type'], form.template_content.data, service_id, form.subject.data or None ) return redirect( url_for('.choose_template', service_id=service_id, template_type=template_type) @@ -54,8 +54,9 @@ def edit_service_template(service_id, template_id): if form.validate_on_submit(): tdao.update_service_template( - template_id, form.name.data, - form.template_content.data, service_id) + template_id, form.name.data, template['template_type'], + form.template_content.data, service_id + ) return redirect(url_for( '.choose_template', service_id=service_id, diff --git a/app/templates/views/check-sms.html b/app/templates/views/check-sms.html index 4b1354847..e0de38dca 100644 --- a/app/templates/views/check-sms.html +++ b/app/templates/views/check-sms.html @@ -42,7 +42,7 @@
- Back + Back
{% else %} {{file_upload(form.file, button_text='Upload a CSV file')}} diff --git a/app/templates/views/choose-email-template.html b/app/templates/views/choose-email-template.html index 06ed1dec6..fb5fe2b1f 100644 --- a/app/templates/views/choose-email-template.html +++ b/app/templates/views/choose-email-template.html @@ -21,8 +21,6 @@
- Add recipients - Send yourself a test Edit template
diff --git a/app/templates/views/choose-sms-template.html b/app/templates/views/choose-sms-template.html index b83217a7c..5508ca4fb 100644 --- a/app/templates/views/choose-sms-template.html +++ b/app/templates/views/choose-sms-template.html @@ -30,8 +30,8 @@
- Add recipients - Send yourself a test + Add recipients + Send yourself a test Edit template
diff --git a/app/templates/views/send-sms.html b/app/templates/views/send.html similarity index 57% rename from app/templates/views/send-sms.html rename to app/templates/views/send.html index c8a91388d..8aef83078 100644 --- a/app/templates/views/send-sms.html +++ b/app/templates/views/send.html @@ -1,5 +1,6 @@ {% extends "withnav_template.html" %} {% from "components/sms-message.html" import sms_message %} +{% from "components/email-message.html" import email_message %} {% from "components/page-footer.html" import page_footer %} {% from "components/file-upload.html" import file_upload %} {% from "components/table.html" import list_table, field %} @@ -14,14 +15,22 @@
- - {{ sms_message(template.formatted_as_markup) }} - + {% if 'sms' == template.template_type %} + {{ sms_message(template.formatted_as_markup) }} + {% elif 'email' == template.template_type %} + {{ email_message( + template.subject, + template.formatted_as_markup, + from_address='{}@notifications.service.gov.uk'.format(service.email_from), + from_name=service.name + ) }} + {% endif %} {{ banner( - 'You can upload real data, but we’ll only send to your mobile number until you request to go live'|safe, + 'You can upload real data, but we’ll only send to your mobile number until you request to go live'.format( + url_for('.service_request_to_go_live', service_id=service_id) + )|safe, 'info' )}} -
diff --git a/app/templates/views/service_dashboard.html b/app/templates/views/service_dashboard.html index 045a90695..b17c935ba 100644 --- a/app/templates/views/service_dashboard.html +++ b/app/templates/views/service_dashboard.html @@ -36,7 +36,7 @@ """.format( url_for(".add_service_template", service_id=service_id), - url_for(".choose_sms_template", service_id=service_id) + url_for(".choose_template", service_id=service_id, template_type="sms") )|safe, subhead='Get started', type="tip" @@ -46,7 +46,7 @@ """ Send yourself a text message """.format( - url_for(".choose_sms_template", service_id=service_id) + url_for(".choose_template", service_id=service_id, template_type="sms") )|safe, subhead='Next step', type="tip" diff --git a/app/utils.py b/app/utils.py index f814f4b81..9468cba77 100644 --- a/app/utils.py +++ b/app/utils.py @@ -1,3 +1,5 @@ +import re + from functools import wraps from flask import abort @@ -28,6 +30,11 @@ class BrowsableItem(object): pass +class InvalidEmailError(Exception): + def __init__(self, message): + self.message = message + + class InvalidPhoneError(Exception): def __init__(self, message): self.message = message @@ -74,6 +81,19 @@ def format_phone_number(number): return '+447{}{}{}'.format(*re.findall('...', number)) +def validate_email_address(email_address): + if re.match(r"(^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+$)", email_address): + return + raise InvalidEmailError('Not a valid email address') + + +def validate_recipient(recipient, template_type): + return { + 'email': validate_email_address, + 'sms': validate_phone_number + }[template_type](recipient) + + def user_has_permissions(*permissions): def wrap(func): @wraps(func) diff --git a/requirements.txt b/requirements.txt index 49dceaccf..af95431af 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.5#egg=notifications-python-client==0.2.7 -git+https://github.com/alphagov/notifications-utils.git@0.1.0#egg=notifications-utils==0.1.0 +git+https://github.com/alphagov/notifications-utils.git@0.1.0#egg=notifications-utils==0.1.1 diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index c48aeed26..7e9e8dc1a 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1,42 +1,56 @@ from io import BytesIO from flask import url_for +import pytest import moto +template_types = ['email', 'sms'] -def test_choose_sms_template(app_, - api_user_active, - mock_login, - mock_get_user, - mock_check_verify_code, - mock_get_service_templates, - mock_get_jobs): + +@pytest.mark.parametrize("template_type", template_types) +def test_choose_template( + template_type, + app_, + api_user_active, + mock_login, + mock_get_user, + mock_get_service, + mock_check_verify_code, + mock_get_service_templates, + mock_get_jobs +): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) - response = client.get(url_for('main.choose_template', template_type='sms', service_id=12345)) + response = client.get(url_for('main.choose_template', template_type=template_type, service_id=12345)) assert response.status_code == 200 content = response.get_data(as_text=True) - assert 'template_one' in content - assert 'template one content' in content - assert 'template_two' in content - assert 'template two content' in content + assert '{}_template_one'.format(template_type) in content + assert '{} template one content'.format(template_type) in content + assert '{}_template_two'.format(template_type) in content + assert '{} template two content'.format(template_type) in content -def test_upload_empty_csvfile_returns_to_upload_page(app_, - api_user_active, - mock_login, - mock_get_user, - mock_get_service_templates, - mock_check_verify_code, - mock_get_service_template): +def test_upload_empty_csvfile_returns_to_upload_page( + app_, + api_user_active, + mock_login, + mock_get_user, + mock_get_service, + mock_get_service_templates, + mock_check_verify_code, + mock_get_service_template +): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) upload_data = {'file': (BytesIO(''.encode('utf-8')), 'emtpy.csv')} - response = client.post(url_for('main.send_sms', service_id=12345, template_id=54321), - data=upload_data, follow_redirects=True) + response = client.post( + url_for('main.send_messages', service_id=12345, template_id=54321), + data=upload_data, + follow_redirects=True + ) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -44,13 +58,15 @@ def test_upload_empty_csvfile_returns_to_upload_page(app_, @moto.mock_s3 -def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(app_, - mocker, - api_user_active, - mock_login, - mock_get_user, - mock_get_user_by_email, - mock_get_service_template): +def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( + app_, + mocker, + api_user_active, + mock_login, + mock_get_user, + mock_get_user_by_email, + mock_get_service_template +): contents = 'phone\n+44 123\n+44 456' file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv') @@ -59,9 +75,11 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(app_, with app_.test_client() as client: client.login(api_user_active) upload_data = {'file': file_data} - response = client.post(url_for('main.send_sms', service_id=12345, template_id=54321), - data=upload_data, - follow_redirects=True) + response = client.post( + url_for('main.send_messages', service_id=12345, template_id=54321), + data=upload_data, + follow_redirects=True + ) assert response.status_code == 200 content = response.get_data(as_text=True) assert 'Your CSV file contained missing or invalid data' in content @@ -85,7 +103,7 @@ def test_send_test_message_to_self( with app_.test_client() as client: client.login(api_user_active) response = client.get( - url_for('main.send_sms_to_self', service_id=12345, template_id=54321), + url_for('main.send_message_to_self', service_id=12345, template_id=54321), follow_redirects=True ) assert response.status_code == 200 @@ -118,13 +136,15 @@ def test_download_example_csv( @moto.mock_s3 -def test_upload_csvfile_with_valid_phone_shows_all_numbers(app_, - mocker, - api_user_active, - mock_login, - mock_get_user, - mock_get_user_by_email, - mock_get_service_template): +def test_upload_csvfile_with_valid_phone_shows_all_numbers( + app_, + mocker, + api_user_active, + mock_login, + mock_get_user, + mock_get_user_by_email, + mock_get_service_template +): contents = 'phone\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 @@ -134,7 +154,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(app_, with app_.test_client() as client: client.login(api_user_active) upload_data = {'file': file_data} - response = client.post(url_for('main.send_sms', service_id=12345, template_id=54321), + response = client.post(url_for('main.send_messages', service_id=12345, template_id=54321), data=upload_data, follow_redirects=True) with client.session_transaction() as sess: @@ -154,16 +174,18 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(app_, @moto.mock_s3 -def test_create_job_should_call_api(app_, - service_one, - api_user_active, - mock_get_user, - mock_get_user_by_email, - mock_login, - job_data, - mock_create_job, - mock_get_job, - mock_get_service_template): +def test_create_job_should_call_api( + app_, + service_one, + api_user_active, + mock_get_user, + mock_get_user_by_email, + mock_login, + job_data, + mock_create_job, + mock_get_job, + mock_get_service_template +): service_id = service_one['id'] job_id = job_data['id'] @@ -178,7 +200,7 @@ def test_create_job_should_call_api(app_, session['upload_data'] = {'original_file_name': original_file_name, 'template_id': template_id, 'notification_count': notification_count} - url = url_for('main.check_sms', service_id=service_one['id'], upload_id=job_id) + url = url_for('main.check_messages', service_id=service_one['id'], upload_id=job_id) response = client.post(url, data=job_data, follow_redirects=True) assert response.status_code == 200 diff --git a/tests/conftest.py b/tests/conftest.py index 308ee42b7..3c1de5a87 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -163,11 +163,20 @@ def mock_update_service_template(mocker): @pytest.fixture(scope='function') def mock_get_service_templates(mocker): def _create(service_id): - template_one = template_json( - 1, "template_one", "sms", "template one content", service_id) - template_two = template_json( - 2, "template_two", "sms", "template two content", service_id) - return {'data': [template_one, template_two]} + return {'data': [ + template_json( + 1, "sms_template_one", "sms", "sms template one content", service_id + ), + template_json( + 2, "sms_template_two", "sms", "sms template two content", service_id + ), + template_json( + 3, "email_template_one", "email", "email template one content", service_id + ), + template_json( + 4, "email_template_two", "email", "email template two content", service_id + ) + ]} return mocker.patch( 'app.notifications_api_client.get_service_templates',