diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 859f2636d..975959154 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -190,32 +190,43 @@ def update_broadcast_message_status(service_id, broadcast_message_id): def _create_broadcast_event(broadcast_message): """ - Creates a broadcast event, stores it in the database, and triggers the task to send the CAP XML off + If the service is live and the broadcast message is not stubbed, creates a broadcast event, stores it in the + database, and triggers the task to send the CAP XML off. """ - msg_types = { - BroadcastStatusType.BROADCASTING: BroadcastEventMessageType.ALERT, - BroadcastStatusType.CANCELLED: BroadcastEventMessageType.CANCEL, - } + service = broadcast_message.service - event = BroadcastEvent( - service=broadcast_message.service, - broadcast_message=broadcast_message, - message_type=msg_types[broadcast_message.status], - transmitted_content={"body": broadcast_message.content}, - transmitted_areas=broadcast_message.areas, - # TODO: Probably move this somewhere more standalone too and imply that it shouldn't change. Should it include - # a service based identifier too? eg "flood-warnings@notifications.service.gov.uk" or similar - transmitted_sender='notifications.service.gov.uk', + if not broadcast_message.stubbed and not service.restricted: + msg_types = { + BroadcastStatusType.BROADCASTING: BroadcastEventMessageType.ALERT, + BroadcastStatusType.CANCELLED: BroadcastEventMessageType.CANCEL, + } - # TODO: Should this be set to now? Or the original starts_at? - transmitted_starts_at=broadcast_message.starts_at, - transmitted_finishes_at=broadcast_message.finishes_at, - ) + event = BroadcastEvent( + service=service, + broadcast_message=broadcast_message, + message_type=msg_types[broadcast_message.status], + transmitted_content={"body": broadcast_message.content}, + transmitted_areas=broadcast_message.areas, + # TODO: Probably move this somewhere more standalone too and imply that it shouldn't change. Should it + # include a service based identifier too? eg "flood-warnings@notifications.service.gov.uk" or similar + transmitted_sender='notifications.service.gov.uk', - dao_save_object(event) + # TODO: Should this be set to now? Or the original starts_at? + transmitted_starts_at=broadcast_message.starts_at, + transmitted_finishes_at=broadcast_message.finishes_at, + ) + + dao_save_object(event) - if not broadcast_message.stubbed: send_broadcast_event.apply_async( kwargs={'broadcast_event_id': str(event.id)}, queue=QueueNames.BROADCASTS ) + elif broadcast_message.stubbed != service.restricted: + # It's possible for a service to create a broadcast in trial mode, and then approve it after the + # service is live (or vice versa). We don't think it's safe to send such broadcasts, as the service + # has changed since they were created. Log an error instead. + current_app.logger.error( + f'Broadcast event not created. Stubbed status of broadcast message was {broadcast_message.stubbed}' + f' but service was {"in trial mode" if service.restricted else "live"}' + ) diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index b067b8bc4..92abfde4f 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -1,5 +1,4 @@ import uuid -from unittest.mock import ANY import pytest from freezegun import freeze_time @@ -562,23 +561,32 @@ def test_update_broadcast_message_status_stores_approved_by_and_approved_at_and_ assert alert_event.transmitted_content == {"body": "emergency broadcast"} -def test_update_broadcast_message_status_updates_details_but_does_not_queue_task_for_stubbed_broadcast_message( +@pytest.mark.parametrize('broadcast_message_stubbed, service_restricted_before_approval', [ + (True, True), + (True, False), + (False, True), +]) +def test_update_broadcast_message_status_updates_details_but_does_not_queue_task_if_bm_is_stubbed_or_service_not_live( admin_request, sample_broadcast_service, - mocker + mocker, + broadcast_message_stubbed, + service_restricted_before_approval, ): - sample_broadcast_service.restricted = True + sample_broadcast_service.restricted = broadcast_message_stubbed t = create_template(sample_broadcast_service, BROADCAST_TYPE, content='emergency broadcast') bm = create_broadcast_message( t, status=BroadcastStatusType.PENDING_APPROVAL, areas={"areas": ["london"], "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]}, - stubbed=True + stubbed=broadcast_message_stubbed ) approver = create_user(email='approver@gov.uk') sample_broadcast_service.users.append(approver) mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async') + sample_broadcast_service.restricted = service_restricted_before_approval + response = admin_request.post( 'broadcast_message.update_broadcast_message_status', _data={'status': BroadcastStatusType.BROADCASTING, 'created_by': str(approver.id)}, @@ -591,17 +599,10 @@ def test_update_broadcast_message_status_updates_details_but_does_not_queue_task assert response['approved_at'] is not None assert response['approved_by_id'] == str(approver.id) - assert len(bm.events) == 1 - alert_event = bm.events[0] - + # The broadcast can be approved, but does not create a broadcast_event in the database or put a task on the queue + assert len(bm.events) == 0 assert len(mock_task.mock_calls) == 0 - assert alert_event.service_id == sample_broadcast_service.id - assert alert_event.transmitted_areas == bm.areas - assert alert_event.message_type == BroadcastEventMessageType.ALERT - assert alert_event.transmitted_finishes_at == bm.finishes_at - assert alert_event.transmitted_content == {"body": "emergency broadcast"} - def test_update_broadcast_message_status_creates_event_with_correct_content_if_broadcast_has_no_template( admin_request, @@ -714,7 +715,7 @@ def test_update_broadcast_message_status_allows_trial_mode_services_to_approve_o assert response['approved_at'] is not None assert response['created_by_id'] == str(t.created_by_id) assert response['approved_by_id'] == str(t.created_by_id) - mock_task.assert_called_once_with(kwargs={'broadcast_event_id': ANY}, queue='broadcast-tasks') + assert not mock_task.called def test_update_broadcast_message_status_rejects_approval_from_user_not_on_that_service(