From d6f7c7d1c9b8c0c4e29b2f48295f507d31b0f0f7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Feb 2016 14:43:44 +0000 Subject: [PATCH] Replace placeholders before sending a message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit replaces placeholders in a template with the user’s data, using the Template class from utils (https://github.com/alphagov/notifications-utils/tree/master/utils#template) It also extracts the personalisation data from the CSV, taking account of the different column headings that SMS and email CSVs will have. At the point of creating the task to send an individual messages, validation of the placeholders matching the template is assumed to have been done either: - in processing the CSV in the admin app - in the endpoint for the API call No exceptions should be raised at this point. --- app/celery/tasks.py | 28 ++++++++++++++++++--------- app/notifications/rest.py | 2 +- requirements.txt | 2 +- test_csv_files/email.csv | 4 ++-- test_csv_files/empty.csv | 2 +- test_csv_files/multiple_email.csv | 3 +-- test_csv_files/multiple_sms.csv | 2 +- test_csv_files/sms.csv | 4 ++-- tests/app/celery/test_tasks.py | 32 +++++++++++++++++-------------- tests/app/conftest.py | 5 +++++ 10 files changed, 51 insertions(+), 33 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c9f55126b..1af2b1967 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -9,8 +9,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 +22,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 +64,10 @@ 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']) + template = Template( + dao_get_template_by_id(notification['template']), + values=notification.get('personalisation', {}) + ) client = firetext_client @@ -78,12 +83,14 @@ 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) notification_db_object.status = 'failed' @@ -99,7 +106,10 @@ 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']), + values=notification.get('personalisation', {}) + ) client = aws_ses_client @@ -123,7 +133,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/notifications/rest.py b/app/notifications/rest.py index 25dff99b5..648e7fa43 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -147,7 +147,7 @@ def send_notification(notification_type): } ), 404 - template_object = Template({'content': template.content}, notification.get('personalisation', {})) + template_object = Template(template.__dict__, notification.get('personalisation', {})) if template_object.missing_data: return jsonify( result="error", diff --git a/requirements.txt b/requirements.txt index 1d835f1df..4c00d9f14 100644 --- a/requirements.txt +++ b/requirements.txt @@ -19,4 +19,4 @@ twilio==4.6.0 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..565d8a8ae 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -85,10 +85,11 @@ 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.id, - "to": "+441234123123" + "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') @@ -97,17 +98,19 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat notification_id = uuid.uuid4() now = datetime.utcnow() send_sms( - sample_template.service_id, + sample_template_with_placeholders.service_id, notification_id, "encrypted-in-reality", 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) + firetext_client.send_sms.assert_called_once_with("+441234123123", "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.id + 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 @@ -145,10 +148,11 @@ 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') @@ -157,7 +161,7 @@ def test_should_use_email_template_and_persist(sample_email_template, mocker): notification_id = uuid.uuid4() now = datetime.utcnow() send_email( - sample_email_template.service_id, + sample_email_template_with_placeholders.service_id, notification_id, 'subject', 'email_from', @@ -168,12 +172,12 @@ def test_should_use_email_template_and_persist(sample_email_template, mocker): "email_from", "my_email@my_email.com", "subject", - sample_email_template.content + "Hello Jo" ) - persisted_notification = notifications_dao.get_notification(sample_email_template.service_id, notification_id) + 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.id + 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' diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d640d11cf..4611e195e 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -174,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,