From 589b4de5f95cec51bc3ddf3d170b2229490d617d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 9 Mar 2016 07:27:26 +0000 Subject: [PATCH] Accept CSV files with additional columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 21 ++++++++++++--------- requirements.txt | 2 +- test_csv_files/multiple_sms.csv | 22 +++++++++++----------- test_csv_files/sms.csv | 4 ++-- tests/app/celery/test_tasks.py | 26 ++++++++++++++++++-------- tests/app/conftest.py | 14 ++++++++++++++ 6 files changed, 58 insertions(+), 31 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c4cd157c8..e7fc8e15c 100644 --- a/app/celery/tasks.py +++ b/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, - drop_values={first_column_heading['sms']} + prefix=service.name ) 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', {}), - drop_values={first_column_heading['email']} + values=notification.get('personalisation', {}) ) client.send_email( diff --git a/requirements.txt b/requirements.txt index 43f900f98..405250156 100644 --- a/requirements.txt +++ b/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 diff --git a/test_csv_files/multiple_sms.csv b/test_csv_files/multiple_sms.csv index fa51924ac..ebfd9d360 100644 --- a/test_csv_files/multiple_sms.csv +++ b/test_csv_files/multiple_sms.csv @@ -1,11 +1,11 @@ -phone number -+441234123121 -+441234123122 -+441234123123 -+441234123124 -+441234123125 -+441234123126 -+441234123127 -+441234123128 -+441234123129 -+441234123120 +phone number,name ++441234123121,chris ++441234123122,chris ++441234123123,chris ++441234123124,chris ++441234123125,chris ++441234123126,chris ++441234123127,chris ++441234123128,chris ++441234123129,chris ++441234123120,chris diff --git a/test_csv_files/sms.csv b/test_csv_files/sms.csv index b430aaf6b..5858f35c9 100644 --- a/test_csv_files/sms.csv +++ b/test_csv_files/sms.csv @@ -1,2 +1,2 @@ -phone number -+441234123123 +phone number, ignore this column ++441234123123, nope diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 830c39e0b..a36d5ee5c 100644 --- a/tests/app/celery/test_tasks.py +++ b/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' diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 3d0cc531e..869c2a3c8 100644 --- a/tests/app/conftest.py +++ b/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,