diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 26221b89a..d04e7c575 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -192,6 +192,9 @@ def update_broadcast_message_status(service_id, broadcast_message_id): validate(data, update_broadcast_message_status_schema) broadcast_message = dao_get_broadcast_message_by_id_and_service_id(broadcast_message_id, service_id) + if not broadcast_message.service.active: + raise InvalidRequest("Updating broadcast message is not allowed: service is inactive ", 403) + new_status = data['status'] updating_user = get_user_by_id(data['created_by']) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 9c05cf35c..cbd6b49c9 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. @@ -36,7 +37,27 @@ def get_retry_delay(retry_count): return min(delay, 240) -def check_provider_message_should_send(broadcast_event, provider): +def check_event_is_authorised_to_be_sent(broadcast_event, provider): + if not broadcast_event.service.active: + raise BroadcastIntegrityError( + f'Cannot send broadcast_event {broadcast_event.id} ' + + f'to provider {provider}: the service is suspended' + ) + + if broadcast_event.service.restricted: + raise BroadcastIntegrityError( + f'Cannot send broadcast_event {broadcast_event.id} ' + + f'to provider {provider}: the service is not live' + ) + + if broadcast_event.broadcast_message.stubbed: + raise BroadcastIntegrityError( + f'Cannot send broadcast_event {broadcast_event.id} ' + + f'to provider {provider}: the broadcast message is stubbed' + ) + + +def check_event_makes_sense_in_sequence(broadcast_event, provider): """ If any previous event hasn't sent yet for that provider, then we shouldn't send the current event. Instead, fail and raise a P1 - so that a notify team member can assess the state of the previous messages, and if necessary, can @@ -58,16 +79,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 +102,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 +112,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' + @@ -151,7 +170,8 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): broadcast_event = dao_get_broadcast_event_by_id(broadcast_event_id) - check_provider_message_should_send(broadcast_event, provider) + check_event_is_authorised_to_be_sent(broadcast_event, provider) + check_event_makes_sense_in_sequence(broadcast_event, provider) # the broadcast_provider_message may already exist if we retried previously broadcast_provider_message = broadcast_event.get_provider_message(provider) 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/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 438aedaf4..66382d8b5 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -645,6 +645,22 @@ def test_update_broadcast_message_status_updates_details_but_does_not_queue_task assert len(mock_task.mock_calls) == 0 +def test_update_broadcast_message_status_aborts_if_service_is_suspended( + admin_request, + sample_broadcast_service, +): + bm = create_broadcast_message(service=sample_broadcast_service, content='test') + sample_broadcast_service.active = False + + admin_request.post( + 'broadcast_message.update_broadcast_message_status', + _data={'status': BroadcastStatusType.BROADCASTING, 'created_by': str(uuid.uuid4())}, + service_id=sample_broadcast_service.id, + broadcast_message_id=bm.id, + _expected_status=403 + ) + + def test_update_broadcast_message_status_creates_event_with_correct_content_if_broadcast_has_no_template( admin_request, sample_broadcast_service, diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 61d5aae03..8d65108b6 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 ( - check_provider_message_should_send, + BroadcastIntegrityError, + check_event_makes_sense_in_sequence, 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, @@ -602,31 +600,31 @@ def test_get_retry_delay_has_capped_backoff(retry_count, expected_delay): @freeze_time('2021-01-01 12:00') -def test_check_provider_message_should_send_doesnt_raise_if_event_hasnt_expired_yet(sample_template): +def test_check_event_makes_sense_in_sequence_doesnt_raise_if_event_hasnt_expired_yet(sample_template): broadcast_message = create_broadcast_message(sample_template) current_event = create_broadcast_event( broadcast_message, transmitted_starts_at=datetime(2021, 1, 1, 0, 0), transmitted_finishes_at=datetime(2021, 1, 1, 12, 1), ) - check_provider_message_should_send(current_event, 'ee') + check_event_makes_sense_in_sequence(current_event, 'ee') @freeze_time('2021-01-01 12:00') -def test_check_provider_message_should_send_raises_if_event_has_expired(sample_template): +def test_send_broadcast_provider_message_raises_if_event_has_expired(sample_template): broadcast_message = create_broadcast_message(sample_template) current_event = create_broadcast_event( broadcast_message, transmitted_starts_at=datetime(2021, 1, 1, 0, 0), transmitted_finishes_at=datetime(2021, 1, 1, 11, 59), ) - with pytest.raises(CBCProxyFatalException) as exc: - check_provider_message_should_send(current_event, 'ee') + with pytest.raises(BroadcastIntegrityError) as exc: + send_broadcast_provider_message(current_event.id, 'ee') assert 'The expiry time of 2021-01-01 11:59:00 has already passed' in str(exc.value) @freeze_time('2021-01-01 12:00') -def test_check_provider_message_should_send_raises_if_older_event_still_sending(sample_template): +def test_send_broadcast_provider_message_raises_if_older_event_still_sending(sample_template): broadcast_message = create_broadcast_message(sample_template) # event approved at midnight past_succesful_event = create_broadcast_event( @@ -651,14 +649,14 @@ 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: - check_provider_message_should_send(current_event, 'ee') + with pytest.raises(BroadcastIntegrityError) as exc: + send_broadcast_provider_message(current_event.id, 'ee') assert f'Previous event {past_still_sending_event.id} (type update) has not finished sending to provider ee' in str(exc.value) # noqa @freeze_time('2021-01-01 12:00') -def test_check_provider_message_should_send_raises_if_older_event_hasnt_started_sending_yet(sample_template): +def test_send_broadcast_provider_message_raises_if_older_event_hasnt_started_sending_yet(sample_template): broadcast_message = create_broadcast_message(sample_template) # event approved at midnight past_succesful_event = create_broadcast_event( @@ -683,14 +681,14 @@ 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: - check_provider_message_should_send(current_event, 'ee') + with pytest.raises(BroadcastIntegrityError) as exc: + send_broadcast_provider_message(current_event.id, 'ee') assert f'Previous event {past_still_sending_event.id} (type update) has no provider_message for provider ee' in str(exc.value) # noqa @freeze_time('2021-01-01 12:00') -def test_check_provider_message_should_send_doesnt_raise_if_newer_event_not_acked_yet(sample_template): +def test_check_event_makes_sense_in_sequence_doesnt_raise_if_newer_event_not_acked_yet(sample_template): broadcast_message = create_broadcast_message(sample_template) # event approved at midnight current_event = create_broadcast_event( @@ -707,34 +705,64 @@ def test_check_provider_message_should_send_doesnt_raise_if_newer_event_not_acke # this doesn't raise, because the alert event got an ack. The cancel doesn't have an event yet # but this task is only interested in the current task (the update) so doesn't worry about that - check_provider_message_should_send(current_event, 'ee') + check_event_makes_sense_in_sequence(current_event, 'ee') @pytest.mark.parametrize('existing_message_status', [ - BroadcastProviderMessageStatus.SENDING, - pytest.param( - BroadcastProviderMessageStatus.ACK, - marks=pytest.mark.xfail(raises=CBCProxyFatalException) - ), - pytest.param( - BroadcastProviderMessageStatus.ERR, - marks=pytest.mark.xfail(raises=CBCProxyFatalException) - ), - pytest.param( - BroadcastProviderMessageStatus.TECHNICAL_FAILURE, - marks=pytest.mark.xfail(raises=CBCProxyFatalException) - ), + BroadcastProviderMessageStatus.ACK, + BroadcastProviderMessageStatus.ERR, + BroadcastProviderMessageStatus.TECHNICAL_FAILURE, ]) -def test_check_provider_message_should_send_raises_if_current_event_already_has_provider_message_not_in_sending( +def test_send_broadcast_provider_message_raises_if_current_event_already_has_provider_message_not_in_sending( sample_template, existing_message_status ): broadcast_message = create_broadcast_message(sample_template) current_event = create_broadcast_event(broadcast_message, message_type='alert') - create_broadcast_provider_message(current_event, provider='ee', status=existing_message_status) - check_provider_message_should_send(current_event, 'ee') + with pytest.raises(BroadcastIntegrityError) as exc: + send_broadcast_provider_message(current_event.id, 'ee') + + assert f'in status {existing_message_status}' in str(exc.value) + + +def test_send_broadcast_provider_message_raises_if_service_is_suspended( + sample_broadcast_service, +): + sample_broadcast_service.active = False + broadcast_message = create_broadcast_message(service=sample_broadcast_service, content='test') + current_event = create_broadcast_event(broadcast_message, message_type='alert') + + with pytest.raises(BroadcastIntegrityError) as exc: + send_broadcast_provider_message(current_event.id, 'ee') + + assert 'service is suspended' in str(exc.value) + + +def test_send_broadcast_provider_message_raises_if_service_is_not_live( + sample_broadcast_service, +): + sample_broadcast_service.restricted = True + broadcast_message = create_broadcast_message(service=sample_broadcast_service, content='test') + current_event = create_broadcast_event(broadcast_message, message_type='alert') + + with pytest.raises(BroadcastIntegrityError) as exc: + send_broadcast_provider_message(current_event.id, 'ee') + + assert 'service is not live' in str(exc.value) + + +def test_send_broadcast_provider_message_raises_if_message_is_stubbed( + sample_template, +): + broadcast_message = create_broadcast_message(sample_template, stubbed=True) + current_event = create_broadcast_event(broadcast_message, message_type='alert') + + with pytest.raises(BroadcastIntegrityError) as exc: + send_broadcast_provider_message(current_event.id, 'ee') + + assert 'message is stubbed' in str(exc.value) def test_send_broadcast_provider_message_does_nothing_if_cbc_proxy_disabled(mocker, notify_api, sample_template):