Merge pull request #109 from alphagov/replace-placeholders

Replace placeholders with personalisation

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,12 @@ 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
+    )
 
     client = firetext_client
 
@@ -78,14 +86,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 +109,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']).__dict__,
+        values=notification.get('personalisation', {})
+    )
 
     client = aws_ses_client
 
@@ -123,7 +136,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)

app/csv.py

@@ -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

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/<string:notification_id>', 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'])
+@notifications.route('/notifications/<string:notification_type>', 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)
-
-
 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())),
+            str(datetime.utcnow())
-            queue='sms')
+        ), 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())),
+            str(datetime.utcnow())
-            queue='email')
+        ), queue='email')
     return jsonify({'notification_id': notification_id}), 201

app/schemas.py

@@ -118,6 +118,7 @@ class RequestVerifyCodeSchema(ma.Schema):
 
 
 class NotificationSchema(ma.Schema):
+    personalisation = fields.Dict(required=False)
     pass
 
 

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

test_csv_files/email.csv

@@ -1,2 +1,2 @@
-to
+email address
-test@test.com
+test@test.com

test_csv_files/empty.csv

@@ -1 +1 @@
-to
+phone number

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
-

test_csv_files/multiple_sms.csv

@@ -1,4 +1,4 @@
-to
+phone number
 +441234123121
 +441234123122
 +441234123123

test_csv_files/sms.csv

@@ -1,2 +1,2 @@
-to
+phone number
-+441234123123
++441234123123

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,
+        "template": sample_template_with_placeholders.id,
-        "to": "+441234123123"
+        "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)
+    firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: Hello Jo")
-    persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id)
+    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
@@ -133,7 +136,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,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,
+        "template": sample_email_template_with_placeholders.id,
-        "to": "my_email@my_email.com"
+        "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,14 @@ 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_with_placeholders.service_id, notification_id
     )
-    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.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'
@@ -198,7 +204,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'

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,

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:

tests/app/test_csv.py

@@ -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'