From a0fe5c69715e6bc3380383b72fb60839a161ddee Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 09:31:03 +0000 Subject: [PATCH 01/27] created dao to get notifications by references --- app/dao/notifications_dao.py | 7 +++++++ .../dao/notification_dao/test_notification_dao.py | 14 +++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 2ea240f5b..fb6e66d5f 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -482,6 +482,13 @@ def dao_get_notifications_by_to_field(service_id, search_term, statuses=None): return results +@statsd(namespace="dao") +def dao_get_notifications_by_reference(references): + return Notification.query.filter( + Notification.reference.in_(references) + ).all() + + @statsd(namespace="dao") def dao_created_scheduled_notification(scheduled_notification): db.session.add(scheduled_notification) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 8d88ef2f9..2f6985fc9 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -53,7 +53,8 @@ from app.dao.notifications_dao import ( is_delivery_slow_for_provider, set_scheduled_notification_to_processed, update_notification_status_by_id, - update_notification_status_by_reference + update_notification_status_by_reference, + dao_get_notifications_by_reference ) from app.dao.services_dao import dao_update_service @@ -2111,6 +2112,17 @@ def test_dao_update_notifications_by_reference_returns_zero_when_no_notification assert updated_count == 0 +def test_dao_get_notifications_by_reference(sample_template): + notification_0 = create_notification(template=sample_template, reference='noref') + notification_1 = create_notification(template=sample_template, reference='ref') + notification_2 = create_notification(template=sample_template, reference='ref') + + noti = dao_get_notifications_by_reference(['ref']) + assert len(noti) == 2 + assert noti[0].id in [notification_1.id, notification_2.id] + assert noti[1].id in [notification_1.id, notification_2.id] + + def test_dao_create_notification_sms_sender_mapping(sample_notification): sms_sender = create_service_sms_sender(service=sample_notification.service, sms_sender='123456') dao_create_notification_sms_sender_mapping(notification_id=sample_notification.id, From 23a40b9fd0c4003103aeab2b8e7564aa11be3185 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 09:54:18 +0000 Subject: [PATCH 02/27] create method _post_status_update for the actual sending of callbacks --- app/celery/tasks.py | 68 +++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 30 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 250f786eb..a176eb9ab 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -499,36 +499,7 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): "date_received": inbound_sms.provider_date.strftime(DATETIME_FORMAT) } - try: - response = request( - method="POST", - url=inbound_api.url, - data=json.dumps(data), - headers={ - 'Content-Type': 'application/json', - 'Authorization': 'Bearer {}'.format(inbound_api.bearer_token) - }, - timeout=60 - ) - current_app.logger.info('send_inbound_sms_to_service sending {} to {}, response {}'.format( - inbound_sms_id, - inbound_api.url, - response.status_code - )) - response.raise_for_status() - except RequestException as e: - current_app.logger.warning( - "send_inbound_sms_to_service request failed for service_id: {} and url: {}. exc: {}".format( - service_id, - inbound_api.url, - e - ) - ) - if not isinstance(e, HTTPError) or e.response.status_code >= 500: - try: - self.retry(queue=QueueNames.RETRY) - except self.MaxRetriesExceededError: - current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') + _post_status_update(self, inbound_api, data, 'send_inbound_sms_to_service') @notify_celery.task(name='process-incomplete-jobs') @@ -566,3 +537,40 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template, resumed=True) + + +def _post_status_update(self, callback_api, data, callback_name): + + try: + response = request( + method="POST", + url=callback_api.url, + data=json.dumps(data), + headers={ + 'Content-Type': 'application/json', + 'Authorization': 'Bearer {}'.format(callback_api.bearer_token) + }, + timeout=60 + ) + current_app.logger.info('{} sending {} to {}, response {}'.format( + callback_name, + callback_api.service_id, + callback_api.url, + response.status_code + )) + response.raise_for_status() + except RequestException as e: + current_app.logger.warning( + "service_name request failed for service_id: {} and url: {}. exc: {}".format( + callback_api.service_id, + callback_api.url, + e + ) + ) + if not isinstance(e, HTTPError) or e.response.status_code >= 500: + try: + self.retry(queue=QueueNames.RETRY, + exc='Unable to {} for service_id: {} and url: {}. \n{}'.format( + callback_name, callback_api.service_id, callback_api.url, e)) + except self.MaxRetriesExceededError: + current_app.logger.exception('Retry: {} has retried the max number of times', callback_name) From 988b22391b8e661b4bb1492346bb8cb338e544c5 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 14:06:13 +0000 Subject: [PATCH 03/27] added send_delivery_status_to_service task worker --- app/celery/tasks.py | 26 +++ tests/app/celery/test_tasks.py | 172 +++++++++++++++++- .../notification_dao/test_notification_dao.py | 10 +- 3 files changed, 201 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a176eb9ab..ad8df005f 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -68,6 +68,7 @@ from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd from notifications_utils.s3 import s3upload +from app.dao.service_callback_api_dao import get_service_callback_api_for_service @worker_process_shutdown.connect @@ -502,6 +503,30 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): _post_status_update(self, inbound_api, data, 'send_inbound_sms_to_service') +@notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def send_delivery_status_to_service(self, notification_id): + # TODO: do we need to do rate limit this? + notification = get_notification_by_id(notification_id) + service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) + if not service_callback_api: + # No delivery receipt API info set + return + + data = { + "id": str(notification_id), + "reference": str(notification.client_reference), + "to": notification.to, + "status": notification.status, + "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request + "updated_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated + "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent + "notification_type": notification.notification_type + } + + _post_status_update(self, service_callback_api, data, 'send_delivery_receipt_to_service') + + @notify_celery.task(name='process-incomplete-jobs') @statsd(namespace="tasks") def process_incomplete_jobs(job_ids): @@ -552,6 +577,7 @@ def _post_status_update(self, callback_api, data, callback_name): }, timeout=60 ) + current_app.logger.info('{} sending {} to {}, response {}'.format( callback_name, callback_api.service_id, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 6d48acf56..149fd5fe5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -29,7 +29,9 @@ from app.celery.tasks import ( process_incomplete_jobs, get_template_class, s3, - send_inbound_sms_to_service) + send_inbound_sms_to_service, + send_delivery_status_to_service +) from app.config import QueueNames from app.dao import jobs_dao, services_dao from app.models import ( @@ -64,7 +66,8 @@ from tests.app.db import ( create_template, create_user, create_reply_to_email, - create_service_with_defined_sms_sender + create_service_with_defined_sms_sender, + create_service_callback_api ) @@ -1247,6 +1250,171 @@ def test_send_inbound_sms_to_service_does_not_retries_if_request_returns_404(not mocked.call_count == 0 +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_post_https_request_to_service(notify_db, + notify_db_session, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + + datestr = datetime(2017, 6, 20) + + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=200) + send_delivery_status_to_service(notification.id) + + mock_data = { + "id": str(notification.id), + "reference": str(notification.client_reference), + "to": notification.to, + "status": notification.status, + "created_at": datestr.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request + "updated_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated + "sent_at": datestr.strftime(DATETIME_FORMAT), # the time the email was sent + "notification_type": notification_type + } + + assert request_mock.call_count == 1 + assert request_mock.request_history[0].url == callback_api.url + assert request_mock.request_history[0].method == 'POST' + assert request_mock.request_history[0].text == json.dumps(mock_data) + assert request_mock.request_history[0].headers["Content-type"] == "application/json" + assert request_mock.request_history[0].headers["Authorization"] == "Bearer {}".format(callback_api.bearer_token) + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_does_not_sent_request_when_service_callback_api_does_not_exist( + notify_db, notify_db_session, mocker, notification_type): + service = create_sample_service(notify_db, notify_db_session, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + datestr = datetime(2017, 6, 20) + + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch("requests.request") + send_delivery_status_to_service(notification.id) + + mocked.call_count == 0 + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch('app.celery.tasks.send_delivery_status_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=500) + send_delivery_status_to_service(notification.id) + + assert mocked.call_count == 1 + assert mocked.call_args[1]['queue'] == 'retry-tasks' + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + + mocked = mocker.patch('app.celery.tasks.send_delivery_status_to_service.retry') + mocker.patch("app.celery.tasks.request", side_effect=RequestException()) + + send_delivery_status_to_service(notification.id) + + assert mocked.call_count == 1 + assert mocked.call_args[1]['queue'] == 'retry-tasks' + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch('app.celery.tasks.send_inbound_sms_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=404) + send_delivery_status_to_service(notification.id) + + mocked.call_count == 0 + + def test_check_job_status_task_does_not_raise_error(sample_template): create_job( template=sample_template, diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 2f6985fc9..a5afa1103 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -2113,14 +2113,14 @@ def test_dao_update_notifications_by_reference_returns_zero_when_no_notification def test_dao_get_notifications_by_reference(sample_template): - notification_0 = create_notification(template=sample_template, reference='noref') + create_notification(template=sample_template, reference='noref') notification_1 = create_notification(template=sample_template, reference='ref') notification_2 = create_notification(template=sample_template, reference='ref') - noti = dao_get_notifications_by_reference(['ref']) - assert len(noti) == 2 - assert noti[0].id in [notification_1.id, notification_2.id] - assert noti[1].id in [notification_1.id, notification_2.id] + notifications = dao_get_notifications_by_reference(['ref']) + assert len(notifications) == 2 + assert notifications[0].id in [notification_1.id, notification_2.id] + assert notifications[1].id in [notification_1.id, notification_2.id] def test_dao_create_notification_sms_sender_mapping(sample_notification): From 489f43a2c9a14cc84f83a93b5638630d4881c944 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 16:15:21 +0000 Subject: [PATCH 04/27] rename callback_tasks.py to process_ses_receipts.py create service_callback_tasks.py for tasks to send delivery statuses to services --- ...tasks.py => process_ses_receipts_tasks.py} | 0 app/celery/research_mode_tasks.py | 2 +- app/celery/service_callback_tasks.py | 71 +++++++ app/celery/tasks.py | 94 +++------ ....py => test_process_ses_receipts_tasks.py} | 6 +- .../app/celery/test_service_callback_tasks.py | 185 ++++++++++++++++++ tests/app/celery/test_tasks.py | 167 ---------------- 7 files changed, 290 insertions(+), 235 deletions(-) rename app/celery/{callback_tasks.py => process_ses_receipts_tasks.py} (100%) create mode 100644 app/celery/service_callback_tasks.py rename tests/app/celery/{test_callback_tasks.py => test_process_ses_receipts_tasks.py} (91%) create mode 100644 tests/app/celery/test_service_callback_tasks.py diff --git a/app/celery/callback_tasks.py b/app/celery/process_ses_receipts_tasks.py similarity index 100% rename from app/celery/callback_tasks.py rename to app/celery/process_ses_receipts_tasks.py diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index b37062826..509ee1681 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -5,7 +5,7 @@ from requests import request, RequestException, HTTPError from app.models import SMS_TYPE from app.config import QueueNames -from app.celery.callback_tasks import process_ses_results +from app.celery.process_ses_receipts_tasks import process_ses_results temp_fail = "7700900003" perm_fail = "7700900002" diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py new file mode 100644 index 000000000..3bc39a3fe --- /dev/null +++ b/app/celery/service_callback_tasks.py @@ -0,0 +1,71 @@ +import json +from app import ( + DATETIME_FORMAT, + notify_celery, +) +from app.dao.notifications_dao import ( + get_notification_by_id, +) + +from app.statsd_decorators import statsd +from app.dao.service_callback_api_dao import get_service_callback_api_for_service +from requests import ( + HTTPError, + request, + RequestException +) +from flask import current_app +from app.config import QueueNames + + +@notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def send_delivery_status_to_service(self, notification_id): + # TODO: do we need to do rate limit this? + notification = get_notification_by_id(notification_id) + service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) + if not service_callback_api: + # No delivery receipt API info set + return + + data = { + "id": str(notification_id), + "reference": str(notification.client_reference), + "to": notification.to, + "status": notification.status, + "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request + "updated_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated + "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent + "notification_type": notification.notification_type + } + + try: + response = request( + method="POST", + url=service_callback_api.url, + data=json.dumps(data), + headers={ + 'Content-Type': 'application/json', + 'Authorization': 'Bearer {}'.format(service_callback_api.bearer_token) + }, + timeout=60 + ) + current_app.logger.info('send_delivery_status_to_service sending {} to {}, response {}'.format( + notification_id, + service_callback_api.url, + response.status_code + )) + response.raise_for_status() + except RequestException as e: + current_app.logger.warning( + "send_inbound_sms_to_service request failed for service_id: {} and url: {}. exc: {}".format( + notification_id, + service_callback_api.url, + e + ) + ) + if not isinstance(e, HTTPError) or e.response.status_code >= 500: + try: + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') diff --git a/app/celery/tasks.py b/app/celery/tasks.py index ad8df005f..250f786eb 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -68,7 +68,6 @@ from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd from notifications_utils.s3 import s3upload -from app.dao.service_callback_api_dao import get_service_callback_api_for_service @worker_process_shutdown.connect @@ -500,31 +499,36 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): "date_received": inbound_sms.provider_date.strftime(DATETIME_FORMAT) } - _post_status_update(self, inbound_api, data, 'send_inbound_sms_to_service') - - -@notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300) -@statsd(namespace="tasks") -def send_delivery_status_to_service(self, notification_id): - # TODO: do we need to do rate limit this? - notification = get_notification_by_id(notification_id) - service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) - if not service_callback_api: - # No delivery receipt API info set - return - - data = { - "id": str(notification_id), - "reference": str(notification.client_reference), - "to": notification.to, - "status": notification.status, - "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request - "updated_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated - "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent - "notification_type": notification.notification_type - } - - _post_status_update(self, service_callback_api, data, 'send_delivery_receipt_to_service') + try: + response = request( + method="POST", + url=inbound_api.url, + data=json.dumps(data), + headers={ + 'Content-Type': 'application/json', + 'Authorization': 'Bearer {}'.format(inbound_api.bearer_token) + }, + timeout=60 + ) + current_app.logger.info('send_inbound_sms_to_service sending {} to {}, response {}'.format( + inbound_sms_id, + inbound_api.url, + response.status_code + )) + response.raise_for_status() + except RequestException as e: + current_app.logger.warning( + "send_inbound_sms_to_service request failed for service_id: {} and url: {}. exc: {}".format( + service_id, + inbound_api.url, + e + ) + ) + if not isinstance(e, HTTPError) or e.response.status_code >= 500: + try: + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') @notify_celery.task(name='process-incomplete-jobs') @@ -562,41 +566,3 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template, resumed=True) - - -def _post_status_update(self, callback_api, data, callback_name): - - try: - response = request( - method="POST", - url=callback_api.url, - data=json.dumps(data), - headers={ - 'Content-Type': 'application/json', - 'Authorization': 'Bearer {}'.format(callback_api.bearer_token) - }, - timeout=60 - ) - - current_app.logger.info('{} sending {} to {}, response {}'.format( - callback_name, - callback_api.service_id, - callback_api.url, - response.status_code - )) - response.raise_for_status() - except RequestException as e: - current_app.logger.warning( - "service_name request failed for service_id: {} and url: {}. exc: {}".format( - callback_api.service_id, - callback_api.url, - e - ) - ) - if not isinstance(e, HTTPError) or e.response.status_code >= 500: - try: - self.retry(queue=QueueNames.RETRY, - exc='Unable to {} for service_id: {} and url: {}. \n{}'.format( - callback_name, callback_api.service_id, callback_api.url, e)) - except self.MaxRetriesExceededError: - current_app.logger.exception('Retry: {} has retried the max number of times', callback_name) diff --git a/tests/app/celery/test_callback_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py similarity index 91% rename from tests/app/celery/test_callback_tasks.py rename to tests/app/celery/test_process_ses_receipts_tasks.py index 4b5ce4547..ddca32e0e 100644 --- a/tests/app/celery/test_callback_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -1,7 +1,7 @@ import json from datetime import datetime -from app.celery.callback_tasks import process_ses_results +from app.celery.process_ses_receipts_tasks import process_ses_results from tests.app.db import create_notification @@ -18,7 +18,7 @@ def test_process_ses_results(sample_email_template): def test_process_ses_results_does_not_retry_if_errors(notify_db, mocker): - mocked = mocker.patch('app.celery.callback_tasks.process_ses_results.retry') + mocked = mocker.patch('app.celery.process_ses_receipts_tasks.process_ses_results.retry') response = json.loads(ses_notification_callback()) process_ses_results(response=response) assert mocked.call_count == 0 @@ -26,7 +26,7 @@ def test_process_ses_results_does_not_retry_if_errors(notify_db, mocker): def test_process_ses_results_retry_called(notify_db, mocker): mocker.patch("app.dao.notifications_dao.update_notification_status_by_reference", side_effect=Exception("EXPECTED")) - mocked = mocker.patch('app.celery.callback_tasks.process_ses_results.retry') + mocked = mocker.patch('app.celery.process_ses_receipts_tasks.process_ses_results.retry') response = json.loads(ses_notification_callback()) process_ses_results(response=response) assert mocked.call_count != 0 diff --git a/tests/app/celery/test_service_callback_tasks.py b/tests/app/celery/test_service_callback_tasks.py new file mode 100644 index 000000000..f09333d2d --- /dev/null +++ b/tests/app/celery/test_service_callback_tasks.py @@ -0,0 +1,185 @@ +import json +from datetime import datetime + +import pytest +import requests_mock + +from requests import RequestException + +from app import (DATETIME_FORMAT) + +from tests.app.conftest import ( + sample_service as create_sample_service, + sample_template as create_sample_template, +) +from tests.app.db import ( + create_notification, + create_user, + create_service_callback_api +) +from app.celery.service_callback_tasks import send_delivery_status_to_service + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_post_https_request_to_service(notify_db, + notify_db_session, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + + datestr = datetime(2017, 6, 20) + + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=200) + send_delivery_status_to_service(notification.id) + + mock_data = { + "id": str(notification.id), + "reference": str(notification.client_reference), + "to": notification.to, + "status": notification.status, + "created_at": datestr.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request + "updated_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated + "sent_at": datestr.strftime(DATETIME_FORMAT), # the time the email was sent + "notification_type": notification_type + } + + assert request_mock.call_count == 1 + assert request_mock.request_history[0].url == callback_api.url + assert request_mock.request_history[0].method == 'POST' + assert request_mock.request_history[0].text == json.dumps(mock_data) + assert request_mock.request_history[0].headers["Content-type"] == "application/json" + assert request_mock.request_history[0].headers["Authorization"] == "Bearer {}".format(callback_api.bearer_token) + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_does_not_sent_request_when_service_callback_api_does_not_exist( + notify_db, notify_db_session, mocker, notification_type): + service = create_sample_service(notify_db, notify_db_session, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + datestr = datetime(2017, 6, 20) + + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch("requests.request") + send_delivery_status_to_service(notification.id) + + mocked.call_count == 0 + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=500) + send_delivery_status_to_service(notification.id) + + assert mocked.call_count == 1 + assert mocked.call_args[1]['queue'] == 'retry-tasks' + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + + mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry') + mocker.patch("app.celery.tasks.request", side_effect=RequestException()) + + send_delivery_status_to_service(notification.id) + + assert mocked.call_count == 1 + assert mocked.call_args[1]['queue'] == 'retry-tasks' + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404(notify_db, + notify_db_session, + mocker, + notification_type): + user = create_user() + service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) + + template = create_sample_template( + notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' + ) + callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", + bearer_token="something_unique") + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=404) + send_delivery_status_to_service(notification.id) + + mocked.call_count == 0 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 149fd5fe5..e33956a56 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -30,7 +30,6 @@ from app.celery.tasks import ( get_template_class, s3, send_inbound_sms_to_service, - send_delivery_status_to_service ) from app.config import QueueNames from app.dao import jobs_dao, services_dao @@ -67,7 +66,6 @@ from tests.app.db import ( create_user, create_reply_to_email, create_service_with_defined_sms_sender, - create_service_callback_api ) @@ -1250,171 +1248,6 @@ def test_send_inbound_sms_to_service_does_not_retries_if_request_returns_404(not mocked.call_count == 0 -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_post_https_request_to_service(notify_db, - notify_db_session, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - - datestr = datetime(2017, 6, 20) - - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - - with requests_mock.Mocker() as request_mock: - request_mock.post(callback_api.url, - json={}, - status_code=200) - send_delivery_status_to_service(notification.id) - - mock_data = { - "id": str(notification.id), - "reference": str(notification.client_reference), - "to": notification.to, - "status": notification.status, - "created_at": datestr.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request - "updated_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated - "sent_at": datestr.strftime(DATETIME_FORMAT), # the time the email was sent - "notification_type": notification_type - } - - assert request_mock.call_count == 1 - assert request_mock.request_history[0].url == callback_api.url - assert request_mock.request_history[0].method == 'POST' - assert request_mock.request_history[0].text == json.dumps(mock_data) - assert request_mock.request_history[0].headers["Content-type"] == "application/json" - assert request_mock.request_history[0].headers["Authorization"] == "Bearer {}".format(callback_api.bearer_token) - - -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_does_not_sent_request_when_service_callback_api_does_not_exist( - notify_db, notify_db_session, mocker, notification_type): - service = create_sample_service(notify_db, notify_db_session, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - datestr = datetime(2017, 6, 20) - - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - mocked = mocker.patch("requests.request") - send_delivery_status_to_service(notification.id) - - mocked.call_count == 0 - - -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_db, - notify_db_session, - mocker, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") - datestr = datetime(2017, 6, 20) - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - mocked = mocker.patch('app.celery.tasks.send_delivery_status_to_service.retry') - with requests_mock.Mocker() as request_mock: - request_mock.post(callback_api.url, - json={}, - status_code=500) - send_delivery_status_to_service(notification.id) - - assert mocked.call_count == 1 - assert mocked.call_args[1]['queue'] == 'retry-tasks' - - -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notify_db, - notify_db_session, - mocker, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") - datestr = datetime(2017, 6, 20) - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - - mocked = mocker.patch('app.celery.tasks.send_delivery_status_to_service.retry') - mocker.patch("app.celery.tasks.request", side_effect=RequestException()) - - send_delivery_status_to_service(notification.id) - - assert mocked.call_count == 1 - assert mocked.call_args[1]['queue'] == 'retry-tasks' - - -@pytest.mark.parametrize("notification_type", - ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404(notify_db, - notify_db_session, - mocker, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") - datestr = datetime(2017, 6, 20) - notification = create_notification(template=template, - created_at=datestr, - updated_at=datestr, - sent_at=datestr, - status='sent' - ) - mocked = mocker.patch('app.celery.tasks.send_inbound_sms_to_service.retry') - with requests_mock.Mocker() as request_mock: - request_mock.post(callback_api.url, - json={}, - status_code=404) - send_delivery_status_to_service(notification.id) - - mocked.call_count == 0 - - def test_check_job_status_task_does_not_raise_error(sample_template): create_job( template=sample_template, From a604fb82bb4b9aedd871fd63d665e110e0f8d7a5 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 20:30:53 +0000 Subject: [PATCH 05/27] added sms delivery status callback --- app/notifications/process_client_response.py | 5 +- .../app/notifications/rest/test_callbacks.py | 47 +++++++++++++++---- .../test_notifications_ses_callback.py | 4 +- .../test_process_client_response.py | 5 +- 4 files changed, 49 insertions(+), 12 deletions(-) diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index 534408ce6..129ef9ae2 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -8,7 +8,8 @@ from app.dao import notifications_dao from app.clients.sms.firetext import get_firetext_responses from app.clients.sms.mmg import get_mmg_responses from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks - +from app.celery.service_callback_tasks import send_delivery_status_to_service +from app.config import QueueNames sms_response_mapper = { 'MMG': get_mmg_responses, @@ -83,5 +84,7 @@ def process_sms_client_response(status, reference, client_name): create_outcome_notification_statistic_tasks(notification) + send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) + success = "{} callback succeeded. reference {} updated".format(client_name, reference) return success, errors diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 73d33c7a8..6202ad7e1 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -18,7 +18,7 @@ def firetext_post(client, data): data=data, headers=[ ('Content-Type', 'application/x-www-form-urlencoded'), - ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') + ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') # fake IPs ]) @@ -28,7 +28,7 @@ def mmg_post(client, data): data=data, headers=[ ('Content-Type', 'application/json'), - ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') + ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') # fake IPs ]) @@ -178,7 +178,9 @@ def test_firetext_callback_should_update_notification_status( notify_db, notify_db_session, client, sample_email_template, mocker ): mocker.patch('app.statsd_client.incr') - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, @@ -202,13 +204,16 @@ def test_firetext_callback_should_update_notification_status( updated = get_notification_by_id(notification.id) assert updated.status == 'delivered' assert get_notification_by_id(notification.id).status == 'delivered' + assert send_mock.called_once_with([notification.id], queue="notify-internal-tasks") def test_firetext_callback_should_update_notification_status_failed( notify_db, notify_db_session, client, sample_template, mocker ): mocker.patch('app.statsd_client.incr') - + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, @@ -235,6 +240,9 @@ def test_firetext_callback_should_update_notification_status_failed( def test_firetext_callback_should_update_notification_status_pending(client, notify_db, notify_db_session, mocker): mocker.patch('app.statsd_client.incr') + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -265,8 +273,11 @@ def test_process_mmg_response_return_200_when_cid_is_send_sms_code(client): def test_process_mmg_response_returns_200_when_cid_is_valid_notification_id( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -286,8 +297,11 @@ def test_process_mmg_response_returns_200_when_cid_is_valid_notification_id( def test_process_mmg_response_status_5_updates_notification_with_permanently_failed( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -306,8 +320,11 @@ def test_process_mmg_response_status_5_updates_notification_with_permanently_fai def test_process_mmg_response_status_2_updates_notification_with_permanently_failed( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -325,8 +342,11 @@ def test_process_mmg_response_status_2_updates_notification_with_permanently_fai def test_process_mmg_response_status_4_updates_notification_with_temporary_failed( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -345,8 +365,11 @@ def test_process_mmg_response_status_4_updates_notification_with_temporary_faile def test_process_mmg_response_unknown_status_updates_notification_with_failed( - notify_db, notify_db_session, client + notify_db, notify_db_session, client, mocker ): + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -392,6 +415,9 @@ def test_process_mmg_response_records_statsd(notify_db, notify_db_session, clien mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) @@ -415,6 +441,9 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') + mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() ) diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index 330466234..afcd8897b 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -24,7 +24,9 @@ def test_ses_callback_should_update_notification_status( stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index a4e3d2413..503d4b9eb 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -51,12 +51,15 @@ def test_outcome_statistics_called_for_successful_callback(sample_notification, 'app.notifications.process_client_response.notifications_dao.update_notification_status_by_id', return_value=sample_notification ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) reference = str(uuid.uuid4()) success, error = process_sms_client_response(status='3', reference=reference, client_name='MMG') assert success == "MMG callback succeeded. reference {} updated".format(str(reference)) assert error is None + assert send_mock.called stats_mock.assert_called_once_with(sample_notification) From 096657799c9351f5521d016f2b4576d63d2ccf8a Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 21:13:01 +0000 Subject: [PATCH 06/27] Added send delivery status to SES callbacks --- app/notifications/notifications_ses_callback.py | 4 +++- .../test_notifications_ses_callback.py | 16 +++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index b35704304..c04a27784 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -12,6 +12,8 @@ from app.dao import ( ) from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks from app.notifications.process_client_response import validate_callback_data +from app.celery.service_callback_tasks import send_delivery_status_to_service +from app.config import QueueNames def process_ses_response(ses_request): @@ -75,7 +77,7 @@ def process_ses_response(ses_request): ) create_outcome_notification_statistic_tasks(notification) - + send_delivery_status_to_service.apply_async([notification.id], queue=QueueNames.NOTIFY) return except KeyError: diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index afcd8897b..fb156dc07 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -46,6 +46,7 @@ def test_ses_callback_should_update_notification_status( ) statsd_client.incr.assert_any_call("callback.ses.delivered") stats_mock.assert_called_once_with(notification) + send_mock.assert_called_once_with([notification.id], queue="notify-internal-tasks") def test_ses_callback_should_update_multiple_notification_status_sent( @@ -58,7 +59,9 @@ def test_ses_callback_should_update_multiple_notification_status_sent( stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification1 = create_sample_notification( notify_db, notify_db_session, @@ -92,6 +95,7 @@ def test_ses_callback_should_update_multiple_notification_status_sent( call(notification2), call(notification3) ]) + assert send_mock.called def test_ses_callback_should_set_status_to_temporary_failure(client, @@ -103,7 +107,9 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, @@ -115,6 +121,7 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, assert get_notification_by_id(notification.id).status == 'sending' assert process_ses_response(ses_soft_bounce_callback(reference='ref')) is None assert get_notification_by_id(notification.id).status == 'temporary-failure' + assert send_mock.called stats_mock.assert_called_once_with(notification) @@ -148,7 +155,9 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - + send_mock = mocker.patch( + 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' + ) notification = create_sample_notification( notify_db, notify_db_session, @@ -161,6 +170,7 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, assert get_notification_by_id(notification.id).status == 'sending' assert process_ses_response(ses_hard_bounce_callback(reference='ref')) is None assert get_notification_by_id(notification.id).status == 'permanent-failure' + assert send_mock.called stats_mock.assert_called_once_with(notification) From f5a0ca918480dd8437ae82975fb10b52f7570ba1 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 1 Dec 2017 21:59:46 +0000 Subject: [PATCH 07/27] 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 47d18a8b820e87549abd800e415cb62d6cdfb756 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Sat, 2 Dec 2017 23:18:50 +0000 Subject: [PATCH 08/27] Update marshmallow from 2.14.0 to 2.15.0 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 4cdb13bdb..a14b97cee 100644 --- a/requirements.txt +++ b/requirements.txt @@ -12,7 +12,7 @@ gunicorn==19.7.1 iso8601==0.1.12 jsonschema==2.6.0 marshmallow-sqlalchemy==0.13.2 -marshmallow==2.14.0 +marshmallow==2.15.0 monotonic==1.4 psycopg2==2.7.3.2 PyJWT==1.5.3 From d62b350fcb932e29db6b086e43c938462477c9d0 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 4 Dec 2017 16:29:04 +0000 Subject: [PATCH 09/27] 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 10/27] 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 11/27] - 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 81ead8b246e13352ae10c784c03580ebe004e350 Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 5 Dec 2017 11:23:21 +0000 Subject: [PATCH 12/27] 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 13/27] 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 64f13a006412112830e1d7cc2ff71ea32cb4c1b6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 24 Nov 2017 16:25:57 +0000 Subject: [PATCH 14/27] Add a new organisation for letter branding --- .../versions/0150_another_letter_org.py | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 migrations/versions/0150_another_letter_org.py diff --git a/migrations/versions/0150_another_letter_org.py b/migrations/versions/0150_another_letter_org.py new file mode 100644 index 000000000..ffbb91d9a --- /dev/null +++ b/migrations/versions/0150_another_letter_org.py @@ -0,0 +1,38 @@ +"""empty message + +Revision ID: 0150_another_letter_org +Revises: 0149_add_crown_to_services +Create Date: 2017-06-29 12:44:16.815039 + +""" + +# revision identifiers, used by Alembic. +revision = '0150_another_letter_org' +down_revision = '0149_add_crown_to_services' + +from alembic import op + + +NEW_ORGANISATIONS = [ + ('006', 'DWP (Welsh)'), + ('007', 'Department for Communities'), + ('008', 'Marine Management Organisation'), +] + + +def upgrade(): + for numeric_id, name in NEW_ORGANISATIONS: + op.execute(""" + INSERT + INTO dvla_organisation + VALUES ('{}', '{}') + """.format(numeric_id, name)) + + +def downgrade(): + for numeric_id, _ in NEW_ORGANISATIONS: + op.execute(""" + DELETE + FROM dvla_organisation + WHERE id = '{}' + """.format(numeric_id)) From 20b306bfdbef942c213d81dfabef41c28988b2ea Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 5 Dec 2017 13:57:46 +0000 Subject: [PATCH 15/27] 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 16/27] 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 7fd0bf8bd87fa3aa5c96f2b65e87ac87bada995d Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 5 Dec 2017 23:18:56 +0000 Subject: [PATCH 17/27] Update requests-mock from 1.3.0 to 1.4.0 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 391a9b75b..dbab7d519 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -6,7 +6,7 @@ pytest-cov==2.5.1 pytest-xdist==1.20.1 coveralls==1.2.0 freezegun==0.3.9 -requests-mock==1.3.0 +requests-mock==1.4.0 # optional requirements for jsonschema strict-rfc3339==0.7 rfc3987==1.3.7 From 0d12b0666764ca37435a520300788544d61b9f52 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 5 Dec 2017 23:50:00 +0000 Subject: [PATCH 18/27] Update awscli from 1.14.2 to 1.14.4 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index d9699e50a..dab6dcc1f 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.2 +awscli==1.14.4 awscli-cwlogs>=1.4,<1.5 git+https://github.com/alphagov/notifications-utils.git@23.2.1#egg=notifications-utils==23.2.1 From b53fe651cbb74067c3c8900bdaacf24459769455 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Wed, 6 Dec 2017 01:21:54 +0000 Subject: [PATCH 19/27] Update pytest from 3.3.0 to 3.3.1 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 391a9b75b..12f29a0f0 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,6 +1,6 @@ -r requirements.txt flake8==3.5.0 -pytest==3.3.0 +pytest==3.3.1 pytest-mock==1.6.3 pytest-cov==2.5.1 pytest-xdist==1.20.1 From af1ab437be9398c13a2e371ea25ca1ad95e2bf06 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Wed, 6 Dec 2017 09:36:23 +0000 Subject: [PATCH 20/27] 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 From b0d4044ff542c76d231ca3a668fe06372dbe3905 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Dec 2017 16:31:21 +0000 Subject: [PATCH 21/27] remove free_sms_fragment_limit from service * remove from model * still required when calling POST /service - we just call through from dao_create_service to add a new annual billing entry. * removed from POST /service/ update_service - if you want to update/add a new one, use POST /service//free-sms-fragment-limit * made sure tests create services with default 250k limit. --- app/dao/annual_billing_dao.py | 4 +- app/dao/services_dao.py | 9 +-- app/models.py | 1 - app/schemas.py | 1 - app/service/rest.py | 18 ++--- tests/app/conftest.py | 12 ++- tests/app/db.py | 7 +- tests/app/service/test_rest.py | 131 ++++++++------------------------- 8 files changed, 58 insertions(+), 125 deletions(-) diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 76a4e2f46..c3522ecbd 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -61,12 +61,12 @@ def dao_get_all_free_sms_fragment_limit(service_id): ).order_by(AnnualBilling.financial_year_start).all() -def dao_insert_annual_billing(service): +def dao_insert_annual_billing_for_this_year(service, free_sms_fragment_limit): """ This method is called from create_service which is wrapped in a transaction. """ annual_billing = AnnualBilling( - free_sms_fragment_limit=service.free_sms_fragment_limit, + free_sms_fragment_limit=free_sms_fragment_limit, financial_year_start=get_current_financial_year_start_year(), service=service, ) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 0b5f3e245..ebf0153c3 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -41,7 +41,7 @@ from app.models import ( ) from app.statsd_decorators import statsd from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc -from app.dao.annual_billing_dao import dao_insert_annual_billing +from app.dao.annual_billing_dao import dao_insert_annual_billing_for_this_year DEFAULT_SERVICE_PERMISSIONS = [ SMS_TYPE, @@ -155,7 +155,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user, service_id=None, service_permissions=None): +def dao_create_service(service, user, free_sms_fragment_limit, service_id=None, service_permissions=None): # the default property does not appear to work when there is a difference between the sqlalchemy schema and the # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender # migration is completed, this code should be able to be removed. @@ -163,9 +163,6 @@ def dao_create_service(service, user, service_id=None, service_permissions=None) if service_permissions is None: service_permissions = DEFAULT_SERVICE_PERMISSIONS - if service.free_sms_fragment_limit is None: - service.free_sms_fragment_limit = current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] - from app.dao.permissions_dao import permission_dao service.users.append(user) permission_dao.add_default_service_permissions_for_user(user, service) @@ -180,7 +177,7 @@ def dao_create_service(service, user, service_id=None, service_permissions=None) # do we just add the default - or will we get a value from FE? insert_service_sms_sender(service, current_app.config['FROM_NUMBER']) - dao_insert_annual_billing(service) + dao_insert_annual_billing_for_this_year(service, free_sms_fragment_limit) db.session.add(service) diff --git a/app/models.py b/app/models.py index fdd00fa9a..498240b2c 100644 --- a/app/models.py +++ b/app/models.py @@ -228,7 +228,6 @@ class Service(db.Model, Versioned): created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) prefix_sms = db.Column(db.Boolean, nullable=True, default=True) organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) - free_sms_fragment_limit = db.Column(db.BigInteger, index=False, unique=False, nullable=True) organisation = db.relationship('Organisation') dvla_organisation_id = db.Column( db.String, diff --git a/app/schemas.py b/app/schemas.py index 419c9b438..12ffa4da1 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -203,7 +203,6 @@ class ServiceSchema(BaseSchema): organisation_type = field_for(models.Service, 'organisation_type') branding = field_for(models.Service, 'branding') dvla_organisation = field_for(models.Service, 'dvla_organisation') - free_sms_fragment_limit = field_for(models.Service, 'free_sms_fragment_limit') permissions = fields.Method("service_permissions") override_flag = False reply_to_email_address = fields.Method(method_name="get_reply_to_email_address") diff --git a/app/service/rest.py b/app/service/rest.py index c4d0e2bb9..f9ea00e38 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -87,7 +87,6 @@ from app.schemas import ( detailed_service_schema ) from app.utils import pagination_links -from app.billing.rest import update_free_sms_fragment_limit_data service_blueprint = Blueprint('service', __name__) @@ -154,19 +153,24 @@ def get_service_by_id(service_id): @service_blueprint.route('', methods=['POST']) def create_service(): data = request.get_json() - if not data.get('user_id', None): - errors = {'user_id': ['Missing data for required field.']} + errors = { + required_field: ['Missing data for required field.'] + for required_field in ['user_id', 'free_sms_fragment_limit'] + if not data.get(required_field, None) + } + if errors: raise InvalidRequest(errors, status_code=400) # validate json with marshmallow - service_schema.load(request.get_json()) + service_schema.load(data) user = get_user_by_id(data.pop('user_id', None)) + free_sms_fragment_limit = data.pop('free_sms_fragment_limit') # unpack valid json into service object valid_service = Service.from_json(data) - dao_create_service(valid_service, user) + dao_create_service(valid_service, user, free_sms_fragment_limit) return jsonify(data=service_schema.dump(valid_service).data), 201 @@ -185,10 +189,6 @@ def update_service(service_id): 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 - if 'free_sms_fragment_limit' in req_json: - update_free_sms_fragment_limit_data(fetched_service.id, req_json['free_sms_fragment_limit']) - if service_going_live: send_notification_to_service_users( service_id=service_id, diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 79a56ed10..6b678c16f 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -162,13 +162,12 @@ def sample_service( 'message_limit': limit, 'restricted': restricted, 'email_from': email_from, - 'created_by': user, - 'free_sms_fragment_limit': free_sms_fragment_limit + 'created_by': user } service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user, service_permissions=permissions) + dao_create_service(service, user, free_sms_fragment_limit, service_permissions=permissions) if research_mode: service.research_mode = research_mode @@ -1002,7 +1001,12 @@ def notify_service(notify_db, notify_db_session): created_by=user, prefix_sms=False, ) - dao_create_service(service=service, service_id=current_app.config['NOTIFY_SERVICE_ID'], user=user) + dao_create_service( + service=service, + service_id=current_app.config['NOTIFY_SERVICE_ID'], + user=user, + free_sms_fragment_limit=250000 # live central gov service + ) data = { 'service': service, diff --git a/tests/app/db.py b/tests/app/db.py index 717b1aa2b..16471d555 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -79,7 +79,12 @@ def create_service( prefix_sms=prefix_sms, ) - dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) + dao_create_service( + service, + service.created_by, + free_sms_fragment_limit=250000, + service_id=service_id, + service_permissions=service_permissions) service.active = active service.research_mode = research_mode diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index cc8f13eaa..90f82da5d 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -38,7 +38,6 @@ from tests.app.db import ( create_service_with_defined_sms_sender ) from tests.app.db import create_user -from app.dao.date_util import get_current_financial_year_start_year def test_get_service_list(client, service_factory): @@ -133,16 +132,8 @@ def test_get_service_by_id(admin_request, sample_service): assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] assert json_resp['data']['prefix_sms'] is True - - -def test_get_service_by_id_returns_free_sms_limit(admin_request, sample_service): - json_resp = admin_request.get('service.get_service_by_id', service_id=sample_service.id) - assert json_resp['data']['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] - - -def test_get_detailed_service_by_id_returns_free_sms_limit(admin_request, sample_service): - json_resp = admin_request.get('service.get_service_by_id', service_id=sample_service.id, detailed=True) - assert json_resp['data']['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] + # deprecated field, no longer exists + assert 'free_sms_fragment_limit' not in json_resp['data'] @pytest.mark.parametrize('detailed', [True, False]) @@ -228,7 +219,9 @@ def test_create_service(client, sample_user): 'restricted': False, 'active': False, 'email_from': 'created.service', - 'created_by': str(sample_user.id)} + 'created_by': str(sample_user.id), + 'free_sms_fragment_limit': 250000 + } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( @@ -243,8 +236,6 @@ def test_create_service(client, sample_user): assert not json_resp['data']['research_mode'] assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] - # TODO: Remove this after the new data is used - assert json_resp['data']['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] service_db = Service.query.get(json_resp['data']['id']) assert service_db.name == 'created service' @@ -274,7 +265,8 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u 'message_limit': 1000, 'restricted': False, 'active': False, - 'created_by': str(fake_uuid) + 'created_by': str(fake_uuid), + 'free_sms_fragment_limit': 250000 } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -288,7 +280,7 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u assert 'Missing data for required field.' in json_resp['message']['user_id'] -def test_create_service_free_sms_fragment_limit_is_optional(client, sample_user): +def test_create_service_free_sms_fragment_limit_is_not_optional(admin_request, sample_user): data1 = { 'name': 'service 1', 'user_id': str(sample_user.id), @@ -296,56 +288,16 @@ def test_create_service_free_sms_fragment_limit_is_optional(client, sample_user) 'restricted': False, 'active': False, 'email_from': 'sample_user.email1', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 9999 + 'created_by': str(sample_user.id) } - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.post( - '/service', - data=json.dumps(data1), - headers=headers) - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 201 - - # Test data from the new annual billing table - service_id = json_resp['data']['id'] - annual_billing = client.get('service/{}/billing/free-sms-fragment-limit?financial_year_start={}' - .format(service_id, get_current_financial_year_start_year()), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - json_resp = json.loads(annual_billing.get_data(as_text=True)) - assert json_resp['free_sms_fragment_limit'] == 9999 - # TODO: Remove this after the new data is used - assert json_resp['free_sms_fragment_limit'] == 9999 - - data2 = { - 'name': 'service 2', - 'user_id': str(sample_user.id), - 'message_limit': 1000, - 'restricted': False, - 'active': False, - 'email_from': 'sample_user.email2', - 'created_by': str(sample_user.id), - } - - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.post( - '/service', - data=json.dumps(data2), - headers=headers) - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 201 - # Test data from the new annual billing table - service_id = json_resp['data']['id'] - annual_billing = client.get('service/{}/billing/free-sms-fragment-limit?financial_year_start={}' - .format(service_id, get_current_financial_year_start_year()), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - json_resp = json.loads(annual_billing.get_data(as_text=True)) - assert json_resp['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] - # TODO: Remove this after the new data is used - assert json_resp['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] + json_resp = admin_request.post( + 'service.create_service', + _data=data1, + _expected_status=400 + ) + assert json_resp['result'] == 'error' + assert 'Missing data for required field.' in json_resp['message']['free_sms_fragment_limit'] def test_should_error_if_created_by_missing(notify_api, sample_user): @@ -357,7 +309,8 @@ def test_should_error_if_created_by_missing(notify_api, sample_user): 'message_limit': 1000, 'restricted': False, 'active': False, - 'user_id': str(sample_user.id) + 'user_id': str(sample_user.id), + 'free_sms_fragment_limit': 250000 } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -384,7 +337,8 @@ def test_should_not_create_service_with_missing_if_user_id_is_not_in_database(no 'message_limit': 1000, 'restricted': False, 'active': False, - 'created_by': str(fake_uuid) + 'created_by': str(fake_uuid), + 'free_sms_fragment_limit': 250000 } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -402,7 +356,8 @@ def test_should_not_create_service_if_missing_data(notify_api, sample_user): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { - 'user_id': str(sample_user.id) + 'user_id': str(sample_user.id), + 'free_sms_fragment_limit': 250000 } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -430,7 +385,9 @@ def test_should_not_create_service_with_duplicate_name(notify_api, 'restricted': False, 'active': False, 'email_from': 'sample.service2', - 'created_by': str(sample_user.id)} + 'created_by': str(sample_user.id), + 'free_sms_fragment_limit': 250000 + } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( @@ -456,7 +413,9 @@ def test_create_service_should_throw_duplicate_key_constraint_for_existing_email 'restricted': False, 'active': False, 'email_from': 'first.service', - 'created_by': str(sample_user.id)} + 'created_by': str(sample_user.id), + 'free_sms_fragment_limit': 250000 + } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( @@ -603,37 +562,6 @@ def test_update_service_flags_will_remove_service_permissions(client, notify_db, assert set([p.permission for p in permissions]) == set([SMS_TYPE, EMAIL_TYPE]) -# TODO: Remove after new table is created and verified -def test_update_service_free_sms_fragment_limit(client, notify_db, sample_service): - org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') - notify_db.session.add(org) - notify_db.session.commit() - - auth_header = create_authorization_header() - resp = client.get( - '/service/{}'.format(sample_service.id), - headers=[auth_header] - ) - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 200 - assert json_resp['data']['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] - - data = { - 'free_sms_fragment_limit': 9999 - } - - 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']['free_sms_fragment_limit'] == 9999 - - def test_update_permissions_will_override_permission_flags(client, service_with_no_permissions): auth_header = create_authorization_header() @@ -916,7 +844,8 @@ def test_default_permissions_are_added_for_user_service(notify_api, 'restricted': False, 'active': False, 'email_from': 'created.service', - 'created_by': str(sample_user.id)} + 'created_by': str(sample_user.id), + 'free_sms_fragment_limit': 250000} auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( From 664187f878267ca5e1097d0de8d501e502c59f9d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Dec 2017 16:51:09 +0000 Subject: [PATCH 22/27] update model to reflect service.prefix_sms not being nullable --- app/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 498240b2c..6eeda9a77 100644 --- a/app/models.py +++ b/app/models.py @@ -226,7 +226,7 @@ class Service(db.Model, Versioned): email_from = db.Column(db.Text, index=False, unique=True, nullable=False) created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) - prefix_sms = db.Column(db.Boolean, nullable=True, default=True) + prefix_sms = db.Column(db.Boolean, nullable=False, default=True) organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) organisation = db.relationship('Organisation') dvla_organisation_id = db.Column( From a4e2a40134c721f75a1de02b0228a19c3382d008 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Dec 2017 17:10:25 +0000 Subject: [PATCH 23/27] update dao tests to pass in free sms fragment limit, remove command --- app/commands.py | 26 --------------------- app/schemas.py | 1 - tests/app/dao/test_services_dao.py | 36 +++++++++++++++++------------- tests/app/service/test_rest.py | 5 +++-- 4 files changed, 23 insertions(+), 45 deletions(-) diff --git a/app/commands.py b/app/commands.py index 4b8f2b37f..ee92f4c2b 100644 --- a/app/commands.py +++ b/app/commands.py @@ -335,32 +335,6 @@ def populate_service_letter_contact(): print("Populated letter contacts for {} services".format(result.rowcount)) -@notify_command() -def populate_service_and_service_history_free_sms_fragment_limit(): - """ - DEPRECATED. Set services to have 250k sms limit. - """ - services_to_update = """ - UPDATE services - SET free_sms_fragment_limit = 250000 - WHERE free_sms_fragment_limit IS NULL - """ - - services_history_to_update = """ - UPDATE services_history - SET free_sms_fragment_limit = 250000 - WHERE free_sms_fragment_limit IS NULL - """ - - services_result = db.session.execute(services_to_update) - services_history_result = db.session.execute(services_history_to_update) - - db.session.commit() - - print("Populated free sms fragment limits for {} services".format(services_result.rowcount)) - print("Populated free sms fragment limits for {} services history".format(services_history_result.rowcount)) - - @notify_command() def populate_annual_billing(): """ diff --git a/app/schemas.py b/app/schemas.py index 12ffa4da1..2afa6270d 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -290,7 +290,6 @@ class DetailedServiceSchema(BaseSchema): 'letter_contact_block', # new exclude from here 'message_limit', 'email_from', - # 'free_sms_fragment_limit', 'inbound_api', 'dvla_organisation', 'whitelist', diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 2caf092fe..abeb06c32 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -51,6 +51,7 @@ from app.models import ( Service, ServicePermission, ServicePermissionTypes, + AnnualBilling, BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, @@ -89,10 +90,10 @@ def test_create_service(sample_user): restricted=False, organisation_type='central', created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert Service.query.count() == 1 - service_db = Service.query.first() + service_db = Service.query.one() assert service_db.name == "service_name" assert service_db.id == service.id assert service_db.branding == BRANDING_GOVUK @@ -105,6 +106,9 @@ def test_create_service(sample_user): assert service_db.free_sms_fragment_limit == 250000 assert service_db.organisation_type == 'central' assert service_db.crown is True + annual_billing = AnnualBilling.query.one() + assert annual_billing.service == service_db + assert annual_billing.free_sms_fragment_limit == 12345 def test_cannot_create_two_services_with_same_name(sample_user): @@ -121,8 +125,8 @@ def test_cannot_create_two_services_with_same_name(sample_user): restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service1, sample_user) - dao_create_service(service2, sample_user) + dao_create_service(service1, sample_user, 12345) + dao_create_service(service2, sample_user, 12345) assert 'duplicate key value violates unique constraint "services_name_key"' in str(excinfo.value) @@ -139,8 +143,8 @@ def test_cannot_create_two_services_with_same_email_from(sample_user): restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service1, sample_user) - dao_create_service(service2, sample_user) + dao_create_service(service1, sample_user, 12345) + dao_create_service(service2, sample_user, 12345) assert 'duplicate key value violates unique constraint "services_email_from_key"' in str(excinfo.value) @@ -152,7 +156,7 @@ def test_cannot_create_service_with_no_user(notify_db_session, sample_user): restricted=False, created_by=sample_user) with pytest.raises(FlushError) as excinfo: - dao_create_service(service, None) + dao_create_service(service, None, 12345) assert "Can't flush None value found in collection Service.users" in str(excinfo.value) @@ -162,7 +166,7 @@ def test_should_add_user_to_service(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert sample_user in Service.query.first().users new_user = User( name='Test User', @@ -181,7 +185,7 @@ def test_should_remove_user_from_service(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) new_user = User( name='Test User', email_address='new_user@digital.cabinet-office.gov.uk', @@ -333,7 +337,7 @@ def test_create_service_creates_a_history_record_with_current_data(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert Service.query.count() == 1 assert Service.get_history_model().query.count() == 1 @@ -360,7 +364,7 @@ def test_update_service_creates_a_history_record_with_current_data(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert Service.query.count() == 1 assert Service.query.first().version == 1 @@ -388,7 +392,7 @@ def test_update_service_permission_creates_a_history_record_with_current_data(sa message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) service.permissions.append(ServicePermission(service_id=service.id, permission='letter')) dao_update_service(service) @@ -431,7 +435,7 @@ def test_create_service_and_history_is_transactional(sample_user): created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert 'column "name" violates not-null constraint' in str(excinfo.value) assert Service.query.count() == 0 @@ -480,7 +484,7 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam restricted=False, created_by=sample_user) - dao_create_service(service_one, sample_user) + dao_create_service(service_one, sample_user, 12345) assert sample_user.id == service_one.users[0].id test_user_permissions = Permission.query.filter_by(service=service_one, user=sample_user).all() assert len(test_user_permissions) == 8 @@ -497,7 +501,7 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam message_limit=1000, restricted=False, created_by=other_user) - dao_create_service(service_two, other_user) + dao_create_service(service_two, other_user, 12345) assert other_user.id == service_two.users[0].id other_user_permissions = Permission.query.filter_by(service=service_two, user=other_user).all() @@ -526,7 +530,7 @@ def test_fetch_stats_filters_on_service(sample_notification): email_from="hello", restricted=False, message_limit=1000) - dao_create_service(service_two, sample_notification.service.created_by) + dao_create_service(service_two, sample_notification.service.created_by, 12345) stats = dao_fetch_stats_for_service(service_two.id) assert len(stats) == 0 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 90f82da5d..765e64e2f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1432,12 +1432,13 @@ def test_set_sms_prefixing_for_service_cant_be_none( admin_request, sample_service, ): - admin_request.post( + resp = admin_request.post( 'service.update_service', service_id=sample_service.id, _data={'prefix_sms': None}, - _expected_status=500, + _expected_status=400, ) + assert resp['message'] == {'prefix_sms': ['Field may not be null.']} @pytest.mark.parametrize('today_only,stats', [ From 8cd422ebeade247d2674486d0d9b9e88e603c01c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 4 Dec 2017 11:17:23 +0000 Subject: [PATCH 24/27] remove unnecessary None from .get --- app/service/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/service/rest.py b/app/service/rest.py index f9ea00e38..a5cd8742a 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -156,7 +156,7 @@ def create_service(): errors = { required_field: ['Missing data for required field.'] for required_field in ['user_id', 'free_sms_fragment_limit'] - if not data.get(required_field, None) + if not data.get(required_field) } if errors: raise InvalidRequest(errors, status_code=400) From 4cdcc4e0355d760a4d787b2eea3a0273e2fb14e6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 6 Dec 2017 11:01:18 +0000 Subject: [PATCH 25/27] no longer add an annual billing entry creating a service this is now handled by a separate call from the admin app --- app/dao/services_dao.py | 4 +-- app/service/rest.py | 14 ++++----- tests/app/conftest.py | 6 ++-- tests/app/dao/test_services_dao.py | 35 ++++++++++------------ tests/app/db.py | 7 +---- tests/app/service/test_rest.py | 47 ++++++------------------------ 6 files changed, 33 insertions(+), 80 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index ebf0153c3..6985a26e8 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -41,7 +41,6 @@ from app.models import ( ) from app.statsd_decorators import statsd from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc -from app.dao.annual_billing_dao import dao_insert_annual_billing_for_this_year DEFAULT_SERVICE_PERMISSIONS = [ SMS_TYPE, @@ -155,7 +154,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user, free_sms_fragment_limit, service_id=None, service_permissions=None): +def dao_create_service(service, user, service_id=None, service_permissions=None): # the default property does not appear to work when there is a difference between the sqlalchemy schema and the # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender # migration is completed, this code should be able to be removed. @@ -177,7 +176,6 @@ def dao_create_service(service, user, free_sms_fragment_limit, service_id=None, # do we just add the default - or will we get a value from FE? insert_service_sms_sender(service, current_app.config['FROM_NUMBER']) - dao_insert_annual_billing_for_this_year(service, free_sms_fragment_limit) db.session.add(service) diff --git a/app/service/rest.py b/app/service/rest.py index a5cd8742a..d71cd2b90 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -153,24 +153,20 @@ def get_service_by_id(service_id): @service_blueprint.route('', methods=['POST']) def create_service(): data = request.get_json() - errors = { - required_field: ['Missing data for required field.'] - for required_field in ['user_id', 'free_sms_fragment_limit'] - if not data.get(required_field) - } - if errors: + + if not data.get('user_id'): + errors = {'user_id': ['Missing data for required field.']} raise InvalidRequest(errors, status_code=400) # validate json with marshmallow service_schema.load(data) - user = get_user_by_id(data.pop('user_id', None)) - free_sms_fragment_limit = data.pop('free_sms_fragment_limit') + user = get_user_by_id(data.pop('user_id')) # unpack valid json into service object valid_service = Service.from_json(data) - dao_create_service(valid_service, user, free_sms_fragment_limit) + dao_create_service(valid_service, user) return jsonify(data=service_schema.dump(valid_service).data), 201 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 6b678c16f..71b510767 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -150,7 +150,6 @@ def sample_service( email_from=None, permissions=None, research_mode=None, - free_sms_fragment_limit=250000 ): if user is None: user = create_user() @@ -167,7 +166,7 @@ def sample_service( service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user, free_sms_fragment_limit, service_permissions=permissions) + dao_create_service(service, user, service_permissions=permissions) if research_mode: service.research_mode = research_mode @@ -1004,8 +1003,7 @@ def notify_service(notify_db, notify_db_session): dao_create_service( service=service, service_id=current_app.config['NOTIFY_SERVICE_ID'], - user=user, - free_sms_fragment_limit=250000 # live central gov service + user=user ) data = { diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index abeb06c32..32f16f27a 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -51,7 +51,6 @@ from app.models import ( Service, ServicePermission, ServicePermissionTypes, - AnnualBilling, BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, @@ -90,7 +89,7 @@ def test_create_service(sample_user): restricted=False, organisation_type='central', created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert Service.query.count() == 1 service_db = Service.query.one() @@ -103,12 +102,8 @@ 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.free_sms_fragment_limit == 250000 assert service_db.organisation_type == 'central' assert service_db.crown is True - annual_billing = AnnualBilling.query.one() - assert annual_billing.service == service_db - assert annual_billing.free_sms_fragment_limit == 12345 def test_cannot_create_two_services_with_same_name(sample_user): @@ -125,8 +120,8 @@ def test_cannot_create_two_services_with_same_name(sample_user): restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service1, sample_user, 12345) - dao_create_service(service2, sample_user, 12345) + dao_create_service(service1, sample_user) + dao_create_service(service2, sample_user) assert 'duplicate key value violates unique constraint "services_name_key"' in str(excinfo.value) @@ -143,8 +138,8 @@ def test_cannot_create_two_services_with_same_email_from(sample_user): restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service1, sample_user, 12345) - dao_create_service(service2, sample_user, 12345) + dao_create_service(service1, sample_user) + dao_create_service(service2, sample_user) assert 'duplicate key value violates unique constraint "services_email_from_key"' in str(excinfo.value) @@ -156,7 +151,7 @@ def test_cannot_create_service_with_no_user(notify_db_session, sample_user): restricted=False, created_by=sample_user) with pytest.raises(FlushError) as excinfo: - dao_create_service(service, None, 12345) + dao_create_service(service, None) assert "Can't flush None value found in collection Service.users" in str(excinfo.value) @@ -166,7 +161,7 @@ def test_should_add_user_to_service(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert sample_user in Service.query.first().users new_user = User( name='Test User', @@ -185,7 +180,7 @@ def test_should_remove_user_from_service(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) new_user = User( name='Test User', email_address='new_user@digital.cabinet-office.gov.uk', @@ -337,7 +332,7 @@ def test_create_service_creates_a_history_record_with_current_data(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert Service.query.count() == 1 assert Service.get_history_model().query.count() == 1 @@ -364,7 +359,7 @@ def test_update_service_creates_a_history_record_with_current_data(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert Service.query.count() == 1 assert Service.query.first().version == 1 @@ -392,7 +387,7 @@ def test_update_service_permission_creates_a_history_record_with_current_data(sa message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) service.permissions.append(ServicePermission(service_id=service.id, permission='letter')) dao_update_service(service) @@ -435,7 +430,7 @@ def test_create_service_and_history_is_transactional(sample_user): created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert 'column "name" violates not-null constraint' in str(excinfo.value) assert Service.query.count() == 0 @@ -484,7 +479,7 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam restricted=False, created_by=sample_user) - dao_create_service(service_one, sample_user, 12345) + dao_create_service(service_one, sample_user) assert sample_user.id == service_one.users[0].id test_user_permissions = Permission.query.filter_by(service=service_one, user=sample_user).all() assert len(test_user_permissions) == 8 @@ -501,7 +496,7 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam message_limit=1000, restricted=False, created_by=other_user) - dao_create_service(service_two, other_user, 12345) + dao_create_service(service_two, other_user) assert other_user.id == service_two.users[0].id other_user_permissions = Permission.query.filter_by(service=service_two, user=other_user).all() @@ -530,7 +525,7 @@ def test_fetch_stats_filters_on_service(sample_notification): email_from="hello", restricted=False, message_limit=1000) - dao_create_service(service_two, sample_notification.service.created_by, 12345) + dao_create_service(service_two, sample_notification.service.created_by) stats = dao_fetch_stats_for_service(service_two.id) assert len(stats) == 0 diff --git a/tests/app/db.py b/tests/app/db.py index 16471d555..717b1aa2b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -79,12 +79,7 @@ def create_service( prefix_sms=prefix_sms, ) - dao_create_service( - service, - service.created_by, - free_sms_fragment_limit=250000, - service_id=service_id, - service_permissions=service_permissions) + dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) service.active = active service.research_mode = research_mode diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 765e64e2f..707bffc40 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -132,8 +132,6 @@ def test_get_service_by_id(admin_request, sample_service): assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] assert json_resp['data']['prefix_sms'] is True - # deprecated field, no longer exists - assert 'free_sms_fragment_limit' not in json_resp['data'] @pytest.mark.parametrize('detailed', [True, False]) @@ -219,8 +217,7 @@ def test_create_service(client, sample_user): 'restricted': False, 'active': False, 'email_from': 'created.service', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'created_by': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -265,8 +262,7 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u 'message_limit': 1000, 'restricted': False, 'active': False, - 'created_by': str(fake_uuid), - 'free_sms_fragment_limit': 250000 + 'created_by': str(fake_uuid) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -280,26 +276,6 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u assert 'Missing data for required field.' in json_resp['message']['user_id'] -def test_create_service_free_sms_fragment_limit_is_not_optional(admin_request, sample_user): - data1 = { - 'name': 'service 1', - 'user_id': str(sample_user.id), - 'message_limit': 1000, - 'restricted': False, - 'active': False, - 'email_from': 'sample_user.email1', - 'created_by': str(sample_user.id) - } - - json_resp = admin_request.post( - 'service.create_service', - _data=data1, - _expected_status=400 - ) - assert json_resp['result'] == 'error' - assert 'Missing data for required field.' in json_resp['message']['free_sms_fragment_limit'] - - def test_should_error_if_created_by_missing(notify_api, sample_user): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -309,8 +285,7 @@ def test_should_error_if_created_by_missing(notify_api, sample_user): 'message_limit': 1000, 'restricted': False, 'active': False, - 'user_id': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'user_id': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -337,8 +312,7 @@ def test_should_not_create_service_with_missing_if_user_id_is_not_in_database(no 'message_limit': 1000, 'restricted': False, 'active': False, - 'created_by': str(fake_uuid), - 'free_sms_fragment_limit': 250000 + 'created_by': str(fake_uuid) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -356,8 +330,7 @@ def test_should_not_create_service_if_missing_data(notify_api, sample_user): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { - 'user_id': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'user_id': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -385,8 +358,7 @@ def test_should_not_create_service_with_duplicate_name(notify_api, 'restricted': False, 'active': False, 'email_from': 'sample.service2', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'created_by': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -413,8 +385,7 @@ def test_create_service_should_throw_duplicate_key_constraint_for_existing_email 'restricted': False, 'active': False, 'email_from': 'first.service', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'created_by': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -844,8 +815,8 @@ def test_default_permissions_are_added_for_user_service(notify_api, 'restricted': False, 'active': False, 'email_from': 'created.service', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 250000} + 'created_by': str(sample_user.id) + } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( From 78099de7763e1bd1b57825e150fa5fc924117e3d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 6 Dec 2017 11:03:42 +0000 Subject: [PATCH 26/27] make sure POST /free-sms-fragment-limit always creates refactored billing/rest.py and annual_billing_dao.py to remove logic from the dao, and simplify the process around creating new rows. Make sure that the POST always creates (it previously wouldn't create rows for years that don't already exist). Clean up some tests that were doing too much set-up/data verification via rest calls rather than directly inserting test data in to the DB. --- app/billing/rest.py | 24 +++-- app/dao/annual_billing_dao.py | 12 +-- tests/app/billing/test_billing.py | 117 +++++++++++------------ tests/app/dao/test_annual_billing_dao.py | 34 ++----- 4 files changed, 81 insertions(+), 106 deletions(-) diff --git a/app/billing/rest.py b/app/billing/rest.py index 1bdeaf6a9..73e742229 100644 --- a/app/billing/rest.py +++ b/app/billing/rest.py @@ -117,6 +117,7 @@ def get_free_sms_fragment_limit(service_id): financial_year_start = get_current_financial_year_start_year() if int(financial_year_start) < get_current_financial_year_start_year(): + # return the earliest historical entry annual_billing = sms_list[0] # The oldest entry else: annual_billing = sms_list[-1] # The newest entry @@ -141,10 +142,21 @@ def create_or_update_free_sms_fragment_limit(service_id): return jsonify(form), 201 -def update_free_sms_fragment_limit_data(service_id, free_sms_fragment_limit, financial_year_start=None): +def update_free_sms_fragment_limit_data(service_id, free_sms_fragment_limit, financial_year_start): current_year = get_current_financial_year_start_year() - if financial_year_start is None or financial_year_start >= current_year: - dao_update_annual_billing_for_current_and_future_years(service_id, free_sms_fragment_limit) - else: - dao_create_or_update_annual_billing_for_year(service_id, - free_sms_fragment_limit, financial_year_start) + if not financial_year_start: + financial_year_start = current_year + + dao_create_or_update_annual_billing_for_year( + service_id, + free_sms_fragment_limit, + financial_year_start + ) + # if we're trying to update historical data, don't touch other rows. + # Otherwise, make sure that future years will get the new updated value. + if financial_year_start >= current_year: + dao_update_annual_billing_for_current_and_future_years( + service_id, + free_sms_fragment_limit, + financial_year_start + ) diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index c3522ecbd..6e43be2d6 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -7,11 +7,7 @@ from app.dao.date_util import get_current_financial_year_start_year @transactional -def dao_create_or_update_annual_billing_for_year(service_id, free_sms_fragment_limit, financial_year_start=None): - - if not financial_year_start: - financial_year_start = get_current_financial_year_start_year() - +def dao_create_or_update_annual_billing_for_year(service_id, free_sms_fragment_limit, financial_year_start): result = dao_get_free_sms_fragment_limit_for_year(service_id, financial_year_start) if result: @@ -30,11 +26,7 @@ def dao_get_annual_billing(service_id): @transactional -def dao_update_annual_billing_for_current_and_future_years(service_id, free_sms_fragment_limit, - financial_year_start=None): - if not financial_year_start: - financial_year_start = get_current_financial_year_start_year() - +def dao_update_annual_billing_for_current_and_future_years(service_id, free_sms_fragment_limit, financial_year_start): AnnualBilling.query.filter( AnnualBilling.service_id == service_id, AnnualBilling.financial_year_start >= financial_year_start diff --git a/tests/app/billing/test_billing.py b/tests/app/billing/test_billing.py index 6a440aaa7..9714af153 100644 --- a/tests/app/billing/test_billing.py +++ b/tests/app/billing/test_billing.py @@ -1,25 +1,25 @@ from datetime import datetime, timedelta import json +import pytest + from app.billing.rest import _transform_billing_for_month from app.dao.monthly_billing_dao import ( create_or_update_monthly_billing, get_monthly_billing_by_notification_type, ) from app.models import SMS_TYPE, EMAIL_TYPE +from app.dao.date_util import get_current_financial_year_start_year +from app.dao.annual_billing_dao import dao_get_free_sms_fragment_limit_for_year from tests.app.db import ( create_notification, create_rate, - create_monthly_billing_entry + create_monthly_billing_entry, + create_annual_billing, ) - -from tests import create_authorization_header - -from app.dao.date_util import get_current_financial_year_start_year -from app.dao.annual_billing_dao import dao_get_free_sms_fragment_limit_for_year -from tests.app.db import create_annual_billing from app.billing.rest import update_free_sms_fragment_limit_data +from tests import create_authorization_header APR_2016_MONTH_START = datetime(2016, 3, 31, 23, 00, 00) APR_2016_MONTH_END = datetime(2016, 4, 30, 22, 59, 59, 99999) @@ -270,70 +270,62 @@ def test_create_update_free_sms_fragment_limit_invalid_schema(client, sample_ser assert 'JSON' in json_resp['message'] -def test_create_free_sms_fragment_limit_current_year(client, sample_service): +def test_create_free_sms_fragment_limit_current_year_updates_future_years(admin_request, sample_service): current_year = get_current_financial_year_start_year() - data = {'free_sms_fragment_limit': 9999} - response = client.post('service/{}/billing/free-sms-fragment-limit'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) + future_billing = create_annual_billing(sample_service.id, 1, current_year + 1) - response_get = client.get( - 'service/{}/billing/free-sms-fragment-limit?financial_year_start={}'.format(sample_service.id, current_year), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) + admin_request.post( + 'billing.create_or_update_free_sms_fragment_limit', + service_id=sample_service.id, + _data={'free_sms_fragment_limit': 9999}, + _expected_status=201 + ) - json_resp = json.loads(response_get.get_data(as_text=True)) - assert response.status_code == 201 - assert response_get.status_code == 200 - assert json_resp['financial_year_start'] == current_year - assert json_resp['free_sms_fragment_limit'] == 9999 + current_billing = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) + assert future_billing.free_sms_fragment_limit == 9999 + assert current_billing.financial_year_start == current_year + assert current_billing.free_sms_fragment_limit == 9999 -def test_create_free_sms_fragment_limit_past_year(client, sample_service): - - data = {'financial_year_start': 2016, 'free_sms_fragment_limit': 9999} - response = client.post('service/{}/billing/free-sms-fragment-limit'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - response_get = client.get( - 'service/{}/billing/free-sms-fragment-limit?financial_year_start=2016'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - json_resp = json.loads(response_get.get_data(as_text=True)) - assert response.status_code == 201 - assert response_get.status_code == 200 - assert json_resp['financial_year_start'] == 2016 - assert json_resp['free_sms_fragment_limit'] == 9999 - - -def test_update_free_sms_fragment_limit(client, sample_service): +@pytest.mark.parametrize('update_existing', [True, False]) +def test_create_or_update_free_sms_fragment_limit_past_year_doenst_update_other_years( + admin_request, + sample_service, + update_existing +): current_year = get_current_financial_year_start_year() + create_annual_billing(sample_service.id, 1, current_year) + if update_existing: + create_annual_billing(sample_service.id, 1, current_year - 1) - annual_billing = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) - assert annual_billing.free_sms_fragment_limit == 250000 + data = {'financial_year_start': current_year - 1, 'free_sms_fragment_limit': 9999} + admin_request.post( + 'billing.create_or_update_free_sms_fragment_limit', + service_id=sample_service.id, + _data=data, + _expected_status=201) - data_new = {'financial_year_start': current_year, 'free_sms_fragment_limit': 9999} - response = client.post('service/{}/billing/free-sms-fragment-limit'.format(sample_service.id), - data=json.dumps(data_new), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - response_get = client.get( - 'service/{}/billing/free-sms-fragment-limit?financial_year_start={}' - .format(sample_service.id, current_year), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - json_resp = json.loads(response_get.get_data(as_text=True)) - - assert response.status_code == 201 - assert response_get.status_code == 200 - assert json_resp['financial_year_start'] == current_year - assert json_resp['free_sms_fragment_limit'] == 9999 + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year - 1).free_sms_fragment_limit == 9999 + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year).free_sms_fragment_limit == 1 -def test_get_free_sms_fragment_limit_current_year(client, sample_service): +def test_create_free_sms_fragment_limit_updates_existing_year(admin_request, sample_service): + current_year = get_current_financial_year_start_year() + annual_billing = create_annual_billing(sample_service.id, 1, current_year) + + admin_request.post( + 'billing.create_or_update_free_sms_fragment_limit', + service_id=sample_service.id, + _data={'financial_year_start': current_year, 'free_sms_fragment_limit': 2}, + _expected_status=201) + + assert annual_billing.free_sms_fragment_limit == 2 + + +def test_get_free_sms_fragment_limit_current_year_creates_new_row(client, sample_service): current_year = get_current_financial_year_start_year() - create_annual_billing(sample_service.id, free_sms_fragment_limit=9999, financial_year_start=current_year - 1) + create_annual_billing(sample_service.id, 9999, current_year - 1) response_get = client.get( 'service/{}/billing/free-sms-fragment-limit'.format(sample_service.id), @@ -342,7 +334,7 @@ def test_get_free_sms_fragment_limit_current_year(client, sample_service): json_resp = json.loads(response_get.get_data(as_text=True)) assert response_get.status_code == 200 assert json_resp['financial_year_start'] == get_current_financial_year_start_year() - assert json_resp['free_sms_fragment_limit'] == 250000 + assert json_resp['free_sms_fragment_limit'] == 9999 def test_get_free_sms_fragment_limit_past_year_not_exist(client, sample_service): @@ -385,10 +377,9 @@ def test_get_free_sms_fragment_limit_future_year_not_exist(client, sample_servic def test_update_free_sms_fragment_limit_data(client, sample_service): current_year = get_current_financial_year_start_year() - annual_billing = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) - assert annual_billing.free_sms_fragment_limit == 250000 + create_annual_billing(sample_service.id, free_sms_fragment_limit=250000, financial_year_start=current_year - 1) - update_free_sms_fragment_limit_data(sample_service.id, 9999) + update_free_sms_fragment_limit_data(sample_service.id, 9999, current_year) annual_billing = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) assert annual_billing.free_sms_fragment_limit == 9999 diff --git a/tests/app/dao/test_annual_billing_dao.py b/tests/app/dao/test_annual_billing_dao.py index 3e475ef5c..559e1c8f4 100644 --- a/tests/app/dao/test_annual_billing_dao.py +++ b/tests/app/dao/test_annual_billing_dao.py @@ -8,16 +8,6 @@ from app.dao.annual_billing_dao import ( from tests.app.db import create_annual_billing -def test_get_sample_service_has_default_free_sms_fragment_limit(notify_db_session, sample_service): - - # when sample_service was created, it automatically create an entry in the annual_billing table - free_limit = dao_get_free_sms_fragment_limit_for_year(sample_service.id, get_current_financial_year_start_year()) - - assert free_limit.free_sms_fragment_limit == 250000 - assert free_limit.financial_year_start == get_current_financial_year_start_year() - assert free_limit.service_id == sample_service.id - - def test_dao_update_free_sms_fragment_limit(notify_db_session, sample_service): new_limit = 9999 year = get_current_financial_year_start_year() @@ -27,16 +17,7 @@ def test_dao_update_free_sms_fragment_limit(notify_db_session, sample_service): assert new_free_limit.free_sms_fragment_limit == new_limit -def test_create_annual_billing_not_specify_year(notify_db_session, sample_service): - - dao_create_or_update_annual_billing_for_year(sample_service.id, 9999) - - free_limit = dao_get_free_sms_fragment_limit_for_year(sample_service.id) - - assert free_limit.free_sms_fragment_limit == 9999 - - -def test_create_annual_billing_specify_year(notify_db_session, sample_service): +def test_create_annual_billing(sample_service): dao_create_or_update_annual_billing_for_year(sample_service.id, 9999, 2016) @@ -47,16 +28,15 @@ def test_create_annual_billing_specify_year(notify_db_session, sample_service): def test_dao_update_annual_billing_for_current_and_future_years(notify_db_session, sample_service): current_year = get_current_financial_year_start_year() - limits = [240000, 250000, 260000, 270000] + limits = [1, 2, 3, 4] create_annual_billing(sample_service.id, limits[0], current_year - 1) create_annual_billing(sample_service.id, limits[2], current_year + 1) create_annual_billing(sample_service.id, limits[3], current_year + 2) dao_update_annual_billing_for_current_and_future_years(sample_service.id, 9999, current_year) - free_limit = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year - 1) - assert free_limit.free_sms_fragment_limit == 240000 - - for year in range(current_year, current_year + 3): - free_limit = dao_get_free_sms_fragment_limit_for_year(sample_service.id, year) - assert free_limit.free_sms_fragment_limit == 9999 + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year - 1).free_sms_fragment_limit == 1 + # current year is not created + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) is None + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year + 1).free_sms_fragment_limit == 9999 + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year + 2).free_sms_fragment_limit == 9999 From f29e08c7781be702abc6946421e6e55e6fcd9fed Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 6 Dec 2017 14:41:32 +0000 Subject: [PATCH 27/27] don't update current year twice --- app/billing/rest.py | 4 ++-- app/dao/annual_billing_dao.py | 4 ++-- tests/app/dao/test_annual_billing_dao.py | 7 ++++--- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/billing/rest.py b/app/billing/rest.py index 73e742229..66726bb5a 100644 --- a/app/billing/rest.py +++ b/app/billing/rest.py @@ -14,7 +14,7 @@ from app.utils import convert_utc_to_bst from app.dao.annual_billing_dao import (dao_get_free_sms_fragment_limit_for_year, dao_get_all_free_sms_fragment_limit, dao_create_or_update_annual_billing_for_year, - dao_update_annual_billing_for_current_and_future_years) + dao_update_annual_billing_for_future_years) from app.billing.billing_schemas import create_or_update_free_sms_fragment_limit_schema from app.errors import InvalidRequest from app.schema_validation import validate @@ -155,7 +155,7 @@ def update_free_sms_fragment_limit_data(service_id, free_sms_fragment_limit, fin # if we're trying to update historical data, don't touch other rows. # Otherwise, make sure that future years will get the new updated value. if financial_year_start >= current_year: - dao_update_annual_billing_for_current_and_future_years( + dao_update_annual_billing_for_future_years( service_id, free_sms_fragment_limit, financial_year_start diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 6e43be2d6..744fc6106 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -26,10 +26,10 @@ def dao_get_annual_billing(service_id): @transactional -def dao_update_annual_billing_for_current_and_future_years(service_id, free_sms_fragment_limit, financial_year_start): +def dao_update_annual_billing_for_future_years(service_id, free_sms_fragment_limit, financial_year_start): AnnualBilling.query.filter( AnnualBilling.service_id == service_id, - AnnualBilling.financial_year_start >= financial_year_start + AnnualBilling.financial_year_start > financial_year_start ).update( {'free_sms_fragment_limit': free_sms_fragment_limit} ) diff --git a/tests/app/dao/test_annual_billing_dao.py b/tests/app/dao/test_annual_billing_dao.py index 559e1c8f4..ac0c4df2f 100644 --- a/tests/app/dao/test_annual_billing_dao.py +++ b/tests/app/dao/test_annual_billing_dao.py @@ -1,9 +1,10 @@ + from app.dao.date_util import get_current_financial_year_start_year from app.dao.annual_billing_dao import ( dao_create_or_update_annual_billing_for_year, dao_get_free_sms_fragment_limit_for_year, - dao_update_annual_billing_for_current_and_future_years, + dao_update_annual_billing_for_future_years, ) from tests.app.db import create_annual_billing @@ -26,14 +27,14 @@ def test_create_annual_billing(sample_service): assert free_limit.free_sms_fragment_limit == 9999 -def test_dao_update_annual_billing_for_current_and_future_years(notify_db_session, sample_service): +def test_dao_update_annual_billing_for_future_years(notify_db_session, sample_service): current_year = get_current_financial_year_start_year() limits = [1, 2, 3, 4] create_annual_billing(sample_service.id, limits[0], current_year - 1) create_annual_billing(sample_service.id, limits[2], current_year + 1) create_annual_billing(sample_service.id, limits[3], current_year + 2) - dao_update_annual_billing_for_current_and_future_years(sample_service.id, 9999, current_year) + dao_update_annual_billing_for_future_years(sample_service.id, 9999, current_year) assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year - 1).free_sms_fragment_limit == 1 # current year is not created