From a0fe5c69715e6bc3380383b72fb60839a161ddee Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 09:31:03 +0000 Subject: [PATCH 01/15] created dao to get notifications by references --- app/dao/notifications_dao.py | 7 +++++++ .../dao/notification_dao/test_notification_dao.py | 14 +++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 2ea240f5b..fb6e66d5f 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -482,6 +482,13 @@ def dao_get_notifications_by_to_field(service_id, search_term, statuses=None): return results +@statsd(namespace="dao") +def dao_get_notifications_by_reference(references): + return Notification.query.filter( + Notification.reference.in_(references) + ).all() + + @statsd(namespace="dao") def dao_created_scheduled_notification(scheduled_notification): db.session.add(scheduled_notification) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 8d88ef2f9..2f6985fc9 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -53,7 +53,8 @@ from app.dao.notifications_dao import ( is_delivery_slow_for_provider, set_scheduled_notification_to_processed, update_notification_status_by_id, - update_notification_status_by_reference + update_notification_status_by_reference, + dao_get_notifications_by_reference ) from app.dao.services_dao import dao_update_service @@ -2111,6 +2112,17 @@ def test_dao_update_notifications_by_reference_returns_zero_when_no_notification assert updated_count == 0 +def test_dao_get_notifications_by_reference(sample_template): + notification_0 = create_notification(template=sample_template, reference='noref') + notification_1 = create_notification(template=sample_template, reference='ref') + notification_2 = create_notification(template=sample_template, reference='ref') + + noti = dao_get_notifications_by_reference(['ref']) + assert len(noti) == 2 + assert noti[0].id in [notification_1.id, notification_2.id] + assert noti[1].id in [notification_1.id, notification_2.id] + + def test_dao_create_notification_sms_sender_mapping(sample_notification): sms_sender = create_service_sms_sender(service=sample_notification.service, sms_sender='123456') dao_create_notification_sms_sender_mapping(notification_id=sample_notification.id, From 23a40b9fd0c4003103aeab2b8e7564aa11be3185 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 09:54:18 +0000 Subject: [PATCH 02/15] create method _post_status_update for the actual sending of callbacks --- app/celery/tasks.py | 68 +++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 30 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 250f786eb..a176eb9ab 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -499,36 +499,7 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): "date_received": inbound_sms.provider_date.strftime(DATETIME_FORMAT) } - try: - response = request( - method="POST", - url=inbound_api.url, - data=json.dumps(data), - headers={ - 'Content-Type': 'application/json', - 'Authorization': 'Bearer {}'.format(inbound_api.bearer_token) - }, - timeout=60 - ) - current_app.logger.info('send_inbound_sms_to_service sending {} to {}, response {}'.format( - inbound_sms_id, - inbound_api.url, - response.status_code - )) - response.raise_for_status() - except RequestException as e: - current_app.logger.warning( - "send_inbound_sms_to_service request failed for service_id: {} and url: {}. exc: {}".format( - service_id, - inbound_api.url, - e - ) - ) - if not isinstance(e, HTTPError) or e.response.status_code >= 500: - try: - self.retry(queue=QueueNames.RETRY) - except self.MaxRetriesExceededError: - current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') + _post_status_update(self, inbound_api, data, 'send_inbound_sms_to_service') @notify_celery.task(name='process-incomplete-jobs') @@ -566,3 +537,40 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template, resumed=True) + + +def _post_status_update(self, callback_api, data, callback_name): + + try: + response = request( + method="POST", + url=callback_api.url, + data=json.dumps(data), + headers={ + 'Content-Type': 'application/json', + 'Authorization': 'Bearer {}'.format(callback_api.bearer_token) + }, + timeout=60 + ) + current_app.logger.info('{} sending {} to {}, response {}'.format( + callback_name, + callback_api.service_id, + callback_api.url, + response.status_code + )) + response.raise_for_status() + except RequestException as e: + current_app.logger.warning( + "service_name request failed for service_id: {} and url: {}. exc: {}".format( + callback_api.service_id, + callback_api.url, + e + ) + ) + if not isinstance(e, HTTPError) or e.response.status_code >= 500: + try: + self.retry(queue=QueueNames.RETRY, + exc='Unable to {} for service_id: {} and url: {}. \n{}'.format( + callback_name, callback_api.service_id, callback_api.url, e)) + except self.MaxRetriesExceededError: + current_app.logger.exception('Retry: {} has retried the max number of times', callback_name) From 988b22391b8e661b4bb1492346bb8cb338e544c5 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 14:06:13 +0000 Subject: [PATCH 03/15] added send_delivery_status_to_service task worker --- app/celery/tasks.py | 26 +++ tests/app/celery/test_tasks.py | 172 +++++++++++++++++- .../notification_dao/test_notification_dao.py | 10 +- 3 files changed, 201 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a176eb9ab..ad8df005f 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -68,6 +68,7 @@ from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd from notifications_utils.s3 import s3upload +from app.dao.service_callback_api_dao import get_service_callback_api_for_service @worker_process_shutdown.connect @@ -502,6 +503,30 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): _post_status_update(self, inbound_api, data, 'send_inbound_sms_to_service') +@notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def send_delivery_status_to_service(self, notification_id): + # TODO: do we need to do rate limit this? + notification = get_notification_by_id(notification_id) + service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) + if not service_callback_api: + # No delivery receipt API info set + return + + data = { + "id": str(notification_id), + "reference": str(notification.client_reference), + "to": notification.to, + "status": notification.status, + "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request + "updated_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated + "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent + "notification_type": notification.notification_type + } + + _post_status_update(self, service_callback_api, data, 'send_delivery_receipt_to_service') + + @notify_celery.task(name='process-incomplete-jobs') @statsd(namespace="tasks") def process_incomplete_jobs(job_ids): @@ -552,6 +577,7 @@ def _post_status_update(self, callback_api, data, callback_name): }, timeout=60 ) + current_app.logger.info('{} sending {} to {}, response {}'.format( callback_name, callback_api.service_id, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 6d48acf56..149fd5fe5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -29,7 +29,9 @@ from app.celery.tasks import ( process_incomplete_jobs, get_template_class, s3, - send_inbound_sms_to_service) + send_inbound_sms_to_service, + send_delivery_status_to_service +) from app.config import QueueNames from app.dao import jobs_dao, services_dao from app.models import ( @@ -64,7 +66,8 @@ from tests.app.db import ( create_template, create_user, create_reply_to_email, - create_service_with_defined_sms_sender + create_service_with_defined_sms_sender, + create_service_callback_api ) @@ -1247,6 +1250,171 @@ def test_send_inbound_sms_to_service_does_not_retries_if_request_returns_404(not mocked.call_count == 0 +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_post_https_request_to_service(notify_db, + notify_db_session, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + + datestr = datetime(2017, 6, 20) + + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=200) + send_delivery_status_to_service(notification.id) + + mock_data = { + "id": str(notification.id), + "reference": str(notification.client_reference), + "to": notification.to, + "status": notification.status, + "created_at": datestr.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request + "updated_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated + "sent_at": datestr.strftime(DATETIME_FORMAT), # the time the email was sent + "notification_type": notification_type + } + + assert request_mock.call_count == 1 + assert request_mock.request_history[0].url == callback_api.url + assert request_mock.request_history[0].method == 'POST' + assert request_mock.request_history[0].text == json.dumps(mock_data) + assert request_mock.request_history[0].headers["Content-type"] == "application/json" + assert request_mock.request_history[0].headers["Authorization"] == "Bearer {}".format(callback_api.bearer_token) + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_does_not_sent_request_when_service_callback_api_does_not_exist( + notify_db, notify_db_session, mocker, notification_type): + service = create_sample_service(notify_db, notify_db_session, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + datestr = datetime(2017, 6, 20) + + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch("requests.request") + send_delivery_status_to_service(notification.id) + + mocked.call_count == 0 + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch('app.celery.tasks.send_delivery_status_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=500) + send_delivery_status_to_service(notification.id) + + assert mocked.call_count == 1 + assert mocked.call_args[1]['queue'] == 'retry-tasks' + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + + mocked = mocker.patch('app.celery.tasks.send_delivery_status_to_service.retry') + mocker.patch("app.celery.tasks.request", side_effect=RequestException()) + + send_delivery_status_to_service(notification.id) + + assert mocked.call_count == 1 + assert mocked.call_args[1]['queue'] == 'retry-tasks' + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch('app.celery.tasks.send_inbound_sms_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=404) + send_delivery_status_to_service(notification.id) + + mocked.call_count == 0 + + def test_check_job_status_task_does_not_raise_error(sample_template): create_job( template=sample_template, diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 2f6985fc9..a5afa1103 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -2113,14 +2113,14 @@ def test_dao_update_notifications_by_reference_returns_zero_when_no_notification def test_dao_get_notifications_by_reference(sample_template): - notification_0 = create_notification(template=sample_template, reference='noref') + create_notification(template=sample_template, reference='noref') notification_1 = create_notification(template=sample_template, reference='ref') notification_2 = create_notification(template=sample_template, reference='ref') - noti = dao_get_notifications_by_reference(['ref']) - assert len(noti) == 2 - assert noti[0].id in [notification_1.id, notification_2.id] - assert noti[1].id in [notification_1.id, notification_2.id] + notifications = dao_get_notifications_by_reference(['ref']) + assert len(notifications) == 2 + assert notifications[0].id in [notification_1.id, notification_2.id] + assert notifications[1].id in [notification_1.id, notification_2.id] def test_dao_create_notification_sms_sender_mapping(sample_notification): From 489f43a2c9a14cc84f83a93b5638630d4881c944 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 16:15:21 +0000 Subject: [PATCH 04/15] rename callback_tasks.py to process_ses_receipts.py create service_callback_tasks.py for tasks to send delivery statuses to services --- ...tasks.py => process_ses_receipts_tasks.py} | 0 app/celery/research_mode_tasks.py | 2 +- app/celery/service_callback_tasks.py | 71 +++++++ app/celery/tasks.py | 94 +++------ ....py => test_process_ses_receipts_tasks.py} | 6 +- .../app/celery/test_service_callback_tasks.py | 185 ++++++++++++++++++ tests/app/celery/test_tasks.py | 167 ---------------- 7 files changed, 290 insertions(+), 235 deletions(-) rename app/celery/{callback_tasks.py => process_ses_receipts_tasks.py} (100%) create mode 100644 app/celery/service_callback_tasks.py rename tests/app/celery/{test_callback_tasks.py => test_process_ses_receipts_tasks.py} (91%) create mode 100644 tests/app/celery/test_service_callback_tasks.py diff --git a/app/celery/callback_tasks.py b/app/celery/process_ses_receipts_tasks.py similarity index 100% rename from app/celery/callback_tasks.py rename to app/celery/process_ses_receipts_tasks.py diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index b37062826..509ee1681 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -5,7 +5,7 @@ from requests import request, RequestException, HTTPError from app.models import SMS_TYPE from app.config import QueueNames -from app.celery.callback_tasks import process_ses_results +from app.celery.process_ses_receipts_tasks import process_ses_results temp_fail = "7700900003" perm_fail = "7700900002" diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py new file mode 100644 index 000000000..3bc39a3fe --- /dev/null +++ b/app/celery/service_callback_tasks.py @@ -0,0 +1,71 @@ +import json +from app import ( + DATETIME_FORMAT, + notify_celery, +) +from app.dao.notifications_dao import ( + get_notification_by_id, +) + +from app.statsd_decorators import statsd +from app.dao.service_callback_api_dao import get_service_callback_api_for_service +from requests import ( + HTTPError, + request, + RequestException +) +from flask import current_app +from app.config import QueueNames + + +@notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def send_delivery_status_to_service(self, notification_id): + # TODO: do we need to do rate limit this? + notification = get_notification_by_id(notification_id) + service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) + if not service_callback_api: + # No delivery receipt API info set + return + + data = { + "id": str(notification_id), + "reference": str(notification.client_reference), + "to": notification.to, + "status": notification.status, + "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request + "updated_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated + "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent + "notification_type": notification.notification_type + } + + try: + response = request( + method="POST", + url=service_callback_api.url, + data=json.dumps(data), + headers={ + 'Content-Type': 'application/json', + 'Authorization': 'Bearer {}'.format(service_callback_api.bearer_token) + }, + timeout=60 + ) + current_app.logger.info('send_delivery_status_to_service sending {} to {}, response {}'.format( + notification_id, + service_callback_api.url, + response.status_code + )) + response.raise_for_status() + except RequestException as e: + current_app.logger.warning( + "send_inbound_sms_to_service request failed for service_id: {} and url: {}. exc: {}".format( + notification_id, + service_callback_api.url, + e + ) + ) + if not isinstance(e, HTTPError) or e.response.status_code >= 500: + try: + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') diff --git a/app/celery/tasks.py b/app/celery/tasks.py index ad8df005f..250f786eb 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -68,7 +68,6 @@ from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd from notifications_utils.s3 import s3upload -from app.dao.service_callback_api_dao import get_service_callback_api_for_service @worker_process_shutdown.connect @@ -500,31 +499,36 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): "date_received": inbound_sms.provider_date.strftime(DATETIME_FORMAT) } - _post_status_update(self, inbound_api, data, 'send_inbound_sms_to_service') - - -@notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300) -@statsd(namespace="tasks") -def send_delivery_status_to_service(self, notification_id): - # TODO: do we need to do rate limit this? - notification = get_notification_by_id(notification_id) - service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) - if not service_callback_api: - # No delivery receipt API info set - return - - data = { - "id": str(notification_id), - "reference": str(notification.client_reference), - "to": notification.to, - "status": notification.status, - "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request - "updated_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated - "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent - "notification_type": notification.notification_type - } - - _post_status_update(self, service_callback_api, data, 'send_delivery_receipt_to_service') + try: + response = request( + method="POST", + url=inbound_api.url, + data=json.dumps(data), + headers={ + 'Content-Type': 'application/json', + 'Authorization': 'Bearer {}'.format(inbound_api.bearer_token) + }, + timeout=60 + ) + current_app.logger.info('send_inbound_sms_to_service sending {} to {}, response {}'.format( + inbound_sms_id, + inbound_api.url, + response.status_code + )) + response.raise_for_status() + except RequestException as e: + current_app.logger.warning( + "send_inbound_sms_to_service request failed for service_id: {} and url: {}. exc: {}".format( + service_id, + inbound_api.url, + e + ) + ) + if not isinstance(e, HTTPError) or e.response.status_code >= 500: + try: + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') @notify_celery.task(name='process-incomplete-jobs') @@ -562,41 +566,3 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template, resumed=True) - - -def _post_status_update(self, callback_api, data, callback_name): - - try: - response = request( - method="POST", - url=callback_api.url, - data=json.dumps(data), - headers={ - 'Content-Type': 'application/json', - 'Authorization': 'Bearer {}'.format(callback_api.bearer_token) - }, - timeout=60 - ) - - current_app.logger.info('{} sending {} to {}, response {}'.format( - callback_name, - callback_api.service_id, - callback_api.url, - response.status_code - )) - response.raise_for_status() - except RequestException as e: - current_app.logger.warning( - "service_name request failed for service_id: {} and url: {}. exc: {}".format( - callback_api.service_id, - callback_api.url, - e - ) - ) - if not isinstance(e, HTTPError) or e.response.status_code >= 500: - try: - self.retry(queue=QueueNames.RETRY, - exc='Unable to {} for service_id: {} and url: {}. \n{}'.format( - callback_name, callback_api.service_id, callback_api.url, e)) - except self.MaxRetriesExceededError: - current_app.logger.exception('Retry: {} has retried the max number of times', callback_name) diff --git a/tests/app/celery/test_callback_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py similarity index 91% rename from tests/app/celery/test_callback_tasks.py rename to tests/app/celery/test_process_ses_receipts_tasks.py index 4b5ce4547..ddca32e0e 100644 --- a/tests/app/celery/test_callback_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -1,7 +1,7 @@ import json from datetime import datetime -from app.celery.callback_tasks import process_ses_results +from app.celery.process_ses_receipts_tasks import process_ses_results from tests.app.db import create_notification @@ -18,7 +18,7 @@ def test_process_ses_results(sample_email_template): def test_process_ses_results_does_not_retry_if_errors(notify_db, mocker): - mocked = mocker.patch('app.celery.callback_tasks.process_ses_results.retry') + mocked = mocker.patch('app.celery.process_ses_receipts_tasks.process_ses_results.retry') response = json.loads(ses_notification_callback()) process_ses_results(response=response) assert mocked.call_count == 0 @@ -26,7 +26,7 @@ def test_process_ses_results_does_not_retry_if_errors(notify_db, mocker): def test_process_ses_results_retry_called(notify_db, mocker): mocker.patch("app.dao.notifications_dao.update_notification_status_by_reference", side_effect=Exception("EXPECTED")) - mocked = mocker.patch('app.celery.callback_tasks.process_ses_results.retry') + mocked = mocker.patch('app.celery.process_ses_receipts_tasks.process_ses_results.retry') response = json.loads(ses_notification_callback()) process_ses_results(response=response) assert mocked.call_count != 0 diff --git a/tests/app/celery/test_service_callback_tasks.py b/tests/app/celery/test_service_callback_tasks.py new file mode 100644 index 000000000..f09333d2d --- /dev/null +++ b/tests/app/celery/test_service_callback_tasks.py @@ -0,0 +1,185 @@ +import json +from datetime import datetime + +import pytest +import requests_mock + +from requests import RequestException + +from app import (DATETIME_FORMAT) + +from tests.app.conftest import ( + sample_service as create_sample_service, + sample_template as create_sample_template, +) +from tests.app.db import ( + create_notification, + create_user, + create_service_callback_api +) +from app.celery.service_callback_tasks import send_delivery_status_to_service + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_post_https_request_to_service(notify_db, + notify_db_session, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + + datestr = datetime(2017, 6, 20) + + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=200) + send_delivery_status_to_service(notification.id) + + mock_data = { + "id": str(notification.id), + "reference": str(notification.client_reference), + "to": notification.to, + "status": notification.status, + "created_at": datestr.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request + "updated_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated + "sent_at": datestr.strftime(DATETIME_FORMAT), # the time the email was sent + "notification_type": notification_type + } + + assert request_mock.call_count == 1 + assert request_mock.request_history[0].url == callback_api.url + assert request_mock.request_history[0].method == 'POST' + assert request_mock.request_history[0].text == json.dumps(mock_data) + assert request_mock.request_history[0].headers["Content-type"] == "application/json" + assert request_mock.request_history[0].headers["Authorization"] == "Bearer {}".format(callback_api.bearer_token) + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_does_not_sent_request_when_service_callback_api_does_not_exist( + notify_db, notify_db_session, mocker, notification_type): + service = create_sample_service(notify_db, notify_db_session, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + datestr = datetime(2017, 6, 20) + + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch("requests.request") + send_delivery_status_to_service(notification.id) + + mocked.call_count == 0 + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=500) + send_delivery_status_to_service(notification.id) + + assert mocked.call_count == 1 + assert mocked.call_args[1]['queue'] == 'retry-tasks' + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + + mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry') + mocker.patch("app.celery.tasks.request", side_effect=RequestException()) + + send_delivery_status_to_service(notification.id) + + assert mocked.call_count == 1 + assert mocked.call_args[1]['queue'] == 'retry-tasks' + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=404) + send_delivery_status_to_service(notification.id) + + mocked.call_count == 0 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 149fd5fe5..e33956a56 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -30,7 +30,6 @@ from app.celery.tasks import ( get_template_class, s3, send_inbound_sms_to_service, - send_delivery_status_to_service ) from app.config import QueueNames from app.dao import jobs_dao, services_dao @@ -67,7 +66,6 @@ from tests.app.db import ( create_user, create_reply_to_email, create_service_with_defined_sms_sender, - create_service_callback_api ) @@ -1250,171 +1248,6 @@ def test_send_inbound_sms_to_service_does_not_retries_if_request_returns_404(not mocked.call_count == 0 -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_post_https_request_to_service(notify_db, - notify_db_session, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - - datestr = datetime(2017, 6, 20) - - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - - with requests_mock.Mocker() as request_mock: - request_mock.post(callback_api.url, - json={}, - status_code=200) - send_delivery_status_to_service(notification.id) - - mock_data = { - "id": str(notification.id), - "reference": str(notification.client_reference), - "to": notification.to, - "status": notification.status, - "created_at": datestr.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request - "updated_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated - "sent_at": datestr.strftime(DATETIME_FORMAT), # the time the email was sent - "notification_type": notification_type - } - - assert request_mock.call_count == 1 - assert request_mock.request_history[0].url == callback_api.url - assert request_mock.request_history[0].method == 'POST' - assert request_mock.request_history[0].text == json.dumps(mock_data) - assert request_mock.request_history[0].headers["Content-type"] == "application/json" - assert request_mock.request_history[0].headers["Authorization"] == "Bearer {}".format(callback_api.bearer_token) - - -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_does_not_sent_request_when_service_callback_api_does_not_exist( - notify_db, notify_db_session, mocker, notification_type): - service = create_sample_service(notify_db, notify_db_session, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - datestr = datetime(2017, 6, 20) - - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - mocked = mocker.patch("requests.request") - send_delivery_status_to_service(notification.id) - - mocked.call_count == 0 - - -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_db, - notify_db_session, - mocker, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") - datestr = datetime(2017, 6, 20) - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - mocked = mocker.patch('app.celery.tasks.send_delivery_status_to_service.retry') - with requests_mock.Mocker() as request_mock: - request_mock.post(callback_api.url, - json={}, - status_code=500) - send_delivery_status_to_service(notification.id) - - assert mocked.call_count == 1 - assert mocked.call_args[1]['queue'] == 'retry-tasks' - - -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notify_db, - notify_db_session, - mocker, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") - datestr = datetime(2017, 6, 20) - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - - mocked = mocker.patch('app.celery.tasks.send_delivery_status_to_service.retry') - mocker.patch("app.celery.tasks.request", side_effect=RequestException()) - - send_delivery_status_to_service(notification.id) - - assert mocked.call_count == 1 - assert mocked.call_args[1]['queue'] == 'retry-tasks' - - -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404(notify_db, - notify_db_session, - mocker, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") - datestr = datetime(2017, 6, 20) - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - mocked = mocker.patch('app.celery.tasks.send_inbound_sms_to_service.retry') - with requests_mock.Mocker() as request_mock: - request_mock.post(callback_api.url, - json={}, - status_code=404) - send_delivery_status_to_service(notification.id) - - mocked.call_count == 0 - - def test_check_job_status_task_does_not_raise_error(sample_template): create_job( template=sample_template, From a604fb82bb4b9aedd871fd63d665e110e0f8d7a5 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 20:30:53 +0000 Subject: [PATCH 05/15] added sms delivery status callback --- app/notifications/process_client_response.py | 5 +- .../app/notifications/rest/test_callbacks.py | 47 +++++++++++++++---- .../test_notifications_ses_callback.py | 4 +- .../test_process_client_response.py | 5 +- 4 files changed, 49 insertions(+), 12 deletions(-) diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index 534408ce6..129ef9ae2 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -8,7 +8,8 @@ from app.dao import notifications_dao from app.clients.sms.firetext import get_firetext_responses from app.clients.sms.mmg import get_mmg_responses from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks - +from app.celery.service_callback_tasks import send_delivery_status_to_service +from app.config import QueueNames sms_response_mapper = { 'MMG': get_mmg_responses, @@ -83,5 +84,7 @@ def process_sms_client_response(status, reference, client_name): create_outcome_notification_statistic_tasks(notification) + send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) + success = "{} callback succeeded. reference {} updated".format(client_name, reference) return success, errors diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 73d33c7a8..6202ad7e1 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -18,7 +18,7 @@ def firetext_post(client, data): data=data, headers=[ ('Content-Type', 'application/x-www-form-urlencoded'), - ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') + ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') # fake IPs ]) @@ -28,7 +28,7 @@ def mmg_post(client, data): data=data, headers=[ ('Content-Type', 'application/json'), - ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') + ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') # fake IPs ]) @@ -178,7 +178,9 @@ def test_firetext_callback_should_update_notification_status( notify_db, notify_db_session, client, sample_email_template, mocker ): mocker.patch('app.statsd_client.incr') - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, @@ -202,13 +204,16 @@ def test_firetext_callback_should_update_notification_status( updated = get_notification_by_id(notification.id) assert updated.status == 'delivered' assert get_notification_by_id(notification.id).status == 'delivered' + assert send_mock.called_once_with([notification.id], queue="notify-internal-tasks") def test_firetext_callback_should_update_notification_status_failed( notify_db, notify_db_session, client, sample_template, mocker ): mocker.patch('app.statsd_client.incr') - + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, @@ -235,6 +240,9 @@ def test_firetext_callback_should_update_notification_status_failed( def test_firetext_callback_should_update_notification_status_pending(client, notify_db, notify_db_session, mocker): mocker.patch('app.statsd_client.incr') + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -265,8 +273,11 @@ def test_process_mmg_response_return_200_when_cid_is_send_sms_code(client): def test_process_mmg_response_returns_200_when_cid_is_valid_notification_id( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -286,8 +297,11 @@ def test_process_mmg_response_returns_200_when_cid_is_valid_notification_id( def test_process_mmg_response_status_5_updates_notification_with_permanently_failed( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -306,8 +320,11 @@ def test_process_mmg_response_status_5_updates_notification_with_permanently_fai def test_process_mmg_response_status_2_updates_notification_with_permanently_failed( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -325,8 +342,11 @@ def test_process_mmg_response_status_2_updates_notification_with_permanently_fai def test_process_mmg_response_status_4_updates_notification_with_temporary_failed( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -345,8 +365,11 @@ def test_process_mmg_response_status_4_updates_notification_with_temporary_faile def test_process_mmg_response_unknown_status_updates_notification_with_failed( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -392,6 +415,9 @@ def test_process_mmg_response_records_statsd(notify_db, notify_db_session, clien mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -415,6 +441,9 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index 330466234..afcd8897b 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -24,7 +24,9 @@ def test_ses_callback_should_update_notification_status( stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index a4e3d2413..503d4b9eb 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -51,12 +51,15 @@ def test_outcome_statistics_called_for_successful_callback(sample_notification, 'app.notifications.process_client_response.notifications_dao.update_notification_status_by_id', return_value=sample_notification ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) reference = str(uuid.uuid4()) success, error = process_sms_client_response(status='3', reference=reference, client_name='MMG') assert success == "MMG callback succeeded. reference {} updated".format(str(reference)) assert error is None + assert send_mock.called stats_mock.assert_called_once_with(sample_notification) From 096657799c9351f5521d016f2b4576d63d2ccf8a Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 21:13:01 +0000 Subject: [PATCH 06/15] Added send delivery status to SES callbacks --- app/notifications/notifications_ses_callback.py | 4 +++- .../test_notifications_ses_callback.py | 16 +++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index b35704304..c04a27784 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -12,6 +12,8 @@ from app.dao import ( ) from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks from app.notifications.process_client_response import validate_callback_data +from app.celery.service_callback_tasks import send_delivery_status_to_service +from app.config import QueueNames def process_ses_response(ses_request): @@ -75,7 +77,7 @@ def process_ses_response(ses_request): ) create_outcome_notification_statistic_tasks(notification) - + send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) return except KeyError: diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index afcd8897b..fb156dc07 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -46,6 +46,7 @@ def test_ses_callback_should_update_notification_status( ) statsd_client.incr.assert_any_call("callback.ses.delivered") stats_mock.assert_called_once_with(notification) + send_mock.assert_called_once_with([notification.id], queue="notify-internal-tasks") def test_ses_callback_should_update_multiple_notification_status_sent( @@ -58,7 +59,9 @@ def test_ses_callback_should_update_multiple_notification_status_sent( stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification1 = create_sample_notification( notify_db, notify_db_session, @@ -92,6 +95,7 @@ def test_ses_callback_should_update_multiple_notification_status_sent( call(notification2), call(notification3) ]) + assert send_mock.called def test_ses_callback_should_set_status_to_temporary_failure(client, @@ -103,7 +107,9 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, @@ -115,6 +121,7 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, assert get_notification_by_id(notification.id).status == 'sending' assert process_ses_response(ses_soft_bounce_callback(reference='ref')) is None assert get_notification_by_id(notification.id).status == 'temporary-failure' + assert send_mock.called stats_mock.assert_called_once_with(notification) @@ -148,7 +155,9 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, @@ -161,6 +170,7 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, assert get_notification_by_id(notification.id).status == 'sending' assert process_ses_response(ses_hard_bounce_callback(reference='ref')) is None assert get_notification_by_id(notification.id).status == 'permanent-failure' + assert send_mock.called stats_mock.assert_called_once_with(notification) From f5a0ca918480dd8437ae82975fb10b52f7570ba1 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 21:59:46 +0000 Subject: [PATCH 07/15] Added delivery status to services for letter callbacks --- app/celery/tasks.py | 14 +++++++++++--- tests/app/celery/test_ftp_update_tasks.py | 11 ++++++++++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 250f786eb..2ec7e1972 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -29,6 +29,7 @@ from app import ( ) from app.aws import s3 from app.celery import provider_tasks +from app.celery.service_callback_tasks import send_delivery_status_to_service from app.config import QueueNames from app.dao.inbound_sms_dao import dao_get_inbound_sms_by_id from app.dao.jobs_dao import ( @@ -42,7 +43,8 @@ from app.dao.notifications_dao import ( get_notification_by_id, dao_update_notifications_for_job_to_sent_to_dvla, dao_update_notifications_by_reference, - dao_get_last_notification_added_for_job_id + dao_get_last_notification_added_for_job_id, + dao_get_notifications_by_reference ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service @@ -391,6 +393,9 @@ def update_letter_notifications_to_error(self, notification_references): ) current_app.logger.info("Updated {} letter notifications to technical-failure".format(updated_count)) + notifications = dao_get_notifications_by_reference(references=notification_references) + for notification in notifications: + send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) def create_dvla_file_contents_for_job(job_id): @@ -455,7 +460,7 @@ def update_letter_notifications_statuses(self, filename): for update in notification_updates: status = NOTIFICATION_DELIVERED if update.status == DVLA_RESPONSE_STATUS_SENT \ else NOTIFICATION_TECHNICAL_FAILURE - notification = dao_update_notifications_by_reference( + updated_count = dao_update_notifications_by_reference( references=[update.reference], update_dict={"status": status, "billable_units": update.page_count, @@ -463,7 +468,7 @@ def update_letter_notifications_statuses(self, filename): } ) - if not notification: + if not updated_count: msg = "Update letter notification file {filename} failed: notification either not found " \ "or already updated from delivered. Status {status} for notification reference {reference}".format( filename=filename, status=status, reference=update.reference) @@ -472,6 +477,9 @@ def update_letter_notifications_statuses(self, filename): current_app.logger.info( 'DVLA file: {filename}, notification updated to {status}: {reference}'.format( filename=filename, status=status, reference=str(update.reference))) + notifications = dao_get_notifications_by_reference(references=[update.reference]) + for notification in notifications: + send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) def process_updates_from_file(response_file): diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index d9b1b22b9..25ea27439 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -101,6 +101,9 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp valid_file = '{}|Sent|1|Unsorted\n{}|Failed|2|Sorted'.format( sent_letter.reference, failed_letter.reference) mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) update_letter_notifications_statuses(filename='foo.txt') @@ -110,6 +113,7 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp assert failed_letter.status == NOTIFICATION_TECHNICAL_FAILURE assert failed_letter.billable_units == 2 assert failed_letter.updated_at + assert send_mock.called def test_update_letter_notifications_to_sent_to_dvla_updates_based_on_notification_references( @@ -132,8 +136,12 @@ def test_update_letter_notifications_to_sent_to_dvla_updates_based_on_notificati def test_update_letter_notifications_to_error_updates_based_on_notification_references( client, - sample_letter_template + sample_letter_template, + mocker ): + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) first = create_notification(sample_letter_template, reference='first ref') second = create_notification(sample_letter_template, reference='second ref') @@ -146,3 +154,4 @@ def test_update_letter_notifications_to_error_updates_based_on_notification_refe assert first.sent_at is None assert first.updated_at == dt assert second.status == NOTIFICATION_CREATED + assert send_mock.called From f73319f5ef886e66969a08c42bc338d74516f713 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 4 Dec 2017 13:24:53 +0000 Subject: [PATCH 08/15] Add and populate crown column to services and services_history - Added the boolean 'crown' column to services and services_history tables - We populate this column in the same migration script by checking the 'organisation_type' of a service --- app/dao/services_dao.py | 1 + app/models.py | 1 + .../versions/0149_add_crown_to_services.py | 42 +++++++++++++++++++ tests/app/dao/test_services_dao.py | 2 + 4 files changed, 46 insertions(+) create mode 100644 migrations/versions/0149_add_crown_to_services.py diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 8753b2dc4..0b5f3e245 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -172,6 +172,7 @@ def dao_create_service(service, user, service_id=None, service_permissions=None) service.id = service_id or uuid.uuid4() # must be set now so version history model can use same id service.active = True service.research_mode = False + service.crown = service.organisation_type == 'central' for permission in service_permissions: service_permission = ServicePermission(service_id=service.id, permission=permission) diff --git a/app/models.py b/app/models.py index 9d81073c7..fdd00fa9a 100644 --- a/app/models.py +++ b/app/models.py @@ -249,6 +249,7 @@ class Service(db.Model, Versioned): db.String(255), nullable=True, ) + crown = db.Column(db.Boolean, index=False, nullable=False, default=True) association_proxy('permissions', 'service_permission_types') diff --git a/migrations/versions/0149_add_crown_to_services.py b/migrations/versions/0149_add_crown_to_services.py new file mode 100644 index 000000000..1b34f9e2a --- /dev/null +++ b/migrations/versions/0149_add_crown_to_services.py @@ -0,0 +1,42 @@ +""" + +Revision ID: 0149_add_crown_column_to_services +Revises: 0148_add_letters_as_pdf_svc_perm +Create Date: 2017-12-04 12:13:35.268712 + +""" +from alembic import op +import sqlalchemy as sa + + +revision = '0149_add_crown_to_services' +down_revision = '0148_add_letters_as_pdf_svc_perm' + + +def upgrade(): + op.add_column('services', sa.Column('crown', sa.Boolean(), nullable=True)) + op.execute(""" + update services set crown = True + where organisation_type = 'central' + """) + op.execute(""" + update services set crown = False + where crown is null + """) + op.alter_column('services', 'crown', nullable=False) + + op.add_column('services_history', sa.Column('crown', sa.Boolean(), nullable=True)) + op.execute(""" + update services_history set crown = True + where organisation_type = 'central' + """) + op.execute(""" + update services_history set crown = False + where crown is null + """) + op.alter_column('services_history', 'crown', nullable=False) + + +def downgrade(): + op.drop_column('services', 'crown') + op.drop_column('services_history', 'crown') diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index d601bd59d..21a88eecf 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -87,6 +87,7 @@ def test_create_service(sample_user): email_from="email_from", message_limit=1000, restricted=False, + organisation_type='central', created_by=sample_user) dao_create_service(service, sample_user) assert Service.query.count() == 1 @@ -100,6 +101,7 @@ def test_create_service(sample_user): assert service_db.prefix_sms is True assert service.active is True assert sample_user in service_db.users + assert service_db.crown is True def test_cannot_create_two_services_with_same_name(sample_user): From 7fa4e7ffc706d1399a4a5b5912f5a41caffc46bf Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 4 Dec 2017 16:07:26 +0000 Subject: [PATCH 09/15] Set crown if organisation_type is updated on service. Add some tests. Update the initial values of crown. --- app/service/rest.py | 4 ++++ .../versions/0149_add_crown_to_services.py | 8 ++++++++ tests/app/dao/test_services_dao.py | 3 +++ tests/app/service/test_rest.py | 20 +++++++++++++++++++ 4 files changed, 35 insertions(+) diff --git a/app/service/rest.py b/app/service/rest.py index efb7044a3..c4d0e2bb9 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -178,7 +178,11 @@ def update_service(service_id): service_going_live = fetched_service.restricted and not req_json.get('restricted', True) current_data = dict(service_schema.dump(fetched_service).data.items()) current_data.update(request.get_json()) + update_dict = service_schema.load(current_data).data + org_type = req_json.get('organisation_type', None) + if org_type: + update_dict.crown = org_type == 'central' dao_update_service(update_dict) # bridging code between frontend is deployed and data has not been migrated yet. Can only update current year diff --git a/migrations/versions/0149_add_crown_to_services.py b/migrations/versions/0149_add_crown_to_services.py index 1b34f9e2a..bfbbf976c 100644 --- a/migrations/versions/0149_add_crown_to_services.py +++ b/migrations/versions/0149_add_crown_to_services.py @@ -19,6 +19,10 @@ def upgrade(): update services set crown = True where organisation_type = 'central' """) + op.execute(""" + update services set crown = True + where organisation_type is null + """) op.execute(""" update services set crown = False where crown is null @@ -30,6 +34,10 @@ def upgrade(): update services_history set crown = True where organisation_type = 'central' """) + op.execute(""" + update services_history set crown = True + where organisation_type is null + """) op.execute(""" update services_history set crown = False where crown is null diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 21a88eecf..2caf092fe 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -97,10 +97,13 @@ def test_create_service(sample_user): assert service_db.id == service.id assert service_db.branding == BRANDING_GOVUK assert service_db.dvla_organisation_id == DVLA_ORG_HM_GOVERNMENT + assert service_db.email_from == 'email_from' assert service_db.research_mode is False assert service_db.prefix_sms is True assert service.active is True assert sample_user in service_db.users + assert service_db.free_sms_fragment_limit == 250000 + assert service_db.organisation_type == 'central' assert service_db.crown is True diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 73679001f..553742a26 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -536,6 +536,26 @@ def test_update_service_flags(client, sample_service): assert set(result['data']['permissions']) == set([LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) +@pytest.mark.parametrize("org_type, expected", + [("central", True), + ('local', False), + ("nhs", False)]) +def test_update_service_sets_crown(client, sample_service, org_type, expected): + data = { + 'organisation_type': org_type, + } + auth_header = create_authorization_header() + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + assert resp.status_code == 200 + assert result['data']['crown'] is expected + + @pytest.fixture(scope='function') def service_with_no_permissions(notify_db, notify_db_session): return create_service(service_permissions=[]) From 5482ee4fe732971f55a2cc2c7a32d1d62c2f84c0 Mon Sep 17 00:00:00 2001 From: venusbb Date: Mon, 4 Dec 2017 14:48:23 +0000 Subject: [PATCH 10/15] - wrap apply_async parameter notification_id in a str() argument - check if service_callback_api exist before putting tasks on queue - create_service_callback_api in tests before asserting if send_delivery_status_to_service has been called. --- app/celery/service_callback_tasks.py | 4 ++-- app/celery/tasks.py | 21 ++++++++++++------- app/dao/notifications_dao.py | 2 +- .../notifications_ses_callback.py | 10 ++++++++- app/notifications/process_client_response.py | 7 ++++++- tests/app/celery/test_ftp_update_tasks.py | 7 ++++--- .../notification_dao/test_notification_dao.py | 4 ++-- .../app/notifications/rest/test_callbacks.py | 3 ++- .../test_notifications_ses_callback.py | 9 +++++--- .../test_process_client_response.py | 2 ++ 10 files changed, 48 insertions(+), 21 deletions(-) diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py index 3bc39a3fe..e91835e56 100644 --- a/app/celery/service_callback_tasks.py +++ b/app/celery/service_callback_tasks.py @@ -58,7 +58,7 @@ def send_delivery_status_to_service(self, notification_id): response.raise_for_status() except RequestException as e: current_app.logger.warning( - "send_inbound_sms_to_service request failed for service_id: {} and url: {}. exc: {}".format( + "send_delivery_status_to_service request failed for service_id: {} and url: {}. exc: {}".format( notification_id, service_callback_api.url, e @@ -68,4 +68,4 @@ def send_delivery_status_to_service(self, notification_id): try: self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: - current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') + current_app.logger.exception('Retry: send_delivery_status_to_service has retried the max number of times') diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 2ec7e1972..53d714213 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -44,12 +44,13 @@ from app.dao.notifications_dao import ( dao_update_notifications_for_job_to_sent_to_dvla, dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id, - dao_get_notifications_by_reference + dao_get_notifications_by_references ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id +from app.dao.service_callback_api_dao import get_service_callback_api_for_service from app.models import ( DVLA_RESPONSE_STATUS_SENT, EMAIL_TYPE, @@ -393,9 +394,12 @@ def update_letter_notifications_to_error(self, notification_references): ) current_app.logger.info("Updated {} letter notifications to technical-failure".format(updated_count)) - notifications = dao_get_notifications_by_reference(references=notification_references) - for notification in notifications: - send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) + notifications = dao_get_notifications_by_references(references=notification_references) + # queue callback task only if the service_callback_api exists + service_callback_api = get_service_callback_api_for_service(service_id=notifications[0].service_id) + if service_callback_api: + for notification in notifications: + send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.NOTIFY) def create_dvla_file_contents_for_job(job_id): @@ -477,9 +481,12 @@ def update_letter_notifications_statuses(self, filename): current_app.logger.info( 'DVLA file: {filename}, notification updated to {status}: {reference}'.format( filename=filename, status=status, reference=str(update.reference))) - notifications = dao_get_notifications_by_reference(references=[update.reference]) - for notification in notifications: - send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) + notifications = dao_get_notifications_by_references(references=[update.reference]) + # queue callback task only if the service_callback_api exists + service_callback_api = get_service_callback_api_for_service(service_id=notifications[0].service_id) + if service_callback_api: + for notification in notifications: + send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.NOTIFY) def process_updates_from_file(response_file): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index be66b4e29..1f7b33167 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -460,7 +460,7 @@ def dao_get_notifications_by_to_field(service_id, search_term, statuses=None): @statsd(namespace="dao") -def dao_get_notifications_by_reference(references): +def dao_get_notifications_by_references(references): return Notification.query.filter( Notification.reference.in_(references) ).all() diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index c04a27784..28fa96f2e 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -10,6 +10,7 @@ from app.clients.email.aws_ses import get_aws_responses from app.dao import ( notifications_dao ) +from app.dao.service_callback_api_dao import get_service_callback_api_for_service from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks from app.notifications.process_client_response import validate_callback_data from app.celery.service_callback_tasks import send_delivery_status_to_service @@ -77,7 +78,7 @@ def process_ses_response(ses_request): ) create_outcome_notification_statistic_tasks(notification) - send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) + _check_and_queue_callback_task(notification.id, notification.service_id) return except KeyError: @@ -92,3 +93,10 @@ def process_ses_response(ses_request): def remove_emails_from_bounce(bounce_dict): for recip in bounce_dict['bouncedRecipients']: recip.pop('emailAddress') + + +def _check_and_queue_callback_task(notification_id, service_id): + # queue callback task only if the service_callback_api exists + service_callback_api = get_service_callback_api_for_service(service_id=service_id) + if service_callback_api: + send_delivery_status_to_service.apply_async([str(notification_id)], queue=QueueNames.NOTIFY) diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index 129ef9ae2..ed825766a 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -10,6 +10,8 @@ from app.clients.sms.mmg import get_mmg_responses from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks from app.celery.service_callback_tasks import send_delivery_status_to_service from app.config import QueueNames +from app.dao.service_callback_api_dao import get_service_callback_api_for_service + sms_response_mapper = { 'MMG': get_mmg_responses, @@ -83,8 +85,11 @@ def process_sms_client_response(status, reference, client_name): ) create_outcome_notification_statistic_tasks(notification) + # queue callback task only if the service_callback_api exists + service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) - send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) + if service_callback_api: + send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.NOTIFY) success = "{} callback succeeded. reference {} updated".format(client_name, reference) return success, errors diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 25ea27439..9700c8394 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -21,7 +21,7 @@ from app.celery.tasks import ( update_letter_notifications_to_sent_to_dvla ) -from tests.app.db import create_notification +from tests.app.db import create_notification, create_service_callback_api from tests.conftest import set_config @@ -97,10 +97,11 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp billable_units=0) failed_letter = create_notification(sample_letter_template, reference='ref-bar', status=NOTIFICATION_SENDING, billable_units=0) - + create_service_callback_api(service=sample_letter_template.service, url="https://original_url.com") valid_file = '{}|Sent|1|Unsorted\n{}|Failed|2|Sorted'.format( sent_letter.reference, failed_letter.reference) mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + send_mock = mocker.patch( 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' ) @@ -144,7 +145,7 @@ def test_update_letter_notifications_to_error_updates_based_on_notification_refe ) first = create_notification(sample_letter_template, reference='first ref') second = create_notification(sample_letter_template, reference='second ref') - + create_service_callback_api(service=sample_letter_template.service, url="https://original_url.com") dt = datetime.utcnow() with freeze_time(dt): update_letter_notifications_to_error([first.reference]) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 2c1230bf8..0bb9b5322 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -30,7 +30,7 @@ from app.dao.notifications_dao import ( set_scheduled_notification_to_processed, update_notification_status_by_id, update_notification_status_by_reference, - dao_get_notifications_by_reference + dao_get_notifications_by_references ) from app.dao.services_dao import dao_update_service from app.models import ( @@ -1997,7 +1997,7 @@ def test_dao_get_notifications_by_reference(sample_template): notification_1 = create_notification(template=sample_template, reference='ref') notification_2 = create_notification(template=sample_template, reference='ref') - notifications = dao_get_notifications_by_reference(['ref']) + notifications = dao_get_notifications_by_references(['ref']) assert len(notifications) == 2 assert notifications[0].id in [notification_1.id, notification_2.id] assert notifications[1].id in [notification_1.id, notification_2.id] diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 48fe7d60f..0e224871b 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -10,6 +10,7 @@ from app.dao.notifications_dao import ( get_notification_by_id ) from tests.app.conftest import sample_notification as create_sample_notification +from tests.app.db import create_service_callback_api def firetext_post(client, data): @@ -377,7 +378,7 @@ def test_process_mmg_response_unknown_status_updates_notification_with_failed( "CID": str(notification.id), "MSISDN": "447777349060", "status": 10}) - + create_service_callback_api(service=notification.service, url="https://original_url.com") response = mmg_post(client, data) assert response.status_code == 200 json_data = json.loads(response.data) diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index fb156dc07..75e0b16bf 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -10,6 +10,7 @@ from app.notifications.notifications_ses_callback import process_ses_response, r from app.celery.research_mode_tasks import ses_hard_bounce_callback, ses_soft_bounce_callback, ses_notification_callback from tests.app.conftest import sample_notification as create_sample_notification +from tests.app.db import create_service_callback_api def test_ses_callback_should_update_notification_status( @@ -35,7 +36,7 @@ def test_ses_callback_should_update_notification_status( status='sending', sent_at=datetime.utcnow() ) - + create_service_callback_api(service=sample_email_template.service, url="https://original_url.com") assert get_notification_by_id(notification.id).status == 'sending' errors = process_ses_response(ses_notification_callback(reference='ref')) @@ -46,7 +47,7 @@ def test_ses_callback_should_update_notification_status( ) statsd_client.incr.assert_any_call("callback.ses.delivered") stats_mock.assert_called_once_with(notification) - send_mock.assert_called_once_with([notification.id], queue="notify-internal-tasks") + send_mock.assert_called_once_with([str(notification.id)], queue="notify-internal-tasks") def test_ses_callback_should_update_multiple_notification_status_sent( @@ -85,7 +86,7 @@ def test_ses_callback_should_update_multiple_notification_status_sent( reference='ref3', sent_at=datetime.utcnow(), status='sending') - + create_service_callback_api(service=sample_email_template.service, url="https://original_url.com") assert process_ses_response(ses_notification_callback(reference='ref1')) is None assert process_ses_response(ses_notification_callback(reference='ref2')) is None assert process_ses_response(ses_notification_callback(reference='ref3')) is None @@ -118,6 +119,7 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, status='sending', sent_at=datetime.utcnow() ) + create_service_callback_api(service=notification.service, url="https://original_url.com") assert get_notification_by_id(notification.id).status == 'sending' assert process_ses_response(ses_soft_bounce_callback(reference='ref')) is None assert get_notification_by_id(notification.id).status == 'temporary-failure' @@ -166,6 +168,7 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, status='sending', sent_at=datetime.utcnow() ) + create_service_callback_api(service=sample_email_template.service, url="https://original_url.com") assert get_notification_by_id(notification.id).status == 'sending' assert process_ses_response(ses_hard_bounce_callback(reference='ref')) is None diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index 503d4b9eb..f19909167 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -4,6 +4,7 @@ from app.notifications.process_client_response import ( validate_callback_data, process_sms_client_response ) +from tests.app.db import create_service_callback_api def test_validate_callback_data_returns_none_when_valid(): @@ -54,6 +55,7 @@ def test_outcome_statistics_called_for_successful_callback(sample_notification, send_mock = mocker.patch( 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' ) + create_service_callback_api(service=sample_notification.service, url="https://original_url.com") reference = str(uuid.uuid4()) success, error = process_sms_client_response(status='3', reference=reference, client_name='MMG') From b4a1d635f75029022d949c88f896ddf7afd1e1c8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 5 Dec 2017 10:27:29 +0000 Subject: [PATCH 11/15] Rebuild the letter_rates table to include everything it needs. The current letter_rates table is not used so it is ok to drop and re-create it --- app/models.py | 15 ++---- .../versions/0150_refactor_letter_rates.py | 47 +++++++++++++++++++ 2 files changed, 52 insertions(+), 10 deletions(-) create mode 100644 migrations/versions/0150_refactor_letter_rates.py diff --git a/app/models.py b/app/models.py index 9d81073c7..0a65ae0a7 100644 --- a/app/models.py +++ b/app/models.py @@ -1484,17 +1484,12 @@ class LetterRate(db.Model): __tablename__ = 'letter_rates' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - valid_from = valid_from = db.Column(db.DateTime, nullable=False) - - -class LetterRateDetail(db.Model): - __tablename__ = 'letter_rate_details' - - id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - letter_rate_id = db.Column(UUID(as_uuid=True), db.ForeignKey('letter_rates.id'), index=True, nullable=False) - letter_rate = db.relationship('LetterRate', backref='letter_rates') - page_total = db.Column(db.Integer, nullable=False) + start_date = db.Column(db.DateTime, nullable=False) + end_date = db.Column(db.DateTime, nullable=True) + sheet_total = db.Column(db.Integer, nullable=False) # double sided sheet rate = db.Column(db.Numeric(), nullable=False) + crown = db.Column(db.Boolean, nullable=False) + post_class = db.Column(db.String, nullable=False) class MonthlyBilling(db.Model): diff --git a/migrations/versions/0150_refactor_letter_rates.py b/migrations/versions/0150_refactor_letter_rates.py new file mode 100644 index 000000000..2429e5222 --- /dev/null +++ b/migrations/versions/0150_refactor_letter_rates.py @@ -0,0 +1,47 @@ +""" + +Revision ID: 0150_refactor_letter_rates +Revises: 0148_add_letters_as_pdf_svc_perm +Create Date: 2017-12-05 10:24:41.232128 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0150_refactor_letter_rates' +down_revision = '0148_add_letters_as_pdf_svc_perm' + + +def upgrade(): + op.drop_table('letter_rate_details') + op.drop_table('letter_rates') + op.create_table('letter_rates', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('start_date', sa.DateTime(), nullable=False), + sa.Column('end_date', sa.DateTime(), nullable=True), + sa.Column('sheet_total', sa.Integer(), nullable=False), + sa.Column('rate', sa.Numeric(), nullable=False), + sa.Column('crown', sa.Boolean(), nullable=False), + sa.Column('post_class', sa.String(), nullable=False), + sa.PrimaryKeyConstraint('id') + ) + + +def downgrade(): + op.drop_table('letter_rates') + op.create_table('letter_rates', + sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('valid_from', postgresql.TIMESTAMP(), autoincrement=False, nullable=False), + sa.PrimaryKeyConstraint('id', name='letter_rates_pkey'), + postgresql_ignore_search_path=False + ) + op.create_table('letter_rate_details', + sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('letter_rate_id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('page_total', sa.INTEGER(), autoincrement=False, nullable=False), + sa.Column('rate', sa.NUMERIC(), autoincrement=False, nullable=False), + sa.ForeignKeyConstraint(['letter_rate_id'], ['letter_rates.id'], + name='letter_rate_details_letter_rate_id_fkey'), + sa.PrimaryKeyConstraint('id', name='letter_rate_details_pkey') + ) From 956907c170428d3e0846e1bc97bc11be31a5ff39 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 5 Dec 2017 10:54:55 +0000 Subject: [PATCH 12/15] Revert the last commit. It was intended for another branch. --- app/models.py | 15 ++++-- .../versions/0150_refactor_letter_rates.py | 47 ------------------- 2 files changed, 10 insertions(+), 52 deletions(-) delete mode 100644 migrations/versions/0150_refactor_letter_rates.py diff --git a/app/models.py b/app/models.py index d5c4ea5af..fdd00fa9a 100644 --- a/app/models.py +++ b/app/models.py @@ -1485,12 +1485,17 @@ class LetterRate(db.Model): __tablename__ = 'letter_rates' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - start_date = db.Column(db.DateTime, nullable=False) - end_date = db.Column(db.DateTime, nullable=True) - sheet_total = db.Column(db.Integer, nullable=False) # double sided sheet + valid_from = valid_from = db.Column(db.DateTime, nullable=False) + + +class LetterRateDetail(db.Model): + __tablename__ = 'letter_rate_details' + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + letter_rate_id = db.Column(UUID(as_uuid=True), db.ForeignKey('letter_rates.id'), index=True, nullable=False) + letter_rate = db.relationship('LetterRate', backref='letter_rates') + page_total = db.Column(db.Integer, nullable=False) rate = db.Column(db.Numeric(), nullable=False) - crown = db.Column(db.Boolean, nullable=False) - post_class = db.Column(db.String, nullable=False) class MonthlyBilling(db.Model): diff --git a/migrations/versions/0150_refactor_letter_rates.py b/migrations/versions/0150_refactor_letter_rates.py deleted file mode 100644 index 2429e5222..000000000 --- a/migrations/versions/0150_refactor_letter_rates.py +++ /dev/null @@ -1,47 +0,0 @@ -""" - -Revision ID: 0150_refactor_letter_rates -Revises: 0148_add_letters_as_pdf_svc_perm -Create Date: 2017-12-05 10:24:41.232128 - -""" -from alembic import op -import sqlalchemy as sa -from sqlalchemy.dialects import postgresql - -revision = '0150_refactor_letter_rates' -down_revision = '0148_add_letters_as_pdf_svc_perm' - - -def upgrade(): - op.drop_table('letter_rate_details') - op.drop_table('letter_rates') - op.create_table('letter_rates', - sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('start_date', sa.DateTime(), nullable=False), - sa.Column('end_date', sa.DateTime(), nullable=True), - sa.Column('sheet_total', sa.Integer(), nullable=False), - sa.Column('rate', sa.Numeric(), nullable=False), - sa.Column('crown', sa.Boolean(), nullable=False), - sa.Column('post_class', sa.String(), nullable=False), - sa.PrimaryKeyConstraint('id') - ) - - -def downgrade(): - op.drop_table('letter_rates') - op.create_table('letter_rates', - sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), - sa.Column('valid_from', postgresql.TIMESTAMP(), autoincrement=False, nullable=False), - sa.PrimaryKeyConstraint('id', name='letter_rates_pkey'), - postgresql_ignore_search_path=False - ) - op.create_table('letter_rate_details', - sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), - sa.Column('letter_rate_id', postgresql.UUID(), autoincrement=False, nullable=False), - sa.Column('page_total', sa.INTEGER(), autoincrement=False, nullable=False), - sa.Column('rate', sa.NUMERIC(), autoincrement=False, nullable=False), - sa.ForeignKeyConstraint(['letter_rate_id'], ['letter_rates.id'], - name='letter_rate_details_letter_rate_id_fkey'), - sa.PrimaryKeyConstraint('id', name='letter_rate_details_pkey') - ) From 81ead8b246e13352ae10c784c03580ebe004e350 Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 5 Dec 2017 11:23:21 +0000 Subject: [PATCH 13/15] code style fix --- app/celery/service_callback_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py index e91835e56..3bf033a80 100644 --- a/app/celery/service_callback_tasks.py +++ b/app/celery/service_callback_tasks.py @@ -68,4 +68,4 @@ def send_delivery_status_to_service(self, notification_id): try: self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: - current_app.logger.exception('Retry: send_delivery_status_to_service has retried the max number of times') + current_app.logger.exception('Retry: send_delivery_status_to_service has retried the max num of times') From 444876222e0f143c5e5c5ac74deb3cd7568d2adf Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 5 Dec 2017 13:36:45 +0000 Subject: [PATCH 14/15] Touch log files in the initialization The logs agent would complain for the missing gunicorn error log file. --- scripts/run_app_paas.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/run_app_paas.sh b/scripts/run_app_paas.sh index 2813f851f..78610c1bd 100755 --- a/scripts/run_app_paas.sh +++ b/scripts/run_app_paas.sh @@ -16,6 +16,10 @@ function check_params { } function configure_aws_logs { + # create files so that aws logs agent doesn't complain + touch /home/vcap/logs/gunicorn_error.log + touch /home/vcap/logs/app.log.json + aws configure set plugins.cwlogs cwlogs export AWS_ACCESS_KEY_ID=$(echo ${VCAP_SERVICES} | jq -r '.["user-provided"][]|select(.name=="notify-aws")|.credentials.aws_access_key_id') From 20b306bfdbef942c213d81dfabef41c28988b2ea Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 5 Dec 2017 13:57:46 +0000 Subject: [PATCH 15/15] added more unit tests --- tests/app/celery/test_ftp_update_tasks.py | 21 +++++++++++++- .../test_notifications_ses_callback.py | 29 +++++++++++++++++++ .../test_process_client_response.py | 15 +++++++++- 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 9700c8394..2c6bc4393 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -23,6 +23,7 @@ from app.celery.tasks import ( from tests.app.db import create_notification, create_service_callback_api from tests.conftest import set_config +from unittest.mock import call def test_update_job_to_sent_to_dvla(sample_letter_template, sample_letter_job): @@ -114,7 +115,25 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp assert failed_letter.status == NOTIFICATION_TECHNICAL_FAILURE assert failed_letter.billable_units == 2 assert failed_letter.updated_at - assert send_mock.called + + calls = [call([str(failed_letter.id)], queue="notify-internal-tasks"), + call([str(sent_letter.id)], queue="notify-internal-tasks")] + send_mock.assert_has_calls(calls, any_order=True) + + +def test_update_letter_notifications_does_not_call_send_callback_if_no_db_entry(notify_api, mocker, + sample_letter_template): + sent_letter = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING, + billable_units=0) + valid_file = '{}|Sent|1|Unsorted\n'.format(sent_letter.reference) + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) + + update_letter_notifications_statuses(filename='foo.txt') + send_mock.assert_not_called() def test_update_letter_notifications_to_sent_to_dvla_updates_based_on_notification_references( diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index 75e0b16bf..49ce90829 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -50,6 +50,35 @@ def test_ses_callback_should_update_notification_status( send_mock.assert_called_once_with([str(notification.id)], queue="notify-internal-tasks") +def test_ses_callback_does_not_call_send_delivery_status_if_no_db_entry( + client, + notify_db, + notify_db_session, + sample_email_template, + mocker): + with freeze_time('2001-01-01T12:00:00'): + + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) + notification = create_sample_notification( + notify_db, + notify_db_session, + template=sample_email_template, + reference='ref', + status='sending', + sent_at=datetime.utcnow() + ) + + assert get_notification_by_id(notification.id).status == 'sending' + + errors = process_ses_response(ses_notification_callback(reference='ref')) + assert errors is None + assert get_notification_by_id(notification.id).status == 'delivered' + + send_mock.assert_not_called() + + def test_ses_callback_should_update_multiple_notification_status_sent( client, notify_db, diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index f19909167..40e2a608c 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -61,10 +61,23 @@ def test_outcome_statistics_called_for_successful_callback(sample_notification, success, error = process_sms_client_response(status='3', reference=reference, client_name='MMG') assert success == "MMG callback succeeded. reference {} updated".format(str(reference)) assert error is None - assert send_mock.called + send_mock.assert_called_once_with([str(sample_notification.id)], queue="notify-internal-tasks") stats_mock.assert_called_once_with(sample_notification) +def test_sms_resonse_does_not_call_send_callback_if_no_db_entry(sample_notification, mocker): + mocker.patch( + 'app.notifications.process_client_response.notifications_dao.update_notification_status_by_id', + return_value=sample_notification + ) + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) + reference = str(uuid.uuid4()) + process_sms_client_response(status='3', reference=reference, client_name='MMG') + send_mock.assert_not_called() + + def test_process_sms_response_return_success_for_send_sms_code_reference(mocker): stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks')