From c580b9c084f2d8efb16d204aa0a0f1e75a20bf2f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 10 Mar 2016 13:22:45 +0000 Subject: [PATCH 01/10] Pass notification ID to fire text as our reference - also handle fire text errors, non-zero response code means error. --- app/celery/tasks.py | 5 +++-- app/clients/sms/firetext.py | 21 +++++++++++++++++---- tests/app/celery/test_tasks.py | 29 ++++++++++++++++++++++++----- 3 files changed, 44 insertions(+), 11 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index b3f090127..8f609ed3f 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -203,8 +203,9 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): ) client.send_sms( - notification['to'], - template.replaced + to=notification['to'], + content=template.replaced, + notification_id=notification_id ) except FiretextClientException as e: current_app.logger.exception(e) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 3a71ac048..d10f0f875 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -11,7 +11,12 @@ logger = logging.getLogger(__name__) class FiretextClientException(SmsClientException): - pass + def __init__(self, response): + self.code = response['code'] + self.description = response['description'] + + def __str__(self): + return "Code {} description {}".format(self.code, self.description) class FiretextClient(SmsClient): @@ -28,7 +33,7 @@ class FiretextClient(SmsClient): def get_name(self): return self.name - def send_sms(self, to, content): + def send_sms(self, to, content, notification_id=None): data = { "apiKey": self.api_key, @@ -37,13 +42,21 @@ class FiretextClient(SmsClient): "message": content } + if notification_id: + data.update({ + "reference": notification_id + }) + + start_time = monotonic() try: - start_time = monotonic() response = request( "POST", - "https://www.firetext.co.uk/api/sendsms", + "https://www.firetext.co.uk/api/sendsms/json", data=data ) + firetext_response = response.json() + if firetext_response['code'] != 0: + raise FiretextClientException(firetext_response) response.raise_for_status() except RequestException as e: api_error = HTTPError.create(e) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index bb2a7bb3b..65abc977e 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -260,7 +260,11 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat now.strftime(DATETIME_FORMAT) ) - firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: Hello Jo") + firetext_client.send_sms.assert_called_once_with( + to="+441234123123", + content="Sample service: Hello Jo", + notification_id=notification_id + ) persisted_notification = notifications_dao.get_notification( sample_template_with_placeholders.service_id, notification_id ) @@ -292,7 +296,11 @@ def test_should_send_sms_without_personalisation(sample_template, mocker): now.strftime(DATETIME_FORMAT) ) - firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") + firetext_client.send_sms.assert_called_once_with( + to="+441234123123", + content="Sample service: This is a template", + notification_id=notification_id + ) def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): @@ -317,7 +325,11 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif now.strftime(DATETIME_FORMAT) ) - firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") + firetext_client.send_sms.assert_called_once_with( + to="+441234123123", + content="Sample service: This is a template", + notification_id=notification_id + ) def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): @@ -394,7 +406,11 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa "encrypted-in-reality", now.strftime(DATETIME_FORMAT) ) - firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") + firetext_client.send_sms.assert_called_once_with( + to="+441234123123", + content="Sample service: This is a template", + notification_id=notification_id + ) persisted_notification = notifications_dao.get_notification(sample_job.template.service_id, notification_id) assert persisted_notification.id == notification_id assert persisted_notification.to == '+441234123123' @@ -490,7 +506,10 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa "encrypted-in-reality", now.strftime(DATETIME_FORMAT) ) - firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") + firetext_client.send_sms.assert_called_once_with( + to="+441234123123", + content="Sample service: This is a template", + notification_id=notification_id) persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) assert persisted_notification.id == notification_id assert persisted_notification.to == '+441234123123' From 1f22f2b7cc3f89032b8689633b71c11bfb326c15 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 10 Mar 2016 15:40:41 +0000 Subject: [PATCH 02/10] Updates to fire text integration: - client updated to raise errors with fire text error codes/messages New endpoint - /notifications/sms/firetext For delivery notifications to be sent to. --- app/__init__.py | 2 +- app/celery/tasks.py | 3 + app/clients/sms/firetext.py | 18 ++ app/dao/notifications_dao.py | 4 + app/models.py | 2 +- app/notifications/rest.py | 64 ++++++- .../versions/0039_more_notification_states.py | 31 ++++ tests/app/celery/test_tasks.py | 8 +- tests/app/notifications/test_rest.py | 172 ++++++++++++++++++ 9 files changed, 299 insertions(+), 5 deletions(-) create mode 100644 migrations/versions/0039_more_notification_states.py diff --git a/app/__init__.py b/app/__init__.py index 17c81d24d..3c06ac77c 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -71,7 +71,7 @@ def create_app(): def init_app(app): @app.before_request def required_authentication(): - if request.path != url_for('status.show_status'): + if request.path not in [url_for('status.show_status'), url_for('notifications.process_firetext_response')]: from app.authentication import auth error = auth.requires_auth() if error: diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 8f609ed3f..9a29ec3d2 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -208,6 +208,9 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): notification_id=notification_id ) except FiretextClientException as e: + current_app.logger.error( + "SMS notification {} failed".format(notification_id) + ) current_app.logger.exception(e) notification_db_object.status = 'failed' dao_update_notification(notification_db_object) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index d10f0f875..9b3793e2c 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -9,6 +9,24 @@ from requests import request, RequestException, HTTPError logger = logging.getLogger(__name__) +firetext_response_status = { + '0': { + "firetext_message": 'delivered', + "success": True, + "notify_status": 'delivered' + }, + '1': { + "firetext_message": 'declined', + "success": False, + "notify_status": 'failed' + }, + '2': { + "firetext_message": 'Undelivered (Pending with Network)', + "success": False, + "notify_status": 'sent' + } +} + class FiretextClientException(SmsClientException): def __init__(self, response): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 95c3fb54d..0c5109233 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -86,6 +86,10 @@ def get_notification(service_id, notification_id): return Notification.query.filter_by(service_id=service_id, id=notification_id).one() +def get_notification_by_id(notification_id): + return Notification.query.filter_by(id=notification_id).first() + + def get_notifications_for_service(service_id, page=1): query = Notification.query.filter_by(service_id=service_id).order_by(desc(Notification.created_at)).paginate( page=page, diff --git a/app/models.py b/app/models.py index d4784d489..f5dc5fb86 100644 --- a/app/models.py +++ b/app/models.py @@ -231,7 +231,7 @@ class VerifyCode(db.Model): return check_hash(cde, self._code) -NOTIFICATION_STATUS_TYPES = ['sent', 'failed'] +NOTIFICATION_STATUS_TYPES = ['sent', 'delivered', 'failed'] class Notification(db.Model): diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 1fc35fe8e..ffe9f5085 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,4 +1,5 @@ from datetime import datetime +import uuid from flask import ( Blueprint, @@ -9,7 +10,7 @@ from flask import ( ) from utils.template import Template - +from app.clients.sms.firetext import firetext_response_status from app import api_user, encryption, create_uuid, DATETIME_FORMAT, DATE_FORMAT from app.authentication.auth import require_admin from app.dao import ( @@ -32,6 +33,67 @@ from app.errors import register_errors register_errors(notifications) +@notifications.route('/notifications/sms/firetext', methods=['POST']) +def process_firetext_response(): + if 'status' not in request.form: + current_app.logger.info( + "Firetext callback failed: status missing" + ) + return jsonify(result="error", message="Firetext callback failed: status missing"), 400 + + if len(request.form.get('reference', '')) <= 0: + current_app.logger.info( + "Firetext callback with no reference" + ) + return jsonify(result="success", message="Firetext callback succeeded"), 200 + + notification_id = request.form['reference'] + status = request.form['status'] + + try: + uuid.UUID(notification_id, version=4) + except ValueError: + current_app.logger.info( + "Firetext callback with invalid reference {}".format(notification_id) + ) + return jsonify( + result="error", message="Firetext callback with invalid reference {}".format(notification_id) + ), 400 + + notification_status = firetext_response_status.get(status, None) + if not notification_status: + current_app.logger.info( + "Firetext callback failed: status {} not found.".format(status) + ) + return jsonify(result="error", message="Firetext callback failed: status {} not found.".format(status)), 400 + + notification = notifications_dao.get_notification_by_id(notification_id) + if not notification: + current_app.logger.info( + "Firetext callback failed: notification {} not found. Status {}".format(notification_id, status) + ) + return jsonify( + result="error", + message="Firetext callback failed: notification {} not found. Status {}".format( + notification_id, + notification_status['firetext_message'] + ) + ), 404 + + if not notification_status['success']: + current_app.logger.info( + "Firetext delivery failed: notification {} has error found. Status {}".format( + notification_id, + firetext_response_status[status]['firetext_message'] + ) + ) + notification.status = notification_status['notify_status'] + notifications_dao.dao_update_notification(notification) + return jsonify( + result="success", message="Firetext callback succeeded. reference {} updated".format(notification_id) + ), 200 + + @notifications.route('/notifications/', methods=['GET']) def get_notifications(notification_id): try: diff --git a/migrations/versions/0039_more_notification_states.py b/migrations/versions/0039_more_notification_states.py new file mode 100644 index 000000000..495c1ea21 --- /dev/null +++ b/migrations/versions/0039_more_notification_states.py @@ -0,0 +1,31 @@ +"""empty message + +Revision ID: 0039_more_notification_states +Revises: 0038_reduce_limits +Create Date: 2016-03-08 11:16:25.659463 + +""" + +# revision identifiers, used by Alembic. +revision = '0039_more_notification_states' +down_revision = '0038_reduce_limits' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + + +def upgrade(): + op.drop_column('notifications', 'status') + op.execute('DROP TYPE notification_status_types') + notification_status_types = sa.Enum('sent', 'delivered', 'failed', name='notification_status_types') + notification_status_types.create(op.get_bind()) + op.add_column('notifications', sa.Column('status', notification_status_types, nullable=True)) + op.get_bind() + op.execute("update notifications set status='delivered'") + op.alter_column('notifications', 'status', nullable=False) + + +def downgrade(): + op.drop_column('notifications', 'status') + op.execute('DROP TYPE notification_status_types') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 65abc977e..a185eadf2 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -35,6 +35,10 @@ from tests.app.conftest import ( ) +def firetext_error(): + return {'code': 0, 'description': 'error'} + + def test_should_call_delete_successful_notifications_in_task(notify_api, mocker): mocker.patch('app.celery.tasks.delete_successful_notifications_created_more_than_a_day_ago') delete_successful_notifications() @@ -494,7 +498,7 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa "to": "+441234123123" } mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.firetext_client.send_sms', side_effect=FiretextClientException()) + mocker.patch('app.firetext_client.send_sms', side_effect=FiretextClientException(firetext_error())) mocker.patch('app.firetext_client.get_name', return_value="firetext") now = datetime.utcnow() @@ -623,7 +627,7 @@ def test_should_throw_firetext_client_exception(mocker): 'secret_code': '12345'} encrypted_notification = encryption.encrypt(notification) - mocker.patch('app.firetext_client.send_sms', side_effect=FiretextClientException) + mocker.patch('app.firetext_client.send_sms', side_effect=FiretextClientException(firetext_error())) send_sms_code(encrypted_notification) firetext_client.send_sms.assert_called_once_with(notification['to'], notification['secret_code']) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index cdc7c1957..88916ab89 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -7,6 +7,7 @@ from flask import json from app.models import Service from app.dao.templates_dao import dao_get_all_templates_for_service from app.dao.services_dao import dao_update_service +from app.dao.notifications_dao import get_notification_by_id from freezegun import freeze_time @@ -858,3 +859,174 @@ def test_should_allow_api_call_if_under_day_limit_regardless_of_type(notify_db, headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 + + +def test_firetext_callback_should_not_need_auth(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=0&reference=&time=2016-03-10 14:17:00', + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + assert response.status_code == 200 + + +def test_firetext_callback_should_return_200_if_empty_reference(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=0&reference=&time=2016-03-10 14:17:00', + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_resp['result'] == 'success' + assert json_resp['message'] == 'Firetext callback succeeded' + + +def test_firetext_callback_should_return_200_if_no_reference(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=0&time=2016-03-10 14:17:00', + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_resp['result'] == 'success' + assert json_resp['message'] == 'Firetext callback succeeded' + + +def test_firetext_callback_should_return_400_if_no_status(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&time=2016-03-10 14:17:00', + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'Firetext callback failed: status missing' + + +def test_firetext_callback_should_return_400_if_unknown_status(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=99&time=2016-03-10 14:17:00&reference={}'.format(uuid.uuid4()), + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'Firetext callback failed: status 99 not found.' + + +def test_firetext_callback_should_return_400_if_invalid_guid_notification_id(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=0&time=2016-03-10 14:17:00&reference=1234', + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'Firetext callback with invalid reference 1234' + + +def test_firetext_callback_should_return_404_if_cannot_find_notification_id(notify_db, notify_db_session, notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + missing_notification_id = uuid.uuid4() + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=0&time=2016-03-10 14:17:00&reference={}'.format( + missing_notification_id + ), + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'Firetext callback failed: notification {} not found. Status {}'.format( + missing_notification_id, + 'delivered' + ) + + +def test_firetext_callback_should_update_notification_status(notify_api, sample_notification): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + original = get_notification_by_id(sample_notification.id) + assert original.status == 'sent' + + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=0&time=2016-03-10 14:17:00&reference={}'.format( + sample_notification.id + ), + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_resp['result'] == 'success' + assert json_resp['message'] == 'Firetext callback succeeded. reference {} updated'.format( + sample_notification.id + ) + updated = get_notification_by_id(sample_notification.id) + assert updated.status == 'delivered' + + +def test_firetext_callback_should_update_notification_status_failed(notify_api, sample_notification): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + original = get_notification_by_id(sample_notification.id) + assert original.status == 'sent' + + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=1&time=2016-03-10 14:17:00&reference={}'.format( + sample_notification.id + ), + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_resp['result'] == 'success' + assert json_resp['message'] == 'Firetext callback succeeded. reference {} updated'.format( + sample_notification.id + ) + updated = get_notification_by_id(sample_notification.id) + assert updated.status == 'failed' + + +def test_firetext_callback_should_update_notification_status_sent(notify_api, notify_db, notify_db_session): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + notification = sample_notification(notify_db, notify_db_session, status='delivered') + original = get_notification_by_id(notification.id) + assert original.status == 'delivered' + + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=2&time=2016-03-10 14:17:00&reference={}'.format( + notification.id + ), + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_resp['result'] == 'success' + assert json_resp['message'] == 'Firetext callback succeeded. reference {} updated'.format( + notification.id + ) + updated = get_notification_by_id(notification.id) + assert updated.status == 'sent' From 2922712f0ba465a601b86c567a4bb99e82078aad Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 10 Mar 2016 15:51:11 +0000 Subject: [PATCH 03/10] Make sms code task use a reference too - makes the fire text callback behave in consistent way --- app/celery/tasks.py | 4 +++- app/clients/sms/firetext.py | 10 +++------- app/notifications/rest.py | 23 +++++++++++++---------- tests/app/celery/test_tasks.py | 4 ++-- tests/app/notifications/test_rest.py | 28 +++++++++++++++++++++------- 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 9a29ec3d2..1808cd7f0 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -295,7 +295,9 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not def send_sms_code(encrypted_verification): verification_message = encryption.decrypt(encrypted_verification) try: - firetext_client.send_sms(verification_message['to'], verification_message['secret_code']) + firetext_client.send_sms( + verification_message['to'], verification_message['secret_code'], 'send-sms-code' + ) except FiretextClientException as e: current_app.logger.exception(e) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 9b3793e2c..5b41c2811 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -51,20 +51,16 @@ class FiretextClient(SmsClient): def get_name(self): return self.name - def send_sms(self, to, content, notification_id=None): + def send_sms(self, to, content, reference): data = { "apiKey": self.api_key, "from": self.from_number, "to": to.replace('+', ''), - "message": content + "message": content, + "reference": reference } - if notification_id: - data.update({ - "reference": notification_id - }) - start_time = monotonic() try: response = request( diff --git a/app/notifications/rest.py b/app/notifications/rest.py index ffe9f5085..67f7a35bc 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -45,19 +45,22 @@ def process_firetext_response(): current_app.logger.info( "Firetext callback with no reference" ) - return jsonify(result="success", message="Firetext callback succeeded"), 200 + return jsonify(result="error", message="Firetext callback failed: reference missing"), 400 - notification_id = request.form['reference'] + reference = request.form['reference'] status = request.form['status'] + if reference == 'send-sms-code': + return jsonify(result="success", message="Firetext callback succeeded: send-sms-code"), 200 + try: - uuid.UUID(notification_id, version=4) + uuid.UUID(reference, version=4) except ValueError: current_app.logger.info( - "Firetext callback with invalid reference {}".format(notification_id) + "Firetext callback with invalid reference {}".format(reference) ) return jsonify( - result="error", message="Firetext callback with invalid reference {}".format(notification_id) + result="error", message="Firetext callback with invalid reference {}".format(reference) ), 400 notification_status = firetext_response_status.get(status, None) @@ -67,15 +70,15 @@ def process_firetext_response(): ) return jsonify(result="error", message="Firetext callback failed: status {} not found.".format(status)), 400 - notification = notifications_dao.get_notification_by_id(notification_id) + notification = notifications_dao.get_notification_by_id(reference) if not notification: current_app.logger.info( - "Firetext callback failed: notification {} not found. Status {}".format(notification_id, status) + "Firetext callback failed: notification {} not found. Status {}".format(reference, status) ) return jsonify( result="error", message="Firetext callback failed: notification {} not found. Status {}".format( - notification_id, + reference, notification_status['firetext_message'] ) ), 404 @@ -83,14 +86,14 @@ def process_firetext_response(): if not notification_status['success']: current_app.logger.info( "Firetext delivery failed: notification {} has error found. Status {}".format( - notification_id, + reference, firetext_response_status[status]['firetext_message'] ) ) notification.status = notification_status['notify_status'] notifications_dao.dao_update_notification(notification) return jsonify( - result="success", message="Firetext callback succeeded. reference {} updated".format(notification_id) + result="success", message="Firetext callback succeeded. reference {} updated".format(reference) ), 200 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index a185eadf2..f596eb034 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -619,7 +619,7 @@ def test_should_send_sms_code(mocker): mocker.patch('app.firetext_client.send_sms') send_sms_code(encrypted_notification) - firetext_client.send_sms.assert_called_once_with(notification['to'], notification['secret_code']) + firetext_client.send_sms.assert_called_once_with(notification['to'], notification['secret_code'], 'send-sms-code') def test_should_throw_firetext_client_exception(mocker): @@ -629,7 +629,7 @@ def test_should_throw_firetext_client_exception(mocker): encrypted_notification = encryption.encrypt(notification) mocker.patch('app.firetext_client.send_sms', side_effect=FiretextClientException(firetext_error())) send_sms_code(encrypted_notification) - firetext_client.send_sms.assert_called_once_with(notification['to'], notification['secret_code']) + firetext_client.send_sms.assert_called_once_with(notification['to'], notification['secret_code'], 'send-sms-code') def test_should_send_email_code(mocker): diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 88916ab89..91940cf04 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -866,13 +866,13 @@ def test_firetext_callback_should_not_need_auth(notify_api): with notify_api.test_client() as client: response = client.post( path='/notifications/sms/firetext', - data='mobile=441234123123&status=0&reference=&time=2016-03-10 14:17:00', + data='mobile=441234123123&status=0&reference=send-sms-code&time=2016-03-10 14:17:00', headers=[('Content-Type', 'application/x-www-form-urlencoded')]) assert response.status_code == 200 -def test_firetext_callback_should_return_200_if_empty_reference(notify_api): +def test_firetext_callback_should_return_400_if_empty_reference(notify_api): with notify_api.test_request_context(): with notify_api.test_client() as client: response = client.post( @@ -881,12 +881,12 @@ def test_firetext_callback_should_return_200_if_empty_reference(notify_api): headers=[('Content-Type', 'application/x-www-form-urlencoded')]) json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert json_resp['result'] == 'success' - assert json_resp['message'] == 'Firetext callback succeeded' + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'Firetext callback failed: reference missing' -def test_firetext_callback_should_return_200_if_no_reference(notify_api): +def test_firetext_callback_should_return_400_if_no_reference(notify_api): with notify_api.test_request_context(): with notify_api.test_client() as client: response = client.post( @@ -894,10 +894,24 @@ def test_firetext_callback_should_return_200_if_no_reference(notify_api): data='mobile=441234123123&status=0&time=2016-03-10 14:17:00', headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'Firetext callback failed: reference missing' + + +def test_firetext_callback_should_return_200_if_send_sms_reference(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post( + path='/notifications/sms/firetext', + data='mobile=441234123123&status=0&time=2016-03-10 14:17:00&reference=send-sms-code', + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 assert json_resp['result'] == 'success' - assert json_resp['message'] == 'Firetext callback succeeded' + assert json_resp['message'] == 'Firetext callback succeeded: send-sms-code' def test_firetext_callback_should_return_400_if_no_status(notify_api): From f88f86a924b55b725470d2b64ae58c20d69e42fc Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 10 Mar 2016 17:29:17 +0000 Subject: [PATCH 04/10] Endpoint to allow SES updates to occur - update notification with delivery state --- app/__init__.py | 7 +- app/clients/email/aws_ses.py | 15 +++ app/dao/notifications_dao.py | 22 +++++ app/models.py | 2 +- app/notifications/rest.py | 73 +++++++++++++- .../versions/0039_more_notification_states.py | 2 +- test_ses_responses/ses_response.json | 32 ++++++ tests/app/__init__.py | 6 ++ tests/app/dao/test_notification_dao.py | 28 +++++- tests/app/notifications/test_rest.py | 98 +++++++++++++++++++ 10 files changed, 275 insertions(+), 10 deletions(-) create mode 100644 test_ses_responses/ses_response.json diff --git a/app/__init__.py b/app/__init__.py index 3c06ac77c..3b92f9265 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -71,7 +71,12 @@ def create_app(): def init_app(app): @app.before_request def required_authentication(): - if request.path not in [url_for('status.show_status'), url_for('notifications.process_firetext_response')]: + no_auth_req = [ + url_for('status.show_status'), + url_for('notifications.process_ses_response'), + url_for('notifications.process_firetext_response') + ] + if request.path not in no_auth_req: from app.authentication import auth error = auth.requires_auth() if error: diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index eeeeb51ac..4255f69e8 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -3,6 +3,21 @@ from flask import current_app from monotonic import monotonic from app.clients.email import (EmailClientException, EmailClient) +ses_response_status = { + 'Bounce': { + "success": False, + "notify_status": 'bounce' + }, + 'Delivery': { + "success": True, + "notify_status": 'delivered' + }, + 'Complaint': { + "success": False, + "notify_status": 'complaint' + } +} + class AwsSesClientException(EmailClientException): pass diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 0c5109233..bb95c5adb 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -68,6 +68,28 @@ def dao_update_notification(notification): db.session.commit() +def update_notification_status_by_id(notification_id, status): + count = db.session.query(Notification).filter_by( + id=notification_id + ).update({ + Notification.status: status, + Notification.updated_at: datetime.utcnow() + }) + db.session.commit() + return count + + +def update_notification_status_by_to(to, status): + count = db.session.query(Notification).filter_by( + to=to + ).update({ + Notification.status: status, + Notification.updated_at: datetime.utcnow() + }) + db.session.commit() + return count + + def get_notification_for_job(service_id, job_id, notification_id): return Notification.query.filter_by(service_id=service_id, job_id=job_id, id=notification_id).one() diff --git a/app/models.py b/app/models.py index f5dc5fb86..9a94a6191 100644 --- a/app/models.py +++ b/app/models.py @@ -231,7 +231,7 @@ class VerifyCode(db.Model): return check_hash(cde, self._code) -NOTIFICATION_STATUS_TYPES = ['sent', 'delivered', 'failed'] +NOTIFICATION_STATUS_TYPES = ['sent', 'delivered', 'failed', 'complaint', 'bounce'] class Notification(db.Model): diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 67f7a35bc..422fd201b 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,4 +1,5 @@ from datetime import datetime +from json import JSONDecodeError import uuid from flask import ( @@ -6,11 +7,13 @@ from flask import ( jsonify, request, current_app, - url_for + url_for, + json ) from utils.template import Template from app.clients.sms.firetext import firetext_response_status +from app.clients.email.aws_ses import ses_response_status from app import api_user, encryption, create_uuid, DATETIME_FORMAT, DATE_FORMAT from app.authentication.auth import require_admin from app.dao import ( @@ -33,6 +36,69 @@ from app.errors import register_errors register_errors(notifications) +@notifications.route('/notifications/email/ses', methods=['POST']) +def process_ses_response(): + try: + ses_request = json.loads(request.data) + + if 'Message' not in ses_request: + current_app.logger.error( + "SES callback failed: message missing" + ) + return jsonify( + result="error", message="SES callback failed: message missing" + ), 400 + + if 'notificationType' not in ses_request['Message']: + current_app.logger.error( + "SES callback failed: notificationType missing" + ) + return jsonify( + result="error", message="SES callback failed: notificationType missing" + ), 400 + + status = ses_response_status.get(ses_request['Message']['notificationType'], None) + if not status: + current_app.logger.info( + "SES callback failed: status {} not found.".format(status) + ) + return jsonify( + result="error", + message="SES callback failed: status {} not found".format(ses_request['Message']['notificationType']) + ), 400 + + try: + recipients = ses_request['Message']['mail']['destination'] + + if notifications_dao.update_notification_status_by_to(recipients[0], status['notify_status']) == 0: + current_app.logger.info( + "SES callback failed: notification not found. Status {}".format(status['notify_status']) + ) + return jsonify( + result="error", + message="SES callback failed: notification not found. Status {}".format(status['notify_status']) + ), 404 + return jsonify( + result="success", message="SES callback succeeded" + ), 200 + + except KeyError: + current_app.logger.error( + "SES callback failed: destination missing" + ) + return jsonify( + result="error", message="SES callback failed: destination missing" + ), 400 + + except JSONDecodeError as ex: + current_app.logger.error( + "SES callback failed: invalid json" + ) + return jsonify( + result="error", message="SES callback failed: invalid json" + ), 400 + + @notifications.route('/notifications/sms/firetext', methods=['POST']) def process_firetext_response(): if 'status' not in request.form: @@ -70,8 +136,7 @@ def process_firetext_response(): ) return jsonify(result="error", message="Firetext callback failed: status {} not found.".format(status)), 400 - notification = notifications_dao.get_notification_by_id(reference) - if not notification: + if notifications_dao.update_notification_status_by_id(reference, notification_status['notify_status']) == 0: current_app.logger.info( "Firetext callback failed: notification {} not found. Status {}".format(reference, status) ) @@ -90,8 +155,6 @@ def process_firetext_response(): firetext_response_status[status]['firetext_message'] ) ) - notification.status = notification_status['notify_status'] - notifications_dao.dao_update_notification(notification) return jsonify( result="success", message="Firetext callback succeeded. reference {} updated".format(reference) ), 200 diff --git a/migrations/versions/0039_more_notification_states.py b/migrations/versions/0039_more_notification_states.py index 495c1ea21..7ac99e6ab 100644 --- a/migrations/versions/0039_more_notification_states.py +++ b/migrations/versions/0039_more_notification_states.py @@ -18,7 +18,7 @@ from sqlalchemy.dialects import postgresql def upgrade(): op.drop_column('notifications', 'status') op.execute('DROP TYPE notification_status_types') - notification_status_types = sa.Enum('sent', 'delivered', 'failed', name='notification_status_types') + notification_status_types = sa.Enum('sent', 'delivered', 'failed', 'complaint', 'bounce', name='notification_status_types') notification_status_types.create(op.get_bind()) op.add_column('notifications', sa.Column('status', notification_status_types, nullable=True)) op.get_bind() diff --git a/test_ses_responses/ses_response.json b/test_ses_responses/ses_response.json new file mode 100644 index 000000000..92e5ffe38 --- /dev/null +++ b/test_ses_responses/ses_response.json @@ -0,0 +1,32 @@ +{ + "MessageId": "35efe472-ba36-5808-89bb-ab2925158b13", + "UnsubscribeURL": "https://sns.eu-west-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:eu-west-1:123456789012:preview-emails:12345678-1234-1234-1234-123456789012", + "SignatureVersion": "1", + "TopicArn": "arn:aws:sns:eu-west-1:123456789012:testing", + "Timestamp": "2016-03-10T16:12:19.876Z", + "Type": "Notification", + "Signature": "sig", + "Message": { + "notificationType": "Delivery", + "mail": { + "timestamp": "2016-03-10T16:12:19.016Z", + "source": "invites@testing-notify.com", + "sourceArn": "arn:aws:ses:eu-west-1:123456789012:identity/testing-notify", + "sendingAccountId": "123456789012", + "messageId": "01020153614cd6c8-2ec4bd32-7ddc-4344-811e-65d05519251f-000000", + "destination": [ + "testing@digital.cabinet-office.gov.uk" + ] + }, + "delivery": { + "timestamp": "2016-03-10T16:12:19.751Z", + "processingTimeMillis": 735, + "recipients": [ + "testing@digital.cabinet-office.gov.uk" + ], + "smtpResponse": "250 2.0.0 OK 1457626339 u62si5491824wme.91 - gsmtp", + "reportingMTA": "a6-15.smtp-out.eu-west-1.amazonses.com" + } + }, + "SigningCertURL": "https://sns.eu-west-1.amazonaws.com/SimpleNotificationService-bb750dd426d95ee9390147a5624348ee.pem" +} \ No newline at end of file diff --git a/tests/app/__init__.py b/tests/app/__init__.py index 75631cf9a..350e701f8 100644 --- a/tests/app/__init__.py +++ b/tests/app/__init__.py @@ -5,3 +5,9 @@ def load_example_csv(file): file_path = os.path.join("test_csv_files", "{}.csv".format(file)) with open(file_path) as f: return f.read() + + +def load_example_ses(file): + file_path = os.path.join("test_ses_responses", "{}.json".format(file)) + with open(file_path) as f: + return f.read() diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index e3c32e05c..02b626d5c 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -16,12 +16,36 @@ from app.dao.notifications_dao import ( dao_get_notification_statistics_for_service, delete_successful_notifications_created_more_than_a_day_ago, delete_failed_notifications_created_more_than_a_week_ago, - dao_get_notification_statistics_for_service_and_day + dao_get_notification_statistics_for_service_and_day, + update_notification_status_by_id, + update_notification_status_by_to ) from tests.app.conftest import sample_job from tests.app.conftest import sample_notification +def test_should_by_able_to_update_status_by_id(sample_notification): + assert Notification.query.get(sample_notification.id).status == 'sent' + count = update_notification_status_by_id(sample_notification.id, 'delivered') + assert count == 1 + assert Notification.query.get(sample_notification.id).status == 'delivered' + + +def test_should_return_zero_count_if_no_notification_with_id(): + assert update_notification_status_by_id(str(uuid.uuid4()), 'delivered') == 0 + + +def test_should_return_zero_count_if_no_notification_with_to(): + assert update_notification_status_by_to('something', 'delivered') == 0 + + +def test_should_by_able_to_update_status_by_to(sample_notification): + assert Notification.query.get(sample_notification.id).status == 'sent' + count = update_notification_status_by_to(sample_notification.to, 'delivered') + assert count == 1 + assert Notification.query.get(sample_notification.id).status == 'delivered' + + def test_should_be_able_to_get_statistics_for_a_service(sample_template): data = { 'to': '+44709123456', @@ -568,7 +592,7 @@ def test_should_not_delete_sent_notifications_before_one_day(notify_db, notify_d def test_should_not_delete_failed_notifications_before_seven_days(notify_db, notify_db_session): expired = datetime.utcnow() - timedelta(hours=24 * 7) - valid = datetime.utcnow() - timedelta(hours=(24 * 6) + 23, minutes=59, seconds=59) + valid = datetime.utcnow() - timedelta(hours=(24 * 6) + 23, minutes=59, seconds=59) sample_notification(notify_db, notify_db_session, created_at=expired, status="failed", to_field="expired") sample_notification(notify_db, notify_db_session, created_at=valid, status="failed", to_field="valid") assert len(Notification.query.all()) == 2 diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 91940cf04..a1628532f 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -9,6 +9,7 @@ from app.dao.templates_dao import dao_get_all_templates_for_service from app.dao.services_dao import dao_update_service from app.dao.notifications_dao import get_notification_by_id from freezegun import freeze_time +from tests.app import load_example_ses def test_get_notification_by_id(notify_api, sample_notification): @@ -1044,3 +1045,100 @@ def test_firetext_callback_should_update_notification_status_sent(notify_api, no ) updated = get_notification_by_id(notification.id) assert updated.status == 'sent' + + +def test_ses_callback_should_not_need_auth(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post( + path='/notifications/email/ses', + data=load_example_ses('ses_response'), + headers=[('Content-Type', 'text/plain; charset=UTF-8')] + ) + assert response.status_code == 404 + + +def test_ses_callback_should_fail_if_invalid_json(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post( + path='/notifications/email/ses', + data="nonsense", + headers=[('Content-Type', 'text/plain; charset=UTF-8')] + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'SES callback failed: invalid json' + + +def test_ses_callback_should_fail_if_invalid_notification_type(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + ses_response = json.loads(load_example_ses('ses_response')) + ses_response['Message']['notificationType'] = 'Unknown' + + response = client.post( + path='/notifications/email/ses', + data=json.dumps(ses_response), + headers=[('Content-Type', 'text/plain; charset=UTF-8')] + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'SES callback failed: status Unknown not found' + + +def test_ses_callback_should_fail_if_missing_destination(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + ses_response = json.loads(load_example_ses('ses_response')) + del(ses_response['Message']['mail']['destination']) + + response = client.post( + path='/notifications/email/ses', + data=json.dumps(ses_response), + headers=[('Content-Type', 'text/plain; charset=UTF-8')] + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'SES callback failed: destination missing' + + +def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, notify_db_session, notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + ses_response = json.loads(load_example_ses('ses_response')) + ses_response['Message']['mail']['destination'] = ['wont find this'] + + response = client.post( + path='/notifications/email/ses', + data=json.dumps(ses_response), + headers=[('Content-Type', 'text/plain; charset=UTF-8')] + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'SES callback failed: notification not found. Status delivered' + + +def test_ses_callback_should_update_notification_status(notify_api, sample_notification): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + assert get_notification_by_id(sample_notification.id).status == 'sent' + + ses_response = json.loads(load_example_ses('ses_response')) + ses_response['Message']['mail']['destination'] = [sample_notification.to] + + response = client.post( + path='/notifications/email/ses', + data=json.dumps(ses_response), + headers=[('Content-Type', 'text/plain; charset=UTF-8')] + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_resp['result'] == 'success' + assert json_resp['message'] == 'SES callback succeeded' + assert get_notification_by_id(sample_notification.id).status == 'delivered' From 2d3364d94609bc46896bf8c82de0c61b57049863 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 11 Mar 2016 08:37:04 +0000 Subject: [PATCH 05/10] Fixing import path to JSON decode exception --- app/notifications/rest.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 422fd201b..cf2344d52 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,5 +1,4 @@ from datetime import datetime -from json import JSONDecodeError import uuid from flask import ( @@ -11,6 +10,8 @@ from flask import ( json ) +from json.decoder import JSONDecodeError + from utils.template import Template from app.clients.sms.firetext import firetext_response_status from app.clients.email.aws_ses import ses_response_status @@ -91,8 +92,8 @@ def process_ses_response(): ), 400 except JSONDecodeError as ex: - current_app.logger.error( - "SES callback failed: invalid json" + current_app.logger.exception( + "SES callback failed: invalid json {}".format(ex) ) return jsonify( result="error", message="SES callback failed: invalid json" From 58c78f864bf520ce04b125e21dbf27d18c95ccb3 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 11 Mar 2016 09:00:02 +0000 Subject: [PATCH 06/10] Changed path to decode error for 3.4.x compatibility --- app/notifications/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index cf2344d52..290dbebae 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -10,7 +10,7 @@ from flask import ( json ) -from json.decoder import JSONDecodeError +from json import JSONDecodeError from utils.template import Template from app.clients.sms.firetext import firetext_response_status From 8d9b5b172b0ec38a3097591fbc99c240230f449e Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 11 Mar 2016 09:06:22 +0000 Subject: [PATCH 07/10] Changed exception type to ValueError --- app/notifications/rest.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 290dbebae..38b4dbb15 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -10,8 +10,6 @@ from flask import ( json ) -from json import JSONDecodeError - from utils.template import Template from app.clients.sms.firetext import firetext_response_status from app.clients.email.aws_ses import ses_response_status @@ -91,7 +89,7 @@ def process_ses_response(): result="error", message="SES callback failed: destination missing" ), 400 - except JSONDecodeError as ex: + except ValueError as ex: current_app.logger.exception( "SES callback failed: invalid json {}".format(ex) ) From 901d04605f576e421cce32b1161b66ab0d4bc72a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 11 Mar 2016 09:40:35 +0000 Subject: [PATCH 08/10] Ad a reference to the model - used if 3rd party needs to record an ID for reconciliation purposes --- app/celery/tasks.py | 8 +++-- app/dao/notifications_dao.py | 23 ++++++++++++++ app/models.py | 1 + migrations/versions/0040_add_reference.py | 24 ++++++++++++++ tests/app/celery/test_tasks.py | 38 +++++++++++++++++++---- tests/app/dao/test_notification_dao.py | 10 +++++- 6 files changed, 94 insertions(+), 10 deletions(-) create mode 100644 migrations/versions/0040_add_reference.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 1808cd7f0..109d010b7 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -9,7 +9,8 @@ from app.dao.notifications_dao import ( dao_update_notification, delete_failed_notifications_created_more_than_a_week_ago, delete_successful_notifications_created_more_than_a_day_ago, - dao_get_notification_statistics_for_service_and_day + dao_get_notification_statistics_for_service_and_day, + update_notification_reference_by_id ) from app.dao.jobs_dao import dao_update_job, dao_get_job_by_id from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago @@ -205,7 +206,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): client.send_sms( to=notification['to'], content=template.replaced, - notification_id=notification_id + reference=str(notification_id) ) except FiretextClientException as e: current_app.logger.error( @@ -273,12 +274,13 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not values=notification.get('personalisation', {}) ) - client.send_email( + reference = client.send_email( from_address, notification['to'], subject, template.replaced ) + update_notification_reference_by_id(notification_id, reference) except AwsSesClientException as e: current_app.logger.debug(e) notification_db_object.status = 'failed' diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index bb95c5adb..6389f04fd 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -64,6 +64,7 @@ def update_job_sent_count(notification): def dao_update_notification(notification): + notification.updated_at = datetime.utcnow() db.session.add(notification) db.session.commit() @@ -90,6 +91,28 @@ def update_notification_status_by_to(to, status): return count +def update_notification_status_by_reference(reference, status): + count = db.session.query(Notification).filter_by( + refernce=reference + ).update({ + Notification.status: status, + Notification.updated_at: datetime.utcnow() + }) + db.session.commit() + return count + + +def update_notification_reference_by_id(id, reference): + count = db.session.query(Notification).filter_by( + id=id + ).update({ + Notification.reference: reference, + Notification.updated_at: datetime.utcnow() + }) + db.session.commit() + return count + + def get_notification_for_job(service_id, job_id, notification_id): return Notification.query.filter_by(service_id=service_id, job_id=job_id, id=notification_id).one() diff --git a/app/models.py b/app/models.py index 9a94a6191..8184a08ba 100644 --- a/app/models.py +++ b/app/models.py @@ -265,6 +265,7 @@ class Notification(db.Model): onupdate=datetime.datetime.now) status = db.Column( db.Enum(*NOTIFICATION_STATUS_TYPES, name='notification_status_types'), nullable=False, default='sent') + reference = db.Column(db.String, nullable=True, index=True) INVITED_USER_STATUS_TYPES = ['pending', 'accepted', 'cancelled'] diff --git a/migrations/versions/0040_add_reference.py b/migrations/versions/0040_add_reference.py new file mode 100644 index 000000000..65a6ad785 --- /dev/null +++ b/migrations/versions/0040_add_reference.py @@ -0,0 +1,24 @@ +"""empty message + +Revision ID: 0040_add_reference +Revises: 0039_more_notification_states +Create Date: 2016-03-11 09:15:57.900192 + +""" + +# revision identifiers, used by Alembic. +revision = '0040_add_reference' +down_revision = '0039_more_notification_states' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('notifications', sa.Column('reference', sa.String(), nullable=True)) + op.create_index(op.f('ix_notifications_reference'), 'notifications', ['reference'], unique=False) + + +def downgrade(): + op.drop_index(op.f('ix_notifications_reference'), table_name='notifications') + op.drop_column('notifications', 'reference') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index f596eb034..66f45c28e 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -267,7 +267,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat firetext_client.send_sms.assert_called_once_with( to="+441234123123", content="Sample service: Hello Jo", - notification_id=notification_id + reference=str(notification_id) ) persisted_notification = notifications_dao.get_notification( sample_template_with_placeholders.service_id, notification_id @@ -303,7 +303,7 @@ def test_should_send_sms_without_personalisation(sample_template, mocker): firetext_client.send_sms.assert_called_once_with( to="+441234123123", content="Sample service: This is a template", - notification_id=notification_id + reference=str(notification_id) ) @@ -332,7 +332,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif firetext_client.send_sms.assert_called_once_with( to="+441234123123", content="Sample service: This is a template", - notification_id=notification_id + reference=str(notification_id) ) @@ -413,7 +413,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa firetext_client.send_sms.assert_called_once_with( to="+441234123123", content="Sample service: This is a template", - notification_id=notification_id + reference=str(notification_id) ) persisted_notification = notifications_dao.get_notification(sample_job.template.service_id, notification_id) assert persisted_notification.id == notification_id @@ -464,6 +464,31 @@ 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_ses_reference(sample_email_template_with_placeholders, mocker): + notification = { + "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', return_value='reference') + + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_email( + sample_email_template_with_placeholders.service_id, + notification_id, + 'subject', + 'email_from', + "encrypted-in-reality", + now.strftime(DATETIME_FORMAT) + ) + persisted_notification = notifications_dao.get_notification( + sample_email_template_with_placeholders.service_id, notification_id + ) + assert persisted_notification.reference == 'reference' + + def test_should_use_email_template_and_persist_without_personalisation( sample_email_template, mocker ): @@ -471,7 +496,7 @@ def test_should_use_email_template_and_persist_without_personalisation( "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.send_email', return_value="ref") mocker.patch('app.aws_ses_client.get_name', return_value='ses') notification_id = uuid.uuid4() @@ -513,7 +538,8 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa firetext_client.send_sms.assert_called_once_with( to="+441234123123", content="Sample service: This is a template", - notification_id=notification_id) + reference=str(notification_id) + ) 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/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 02b626d5c..eab9154ce 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -18,12 +18,20 @@ from app.dao.notifications_dao import ( delete_failed_notifications_created_more_than_a_week_ago, dao_get_notification_statistics_for_service_and_day, update_notification_status_by_id, - update_notification_status_by_to + update_notification_status_by_to, + update_notification_reference_by_id ) from tests.app.conftest import sample_job from tests.app.conftest import sample_notification +def test_should_by_able_to_update_reference_by_id(sample_notification): + assert not Notification.query.get(sample_notification.id).reference + count = update_notification_reference_by_id(sample_notification.id, 'reference') + assert count == 1 + assert Notification.query.get(sample_notification.id).reference == 'reference' + + def test_should_by_able_to_update_status_by_id(sample_notification): assert Notification.query.get(sample_notification.id).status == 'sent' count = update_notification_status_by_id(sample_notification.id, 'delivered') From 62a7b8bcd0a1ab65bd83e850226fdc2fa3b5829d Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 11 Mar 2016 10:19:40 +0000 Subject: [PATCH 09/10] Update notification status by message reference - SES sends a reference to allow us to identify the notification - use this to update status If source of email is one of our internal emails (invites or validations) - don't try and update a notification. --- app/dao/notifications_dao.py | 13 +----- app/notifications/rest.py | 28 ++++++++++-- tests/app/conftest.py | 2 + tests/app/dao/test_notification_dao.py | 22 ++++----- tests/app/notifications/test_rest.py | 62 ++++++++++++++++++++++---- 5 files changed, 92 insertions(+), 35 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6389f04fd..be36ee1fc 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -80,20 +80,9 @@ def update_notification_status_by_id(notification_id, status): return count -def update_notification_status_by_to(to, status): - count = db.session.query(Notification).filter_by( - to=to - ).update({ - Notification.status: status, - Notification.updated_at: datetime.utcnow() - }) - db.session.commit() - return count - - def update_notification_status_by_reference(reference, status): count = db.session.query(Notification).filter_by( - refernce=reference + reference=reference ).update({ Notification.status: status, Notification.updated_at: datetime.utcnow() diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 38b4dbb15..5756b9617 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -67,9 +67,17 @@ def process_ses_response(): ), 400 try: - recipients = ses_request['Message']['mail']['destination'] + source = ses_request['Message']['mail']['source'] + if is_not_a_notification(ses_request['Message']['mail']['source']): + current_app.logger.info( + "SES callback for notify success:. source {} status {}".format(source, status['notify_status']) + ) + return jsonify( + result="success", message="SES callback succeeded" + ), 200 - if notifications_dao.update_notification_status_by_to(recipients[0], status['notify_status']) == 0: + reference = ses_request['Message']['mail']['messageId'] + if notifications_dao.update_notification_status_by_reference(reference, status['notify_status']) == 0: current_app.logger.info( "SES callback failed: notification not found. Status {}".format(status['notify_status']) ) @@ -83,10 +91,10 @@ def process_ses_response(): except KeyError: current_app.logger.error( - "SES callback failed: destination missing" + "SES callback failed: messageId missing" ) return jsonify( - result="error", message="SES callback failed: destination missing" + result="error", message="SES callback failed: messageId missing" ), 400 except ValueError as ex: @@ -98,6 +106,18 @@ def process_ses_response(): ), 400 +def is_not_a_notification(source): + invite_email = "{}@{}".format( + current_app.config['INVITATION_EMAIL_FROM'], + current_app.config['NOTIFY_EMAIL_DOMAIN'] + ) + if current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'] == source: + return True + if invite_email == source: + return True + return False + + @notifications.route('/notifications/sms/firetext', methods=['POST']) def process_firetext_response(): if 'status' not in request.form: diff --git a/tests/app/conftest.py b/tests/app/conftest.py index ca637ae0b..37dc69d41 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -282,6 +282,7 @@ def sample_notification(notify_db, job=None, to_field=None, status='sent', + reference=None, created_at=datetime.utcnow()): if service is None: service = sample_service(notify_db, notify_db_session) @@ -305,6 +306,7 @@ def sample_notification(notify_db, 'service': service, 'template': template, 'status': status, + 'reference': reference, 'created_at': created_at } notification = Notification(**data) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index eab9154ce..d6a5a95b5 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -18,8 +18,8 @@ from app.dao.notifications_dao import ( delete_failed_notifications_created_more_than_a_week_ago, dao_get_notification_statistics_for_service_and_day, update_notification_status_by_id, - update_notification_status_by_to, - update_notification_reference_by_id + update_notification_reference_by_id, + update_notification_status_by_reference ) from tests.app.conftest import sample_job from tests.app.conftest import sample_notification @@ -32,6 +32,13 @@ def test_should_by_able_to_update_reference_by_id(sample_notification): assert Notification.query.get(sample_notification.id).reference == 'reference' +def test_should_by_able_to_update_status_by_reference(sample_notification): + assert Notification.query.get(sample_notification.id).status == "sent" + update_notification_reference_by_id(sample_notification.id, 'reference') + update_notification_status_by_reference('reference', 'delivered') + assert Notification.query.get(sample_notification.id).status == 'delivered' + + def test_should_by_able_to_update_status_by_id(sample_notification): assert Notification.query.get(sample_notification.id).status == 'sent' count = update_notification_status_by_id(sample_notification.id, 'delivered') @@ -43,15 +50,8 @@ def test_should_return_zero_count_if_no_notification_with_id(): assert update_notification_status_by_id(str(uuid.uuid4()), 'delivered') == 0 -def test_should_return_zero_count_if_no_notification_with_to(): - assert update_notification_status_by_to('something', 'delivered') == 0 - - -def test_should_by_able_to_update_status_by_to(sample_notification): - assert Notification.query.get(sample_notification.id).status == 'sent' - count = update_notification_status_by_to(sample_notification.to, 'delivered') - assert count == 1 - assert Notification.query.get(sample_notification.id).status == 'delivered' +def test_should_return_zero_count_if_no_notification_with_reference(): + assert update_notification_status_by_reference('something', 'delivered') == 0 def test_should_be_able_to_get_statistics_for_a_service(sample_template): diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index a1628532f..a7386a4d5 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -1089,11 +1089,11 @@ def test_ses_callback_should_fail_if_invalid_notification_type(notify_api): assert json_resp['message'] == 'SES callback failed: status Unknown not found' -def test_ses_callback_should_fail_if_missing_destination(notify_api): +def test_ses_callback_should_fail_if_missing_message_id(notify_api): with notify_api.test_request_context(): with notify_api.test_client() as client: ses_response = json.loads(load_example_ses('ses_response')) - del(ses_response['Message']['mail']['destination']) + del(ses_response['Message']['mail']['messageId']) response = client.post( path='/notifications/email/ses', @@ -1103,14 +1103,14 @@ def test_ses_callback_should_fail_if_missing_destination(notify_api): json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 400 assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: destination missing' + assert json_resp['message'] == 'SES callback failed: messageId missing' def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, notify_db_session, notify_api): with notify_api.test_request_context(): with notify_api.test_client() as client: ses_response = json.loads(load_example_ses('ses_response')) - ses_response['Message']['mail']['destination'] = ['wont find this'] + ses_response['Message']['mail']['messageId'] = 'wont find this' response = client.post( path='/notifications/email/ses', @@ -1118,19 +1118,66 @@ def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, not headers=[('Content-Type', 'text/plain; charset=UTF-8')] ) json_resp = json.loads(response.get_data(as_text=True)) + print(json_resp) assert response.status_code == 404 assert json_resp['result'] == 'error' assert json_resp['message'] == 'SES callback failed: notification not found. Status delivered' -def test_ses_callback_should_update_notification_status(notify_api, sample_notification): +def test_ses_callback_should_update_notification_status(notify_api, notify_db, notify_db_session): with notify_api.test_request_context(): with notify_api.test_client() as client: - assert get_notification_by_id(sample_notification.id).status == 'sent' + notification = sample_notification(notify_db, notify_db_session, reference='ref') + + assert get_notification_by_id(notification.id).status == 'sent' ses_response = json.loads(load_example_ses('ses_response')) - ses_response['Message']['mail']['destination'] = [sample_notification.to] + ses_response['Message']['mail']['messageId'] = 'ref' + + response = client.post( + path='/notifications/email/ses', + data=json.dumps(ses_response), + headers=[('Content-Type', 'text/plain; charset=UTF-8')] + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_resp['result'] == 'success' + assert json_resp['message'] == 'SES callback succeeded' + assert get_notification_by_id(notification.id).status == 'delivered' + + +def test_should_handle_invite_email_callbacks(notify_api, notify_db, notify_db_session): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + notify_api.config['INVITATION_EMAIL_FROM'] = 'test-invite' + notify_api.config['NOTIFY_EMAIL_DOMAIN'] = 'test-domain.com' + + ses_response = json.loads(load_example_ses('ses_response')) + ses_response['Message']['mail']['messageId'] = 'ref' + ses_response['Message']['mail']['source'] = 'test-invite@test-domain.com' + + response = client.post( + path='/notifications/email/ses', + data=json.dumps(ses_response), + headers=[('Content-Type', 'text/plain; charset=UTF-8')] + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_resp['result'] == 'success' + assert json_resp['message'] == 'SES callback succeeded' + + +def test_should_handle_validation_code_callbacks(notify_api, notify_db, notify_db_session): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + notify_api.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'] = 'valid-code@test.com' + + ses_response = json.loads(load_example_ses('ses_response')) + ses_response['Message']['mail']['messageId'] = 'ref' + ses_response['Message']['mail']['source'] = 'valid-code@test.com' response = client.post( path='/notifications/email/ses', @@ -1141,4 +1188,3 @@ def test_ses_callback_should_update_notification_status(notify_api, sample_notif assert response.status_code == 200 assert json_resp['result'] == 'success' assert json_resp['message'] == 'SES callback succeeded' - assert get_notification_by_id(sample_notification.id).status == 'delivered' From fd973179cc792ad159ad8de378c38d58132d7444 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 14 Mar 2016 11:34:09 +0000 Subject: [PATCH 10/10] - Commit session on job update - log at exception level --- app/celery/tasks.py | 2 +- app/dao/notifications_dao.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 109d010b7..85c681ce9 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -282,7 +282,7 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not ) update_notification_reference_by_id(notification_id, reference) except AwsSesClientException as e: - current_app.logger.debug(e) + current_app.logger.exception(e) notification_db_object.status = 'failed' dao_update_notification(notification_db_object) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index be36ee1fc..ce2eb1093 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -61,6 +61,7 @@ def update_job_sent_count(notification): Job.notifications_sent: Job.notifications_sent + 1, Job.updated_at: datetime.utcnow() }) + db.session.commit() def dao_update_notification(notification):