From 382f429c6c82b493f07d6158876d8bfb23d6b405 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 30 Nov 2017 20:19:45 +0000 Subject: [PATCH 01/26] Update awscli from 1.14.1 to 1.14.2 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index a692fb866..9c9caa2ae 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,7 +23,7 @@ statsd==3.2.1 notifications-python-client==4.6.0 # PaaS -awscli==1.14.1 +awscli==1.14.2 awscli-cwlogs>=1.4,<1.5 git+https://github.com/alphagov/notifications-utils.git@23.1.0#egg=notifications-utils==23.1.0 From a0fe5c69715e6bc3380383b72fb60839a161ddee Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 09:31:03 +0000 Subject: [PATCH 02/26] 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 03/26] 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 04/26] 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 5f25d3162a07c824b5ab5273ef7a533b6900c5e9 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Fri, 1 Dec 2017 15:30:18 +0000 Subject: [PATCH 05/26] Disable Redis for Staging added statsd for post methods - Disable Redis as there is a current connection limit of 256 which could slow down the request if they are all used - Added statd to methods in the post to help spot any bottlenecks --- app/config.py | 1 + app/notifications/process_notifications.py | 5 +++++ app/notifications/validators.py | 5 +++++ app/v2/notifications/create_response.py | 3 +++ app/v2/notifications/post_notifications.py | 4 ++++ 5 files changed, 18 insertions(+) diff --git a/app/config.py b/app/config.py index 3199fbe93..22d9fdd10 100644 --- a/app/config.py +++ b/app/config.py @@ -388,6 +388,7 @@ class Staging(Config): FROM_NUMBER = 'stage' API_RATE_LIMIT_ENABLED = True CHECK_PROXY_HEADER = True + REDIS_ENABLED = False API_KEY_LIMITS = { KEY_TYPE_TEAM: { diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index df46b0f02..1277d9921 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -13,6 +13,7 @@ from notifications_utils.recipients import ( from app import redis_store from app.celery import provider_tasks from app.config import QueueNames + from app.models import ( EMAIL_TYPE, KEY_TYPE_TEST, @@ -26,6 +27,8 @@ from app.dao.notifications_dao import ( dao_delete_notifications_and_history_by_id, dao_created_scheduled_notification ) + +from app.statsd_decorators import statsd from app.v2.errors import BadRequestError from app.utils import get_template_instance, cache_key_for_service_template_counter, convert_bst_to_utc @@ -43,6 +46,7 @@ def check_placeholders(template_object): raise BadRequestError(fields=[{'template': message}], message=message) +@statsd(namespace="performance-testing") def persist_notification( *, template_id, @@ -112,6 +116,7 @@ def persist_notification( return notification +@statsd(namespace="performance-testing") def send_notification_to_queue(notification, research_mode, queue=None): if research_mode or notification.key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 344f05ebe..2fc01b64f 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -14,6 +14,7 @@ from app.models import ( KEY_TYPE_TEST, KEY_TYPE_TEAM, SCHEDULE_NOTIFICATIONS ) from app.service.utils import service_allowed_to_send_to +from app.statsd_decorators import statsd from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError from app import redis_store from app.notifications.process_notifications import create_content_for_notification @@ -46,6 +47,7 @@ def check_service_over_daily_message_limit(key_type, service): raise TooManyRequestsError(service.message_limit) +@statsd(namespace="performance-testing") def check_rate_limiting(service, api_key): check_service_over_api_rate_limit(service, api_key) check_service_over_daily_message_limit(api_key.key_type, service) @@ -80,12 +82,14 @@ def service_has_permission(notify_type, permissions): return notify_type in [p.permission for p in permissions] +@statsd(namespace="performance-testing") def check_service_has_permission(notify_type, permissions): if not service_has_permission(notify_type, permissions): raise BadRequestError(message="Cannot send {}".format( get_public_notify_type_text(notify_type, plural=True))) +@statsd(namespace="performance-testing") def check_service_can_schedule_notification(permissions, scheduled_for): if scheduled_for: if not service_has_permission(SCHEDULE_NOTIFICATIONS, permissions): @@ -117,6 +121,7 @@ def check_sms_content_char_count(content_count): raise BadRequestError(message=message) +@statsd(namespace="performance-testing") def validate_template(template_id, personalisation, service, notification_type): try: template = templates_dao.dao_get_template_by_id_and_service_id( diff --git a/app/v2/notifications/create_response.py b/app/v2/notifications/create_response.py index 7eecfb1b9..a2e44001c 100644 --- a/app/v2/notifications/create_response.py +++ b/app/v2/notifications/create_response.py @@ -1,3 +1,5 @@ +from app.statsd_decorators import statsd + def create_post_sms_response_from_notification(notification, content, from_number, url_root, scheduled_for): noti = __create_notification_response(notification, url_root, scheduled_for) @@ -8,6 +10,7 @@ def create_post_sms_response_from_notification(notification, content, from_numbe return noti +@statsd(namespace="performance-testing") def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, scheduled_for): noti = __create_notification_response(notification, url_root, scheduled_for) noti['content'] = { diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index be35f30b4..1026331a5 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -34,6 +34,7 @@ from app.notifications.validators import ( check_service_sms_sender_id ) from app.schema_validation import validate +from app.statsd_decorators import statsd from app.v2.errors import BadRequestError from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import ( @@ -118,6 +119,7 @@ def post_notification(notification_type): return jsonify(resp), 201 +@statsd(namespace="performance-testing") def process_sms_or_email_notification(*, form, notification_type, api_key, template, service, reply_to_text=None): form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number'] @@ -160,6 +162,7 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ return notification +@statsd(namespace="performance-testing") def process_letter_notification(*, letter_data, api_key, template): if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) @@ -187,6 +190,7 @@ def process_letter_notification(*, letter_data, api_key, template): return notification +@statsd(namespace="performance-testing") def get_reply_to_text(notification_type, form): reply_to = None if notification_type == EMAIL_TYPE: From 344e5c1f5df483e304d8d82ab1b014f1abd26198 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Fri, 1 Dec 2017 15:46:31 +0000 Subject: [PATCH 06/26] dded statsd to the endpoint and removed it from a letters only method as we are not interested int he letters performance at the moment. --- app/v2/notifications/post_notifications.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 1026331a5..08847f2cb 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -50,6 +50,7 @@ from app.v2.notifications.create_response import ( @v2_notification_blueprint.route('/', methods=['POST']) +@statsd(namespace="performance-testing") def post_notification(notification_type): if notification_type == EMAIL_TYPE: form = validate(request.get_json(), post_email_request) @@ -162,7 +163,6 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ return notification -@statsd(namespace="performance-testing") def process_letter_notification(*, letter_data, api_key, template): if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) From 489f43a2c9a14cc84f83a93b5638630d4881c944 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 16:15:21 +0000 Subject: [PATCH 07/26] 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 907a9d62a1bb4148403a616e55b0c12c96c86725 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 1 Dec 2017 13:45:49 +0000 Subject: [PATCH 08/26] Add letters_as_pdf service permission --- app/models.py | 2 ++ .../0148_add_letters_as_pdf_svc_perm.py | 24 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 migrations/versions/0148_add_letters_as_pdf_svc_perm.py diff --git a/app/models.py b/app/models.py index fb3a2610d..9d81073c7 100644 --- a/app/models.py +++ b/app/models.py @@ -178,6 +178,7 @@ INTERNATIONAL_SMS_TYPE = 'international_sms' INBOUND_SMS_TYPE = 'inbound_sms' SCHEDULE_NOTIFICATIONS = 'schedule_notifications' EMAIL_AUTH = 'email_auth' +LETTERS_AS_PDF = 'letters_as_pdf' SERVICE_PERMISSION_TYPES = [ EMAIL_TYPE, @@ -187,6 +188,7 @@ SERVICE_PERMISSION_TYPES = [ INBOUND_SMS_TYPE, SCHEDULE_NOTIFICATIONS, EMAIL_AUTH, + LETTERS_AS_PDF, ] diff --git a/migrations/versions/0148_add_letters_as_pdf_svc_perm.py b/migrations/versions/0148_add_letters_as_pdf_svc_perm.py new file mode 100644 index 000000000..94f56a2af --- /dev/null +++ b/migrations/versions/0148_add_letters_as_pdf_svc_perm.py @@ -0,0 +1,24 @@ +"""empty message + +Revision ID: 0148_add_letters_as_pdf_svc_perm +Revises: 0147_drop_mapping_tables +Create Date: 2017-12-01 13:33:18.581320 + +""" + +# revision identifiers, used by Alembic. +revision = '0148_add_letters_as_pdf_svc_perm' +down_revision = '0147_drop_mapping_tables' + +from alembic import op + + +def upgrade(): + op.get_bind() + op.execute("insert into service_permission_types values('letters_as_pdf')") + + +def downgrade(): + op.get_bind() + op.execute("delete from service_permissions where permission = 'letters_as_pdf'") + op.execute("delete from service_permission_types where name = 'letters_as_pdf'") From a604fb82bb4b9aedd871fd63d665e110e0f8d7a5 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 20:30:53 +0000 Subject: [PATCH 09/26] 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 10/26] 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 11/26] 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 88c878c83e2d514d08dd5bd16e14ba514d4da434 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 4 Dec 2017 11:12:26 +0000 Subject: [PATCH 12/26] don't hit the query to get daily msg stats if redis is disabled --- app/notifications/validators.py | 4 ++-- tests/app/notifications/test_validators.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 2fc01b64f..176241f1e 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -23,7 +23,7 @@ from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id def check_service_over_api_rate_limit(service, api_key): - if current_app.config['API_RATE_LIMIT_ENABLED']: + if current_app.config['API_RATE_LIMIT_ENABLED'] and current_app.config['REDIS_ENABLED']: cache_key = rate_limit_cache_key(service.id, api_key.key_type) rate_limit = current_app.config['API_KEY_LIMITS'][api_key.key_type]['limit'] interval = current_app.config['API_KEY_LIMITS'][api_key.key_type]['interval'] @@ -33,7 +33,7 @@ def check_service_over_api_rate_limit(service, api_key): def check_service_over_daily_message_limit(key_type, service): - if key_type != KEY_TYPE_TEST: + if key_type != KEY_TYPE_TEST and current_app.config['REDIS_ENABLED']: cache_key = daily_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) if not service_stats: diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index cb841e431..1c9fbb579 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -1,6 +1,7 @@ import pytest from freezegun import freeze_time from flask import current_app + import app from app.models import INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE from app.notifications.validators import ( @@ -18,6 +19,8 @@ from app.v2.errors import ( BadRequestError, TooManyRequestsError, RateLimitError) + +from tests.conftest import set_config from tests.app.conftest import ( sample_notification as create_notification, sample_service as create_service, @@ -26,6 +29,13 @@ from tests.app.conftest import ( from tests.app.db import create_reply_to_email, create_service_sms_sender +# all of these tests should have redis enabled (except where we specifically disable it) +@pytest.fixture(scope='module', autouse=True) +def enable_redis(notify_api): + with set_config(notify_api, 'REDIS_ENABLED', True): + yield + + @pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) def test_check_service_message_limit_in_cache_with_unrestricted_service_is_allowed( key_type, @@ -78,6 +88,15 @@ def test_should_set_cache_value_as_value_from_database_if_cache_not_set( ) +def test_should_not_access_database_if_redis_disabled(notify_api, sample_service, mocker): + with set_config(notify_api, 'REDIS_ENABLED', False): + db_mock = mocker.patch('app.notifications.validators.services_dao') + + check_service_over_daily_message_limit('normal', sample_service) + + assert db_mock.method_calls == [] + + @pytest.mark.parametrize('key_type', ['team', 'normal']) def test_check_service_message_limit_over_message_limit_fails(key_type, notify_db, notify_db_session, mocker): with freeze_time("2016-01-01 12:00:00.000000"): From f73319f5ef886e66969a08c42bc338d74516f713 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 4 Dec 2017 13:24:53 +0000 Subject: [PATCH 13/26] 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 5bb6b68e57181bb75d54f5e761cfbb92e04bb1a3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 4 Dec 2017 14:13:43 +0000 Subject: [PATCH 14/26] fix tests that expect to be rate limited (which is off if redis is disabled) --- .../notifications/rest/test_send_notification.py | 16 ++++++++++++---- tests/app/service/test_rest.py | 9 ++++----- .../service/test_send_one_off_notification.py | 6 +++++- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 4b57c4128..480455e75 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -16,7 +16,10 @@ from app.models import ( from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template from app.dao.services_dao import dao_update_service from app.dao.api_key_dao import save_model_api_key -from app.v2.errors import RateLimitError +from app.errors import InvalidRequest +from app.models import Template +from app.v2.errors import RateLimitError, TooManyRequestsError + from tests import create_authorization_header from tests.app.conftest import ( sample_notification as create_sample_notification, @@ -28,9 +31,6 @@ from tests.app.conftest import ( sample_service, sample_template_without_sms_permission, sample_template_without_email_permission) - -from app.models import Template -from app.errors import InvalidRequest from tests.app.db import create_service, create_reply_to_email @@ -414,6 +414,10 @@ def test_should_block_api_call_if_over_day_limit_for_live_service( mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: + mocker.patch( + 'app.notifications.validators.check_service_over_daily_message_limit', + side_effect=TooManyRequestsError(1) + ) mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=False) @@ -446,6 +450,10 @@ def test_should_block_api_call_if_over_day_limit_for_restricted_service( with notify_api.test_request_context(): with notify_api.test_client() as client: mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch( + 'app.notifications.validators.check_service_over_daily_message_limit', + side_effect=TooManyRequestsError(1) + ) service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=True) email_template = create_sample_email_template(notify_db, notify_db_session, service=service) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 73679001f..2c09939b0 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2163,18 +2163,17 @@ def test_search_for_notification_by_to_field_returns_content( assert notifications[0]['template']['content'] == 'Hello (( Name))\nYour thing is due soon' -def test_send_one_off_notification(admin_request, mocker): - service = create_service() - template = create_template(service=service) +def test_send_one_off_notification(sample_service, admin_request, mocker): + template = create_template(service=sample_service) mocker.patch('app.service.send_notification.send_notification_to_queue') response = admin_request.post( 'service.create_one_off_notification', - service_id=service.id, + service_id=sample_service.id, _data={ 'template_id': str(template.id), 'to': '07700900001', - 'created_by': str(service.created_by_id) + 'created_by': str(sample_service.created_by_id) }, _expected_status=201 ) diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 14c49aa56..6a9a681ff 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -151,9 +151,13 @@ def test_send_one_off_notification_raises_if_cant_send_to_recipient(notify_db_se assert 'service is in trial mode' in e.value.message -def test_send_one_off_notification_raises_if_over_limit(notify_db_session): +def test_send_one_off_notification_raises_if_over_limit(notify_db_session, mocker): service = create_service(message_limit=0) template = create_template(service=service) + mocker.patch( + 'app.service.send_notification.check_service_over_daily_message_limit', + side_effect=TooManyRequestsError(1) + ) post_data = { 'template_id': str(template.id), From 7fa4e7ffc706d1399a4a5b5912f5a41caffc46bf Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 4 Dec 2017 16:07:26 +0000 Subject: [PATCH 15/26] 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 c2228bb7d2aa6ec0f965a7d79a3b2b2b14134c6a Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 28 Nov 2017 11:59:17 +0000 Subject: [PATCH 16/26] Enable proxy header check on live --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 22d9fdd10..1ba975619 100644 --- a/app/config.py +++ b/app/config.py @@ -417,7 +417,7 @@ class Live(Config): FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = 'ba9e1789-a804-40b8-871f-cc60d4c1286f' PERFORMANCE_PLATFORM_ENABLED = True API_RATE_LIMIT_ENABLED = True - CHECK_PROXY_HEADER = False + CHECK_PROXY_HEADER = True class CloudFoundryConfig(Config): From d62b350fcb932e29db6b086e43c938462477c9d0 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 4 Dec 2017 16:29:04 +0000 Subject: [PATCH 17/26] Add config for letters pdf s3 bucket different environments --- app/config.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/config.py b/app/config.py index 1ba975619..2294016d5 100644 --- a/app/config.py +++ b/app/config.py @@ -314,6 +314,7 @@ class Development(Config): SQLALCHEMY_ECHO = False NOTIFY_EMAIL_DOMAIN = 'notify.tools' CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' + LETTERS_PDF_BUCKET_NAME = 'development-letters-pdf' DVLA_RESPONSE_BUCKET_NAME = 'notify.tools-ftp' NOTIFY_ENVIRONMENT = 'development' NOTIFICATION_QUEUE_PREFIX = 'development' @@ -335,6 +336,7 @@ class Test(Config): DEBUG = True TESTING = True CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' + LETTERS_PDF_BUCKET_NAME = 'test-letters-pdf' DVLA_RESPONSE_BUCKET_NAME = 'test.notify.com-ftp' STATSD_ENABLED = True STATSD_HOST = "localhost" @@ -373,6 +375,7 @@ class Preview(Config): NOTIFY_EMAIL_DOMAIN = 'notify.works' NOTIFY_ENVIRONMENT = 'preview' CSV_UPLOAD_BUCKET_NAME = 'preview-notifications-csv-upload' + LETTERS_PDF_BUCKET_NAME = 'preview-letters-pdf' DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' FROM_NUMBER = 'preview' API_RATE_LIMIT_ENABLED = True @@ -383,6 +386,7 @@ class Staging(Config): NOTIFY_EMAIL_DOMAIN = 'staging-notify.works' NOTIFY_ENVIRONMENT = 'staging' CSV_UPLOAD_BUCKET_NAME = 'staging-notify-csv-upload' + LETTERS_PDF_BUCKET_NAME = 'staging-letters-pdf' DVLA_RESPONSE_BUCKET_NAME = 'staging-notify.works-ftp' STATSD_ENABLED = True FROM_NUMBER = 'stage' @@ -410,6 +414,7 @@ class Live(Config): NOTIFY_EMAIL_DOMAIN = 'notifications.service.gov.uk' NOTIFY_ENVIRONMENT = 'live' CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' + LETTERS_PDF_BUCKET_NAME = 'live-letters-pdf' DVLA_RESPONSE_BUCKET_NAME = 'notifications.service.gov.uk-ftp' STATSD_ENABLED = True FROM_NUMBER = 'GOVUK' @@ -429,6 +434,7 @@ class Sandbox(CloudFoundryConfig): NOTIFY_EMAIL_DOMAIN = 'notify.works' NOTIFY_ENVIRONMENT = 'sandbox' CSV_UPLOAD_BUCKET_NAME = 'cf-sandbox-notifications-csv-upload' + LETTERS_PDF_BUCKET_NAME = 'cf-sandbox-letters-pdf' DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' FROM_NUMBER = 'sandbox' REDIS_ENABLED = False From aa39f47d8e3f78f6372cea091bae26f3417dc441 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 4 Dec 2017 16:39:32 +0000 Subject: [PATCH 18/26] Added letters pdf S3 upload function - uses s3upload from notifications_utils which will create an s3 bucket if it doesn't already exist --- app/aws/s3.py | 23 +++++++++++++++++++++++ tests/app/aws/test_s3.py | 22 +++++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 927eb0ad1..d5acb69fd 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -5,7 +5,10 @@ from flask import current_app import pytz from boto3 import client, resource +from notifications_utils.s3 import s3upload as utils_s3upload + FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv' +LETTERS_PDF_FILE_LOCATION_STRUCTURE = '{folder}/NOTIFY.{reference}.{duplex}.{letter_class}.{colour}.{crown}.{date}.pdf' def get_s3_file(bucket_name, file_location): @@ -76,3 +79,23 @@ def remove_transformed_dvla_file(job_id): file_location = '{}-dvla-job.text'.format(job_id) obj = get_s3_object(bucket_name, file_location) return obj.delete() + + +def upload_letters_pdf(reference, crown, filedata): + now = datetime.utcnow() + upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( + folder=now.date().isoformat(), + reference=reference, + duplex="D", + letter_class="2", + colour="C", + crown="C" if crown else "N", + date=now.strftime('%Y%m%d%H%M%S') + ) + + utils_s3upload( + filedata=filedata, + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + file_location=upload_file_name + ) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index d6e4c1b75..d144a29f0 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -1,5 +1,6 @@ from unittest.mock import call from datetime import datetime, timedelta +import pytest from flask import current_app @@ -9,7 +10,8 @@ from app.aws.s3 import ( get_s3_bucket_objects, get_s3_file, filter_s3_bucket_objects_within_date_range, - remove_transformed_dvla_file + remove_transformed_dvla_file, + upload_letters_pdf ) from tests.app.conftest import datetime_in_past @@ -139,3 +141,21 @@ def test_get_s3_bucket_objects_does_not_return_outside_of_date_range(notify_api, filtered_items = filter_s3_bucket_objects_within_date_range(s3_objects_stub) assert len(filtered_items) == 0 + + +@pytest.mark.parametrize('crown_flag,expected_crown_text', [ + (True, 'C'), + (False, 'N'), +]) +@freeze_time("2017-12-04 15:00:00") +def test_upload_letters_pdf_calls_utils_s3upload_with_correct_args( + notify_api, mocker, crown_flag, expected_crown_text): + s3_upload_mock = mocker.patch('app.aws.s3.utils_s3upload') + upload_letters_pdf(reference='foo', crown=crown_flag, filedata='some_data') + + s3_upload_mock.assert_called_with( + filedata='some_data', + region='eu-west-1', + bucket_name='test-letters-pdf', + file_location='2017-12-04/NOTIFY.foo.D.2.C.{}.20171204150000.pdf'.format(expected_crown_text) + ) From 5482ee4fe732971f55a2cc2c7a32d1d62c2f84c0 Mon Sep 17 00:00:00 2001 From: venusbb Date: Mon, 4 Dec 2017 14:48:23 +0000 Subject: [PATCH 19/26] - 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 20/26] 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 21/26] 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 22/26] 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 23/26] 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 24/26] 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') From 7b390e74c4733220d35d6056786ba6467c419f83 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 5 Dec 2017 16:03:08 +0000 Subject: [PATCH 25/26] Upper case the filename to meet requirements --- app/aws/s3.py | 2 +- tests/app/aws/test_s3.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index d5acb69fd..a64be797c 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -91,7 +91,7 @@ def upload_letters_pdf(reference, crown, filedata): colour="C", crown="C" if crown else "N", date=now.strftime('%Y%m%d%H%M%S') - ) + ).upper() utils_s3upload( filedata=filedata, diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index d144a29f0..e610560db 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -157,5 +157,5 @@ def test_upload_letters_pdf_calls_utils_s3upload_with_correct_args( filedata='some_data', region='eu-west-1', bucket_name='test-letters-pdf', - file_location='2017-12-04/NOTIFY.foo.D.2.C.{}.20171204150000.pdf'.format(expected_crown_text) + file_location='2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204150000.PDF'.format(expected_crown_text) ) From af1ab437be9398c13a2e371ea25ca1ad95e2bf06 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Wed, 6 Dec 2017 09:36:23 +0000 Subject: [PATCH 26/26] Revert changes to staging for performance tests and increased rate limit - Reverted the Gunicorn worker number to 5 (this should be investigated further on a well baselined system to compare) - Enabled REDIS - Increased the rate limit to 400 req/sec as using early testing yesterday 450+ was being achieved --- app/config.py | 8 ++++---- manifest-api-staging.yml | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/config.py b/app/config.py index 2294016d5..d553041e3 100644 --- a/app/config.py +++ b/app/config.py @@ -392,19 +392,19 @@ class Staging(Config): FROM_NUMBER = 'stage' API_RATE_LIMIT_ENABLED = True CHECK_PROXY_HEADER = True - REDIS_ENABLED = False + REDIS_ENABLED = True API_KEY_LIMITS = { KEY_TYPE_TEAM: { - "limit": 21000, + "limit": 24000, "interval": 60 }, KEY_TYPE_NORMAL: { - "limit": 21000, + "limit": 24000, "interval": 60 }, KEY_TYPE_TEST: { - "limit": 21000, + "limit": 24000, "interval": 60 } } diff --git a/manifest-api-staging.yml b/manifest-api-staging.yml index 05b1d420d..868d2853b 100644 --- a/manifest-api-staging.yml +++ b/manifest-api-staging.yml @@ -1,7 +1,6 @@ --- inherit: manifest-api-base.yml -command: scripts/run_app_paas.sh gunicorn -c /home/vcap/app/gunicorn_config.py --error-logfile /home/vcap/logs/gunicorn_error.log -w 6 -b 0.0.0.0:$PORT application services: - notify-aws - notify-config