From 04da017558e7aaf2c47c3e46e9a7b8c8005b4477 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 26 Nov 2021 15:18:53 +0000 Subject: [PATCH] DRY-up conditionally creating callback tasks This removes 3 duplicate instances of the same code, which is still tested implicitly via test_process_ses_receipt_tasks [1]. In the next commit we'll make this test more explicit, to reflect that it's now being reused elsewhere and shouldn't change arbitrarily. We do lose the "print" statement from the command instance of the code, but I think that's a very tolerable loss. [1]: https://github.com/alphagov/notifications-api/blob/16ec8ccb8a6e4284eef7ec27c389419b761f39f0/tests/app/celery/test_process_ses_receipts_tasks.py#L94 --- app/celery/nightly_tasks.py | 17 +++-------- app/celery/process_ses_receipts_tasks.py | 4 +-- .../process_sms_client_response_tasks.py | 18 +++--------- app/commands.py | 28 +++---------------- .../notifications_ses_callback.py | 2 +- tests/app/celery/test_nightly_tasks.py | 23 ++------------- .../test_process_client_response.py | 26 +++-------------- tests/app/test_commands.py | 24 ++-------------- 8 files changed, 25 insertions(+), 117 deletions(-) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index 410116afc..92d401f46 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -10,10 +10,6 @@ from sqlalchemy.exc import SQLAlchemyError from app import notify_celery, zendesk_client from app.aws import s3 -from app.celery.service_callback_tasks import ( - create_delivery_status_callback_data, - send_delivery_status_to_service, -) from app.config import QueueNames from app.cronitor import cronitor from app.dao.fact_processing_time_dao import insert_update_processing_time @@ -27,9 +23,6 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, delete_notifications_older_than_retention_by_type, ) -from app.dao.service_callback_api_dao import ( - get_service_delivery_status_callback_api_for_service, -) from app.models import ( EMAIL_TYPE, KEY_TYPE_NORMAL, @@ -39,6 +32,9 @@ from app.models import ( FactProcessingTime, Notification, ) +from app.notifications.notifications_ses_callback import ( + check_and_queue_callback_task, +) from app.utils import get_london_midnight_in_utc @@ -126,12 +122,7 @@ def timeout_notifications(): dao_timeout_notifications(current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')) for notification in notifications: - # queue callback task only if the service_callback_api exists - service_callback_api = get_service_delivery_status_callback_api_for_service(service_id=notification.service_id) # noqa: E501 - if service_callback_api: - encrypted_notification = create_delivery_status_callback_data(notification, service_callback_api) - send_delivery_status_to_service.apply_async([str(notification.id), encrypted_notification], - queue=QueueNames.CALLBACKS) + check_and_queue_callback_task(notification) current_app.logger.info( "Timeout period reached for {} notifications, status has been updated.".format(len(notifications))) diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py index fef0fb1f7..04a8d14f1 100644 --- a/app/celery/process_ses_receipts_tasks.py +++ b/app/celery/process_ses_receipts_tasks.py @@ -11,8 +11,8 @@ from app.config import QueueNames from app.dao import notifications_dao from app.models import NOTIFICATION_PENDING, NOTIFICATION_SENDING from app.notifications.notifications_ses_callback import ( - _check_and_queue_callback_task, _check_and_queue_complaint_callback_task, + check_and_queue_callback_task, determine_notification_bounce_type, handle_complaint, ) @@ -76,7 +76,7 @@ def process_ses_results(self, response): notification.sent_at ) - _check_and_queue_callback_task(notification) + check_and_queue_callback_task(notification) return True diff --git a/app/celery/process_sms_client_response_tasks.py b/app/celery/process_sms_client_response_tasks.py index 6cc04d7fb..e16175646 100644 --- a/app/celery/process_sms_client_response_tasks.py +++ b/app/celery/process_sms_client_response_tasks.py @@ -5,20 +5,15 @@ from flask import current_app from notifications_utils.template import SMSMessageTemplate from app import notify_celery, statsd_client -from app.celery.service_callback_tasks import ( - create_delivery_status_callback_data, - send_delivery_status_to_service, -) from app.clients import ClientException from app.clients.sms.firetext import get_firetext_responses from app.clients.sms.mmg import get_mmg_responses -from app.config import QueueNames from app.dao import notifications_dao -from app.dao.service_callback_api_dao import ( - get_service_delivery_status_callback_api_for_service, -) from app.dao.templates_dao import dao_get_template_by_id from app.models import NOTIFICATION_PENDING +from app.notifications.notifications_ses_callback import ( + check_and_queue_callback_task, +) sms_response_mapper = { 'MMG': get_mmg_responses, @@ -94,9 +89,4 @@ def _process_for_status(notification_status, client_name, provider_reference, de notifications_dao.dao_update_notification(notification) if notification_status != NOTIFICATION_PENDING: - service_callback_api = get_service_delivery_status_callback_api_for_service(service_id=notification.service_id) - # queue callback task only if the service_callback_api exists - if service_callback_api: - encrypted_notification = create_delivery_status_callback_data(notification, service_callback_api) - send_delivery_status_to_service.apply_async([str(notification.id), encrypted_notification], - queue=QueueNames.CALLBACKS) + check_and_queue_callback_task(notification) diff --git a/app/commands.py b/app/commands.py index 2cf43f8f6..1b8230922 100644 --- a/app/commands.py +++ b/app/commands.py @@ -26,10 +26,6 @@ from app.celery.letters_pdf_tasks import ( from app.celery.reporting_tasks import ( create_nightly_notification_status_for_day, ) -from app.celery.service_callback_tasks import ( - create_delivery_status_callback_data, - send_delivery_status_to_service, -) from app.celery.tasks import process_row, record_daily_sorted_counts from app.config import QueueNames from app.dao.annual_billing_dao import ( @@ -52,9 +48,6 @@ from app.dao.permissions_dao import permission_dao from app.dao.provider_rates_dao import ( create_provider_rates as dao_create_provider_rates, ) -from app.dao.service_callback_api_dao import ( - get_service_delivery_status_callback_api_for_service, -) from app.dao.services_dao import ( dao_fetch_all_services_by_user, dao_fetch_all_services_created_by_user, @@ -85,6 +78,9 @@ from app.models import ( Service, User, ) +from app.notifications.notifications_ses_callback import ( + check_and_queue_callback_task, +) from app.utils import get_london_midnight_in_utc @@ -295,23 +291,7 @@ def replay_callbacks(file_name): for id in [id.strip() for id in file]: try: notification = Notification.query.filter_by(id=id).one() - - callback_api = get_service_delivery_status_callback_api_for_service( - service_id=notification.service_id - ) - - if not callback_api: - print(f"Callback api was not found for notification: {id}.") - continue - - encrypted_status_update = create_delivery_status_callback_data( - notification, callback_api - ) - - send_delivery_status_to_service.apply_async( - [id, encrypted_status_update], queue=QueueNames.CALLBACKS - ) - + check_and_queue_callback_task(notification) print(f"Created callback task for notification: {id}.") except NoResultFound: print(f"ID: {id} was not found in notifications.") diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 484d78f30..c1fdceaa0 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -68,7 +68,7 @@ def remove_emails_from_complaint(complaint_dict): return complaint_dict['mail'].pop('destination') -def _check_and_queue_callback_task(notification): +def check_and_queue_callback_task(notification): # queue callback task only if the service_callback_api exists service_callback_api = get_service_delivery_status_callback_api_for_service(service_id=notification.service_id) if service_callback_api: diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index 6bcb3a139..7a6167aa2 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -24,16 +24,11 @@ from app.celery.nightly_tasks import ( save_daily_notification_processing_time, timeout_notifications, ) -from app.celery.service_callback_tasks import ( - create_delivery_status_callback_data, -) -from app.config import QueueNames from app.models import EMAIL_TYPE, LETTER_TYPE, SMS_TYPE, FactProcessingTime from tests.app.db import ( create_job, create_notification, create_service, - create_service_callback_api, create_service_data_retention, create_template, ) @@ -166,8 +161,8 @@ def test_delete_letter_notifications_older_than_retention_calls_child_task(notif mocked.assert_called_once_with('letter') -def test_timeout_notifications_no_callbacks(mocker, sample_notification): - mock_update = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async') +def test_timeout_notifications(mocker, sample_notification): + mock_update = mocker.patch('app.celery.nightly_tasks.check_and_queue_callback_task') mock_dao = mocker.patch('app.celery.nightly_tasks.dao_timeout_notifications') mock_dao.return_value = [sample_notification] @@ -177,19 +172,7 @@ def test_timeout_notifications_no_callbacks(mocker, sample_notification): current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') ) - mock_update.assert_not_called() - - -def test_timeout_notifications_with_callbacks(mocker, sample_notification): - mock_update = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async') - mock_dao = mocker.patch('app.celery.nightly_tasks.dao_timeout_notifications') - mock_dao.return_value = [sample_notification] - - callback_api = create_service_callback_api(service=sample_notification.service) - timeout_notifications() - - encrypted_data = create_delivery_status_callback_data(sample_notification, callback_api) - mock_update.assert_called_once_with([str(sample_notification.id), encrypted_data], queue=QueueNames.CALLBACKS) + mock_update.assert_called_once_with(sample_notification) def test_delete_inbound_sms_calls_child_task(notify_api, mocker): diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index f31a9c9ed..6005fbce3 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -8,12 +8,8 @@ from app import statsd_client from app.celery.process_sms_client_response_tasks import ( process_sms_client_response, ) -from app.celery.service_callback_tasks import ( - create_delivery_status_callback_data, -) from app.clients import ClientException from app.models import NOTIFICATION_TECHNICAL_FAILURE -from tests.app.db import create_service_callback_api def test_process_sms_client_response_raises_error_if_reference_is_not_a_valid_uuid(client): @@ -121,12 +117,7 @@ def test_process_sms_client_response_updates_notification_status_when_detailed_s def test_sms_response_does_not_send_callback_if_notification_is_not_in_the_db(sample_service, mocker): - mocker.patch( - 'app.celery.process_sms_client_response_tasks.get_service_delivery_status_callback_api_for_service', - return_value='mock-delivery-callback-for-service') - send_mock = mocker.patch( - 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' - ) + send_mock = mocker.patch('app.celery.process_sms_client_response_tasks.check_and_queue_callback_task') reference = str(uuid.uuid4()) process_sms_client_response(status='3', provider_reference=reference, client_name='MMG') send_mock.assert_not_called() @@ -156,26 +147,17 @@ def test_process_sms_updates_billable_units_if_zero(sample_notification): def test_process_sms_response_does_not_send_service_callback_for_pending_notifications(sample_notification, mocker): - mocker.patch( - 'app.celery.process_sms_client_response_tasks.get_service_delivery_status_callback_api_for_service', - return_value='fake-callback') - send_mock = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async') + send_mock = mocker.patch('app.celery.process_sms_client_response_tasks.check_and_queue_callback_task') process_sms_client_response('2', str(sample_notification.id), 'Firetext') send_mock.assert_not_called() def test_outcome_statistics_called_for_successful_callback(sample_notification, mocker): - send_mock = mocker.patch( - 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' - ) - callback_api = create_service_callback_api(service=sample_notification.service, url="https://original_url.com") + send_mock = mocker.patch('app.celery.process_sms_client_response_tasks.check_and_queue_callback_task') reference = str(sample_notification.id) process_sms_client_response('3', reference, 'MMG') - - encrypted_data = create_delivery_status_callback_data(sample_notification, callback_api) - send_mock.assert_called_once_with([reference, encrypted_data], - queue="service-callbacks") + send_mock.assert_called_once_with(sample_notification) def test_process_sms_updates_sent_by_with_client_name_if_not_in_noti(sample_notification): diff --git a/tests/app/test_commands.py b/tests/app/test_commands.py index 98f092a89..aef033ca5 100644 --- a/tests/app/test_commands.py +++ b/tests/app/test_commands.py @@ -1,9 +1,8 @@ import uuid from app.commands import local_dev_broadcast_permissions, replay_callbacks -from app.config import QueueNames from app.dao.services_dao import dao_add_user_to_service -from tests.app.db import create_service_callback_api, create_user +from tests.app.db import create_user def test_local_dev_broadcast_permissions( @@ -33,10 +32,7 @@ def test_replay_callbacks( tmpdir, notify_api, ): - mock_apply = mocker.patch('app.commands.send_delivery_status_to_service.apply_async') - mock_update = mocker.patch('app.commands.create_delivery_status_callback_data') - mock_update.return_value = 'encrypted_status_update' - + mock_task = mocker.patch('app.commands.check_and_queue_callback_task') file_path = tmpdir + 'callback_ids.txt' missing_notification_id = uuid.uuid4() @@ -48,20 +44,6 @@ def test_replay_callbacks( replay_callbacks, ['-f', file_path] ) - mock_apply.assert_not_called() assert f'{missing_notification_id} was not found' in result.output - assert "Callback api was not found" in result.output - - # Now re-run with the callback API in place - create_service_callback_api(service=sample_service, bearer_token='foo') - - result = notify_api.test_cli_runner().invoke( - replay_callbacks, ['-f', file_path] - ) - - mock_apply.assert_called_once_with( - [str(sample_notification.id), 'encrypted_status_update'], - queue=QueueNames.CALLBACKS - ) - + mock_task.assert_called_once_with(sample_notification) assert result.exit_code == 0