diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c9f55126b..873159e7c 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -2,6 +2,7 @@ from app import create_uuid from app import notify_celery, encryption, firetext_client, aws_ses_client from app.clients.email.aws_ses import AwsSesClientException from app.clients.sms.firetext import FiretextClientException +from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id from app.dao.notifications_dao import dao_create_notification, dao_update_notification from app.dao.jobs_dao import dao_update_job, dao_get_job_by_id @@ -9,8 +10,9 @@ from app.models import Notification from flask import current_app from sqlalchemy.exc import SQLAlchemyError from app.aws import s3 -from app.csv import get_recipient_from_csv from datetime import datetime +from utils.template import Template +from utils.process_csv import get_rows_from_csv, get_recipient_from_row, first_column_heading @notify_celery.task(name="process-job") @@ -21,13 +23,14 @@ def process_job(job_id): dao_update_job(job) file = s3.get_job_from_s3(job.bucket_name, job_id) - recipients = get_recipient_from_csv(file) - for recipient in recipients: + for row in get_rows_from_csv(file): + encrypted = encryption.encrypt({ 'template': job.template_id, 'job': str(job.id), - 'to': recipient + 'to': get_recipient_from_row(row, job.template.template_type), + 'personalisation': row }) if job.template.template_type == 'sms': @@ -62,7 +65,13 @@ def process_job(job_id): @notify_celery.task(name="send-sms") def send_sms(service_id, notification_id, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) - template = dao_get_template_by_id(notification['template']) + service = dao_fetch_service_by_id(service_id) + template = Template( + dao_get_template_by_id(notification['template']).__dict__, + values=notification.get('personalisation', {}), + prefix=service.name, + drop_values={first_column_heading['sms']} + ) client = firetext_client @@ -78,14 +87,16 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): created_at=created_at, sent_at=sent_at, sent_by=client.get_name() - ) dao_create_notification(notification_db_object) try: - client.send_sms(notification['to'], template.content) + client.send_sms( + notification['to'], + template.replaced + ) except FiretextClientException as e: - current_app.logger.debug(e) + current_app.logger.exception(e) notification_db_object.status = 'failed' dao_update_notification(notification_db_object) @@ -99,7 +110,11 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): @notify_celery.task(name="send-email") def send_email(service_id, notification_id, subject, from_address, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) - template = dao_get_template_by_id(notification['template']) + template = Template( + dao_get_template_by_id(notification['template']).__dict__, + values=notification.get('personalisation', {}), + drop_values={first_column_heading['email']} + ) client = aws_ses_client @@ -123,7 +138,7 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not from_address, notification['to'], subject, - template.content + template.replaced ) except AwsSesClientException as e: current_app.logger.debug(e) diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index c1f355b43..eeeeb51ac 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -1,5 +1,6 @@ import boto3 - +from flask import current_app +from monotonic import monotonic from app.clients.email import (EmailClientException, EmailClient) @@ -34,6 +35,7 @@ class AwsSesClient(EmailClient): elif reply_to_addresses is None: reply_to_addresses = [] + start_time = monotonic() response = self._client.send_email( Source=source, Destination={ @@ -50,6 +52,8 @@ class AwsSesClient(EmailClient): 'Data': body}} }, ReplyToAddresses=reply_to_addresses) + elapsed_time = monotonic() - start_time + current_app.logger.info("AWS SES request finished in {}".format(elapsed_time)) return response['MessageId'] except Exception as e: # TODO logging exceptions diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 235880e83..3a71ac048 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -1,8 +1,10 @@ import logging +from monotonic import monotonic from app.clients.sms import ( SmsClient, SmsClientException ) +from flask import current_app from requests import request, RequestException, HTTPError logger = logging.getLogger(__name__) @@ -36,6 +38,7 @@ class FiretextClient(SmsClient): } try: + start_time = monotonic() response = request( "POST", "https://www.firetext.co.uk/api/sendsms", @@ -53,4 +56,7 @@ class FiretextClient(SmsClient): ) ) raise api_error + finally: + elapsed_time = monotonic() - start_time + current_app.logger.info("Firetext request finished in {}".format(elapsed_time)) return response diff --git a/app/clients/sms/twilio.py b/app/clients/sms/twilio.py index ca33960fb..085fe96cd 100644 --- a/app/clients/sms/twilio.py +++ b/app/clients/sms/twilio.py @@ -1,11 +1,9 @@ -import logging +from monotonic import monotonic from app.clients.sms import ( SmsClient, SmsClientException) from twilio.rest import TwilioRestClient from twilio import TwilioRestException - - -logger = logging.getLogger(__name__) +from flask import current_app class TwilioClientException(SmsClientException): @@ -28,6 +26,7 @@ class TwilioClient(SmsClient): return self.name def send_sms(self, to, content): + start_time = monotonic() try: response = self.client.messages.create( body=content, @@ -36,8 +35,11 @@ class TwilioClient(SmsClient): ) return response.sid except TwilioRestException as e: - logger.exception(e) + current_app.logger.exception(e) raise TwilioClientException(e) + finally: + elapsed_time = monotonic() - start_time + current_app.logger.info("Twilio request finished in {}".format(elapsed_time)) def status(self, message_id): try: @@ -46,5 +48,5 @@ class TwilioClient(SmsClient): return response.status return None except TwilioRestException as e: - logger.exception(e) + current_app.logger.exception(e) raise TwilioClientException(e) diff --git a/app/csv.py b/app/csv.py deleted file mode 100644 index db0199a48..000000000 --- a/app/csv.py +++ /dev/null @@ -1,12 +0,0 @@ -import csv - - -def get_recipient_from_csv(file_data): - numbers = [] - reader = csv.DictReader( - file_data.splitlines(), - lineterminator='\n', - quoting=csv.QUOTE_NONE) - for i, row in enumerate(reader): - numbers.append(row['to'].replace(' ', '')) - return numbers diff --git a/app/notifications/rest.py b/app/notifications/rest.py index c7490ce78..648e7fa43 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -8,6 +8,8 @@ from flask import ( url_for ) +from utils.template import Template, NeededByTemplateError, NoPlaceholderForDataError + from app import api_user, encryption, create_uuid from app.authentication.auth import require_admin from app.dao import ( @@ -29,9 +31,6 @@ from app.errors import register_errors register_errors(notifications) -SMS_NOTIFICATION = 'sms' -EMAIL_NOTIFICATION = 'email' - @notifications.route('/notifications/', methods=['GET']) def get_notifications(notification_id): @@ -122,24 +121,17 @@ def pagination_links(pagination, endpoint, args): return links -@notifications.route('/notifications/sms', methods=['POST']) -def create_sms_notification(): - return send_notification(notification_type=SMS_NOTIFICATION) - - -@notifications.route('/notifications/email', methods=['POST']) -def create_email_notification(): - return send_notification(notification_type=EMAIL_NOTIFICATION) - - +@notifications.route('/notifications/', methods=['POST']) def send_notification(notification_type): - assert notification_type + if notification_type not in ['sms', 'email']: + assert False service_id = api_user['client'] - schema = sms_template_notification_schema if notification_type is SMS_NOTIFICATION else email_notification_schema + notification, errors = ( + sms_template_notification_schema if notification_type == 'sms' else email_notification_schema + ).load(request.get_json()) - notification, errors = schema.load(request.get_json()) if errors: return jsonify(result="error", message=errors), 400 @@ -147,7 +139,6 @@ def send_notification(notification_type): template_id=notification['template'], service_id=service_id ) - if not template: return jsonify( result="error", @@ -156,34 +147,49 @@ def send_notification(notification_type): } ), 404 + template_object = Template(template.__dict__, notification.get('personalisation', {})) + if template_object.missing_data: + return jsonify( + result="error", + message={ + 'template': ['Missing personalisation: {}'.format( + ", ".join(template_object.missing_data) + )] + } + ), 400 + if template_object.additional_data: + return jsonify( + result="error", + message={ + 'template': ['Personalisation not needed for template: {}'.format( + ", ".join(template_object.additional_data) + )] + } + ), 400 + service = services_dao.dao_fetch_service_by_id(api_user['client']) - - if service.restricted: - if notification_type is SMS_NOTIFICATION: - if notification['to'] not in [user.mobile_number for user in service.users]: - return jsonify( - result="error", message={'to': ['Invalid phone number for restricted service']}), 400 - else: - if notification['to'] not in [user.email_address for user in service.users]: - return jsonify( - result="error", message={'to': ['Email address not permitted for restricted service']}), 400 - notification_id = create_uuid() - if notification_type is SMS_NOTIFICATION: + if notification_type == 'sms': + if service.restricted and notification['to'] not in [user.mobile_number for user in service.users]: + return jsonify( + result="error", message={'to': ['Invalid phone number for restricted service']}), 400 send_sms.apply_async(( service_id, notification_id, encryption.encrypt(notification), - str(datetime.utcnow())), - queue='sms') + str(datetime.utcnow()) + ), queue='sms') else: + if service.restricted and notification['to'] not in [user.email_address for user in service.users]: + return jsonify( + result="error", message={'to': ['Email address not permitted for restricted service']}), 400 send_email.apply_async(( service_id, notification_id, template.subject, "{}@{}".format(service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), encryption.encrypt(notification), - str(datetime.utcnow())), - queue='email') + str(datetime.utcnow()) + ), queue='email') return jsonify({'notification_id': notification_id}), 201 diff --git a/app/schemas.py b/app/schemas.py index d2e9d53c8..2114a2111 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -118,6 +118,7 @@ class RequestVerifyCodeSchema(ma.Schema): class NotificationSchema(ma.Schema): + personalisation = fields.Dict(required=False) pass diff --git a/requirements.txt b/requirements.txt index 1d835f1df..c41e2e6f4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -15,8 +15,9 @@ boto3==1.2.3 boto==2.39.0 celery==3.1.20 twilio==4.6.0 +monotonic==0.3 git+https://github.com/alphagov/notifications-python-client.git@0.2.6#egg=notifications-python-client==0.2.6 -git+https://github.com/alphagov/notifications-utils.git@0.2.1#egg=notifications-utils==0.2.1 +git+https://github.com/alphagov/notifications-utils.git@1.0.0#egg=notifications-utils==1.0.0 diff --git a/test_csv_files/email.csv b/test_csv_files/email.csv index 581e64627..3d720f5e2 100644 --- a/test_csv_files/email.csv +++ b/test_csv_files/email.csv @@ -1,2 +1,2 @@ -to -test@test.com \ No newline at end of file +email address +test@test.com diff --git a/test_csv_files/empty.csv b/test_csv_files/empty.csv index bf3efa02f..64e5bd0a3 100644 --- a/test_csv_files/empty.csv +++ b/test_csv_files/empty.csv @@ -1 +1 @@ -to +phone number diff --git a/test_csv_files/multiple_email.csv b/test_csv_files/multiple_email.csv index 2e7bab603..e57e35a2a 100644 --- a/test_csv_files/multiple_email.csv +++ b/test_csv_files/multiple_email.csv @@ -1,4 +1,4 @@ -to +email address test1@test.com test2@test.com test3@test.com @@ -9,4 +9,3 @@ test7@test.com test8@test.com test9@test.com test0@test.com - diff --git a/test_csv_files/multiple_sms.csv b/test_csv_files/multiple_sms.csv index 224a1a4d7..fa51924ac 100644 --- a/test_csv_files/multiple_sms.csv +++ b/test_csv_files/multiple_sms.csv @@ -1,4 +1,4 @@ -to +phone number +441234123121 +441234123122 +441234123123 diff --git a/test_csv_files/sms.csv b/test_csv_files/sms.csv index 3776c8832..b430aaf6b 100644 --- a/test_csv_files/sms.csv +++ b/test_csv_files/sms.csv @@ -1,2 +1,2 @@ -to -+441234123123 \ No newline at end of file +phone number ++441234123123 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 4b094344f..9f5fea5ed 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -85,7 +85,40 @@ def test_should_process_all_sms_job(sample_job, mocker): assert job.status == 'finished' -def test_should_send_template_to_correct_sms_provider_and_persist(sample_template, mocker): +def test_should_send_template_to_correct_sms_provider_and_persist(sample_template_with_placeholders, mocker): + notification = { + "template": sample_template_with_placeholders.id, + "to": "+441234123123", + "personalisation": {"name": "Jo"} + } + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.firetext_client.send_sms') + mocker.patch('app.firetext_client.get_name', return_value="firetext") + + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_sms( + sample_template_with_placeholders.service_id, + notification_id, + "encrypted-in-reality", + now + ) + + firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: Hello Jo") + persisted_notification = notifications_dao.get_notification( + sample_template_with_placeholders.service_id, notification_id + ) + assert persisted_notification.id == notification_id + assert persisted_notification.to == '+441234123123' + assert persisted_notification.template_id == sample_template_with_placeholders.id + assert persisted_notification.status == 'sent' + assert persisted_notification.created_at == now + assert persisted_notification.sent_at > now + assert persisted_notification.sent_by == 'firetext' + assert not persisted_notification.job_id + + +def test_should_send_sms_without_personalisation(sample_template, mocker): notification = { "template": sample_template.id, "to": "+441234123123" @@ -103,16 +136,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat now ) - firetext_client.send_sms.assert_called_once_with("+441234123123", sample_template.content) - persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) - assert persisted_notification.id == notification_id - assert persisted_notification.to == '+441234123123' - assert persisted_notification.template_id == sample_template.id - assert persisted_notification.status == 'sent' - assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.sent_by == 'firetext' - assert not persisted_notification.job_id + firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sample_job, mocker): @@ -133,7 +157,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa "encrypted-in-reality", now) - firetext_client.send_sms.assert_called_once_with("+441234123123", sample_job.template.content) + firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") persisted_notification = notifications_dao.get_notification(sample_job.template.service_id, notification_id) assert persisted_notification.id == notification_id assert persisted_notification.to == '+441234123123' @@ -145,15 +169,54 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa assert persisted_notification.sent_by == 'firetext' -def test_should_use_email_template_and_persist(sample_email_template, mocker): +def test_should_use_email_template_and_persist(sample_email_template_with_placeholders, mocker): notification = { - "template": sample_email_template.id, - "to": "my_email@my_email.com" + "template": sample_email_template_with_placeholders.id, + "to": "my_email@my_email.com", + "personalisation": {"name": "Jo"} } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') mocker.patch('app.aws_ses_client.get_name', return_value='ses') + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_email( + sample_email_template_with_placeholders.service_id, + notification_id, + 'subject', + 'email_from', + "encrypted-in-reality", + now) + + aws_ses_client.send_email.assert_called_once_with( + "email_from", + "my_email@my_email.com", + "subject", + "Hello Jo" + ) + persisted_notification = notifications_dao.get_notification( + sample_email_template_with_placeholders.service_id, notification_id + ) + assert persisted_notification.id == notification_id + assert persisted_notification.to == 'my_email@my_email.com' + assert persisted_notification.template_id == sample_email_template_with_placeholders.id + assert persisted_notification.created_at == now + assert persisted_notification.sent_at > now + assert persisted_notification.status == 'sent' + assert persisted_notification.sent_by == 'ses' + + +def test_should_use_email_template_and_persist_without_personalisation( + sample_email_template, mocker +): + mocker.patch('app.encryption.decrypt', return_value={ + "template": sample_email_template.id, + "to": "my_email@my_email.com", + }) + mocker.patch('app.aws_ses_client.send_email') + mocker.patch('app.aws_ses_client.get_name', return_value='ses') + notification_id = uuid.uuid4() now = datetime.utcnow() send_email( @@ -168,16 +231,8 @@ def test_should_use_email_template_and_persist(sample_email_template, mocker): "email_from", "my_email@my_email.com", "subject", - sample_email_template.content + "This is a template" ) - persisted_notification = notifications_dao.get_notification(sample_email_template.service_id, notification_id) - assert persisted_notification.id == notification_id - assert persisted_notification.to == 'my_email@my_email.com' - assert persisted_notification.template_id == sample_email_template.id - assert persisted_notification.created_at == now - assert persisted_notification.sent_at > now - assert persisted_notification.status == 'sent' - assert persisted_notification.sent_by == 'ses' def test_should_persist_notification_as_failed_if_sms_client_fails(sample_template, mocker): @@ -198,7 +253,7 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa "encrypted-in-reality", now) - firetext_client.send_sms.assert_called_once_with("+441234123123", sample_template.content) + firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) assert persisted_notification.id == notification_id assert persisted_notification.to == '+441234123123' diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 1c0a2657c..4611e195e 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -142,6 +142,11 @@ def sample_template(notify_db, return template +@pytest.fixture(scope='function') +def sample_template_with_placeholders(notify_db, notify_db_session): + return sample_template(notify_db, notify_db_session, content="Hello ((name))") + + @pytest.fixture(scope='function') def sample_email_template( notify_db, @@ -169,6 +174,11 @@ def sample_email_template( return template +@pytest.fixture(scope='function') +def sample_email_template_with_placeholders(notify_db, notify_db_session): + return sample_email_template(notify_db, notify_db_session, content="Hello ((name))") + + @pytest.fixture(scope='function') def sample_api_key(notify_db, notify_db_session, diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index dce8b1aa9..650cd85ec 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -329,6 +329,106 @@ def test_send_notification_invalid_template_id(notify_api, sample_template, mock assert test_string in json_resp['message']['template'] +@freeze_time("2016-01-01 11:09:00.061258") +def test_send_notification_with_placeholders_replaced(notify_api, sample_template_with_placeholders, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + data = { + 'to': '+441234123123', + 'template': sample_template_with_placeholders.id, + 'personalisation': { + 'name': 'Jo' + } + } + auth_header = create_authorization_header( + service_id=sample_template_with_placeholders.service.id, + request_body=json.dumps(data), + path='/notifications/sms', + method='POST') + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + notification_id = json.loads(response.data)['notification_id'] + app.celery.tasks.send_sms.apply_async.assert_called_once_with( + (str(sample_template_with_placeholders.service.id), + notification_id, + "something_encrypted", + "2016-01-01 11:09:00.061258"), + queue="sms" + ) + assert response.status_code == 201 + + +def test_send_notification_with_missing_personalisation( + notify_api, sample_template_with_placeholders, mocker +): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + + data = { + 'to': '+441234123123', + 'template': sample_template_with_placeholders.id, + 'personalisation': { + 'foo': 'bar' + } + } + auth_header = create_authorization_header( + service_id=sample_template_with_placeholders.service.id, + request_body=json.dumps(data), + path='/notifications/sms', + method='POST') + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_sms.apply_async.assert_not_called() + + assert response.status_code == 400 + assert 'Missing personalisation: name' in json_resp['message']['template'] + + +def test_send_notification_with_too_much_personalisation_data( + notify_api, sample_template_with_placeholders, mocker +): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + + data = { + 'to': '+441234123123', + 'template': sample_template_with_placeholders.id, + 'personalisation': { + 'name': 'Jo', 'foo': 'bar' + } + } + auth_header = create_authorization_header( + service_id=sample_template_with_placeholders.service.id, + request_body=json.dumps(data), + path='/notifications/sms', + method='POST') + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_sms.apply_async.assert_not_called() + + assert response.status_code == 400 + assert 'Personalisation not needed for template: foo' in json_resp['message']['template'] + + def test_prevents_sending_to_any_mobile_on_restricted_service(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: diff --git a/tests/app/test_csv.py b/tests/app/test_csv.py deleted file mode 100644 index df8fdcb70..000000000 --- a/tests/app/test_csv.py +++ /dev/null @@ -1,28 +0,0 @@ -from app.csv import get_recipient_from_csv -from tests.app import load_example_csv - - -def test_should_process_single_phone_number_file(): - sms_file = load_example_csv('sms') - len(get_recipient_from_csv(sms_file)) == 1 - assert get_recipient_from_csv(sms_file)[0] == '+441234123123' - - -def test_should_process_multple_phone_number_file_in_order(): - sms_file = load_example_csv('multiple_sms') - len(get_recipient_from_csv(sms_file)) == 10 - assert get_recipient_from_csv(sms_file)[0] == '+441234123121' - assert get_recipient_from_csv(sms_file)[9] == '+441234123120' - - -def test_should_process_single_email_file(): - sms_file = load_example_csv('email') - len(get_recipient_from_csv(sms_file)) == 1 - assert get_recipient_from_csv(sms_file)[0] == 'test@test.com' - - -def test_should_process_multple_email_file_in_order(): - sms_file = load_example_csv('multiple_email') - len(get_recipient_from_csv(sms_file)) == 10 - assert get_recipient_from_csv(sms_file)[0] == 'test1@test.com' - assert get_recipient_from_csv(sms_file)[9] == 'test0@test.com'