From 2acc4ee67d2e3335bce89d0fa71e5ab85658ef23 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 25 Nov 2021 17:02:57 +0000 Subject: [PATCH] Repurpose command to replay notification callbacks This is so we can use it to address issues highlighted by the new alert, if it's not possible to actually send the notifications e.g. if they are somehow 'invalid'. Previously this was added for a one-off use case [1]. This rewrites the task to operate on arbitrary notification IDs instead of client refs, which aren't always present for notifications we may want to send / replay callbacks for. Since the task may now need to work on notifications more than one service, I had to restructure it to cope with multiple callback APIs. Note that, in the test, I've chosen to do a chain of invocations and assertions, rather than duplicate a load of boilerplate or introduce funky parametrize flags for a service with/out a callback API. We'll refactor this in a later commit. [1]: https://github.com/alphagov/notifications-api/commit/e95740a6b517b4da7141087c19dbeb26e1c38df7 --- app/commands.py | 57 +++++++++++++++++--------------------- tests/app/test_commands.py | 48 ++++++++++++++++++++++++++++++-- 2 files changed, 72 insertions(+), 33 deletions(-) diff --git a/app/commands.py b/app/commands.py index 4edbce9d1..2cf43f8f6 100644 --- a/app/commands.py +++ b/app/commands.py @@ -284,42 +284,37 @@ def recreate_pdf_for_precompiled_or_uploaded_letter(notification_id): resanitise_pdf.apply_async([str(notification_id)], queue=QueueNames.LETTERS) -@notify_command(name='replay-service-callbacks') -@click.option('-f', '--file_name', required=True, - help="""Full path of the file to upload, file is a contains client references of +@notify_command(name='replay-callbacks') +@click.option('-f', '--file-name', required=True, + help="""Full path of the file to upload, containing IDs of notifications that need the status to be sent to the service.""") -@click.option('-s', '--service_id', required=True, - help="""The service that the callbacks are for""") -def replay_service_callbacks(file_name, service_id): - print("Start send service callbacks for service: ", service_id) - callback_api = get_service_delivery_status_callback_api_for_service(service_id=service_id) - if not callback_api: - print("Callback api was not found for service: {}".format(service_id)) - return - - errors = [] - notifications = [] +def replay_callbacks(file_name): + print("Replaying callbacks.") file = open(file_name) - for ref in file: + for id in [id.strip() for id in file]: try: - notification = Notification.query.filter_by(client_reference=ref.strip()).one() - notifications.append(notification) + 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 + ) + + print(f"Created callback task for notification: {id}.") except NoResultFound: - errors.append("Reference: {} was not found in notifications.".format(ref)) - - for e in errors: - print(e) - if errors: - raise Exception("Some notifications for the given references were not found") - - for n in notifications: - encrypted_status_update = create_delivery_status_callback_data(n, callback_api) - send_delivery_status_to_service.apply_async([str(n.id), encrypted_status_update], - queue=QueueNames.CALLBACKS) - - print("Replay service status for service: {}. Sent {} notification status updates to the queue".format( - service_id, len(notifications))) + print(f"ID: {id} was not found in notifications.") def setup_commands(application): diff --git a/tests/app/test_commands.py b/tests/app/test_commands.py index dd8f1c15b..98f092a89 100644 --- a/tests/app/test_commands.py +++ b/tests/app/test_commands.py @@ -1,6 +1,9 @@ -from app.commands import local_dev_broadcast_permissions +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_user +from tests.app.db import create_service_callback_api, create_user def test_local_dev_broadcast_permissions( @@ -21,3 +24,44 @@ def test_local_dev_broadcast_permissions( assert len(user.get_permissions(sample_service.id)) == 0 assert len(user.get_permissions(sample_broadcast_service.id)) > 0 + + +def test_replay_callbacks( + mocker, + sample_service, + sample_notification, + 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' + + file_path = tmpdir + 'callback_ids.txt' + missing_notification_id = uuid.uuid4() + + with open(file_path, 'w') as f: + f.write(str(sample_notification.id) + "\n") + f.write(str(missing_notification_id) + "\n") + + result = notify_api.test_cli_runner().invoke( + 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 + ) + + assert result.exit_code == 0