Accept CSV files with additional columns

Currently when the Celery task processes a CSV it will call the API with the
values for all the non-recipient columns in the `personalisation` field. This
means that those API calls would fail, even though the CSV has been processed
‘successfully’.

This was not being caught by the tests, so this commit adds extra tests to check
what data the task is passing to the API call.

It then updates utils to version 2.0.1 which brings in this fix:
https://github.com/alphagov/notifications-utils/pull/10

app/celery/tasks.py

@@ -22,19 +22,24 @@ def process_job(job_id):
     job.status = 'in progress'
     dao_update_job(job)
 
+    template = Template(
+        dao_get_template_by_id(job.template_id).__dict__
+    )
+
     for recipient, personalisation in RecipientCSV(
         s3.get_job_from_s3(job.bucket_name, job_id),
-        template_type=job.template.template_type
+        template_type=template.template_type,
+        placeholders=template.placeholders
     ).recipients_and_personalisation:
 
         encrypted = encryption.encrypt({
-            'template': job.template_id,
+            'template': template.id,
             'job': str(job.id),
             'to': recipient,
             'personalisation': personalisation
         })
 
-        if job.template.template_type == 'sms':
+        if template.template_type == 'sms':
             send_sms.apply_async((
                 str(job.service_id),
                 str(create_uuid()),
@@ -43,11 +48,11 @@ def process_job(job_id):
                 queue='bulk-sms'
             )
 
-        if job.template.template_type == 'email':
+        if template.template_type == 'email':
             send_email.apply_async((
                 str(job.service_id),
                 str(create_uuid()),
-                job.template.subject,
+                template.subject,
                 "{}@{}".format(job.service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']),
                 encrypted,
                 datetime.utcnow().strftime(DATETIME_FORMAT)),
@@ -101,8 +106,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at):
                 template = Template(
                     dao_get_template_by_id(notification['template']).__dict__,
                     values=notification.get('personalisation', {}),
-                    prefix=service.name,
+                    prefix=service.name
-                    drop_values={first_column_heading['sms']}
                 )
 
                 client.send_sms(
@@ -169,8 +173,7 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not
             try:
                 template = Template(
                     dao_get_template_by_id(notification['template']).__dict__,
-                    values=notification.get('personalisation', {}),
+                    values=notification.get('personalisation', {})
-                    drop_values={first_column_heading['email']}
                 )
 
                 client.send_email(

requirements.txt

@@ -21,4 +21,4 @@ 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@2.0.0#egg=notifications-utils==2.0.0
+git+https://github.com/alphagov/notifications-utils.git@2.0.1#egg=notifications-utils==2.0.1

test_csv_files/multiple_sms.csv

@@ -1,11 +1,11 @@
-phone number
+phone number,name
-+441234123121
++441234123121,chris
-+441234123122
++441234123122,chris
-+441234123123
++441234123123,chris
-+441234123124
++441234123124,chris
-+441234123125
++441234123125,chris
-+441234123126
++441234123126,chris
-+441234123127
++441234123127,chris
-+441234123128
++441234123128,chris
-+441234123129
++441234123129,chris
-+441234123120
++441234123120,chris

test_csv_files/sms.csv

@@ -1,2 +1,2 @@
-phone number
+phone number, ignore this column
-+441234123123
++441234123123, nope

tests/app/celery/test_tasks.py

@@ -16,12 +16,13 @@ from freezegun import freeze_time
 from tests.app.conftest import (
     sample_service,
     sample_user,
-    sample_template
+    sample_template,
+    sample_template_with_placeholders
 )
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_should_process_sms_job(sample_job, mocker):
+def test_should_process_sms_job(sample_job, sample_template, mocker):
     mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms'))
     mocker.patch('app.celery.tasks.send_sms.apply_async')
     mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
@@ -30,6 +31,8 @@ def test_should_process_sms_job(sample_job, mocker):
     process_job(sample_job.id)
 
     s3.get_job_from_s3.assert_called_once_with(sample_job.bucket_name, sample_job.id)
+    assert encryption.encrypt.call_args[0][0]['to'] == '+441234123123'
+    assert encryption.encrypt.call_args[0][0]['personalisation'] == {}
     tasks.send_sms.apply_async.assert_called_once_with(
         (str(sample_job.service_id),
          "uuid",
@@ -41,7 +44,7 @@ def test_should_process_sms_job(sample_job, mocker):
     assert job.status == 'finished'
 
 
-def test_should_not_create_send_task_for_empty_file(sample_job, mocker):
+def test_should_not_create_send_task_for_empty_file(sample_job, sample_template, mocker):
     mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('empty'))
     mocker.patch('app.celery.tasks.send_sms.apply_async')
 
@@ -54,7 +57,7 @@ def test_should_not_create_send_task_for_empty_file(sample_job, mocker):
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_should_process_email_job(sample_email_job, mocker):
+def test_should_process_email_job(sample_email_job, sample_template, mocker):
     mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('email'))
     mocker.patch('app.celery.tasks.send_email.apply_async')
     mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
@@ -63,6 +66,8 @@ def test_should_process_email_job(sample_email_job, mocker):
     process_job(sample_email_job.id)
 
     s3.get_job_from_s3.assert_called_once_with(sample_email_job.bucket_name, sample_email_job.id)
+    assert encryption.encrypt.call_args[0][0]['to'] == 'test@test.com'
+    assert encryption.encrypt.call_args[0][0]['personalisation'] == {}
     tasks.send_email.apply_async.assert_called_once_with(
         (str(sample_email_job.service_id),
          "uuid",
@@ -76,17 +81,22 @@ def test_should_process_email_job(sample_email_job, mocker):
     assert job.status == 'finished'
 
 
-def test_should_process_all_sms_job(sample_job, mocker):
+def test_should_process_all_sms_job(sample_job, sample_job_with_placeholdered_template, mocker):
     mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms'))
     mocker.patch('app.celery.tasks.send_sms.apply_async')
     mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
     mocker.patch('app.celery.tasks.create_uuid', return_value="uuid")
 
-    process_job(sample_job.id)
+    process_job(sample_job_with_placeholdered_template.id)
 
-    s3.get_job_from_s3.assert_called_once_with(sample_job.bucket_name, sample_job.id)
+    s3.get_job_from_s3.assert_called_once_with(
+        sample_job_with_placeholdered_template.bucket_name,
+        sample_job_with_placeholdered_template.id
+    )
+    assert encryption.encrypt.call_args[0][0]['to'] == '+441234123120'
+    assert encryption.encrypt.call_args[0][0]['personalisation'] == {'name': 'chris'}
     tasks.send_sms.apply_async.call_count == 10
-    job = jobs_dao.dao_get_job_by_id(sample_job.id)
+    job = jobs_dao.dao_get_job_by_id(sample_job_with_placeholdered_template.id)
     assert job.status == 'finished'
 
 

tests/app/conftest.py

@@ -220,6 +220,20 @@ def sample_job(notify_db,
     return job
 
 
+@pytest.fixture(scope='function')
+def sample_job_with_placeholdered_template(
+    notify_db,
+    notify_db_session,
+    service=None
+):
+    return sample_job(
+        notify_db,
+        notify_db_session,
+        service=service,
+        template=sample_template_with_placeholders(notify_db, notify_db_session)
+    )
+
+
 @pytest.fixture(scope='function')
 def sample_email_job(notify_db,
                      notify_db_session,