From 96a0935d1c5da01584468c4f6e69449eb4fdc644 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 1 Feb 2021 16:37:22 +0000 Subject: [PATCH] update broadcast provider message status on success/error so we can distinguish errorring messages that are currently retrying from those that sent succesfully. --- app/celery/broadcast_message_tasks.py | 16 +++++++-- app/dao/broadcast_message_dao.py | 5 +++ .../celery/test_broadcast_message_tasks.py | 33 ++++++++++++------- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 7abbedddd..e60607f4c 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -9,8 +9,12 @@ from celery.exceptions import MaxRetriesExceededError from app import cbc_proxy_client, db, notify_celery from app.clients.cbc_proxy import CBCProxyFatalException, CBCProxyRetryableException from app.config import QueueNames -from app.models import BroadcastEventMessageType, BroadcastProvider -from app.dao.broadcast_message_dao import dao_get_broadcast_event_by_id, create_broadcast_provider_message +from app.models import BroadcastEventMessageType, BroadcastProvider, BroadcastProviderMessageStatus +from app.dao.broadcast_message_dao import ( + dao_get_broadcast_event_by_id, + create_broadcast_provider_message, + update_broadcast_provider_message_status +) from app.utils import format_sequential_number @@ -138,12 +142,20 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): # (because the message has expired) check_provider_message_should_retry(broadcast_provider_message) + # TODO: Decide whether to set to TECHNICAL_FAILURE or ERROR based on response codes from cbc proxy + update_broadcast_provider_message_status( + broadcast_provider_message, + status=BroadcastProviderMessageStatus.TECHNICAL_FAILURE + ) + self.retry( exc=exc, countdown=get_retry_delay(self.request.retries), queue=QueueNames.BROADCASTS, ) + update_broadcast_provider_message_status(broadcast_provider_message, status=BroadcastProviderMessageStatus.ACK) + @notify_celery.task(name='trigger-link-test') def trigger_link_test(provider): diff --git a/app/dao/broadcast_message_dao.py b/app/dao/broadcast_message_dao.py index f7016fdcb..746bb175c 100644 --- a/app/dao/broadcast_message_dao.py +++ b/app/dao/broadcast_message_dao.py @@ -61,3 +61,8 @@ def create_broadcast_provider_message(broadcast_event, provider): db.session.add(provider_message_number) db.session.commit() return provider_message + + +@transactional +def update_broadcast_provider_message_status(broadcast_provider_message, status): + broadcast_provider_message.status = status diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index dc3720e21..d09027ffc 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -3,7 +3,7 @@ from datetime import datetime from unittest.mock import call, ANY from freezegun import freeze_time -from celery.exceptions import MaxRetriesExceededError +from celery.exceptions import MaxRetriesExceededError, Retry import pytest from app.models import ( @@ -147,7 +147,7 @@ def test_send_broadcast_provider_message_sends_data_correctly( send_broadcast_provider_message(provider=provider, broadcast_event_id=str(event.id)) broadcast_provider_message = event.get_provider_message(provider) - assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING + assert broadcast_provider_message.status == BroadcastProviderMessageStatus.ACK mock_create_broadcast.assert_called_once_with( identifier=str(broadcast_provider_message.id), @@ -280,9 +280,9 @@ def test_send_broadcast_provider_message_works_if_we_retried_previously(mocker, assert len(event.provider_messages) == 1 broadcast_provider_message = event.get_provider_message('ee') - # TODO: Should be ACK, and should have an updated_at - assert broadcast_provider_message.status == BroadcastProviderMessageStatus.TECHNICAL_FAILURE - assert broadcast_provider_message.updated_at is None + + assert broadcast_provider_message.status == BroadcastProviderMessageStatus.ACK + assert broadcast_provider_message.updated_at is not None mock_create_broadcast.assert_called_once_with( identifier=str(broadcast_provider_message.id), @@ -374,7 +374,7 @@ def test_send_broadcast_provider_message_sends_update_with_references( send_broadcast_provider_message(provider=provider, broadcast_event_id=str(update_event.id)) broadcast_provider_message = update_event.get_provider_message(provider) - assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING + assert broadcast_provider_message.status == BroadcastProviderMessageStatus.ACK mock_update_broadcast.assert_called_once_with( identifier=str(broadcast_provider_message.id), @@ -429,7 +429,7 @@ def test_send_broadcast_provider_message_sends_cancel_with_references( send_broadcast_provider_message(provider=provider, broadcast_event_id=str(cancel_event.id)) broadcast_provider_message = cancel_event.get_provider_message(provider) - assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING + assert broadcast_provider_message.status == BroadcastProviderMessageStatus.ACK mock_cancel_broadcast.assert_called_once_with( identifier=str(broadcast_provider_message.id), @@ -468,9 +468,13 @@ def test_send_broadcast_provider_message_errors(mocker, sample_service, provider f'app.clients.cbc_proxy.CBCProxy{provider_capitalised}.create_and_send_broadcast', side_effect=CBCProxyRetryableException('oh no'), ) - mock_retry = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_provider_message.retry') + mock_retry = mocker.patch( + 'app.celery.broadcast_message_tasks.send_broadcast_provider_message.retry', + side_effect=Retry + ) - send_broadcast_provider_message(provider=provider, broadcast_event_id=str(event.id)) + with pytest.raises(Retry): + send_broadcast_provider_message(provider=provider, broadcast_event_id=str(event.id)) mock_create_broadcast.assert_called_once_with( identifier=ANY, @@ -493,6 +497,9 @@ def test_send_broadcast_provider_message_errors(mocker, sample_service, provider exc=mock_create_broadcast.side_effect, queue='broadcast-tasks' ) + broadcast_provider_message = event.get_provider_message(provider) + assert broadcast_provider_message.status == BroadcastProviderMessageStatus.TECHNICAL_FAILURE + @pytest.mark.parametrize('num_retries, expected_countdown', [ @@ -515,13 +522,17 @@ def test_send_broadcast_provider_message_delays_retry_exponentially( 'app.clients.cbc_proxy.CBCProxyEE.create_and_send_broadcast', side_effect=CBCProxyRetryableException('oh no'), ) - mock_retry = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_provider_message.retry') + mock_retry = mocker.patch( + 'app.celery.broadcast_message_tasks.send_broadcast_provider_message.retry', + side_effect=Retry + ) # patch celery request context as shown here: https://stackoverflow.com/a/59870468 mock_celery_task_request_context = mocker.patch("celery.app.task.Task.request") mock_celery_task_request_context.retries = num_retries - send_broadcast_provider_message(provider='ee', broadcast_event_id=str(event.id)) + with pytest.raises(Retry): + send_broadcast_provider_message(provider='ee', broadcast_event_id=str(event.id)) mock_create_broadcast.assert_called_once_with( identifier=ANY,