From df82b514d1805afad4e20e8431877dfe1bec1950 Mon Sep 17 00:00:00 2001
From: Katie Smith <katie.smith@digital.cabinet-office.gov.uk>
Date: Tue, 13 Apr 2021 16:50:26 +0100
Subject: [PATCH] Don't create broadcast event unless necessary

If we're not going to send a broadcast, we don't need to create the
BroadcastEvent in the database. The BroadcastMessage contains all the
data we need - the BroadcastEvent is not used.

Not creating the event when we won't send the broadcast (e.g. when the
broadcast message was created when the service was in trial mode) adds
an extra layer of security.
---
 app/broadcast_message/rest.py            | 44 ++++++++++++------------
 tests/app/broadcast_message/test_rest.py | 11 ++----
 2 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py
index 859f2636d..f909bc958 100644
--- a/app/broadcast_message/rest.py
+++ b/app/broadcast_message/rest.py
@@ -192,29 +192,29 @@ 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
     """
-    msg_types = {
-        BroadcastStatusType.BROADCASTING: BroadcastEventMessageType.ALERT,
-        BroadcastStatusType.CANCELLED: BroadcastEventMessageType.CANCEL,
-    }
-
-    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',
-
-        # 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:
+        msg_types = {
+            BroadcastStatusType.BROADCASTING: BroadcastEventMessageType.ALERT,
+            BroadcastStatusType.CANCELLED: BroadcastEventMessageType.CANCEL,
+        }
+
+        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',
+
+            # 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)
+
         send_broadcast_event.apply_async(
             kwargs={'broadcast_event_id': str(event.id)},
             queue=QueueNames.BROADCASTS
diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py
index b067b8bc4..440b4f81c 100644
--- a/tests/app/broadcast_message/test_rest.py
+++ b/tests/app/broadcast_message/test_rest.py
@@ -591,17 +591,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,