From b2398fcaf4f252d4ef338f0dd64ab99542d95ee0 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Apr 2021 14:54:15 +0100 Subject: [PATCH] Rename CBCProxyFatalException We only actually use this when the data we're working with is in an unexpected state, which is unrelated to the CBC Proxy. Using this name also means we can re-use this exception in the next commits. Note that we may still care if a broadcast message has expired, since it's not expected that someone would send one in this condition. --- app/celery/broadcast_message_tasks.py | 19 +++++++++---------- app/clients/cbc_proxy.py | 4 ---- .../celery/test_broadcast_message_tasks.py | 18 ++++++++---------- 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 422feaa59..a3a61f96c 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -5,10 +5,7 @@ from flask import current_app from sqlalchemy.schema import Sequence from app import cbc_proxy_client, db, notify_celery, zendesk_client -from app.clients.cbc_proxy import ( - CBCProxyFatalException, - CBCProxyRetryableException, -) +from app.clients.cbc_proxy import CBCProxyRetryableException from app.config import QueueNames from app.dao.broadcast_message_dao import ( create_broadcast_provider_message, @@ -23,6 +20,10 @@ from app.models import ( from app.utils import format_sequential_number +class BroadcastIntegrityError(Exception): + pass + + def get_retry_delay(retry_count): """ Given a count of retries so far, return a delay for the next one. @@ -58,16 +59,14 @@ def check_provider_message_should_send(broadcast_event, provider): current_provider_message = broadcast_event.get_provider_message(provider) # if this is the first time a task is being executed, it won't have a provider message yet if current_provider_message and current_provider_message.status != BroadcastProviderMessageStatus.SENDING: - raise CBCProxyFatalException( + raise BroadcastIntegrityError( f'Cannot send broadcast_event {broadcast_event.id} ' + f'to provider {provider}: ' + f'It is in status {current_provider_message.status}' ) if broadcast_event.transmitted_finishes_at < datetime.utcnow(): - # TODO: This should be a different kind of exception to distinguish "We should know something went wrong, but - # no immediate action" from "We need to fix this immediately" - raise CBCProxyFatalException( + raise BroadcastIntegrityError( f'Cannot send broadcast_event {broadcast_event.id} ' + f'to provider {provider}: ' + f'The expiry time of {broadcast_event.transmitted_finishes_at} has already passed' @@ -83,7 +82,7 @@ def check_provider_message_should_send(broadcast_event, provider): # the previous message hasn't even got round to running `send_broadcast_provider_message` yet. if not prev_provider_message: - raise CBCProxyFatalException( + raise BroadcastIntegrityError( f'Cannot send {broadcast_event.id}. Previous event {prev_event.id} ' + f'(type {prev_event.message_type}) has no provider_message for provider {provider} yet.\n' + 'You must ensure that the other event sends succesfully, then manually kick off this event ' + @@ -93,7 +92,7 @@ def check_provider_message_should_send(broadcast_event, provider): # if there's a previous message that has started but not finished sending (whether it fatally errored or is # currently retrying) if prev_provider_message.status != BroadcastProviderMessageStatus.ACK: - raise CBCProxyFatalException( + raise BroadcastIntegrityError( f'Cannot send {broadcast_event.id}. Previous event {prev_event.id} ' + f'(type {prev_event.message_type}) has not finished sending to provider {provider} yet.\n' + f'It is currently in status "{prev_provider_message.status}".\n' + diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index 29670c2b9..63a69f892 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -26,10 +26,6 @@ from app.utils import DATETIME_FORMAT, format_sequential_number # the preceeding Alert message in the previous_provider_messages field -class CBCProxyFatalException(Exception): - pass - - class CBCProxyRetryableException(Exception): pass diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 61d5aae03..644d19b97 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -7,16 +7,14 @@ from celery.exceptions import Retry from freezegun import freeze_time from app.celery.broadcast_message_tasks import ( + BroadcastIntegrityError, check_provider_message_should_send, get_retry_delay, send_broadcast_event, send_broadcast_provider_message, trigger_link_test, ) -from app.clients.cbc_proxy import ( - CBCProxyFatalException, - CBCProxyRetryableException, -) +from app.clients.cbc_proxy import CBCProxyRetryableException from app.models import ( BROADCAST_TYPE, BroadcastEventMessageType, @@ -620,7 +618,7 @@ def test_check_provider_message_should_send_raises_if_event_has_expired(sample_t transmitted_starts_at=datetime(2021, 1, 1, 0, 0), transmitted_finishes_at=datetime(2021, 1, 1, 11, 59), ) - with pytest.raises(CBCProxyFatalException) as exc: + with pytest.raises(BroadcastIntegrityError) as exc: check_provider_message_should_send(current_event, 'ee') assert 'The expiry time of 2021-01-01 11:59:00 has already passed' in str(exc.value) @@ -651,7 +649,7 @@ def test_check_provider_message_should_send_raises_if_older_event_still_sending( create_broadcast_provider_message(past_still_sending_event, provider='ee', status=BroadcastProviderMessageStatus.SENDING) # noqa # we havent sent the previous update yet - it's still in sending - so don't try and send this one. - with pytest.raises(CBCProxyFatalException) as exc: + with pytest.raises(BroadcastIntegrityError) as exc: check_provider_message_should_send(current_event, 'ee') assert f'Previous event {past_still_sending_event.id} (type update) has not finished sending to provider ee' in str(exc.value) # noqa @@ -683,7 +681,7 @@ def test_check_provider_message_should_send_raises_if_older_event_hasnt_started_ create_broadcast_provider_message(past_succesful_event, provider='ee', status=BroadcastProviderMessageStatus.ACK) # we shouldn't send the update now, because a previous event is still stuck in sending - with pytest.raises(CBCProxyFatalException) as exc: + with pytest.raises(BroadcastIntegrityError) as exc: check_provider_message_should_send(current_event, 'ee') assert f'Previous event {past_still_sending_event.id} (type update) has no provider_message for provider ee' in str(exc.value) # noqa @@ -714,15 +712,15 @@ def test_check_provider_message_should_send_doesnt_raise_if_newer_event_not_acke BroadcastProviderMessageStatus.SENDING, pytest.param( BroadcastProviderMessageStatus.ACK, - marks=pytest.mark.xfail(raises=CBCProxyFatalException) + marks=pytest.mark.xfail(raises=BroadcastIntegrityError) ), pytest.param( BroadcastProviderMessageStatus.ERR, - marks=pytest.mark.xfail(raises=CBCProxyFatalException) + marks=pytest.mark.xfail(raises=BroadcastIntegrityError) ), pytest.param( BroadcastProviderMessageStatus.TECHNICAL_FAILURE, - marks=pytest.mark.xfail(raises=CBCProxyFatalException) + marks=pytest.mark.xfail(raises=BroadcastIntegrityError) ), ]) def test_check_provider_message_should_send_raises_if_current_event_already_has_provider_message_not_in_sending(