From b2398fcaf4f252d4ef338f0dd64ab99542d95ee0 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Apr 2021 14:54:15 +0100 Subject: [PATCH 1/6] 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( From 0070473f3123a3382a3db0231717ddcf5eb4555d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Apr 2021 15:27:20 +0100 Subject: [PATCH 2/6] Check for suspension before sending a broadcast This mirrors the check we do for jobs, which are also a high-impact task [1]. While this shouldn't be possible, just like other checks we're adding it here to be doubly certain. [1]: https://github.com/alphagov/notifications-api/blob/3d71815956eb930a1683d47e60d8c4a53974fd32/app/celery/tasks.py#L74 --- app/celery/broadcast_message_tasks.py | 6 ++++++ tests/app/celery/test_broadcast_message_tasks.py | 13 +++++++++++++ 2 files changed, 19 insertions(+) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index a3a61f96c..e4e4c2060 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -56,6 +56,12 @@ def check_provider_message_should_send(broadcast_event, provider): 4. If you need to re-send this task off again, you'll need to run the following command on paas: `send_broadcast_provider_message.apply_async(args=(broadcast_event_id, provider), queue=QueueNames.BROADCASTS)` """ + if not broadcast_event.service.active: + raise BroadcastIntegrityError( + f'Cannot send broadcast_event {broadcast_event.id} ' + + f'to provider {provider}: the service is suspended' + ) + 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: diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 644d19b97..156494b1f 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -735,6 +735,19 @@ def test_check_provider_message_should_send_raises_if_current_event_already_has_ check_provider_message_should_send(current_event, 'ee') +def test_check_provider_message_should_send_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: + check_provider_message_should_send(current_event, 'ee') + + assert 'service is suspended' in str(exc.value) + + def test_send_broadcast_provider_message_does_nothing_if_cbc_proxy_disabled(mocker, notify_api, sample_template): mock_proxy_client_getter = mocker.patch( 'app.celery.broadcast_message_tasks.cbc_proxy_client', From ee52e3e2c9d91c5283927a115f9775d9a42f0b82 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Apr 2021 15:36:10 +0100 Subject: [PATCH 3/6] Mirror integrity checks from the API It makes sense to have these checks [1] here, since in future we may add other ways of creating a broadcast event and omit them. [1]: https://github.com/alphagov/notifications-api/blob/3d71815956eb930a1683d47e60d8c4a53974fd32/app/broadcast_message/rest.py#L198 --- app/celery/broadcast_message_tasks.py | 12 +++++++++ .../celery/test_broadcast_message_tasks.py | 25 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index e4e4c2060..84173f94d 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -62,6 +62,18 @@ def check_provider_message_should_send(broadcast_event, provider): 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' + ) + 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: diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 156494b1f..167940291 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -748,6 +748,31 @@ def test_check_provider_message_should_send_raises_if_service_is_suspended( assert 'service is suspended' in str(exc.value) +def test_check_provider_message_should_send_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: + check_provider_message_should_send(current_event, 'ee') + + assert 'service is not live' in str(exc.value) + + +def test_check_provider_message_should_send_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: + check_provider_message_should_send(current_event, '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): mock_proxy_client_getter = mocker.patch( 'app.celery.broadcast_message_tasks.cbc_proxy_client', From 936c9ebdfe7ce57bbb320681573a73f7e2cdfbc6 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Apr 2021 15:58:56 +0100 Subject: [PATCH 4/6] Test sanity checks by calling top-level task Since the checks are only performed in one place we can easily take extra care to ensure this in the tests, noting that we don't need to do any additional setup, except if no exception is raised - I've left these tests as-is, to avoid doing more setup. Note that we still check the happy path for when a provider message is already sending - just in a different test [1]. [1]: https://github.com/alphagov/notifications-api/blob/3d71815956eb930a1683d47e60d8c4a53974fd32/tests/app/celery/test_broadcast_message_tasks.py#L263 --- .../celery/test_broadcast_message_tasks.py | 48 ++++++++----------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 167940291..69673302d 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -611,7 +611,7 @@ def test_check_provider_message_should_send_doesnt_raise_if_event_hasnt_expired_ @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, @@ -619,12 +619,12 @@ def test_check_provider_message_should_send_raises_if_event_has_expired(sample_t transmitted_finishes_at=datetime(2021, 1, 1, 11, 59), ) with pytest.raises(BroadcastIntegrityError) as exc: - check_provider_message_should_send(current_event, 'ee') + 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( @@ -650,13 +650,13 @@ def test_check_provider_message_should_send_raises_if_older_event_still_sending( # we havent sent the previous update yet - it's still in sending - so don't try and send this one. with pytest.raises(BroadcastIntegrityError) as exc: - check_provider_message_should_send(current_event, 'ee') + 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( @@ -682,7 +682,7 @@ def test_check_provider_message_should_send_raises_if_older_event_hasnt_started_ # we shouldn't send the update now, because a previous event is still stuck in sending with pytest.raises(BroadcastIntegrityError) as exc: - check_provider_message_should_send(current_event, 'ee') + 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 @@ -709,33 +709,25 @@ def test_check_provider_message_should_send_doesnt_raise_if_newer_event_not_acke @pytest.mark.parametrize('existing_message_status', [ - BroadcastProviderMessageStatus.SENDING, - pytest.param( - BroadcastProviderMessageStatus.ACK, - marks=pytest.mark.xfail(raises=BroadcastIntegrityError) - ), - pytest.param( - BroadcastProviderMessageStatus.ERR, - marks=pytest.mark.xfail(raises=BroadcastIntegrityError) - ), - pytest.param( - BroadcastProviderMessageStatus.TECHNICAL_FAILURE, - marks=pytest.mark.xfail(raises=BroadcastIntegrityError) - ), + 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_check_provider_message_should_send_raises_if_service_is_suspended( +def test_send_broadcast_provider_message_raises_if_service_is_suspended( sample_broadcast_service, ): sample_broadcast_service.active = False @@ -743,12 +735,12 @@ def test_check_provider_message_should_send_raises_if_service_is_suspended( current_event = create_broadcast_event(broadcast_message, message_type='alert') with pytest.raises(BroadcastIntegrityError) as exc: - check_provider_message_should_send(current_event, 'ee') + send_broadcast_provider_message(current_event.id, 'ee') assert 'service is suspended' in str(exc.value) -def test_check_provider_message_should_send_raises_if_service_is_not_live( +def test_send_broadcast_provider_message_raises_if_service_is_not_live( sample_broadcast_service, ): sample_broadcast_service.restricted = True @@ -756,19 +748,19 @@ def test_check_provider_message_should_send_raises_if_service_is_not_live( current_event = create_broadcast_event(broadcast_message, message_type='alert') with pytest.raises(BroadcastIntegrityError) as exc: - check_provider_message_should_send(current_event, 'ee') + send_broadcast_provider_message(current_event.id, 'ee') assert 'service is not live' in str(exc.value) -def test_check_provider_message_should_send_raises_if_message_is_stubbed( +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: - check_provider_message_should_send(current_event, 'ee') + send_broadcast_provider_message(current_event.id, 'ee') assert 'message is stubbed' in str(exc.value) From a2af8b052a0737ea4fb93ca3792e033911314758 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Apr 2021 16:06:13 +0100 Subject: [PATCH 5/6] Split up authorisation vs. sequencing checks While both of these are integrity errors (since we should never reach this point in the code + data), this just means the original method comment is still relevant to what immediately follows it. --- app/celery/broadcast_message_tasks.py | 43 ++++++++++--------- .../celery/test_broadcast_message_tasks.py | 10 ++--- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 84173f94d..211e0bcee 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -37,25 +37,7 @@ def get_retry_delay(retry_count): return min(delay, 240) -def check_provider_message_should_send(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 - replay the `send_broadcast_provider_message` task if the previous message has now been sent. - - Note: This is called before the new broadcast_provider_message is created. - - # Help, I've come across this code following a pagerduty alert, what should I do? - - 1. Find the failing broadcast_provider_message associated with the previous event that caused this to trip. - 2. If that provider message is still failing to send, fix the issue causing that. The task to send that previous - message might still be retrying in the background - look for logs related to that task. - 3. If that provider message has sent succesfully, you might need to send this task off depending on context. This - might not always be true though, for example, it may not be necessary to send a cancel if the original alert has - already expired. - 4. If you need to re-send this task off again, you'll need to run the following command on paas: - `send_broadcast_provider_message.apply_async(args=(broadcast_event_id, provider), queue=QueueNames.BROADCASTS)` - """ +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} ' + @@ -74,6 +56,26 @@ def check_provider_message_should_send(broadcast_event, provider): 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 + replay the `send_broadcast_provider_message` task if the previous message has now been sent. + + Note: This is called before the new broadcast_provider_message is created. + + # Help, I've come across this code following a pagerduty alert, what should I do? + + 1. Find the failing broadcast_provider_message associated with the previous event that caused this to trip. + 2. If that provider message is still failing to send, fix the issue causing that. The task to send that previous + message might still be retrying in the background - look for logs related to that task. + 3. If that provider message has sent succesfully, you might need to send this task off depending on context. This + might not always be true though, for example, it may not be necessary to send a cancel if the original alert has + already expired. + 4. If you need to re-send this task off again, you'll need to run the following command on paas: + `send_broadcast_provider_message.apply_async(args=(broadcast_event_id, provider), queue=QueueNames.BROADCASTS)` + """ 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: @@ -168,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/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 69673302d..8d65108b6 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -8,7 +8,7 @@ from freezegun import freeze_time from app.celery.broadcast_message_tasks import ( BroadcastIntegrityError, - check_provider_message_should_send, + check_event_makes_sense_in_sequence, get_retry_delay, send_broadcast_event, send_broadcast_provider_message, @@ -600,14 +600,14 @@ 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') @@ -688,7 +688,7 @@ def test_send_broadcast_provider_message_raises_if_older_event_hasnt_started_sen @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( @@ -705,7 +705,7 @@ 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', [ From 0507e8d9ada84f146dfd6840fbe6d7add0e0e6c1 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Apr 2021 16:54:25 +0100 Subject: [PATCH 6/6] Raise 403 when broadcasting on a suspended service This mirrors the approach we take for jobs [1]. [1]: https://github.com/alphagov/notifications-api/blob/3d71815956eb930a1683d47e60d8c4a53974fd32/app/job/rest.py#L146 --- app/broadcast_message/rest.py | 3 +++ tests/app/broadcast_message/test_rest.py | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 975959154..37f0385aa 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -176,6 +176,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/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 92abfde4f..494c2aa5d 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -604,6 +604,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,