From 5482ee4fe732971f55a2cc2c7a32d1d62c2f84c0 Mon Sep 17 00:00:00 2001 From: venusbb Date: Mon, 4 Dec 2017 14:48:23 +0000 Subject: [PATCH] - 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')