From 68f31c6f84cff3d10540ac73b7e7beab0b1e2566 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Feb 2016 11:03:48 +0000 Subject: [PATCH 1/8] Refactor send notification into one route Using a URL parameter means that sending a notification can be done in one route, rather than two separate routes and an extra method. This commit also refactors that one remaining method to be shorter/cleaner/more readable (or I think so anyway). No functional changes in this commit. --- app/notifications/rest.py | 56 +++++++++++++++------------------------ 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index c7490ce78..b6f69e0d5 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 + 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,32 +121,24 @@ 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 - template = templates_dao.dao_get_template_by_id_and_service_id( + template = Template(templates_dao.dao_get_template_by_id_and_service_id( template_id=notification['template'], service_id=service_id - ) - + )) if not template: return jsonify( result="error", @@ -157,33 +148,28 @@ def send_notification(notification_type): ), 404 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 From 68eaacaafb4aa3c0aab072c7d9686f4db95645b3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Feb 2016 11:23:34 +0000 Subject: [PATCH 2/8] Accept and validate personalisation This commit allows the send notification endpoint to accept an extra parameter, `personalisation`, the contents of which will be used (later) to replace the placeholders in the template. It does validation in the following places: - at the schema level, to validate the type and (optional) presence of personalisation - at the endpoint, to check whether the personalisation provided matches exactly the placeholders in the template It does not do validation when processing CSV files, as these are assumed to already have been validated by the admin app. It explicitly does not persist either the names of the placeholders (these should always be derived from the template contents unless it really becomes a performance concern) or the values of the placeholders (because they might be personal data). --- app/notifications/rest.py | 26 +++++++- app/schemas.py | 1 + tests/app/conftest.py | 5 ++ tests/app/notifications/test_rest.py | 99 ++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 3 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index b6f69e0d5..25dff99b5 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -8,7 +8,7 @@ from flask import ( url_for ) -from utils.template import Template +from utils.template import Template, NeededByTemplateError, NoPlaceholderForDataError from app import api_user, encryption, create_uuid from app.authentication.auth import require_admin @@ -135,10 +135,10 @@ def send_notification(notification_type): if errors: return jsonify(result="error", message=errors), 400 - template = Template(templates_dao.dao_get_template_by_id_and_service_id( + template = templates_dao.dao_get_template_by_id_and_service_id( template_id=notification['template'], service_id=service_id - )) + ) if not template: return jsonify( result="error", @@ -147,6 +147,26 @@ def send_notification(notification_type): } ), 404 + template_object = Template({'content': template.content}, 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']) notification_id = create_uuid() 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/tests/app/conftest.py b/tests/app/conftest.py index 1c0a2657c..d640d11cf 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, diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index dce8b1aa9..66fb9ce3c 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -328,6 +328,105 @@ def test_send_notification_invalid_template_id(notify_api, sample_template, mock test_string = 'Template {} not found for service {}'.format(9999, sample_template.service.id) 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(): From d6f7c7d1c9b8c0c4e29b2f48295f507d31b0f0f7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Feb 2016 14:43:44 +0000 Subject: [PATCH 3/8] 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, From fa4b2e16e7e2d96ff7dc4b2e4c00f8fddc004a43 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Feb 2016 14:44:19 +0000 Subject: [PATCH 4/8] Remove CSV utils These are in the utils repo since https://github.com/alphagov/notifications-utils/releases/tag/0.2.1 --- app/csv.py | 12 ------------ tests/app/test_csv.py | 28 ---------------------------- 2 files changed, 40 deletions(-) delete mode 100644 app/csv.py delete mode 100644 tests/app/test_csv.py 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/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' From 35b7b884f85cc720ac0e7ad8e4a9fc97d79457b9 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 2 Mar 2016 09:33:20 +0000 Subject: [PATCH 5/8] Add logging around 3rd party delivery calls - time SES, Twilio, fire text calls - use monotonic for accuracy --- app/clients/email/aws_ses.py | 6 +++++- app/clients/sms/firetext.py | 6 ++++++ app/clients/sms/twilio.py | 14 ++++++++------ requirements.txt | 1 + 4 files changed, 20 insertions(+), 7 deletions(-) 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/requirements.txt b/requirements.txt index 1d835f1df..ddcceaced 100644 --- a/requirements.txt +++ b/requirements.txt @@ -15,6 +15,7 @@ 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 From 0e5d72494e9e3eed23f020b261c236f68b7129c4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 1 Mar 2016 08:48:27 +0000 Subject: [PATCH 6/8] Prefix all SMS messages with service name Implements https://github.com/alphagov/notifications-utils/pull/4 --- app/celery/tasks.py | 9 ++++++--- tests/app/celery/test_tasks.py | 10 ++++++---- tests/app/notifications/test_rest.py | 1 + 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 1af2b1967..d7cb2da23 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 @@ -64,9 +65,11 @@ 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) + service = dao_fetch_service_by_id(service_id) template = Template( - dao_get_template_by_id(notification['template']), - values=notification.get('personalisation', {}) + dao_get_template_by_id(notification['template']).__dict__, + values=notification.get('personalisation', {}), + prefix=service.name ) client = firetext_client @@ -107,7 +110,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): def send_email(service_id, notification_id, subject, from_address, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) template = Template( - dao_get_template_by_id(notification['template']), + dao_get_template_by_id(notification['template']).__dict__, values=notification.get('personalisation', {}) ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 565d8a8ae..661c09f78 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -104,7 +104,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat now ) - firetext_client.send_sms.assert_called_once_with("+441234123123", "Hello Jo") + 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 ) @@ -136,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' @@ -174,7 +174,9 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh "subject", "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_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 @@ -202,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' diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 66fb9ce3c..650cd85ec 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -328,6 +328,7 @@ def test_send_notification_invalid_template_id(notify_api, sample_template, mock test_string = 'Template {} not found for service {}'.format(9999, sample_template.service.id) 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(): From cc741003c0d2f6fa26c1f20cb0a275cb0c39564e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Mar 2016 10:55:55 +0000 Subject: [PATCH 7/8] Log FireText exceptions as exceptions --- app/celery/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d7cb2da23..b12768c58 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -95,7 +95,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): 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) From b202af716ddec68e8d63c0ed9354beabf37c79bc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Mar 2016 12:27:07 +0000 Subject: [PATCH 8/8] Fix bug where sending messages failed When building the template it was looking for a placeholder called ((phone number)). This caused it to fail because the template it had did not match the personalisation it was being given. `Template` has an optional parameter for specifying personalisation values that should be ignored. The recipient of a message is an example of such a value. This commit passes that extra parameter, which fixes that bug. --- app/celery/tasks.py | 6 +++-- tests/app/celery/test_tasks.py | 49 ++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index b12768c58..873159e7c 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -69,7 +69,8 @@ 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 = firetext_client @@ -111,7 +112,8 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not notification = encryption.decrypt(encrypted_notification) 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 = aws_ses_client diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 661c09f78..9f5fea5ed 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -118,6 +118,27 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat assert not persisted_notification.job_id +def test_should_send_sms_without_personalisation(sample_template, mocker): + notification = { + "template": sample_template.id, + "to": "+441234123123" + } + 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.service_id, + notification_id, + "encrypted-in-reality", + now + ) + + 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): notification = { "template": sample_job.template.id, @@ -186,6 +207,34 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh 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( + sample_email_template.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", + "This is a template" + ) + + def test_should_persist_notification_as_failed_if_sms_client_fails(sample_template, mocker): notification = { "template": sample_template.id,