From 8f39d476bd1296c124f991bcede1b2c41a77e395 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 25 Aug 2021 15:48:09 +0100 Subject: [PATCH] Start dual running with "areas" and (area) "ids" This is necessary until: - The Admin app is using the new "areas(_2)" format to store and retrieve data. - We've migrated all existing broadcast messages to use the new format. Note that "areas" / "ids" isn't actually used for anything except printing out the PagerDuty message - it's not sent to the proxy [1]. [1]: https://github.com/alphagov/notifications-api/blob/6edc6c70aac4e447516b2459ab7fb386cf8a1fb2/app/celery/broadcast_message_tasks.py#L190-L193 --- app/broadcast_message/rest.py | 16 +++++++---- app/celery/broadcast_message_tasks.py | 2 +- app/models.py | 2 +- tests/app/broadcast_message/test_rest.py | 32 +++++++++++++-------- tests/app/dao/test_broadcast_message_dao.py | 4 +-- tests/app/db.py | 2 +- 6 files changed, 35 insertions(+), 23 deletions(-) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 50cf6908c..02b734ec2 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -139,10 +139,12 @@ def create_broadcast_message(service_id): if "areas" in data: areas["areas"] = data["areas"] + areas["ids"] = data["areas"] if "simple_polygons" in data: areas["simple_polygons"] = data["simple_polygons"] - if "areas" in areas_2: - areas["areas"] = areas_2["areas"] + if "ids" in areas_2: + areas["areas"] = areas_2["ids"] + areas["ids"] = areas_2["ids"] if "simple_polygons" in areas_2: areas["simple_polygons"] = areas_2["simple_polygons"] @@ -189,16 +191,18 @@ def update_broadcast_message(service_id, broadcast_message_id): if "areas" in data: areas["areas"] = data["areas"] + areas["ids"] = data["areas"] if "simple_polygons" in data: areas["simple_polygons"] = data["simple_polygons"] - if "areas" in areas_2: - areas["areas"] = areas_2["areas"] + if "ids" in areas_2: + areas["areas"] = areas_2["ids"] + areas["ids"] = areas_2["ids"] if "simple_polygons" in areas_2: areas["simple_polygons"] = areas_2["simple_polygons"] - if ('areas' in areas and 'simple_polygons' not in areas) or ('areas' not in areas and 'simple_polygons' in areas): + if ('ids' in areas and 'simple_polygons' not in areas) or ('ids' not in areas and 'simple_polygons' in areas): raise InvalidRequest( - f'Cannot update broadcast_message {broadcast_message.id}, areas or polygons are missing.', + f'Cannot update broadcast_message {broadcast_message.id}, area IDs or polygons are missing.', status_code=400 ) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 8a5d2ef99..6a0b06886 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -135,7 +135,7 @@ def send_broadcast_event(broadcast_event_id): f'https://www.notifications.service.gov.uk/services/{broadcast_message.service_id}/current-alerts/{broadcast_message.id}', # noqa '', f'This broacast has been sent on channel {broadcast_message.service.broadcast_channel}.', - f'This broadcast is targeted at areas {broadcast_message.areas.get("areas")}.', + f'This broadcast is targeted at areas {broadcast_message.areas.get("ids", broadcast_message.areas.get("areas"))}.', # noqa '' f'This broadcast\'s content starts "{broadcast_message.content[:100]}"' '', diff --git a/app/models.py b/app/models.py index e94277e58..6895cd029 100644 --- a/app/models.py +++ b/app/models.py @@ -2339,7 +2339,7 @@ class BroadcastMessage(db.Model): # TEMPORARY: while we repurpose "areas" areas_2 = dict(self.areas) areas_2["simple_polygons"] = areas_2.get("simple_polygons", []) - areas_2["areas"] = areas_2.get("areas", []) + areas_2["ids"] = areas_2.pop("areas", []) return { 'id': str(self.id), diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 1e927d5de..d2f50776e 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -51,7 +51,7 @@ def test_get_broadcast_message(admin_request, sample_broadcast_service): assert response['created_at'] is not None assert response['starts_at'] is None assert response['areas'] == ['place A', 'region B'] - assert response['areas_2']['areas'] == ['place A', 'region B'] + assert response['areas_2']['ids'] == ['place A', 'region B'] assert response['personalisation'] == {'thing': 'test'} @@ -81,7 +81,7 @@ def test_get_broadcast_message_without_template(admin_request, sample_broadcast_ assert response['created_at'] is not None assert response['starts_at'] is None assert response['areas'] == ['place A', 'region B'] - assert response['areas_2']['areas'] == ['place A', 'region B'] + assert response['areas_2']['ids'] == ['place A', 'region B'] assert response['personalisation'] is None @@ -155,7 +155,7 @@ def test_create_broadcast_message(admin_request, sample_broadcast_service, train assert response['created_by_id'] == str(t.created_by_id) assert response['personalisation'] == {} assert response['areas'] == [] - assert response['areas_2']['areas'] == [] + assert response['areas_2']['ids'] == [] assert response['areas_2']['simple_polygons'] == [] assert response['content'] == 'Some content\n€ŷŵ~\n\'\'""---' @@ -365,7 +365,7 @@ def test_create_broadcast_message_400s_if_no_content_or_template( }, { "areas_2": { - "areas": ["london", "glasgow"], + "ids": ["london", "glasgow"], "simple_polygons": [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] }, } @@ -401,7 +401,7 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live( assert response['starts_at'] == '2020-06-01T20:00:01.000000Z' assert response['areas'] == ['london', 'glasgow'] - assert response['areas_2']['areas'] == ['london', 'glasgow'] + assert response['areas_2']['ids'] == ['london', 'glasgow'] assert response['simple_polygons'] == [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] assert response['areas_2']['simple_polygons'] == [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] assert response['updated_at'] is not None @@ -413,7 +413,7 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live( ]) @pytest.mark.parametrize('area_data', [ # TEMPORARY: while we repurpose "areas" {'areas': ['london', 'glasgow']}, - {'areas_2': {'areas': ['london', 'glasgow']}}, + {'areas_2': {'ids': ['london', 'glasgow']}}, ]) @pytest.mark.parametrize('status', [ BroadcastStatusType.BROADCASTING, @@ -490,7 +490,7 @@ def test_update_broadcast_message_allows_sensible_datetime_formats(admin_request }, { "areas_2": { - "areas": ["glasgow"], + "ids": ["glasgow"], "simple_polygons": [[[55.86, -4.25], [55.85, -4.25], [55.87, -4.24]]], } }, @@ -518,7 +518,7 @@ def test_update_broadcast_message_doesnt_let_you_update_status(area_data, admin_ @pytest.mark.parametrize("incomplete_area_data", [ {"areas": ["cardiff"]}, # TEMPORARY: while we repurpose "areas" {"simple_polygons": [[[51.28, -3.11], [51.29, -3.12], [51.27, -3.10]]]}, # TEMPORARY: while we repurpose "areas" - {"areas_2": {"areas": ["cardiff"]}}, + {"areas_2": {"ids": ["cardiff"]}}, {"areas_2": {"simple_polygons": [[[51.28, -3.11], [51.29, -3.12], [51.27, -3.10]]]}}, ]) def test_update_broadcast_message_doesnt_let_you_update_areas_but_not_polygons( @@ -537,7 +537,7 @@ def test_update_broadcast_message_doesnt_let_you_update_areas_but_not_polygons( assert response[ 'message' - ] == f'Cannot update broadcast_message {broadcast_message.id}, areas or polygons are missing.' + ] == f'Cannot update broadcast_message {broadcast_message.id}, area IDs or polygons are missing.' def test_update_broadcast_message_status(admin_request, sample_broadcast_service): @@ -558,7 +558,7 @@ def test_update_broadcast_message_status(admin_request, sample_broadcast_service @pytest.mark.parametrize(('area_data', 'error'), [ # TEMPORARY: while we repurpose "areas" ({'areas': ['glasgow']}, 'areas was unexpected'), - ({'areas_2': {'areas': ['glasgow']}}, 'areas_2 was unexpected') + ({'areas_2': {'ids': ['glasgow']}}, 'areas_2 was unexpected') ]) def test_update_broadcast_message_status_doesnt_let_you_update_other_things( area_data, @@ -637,7 +637,11 @@ def test_update_broadcast_message_status_stores_approved_by_and_approved_at_and_ 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]]]} + areas={ + "areas": ["london"], + "ids": ["london"], + "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]] + } ) approver = create_user(email='approver@gov.uk') sample_broadcast_service.users.append(approver) @@ -736,7 +740,11 @@ def test_update_broadcast_message_status_creates_event_with_correct_content_if_b template=None, content='tailor made emergency broadcast content', status=BroadcastStatusType.PENDING_APPROVAL, - areas={"areas": ["london"], "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]} + areas={ + "areas": ["london"], + "ids": ["london"], + "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]] + } ) approver = create_user(email='approver@gov.uk') sample_broadcast_service.users.append(approver) diff --git a/tests/app/dao/test_broadcast_message_dao.py b/tests/app/dao/test_broadcast_message_dao.py index fe4b4ebfa..144e896a2 100644 --- a/tests/app/dao/test_broadcast_message_dao.py +++ b/tests/app/dao/test_broadcast_message_dao.py @@ -122,10 +122,10 @@ def test_dao_get_all_broadcast_messages(sample_broadcast_service): assert broadcast_messages == [ ( broadcast_message_2.id, None, 'severe', 'Dear Sir/Madam, Hello. Yours Truly, The Government.', - {'areas': [], 'simple_polygons': []}, 'broadcasting', datetime(2021, 6, 20, 12, 0), + {'areas': [], 'ids': [], 'simple_polygons': []}, 'broadcasting', datetime(2021, 6, 20, 12, 0), None, None, None), ( broadcast_message_1.id, None, 'severe', 'Dear Sir/Madam, Hello. Yours Truly, The Government.', - {'areas': [], 'simple_polygons': []}, 'cancelled', datetime(2021, 6, 15, 12, 0), + {'areas': [], 'ids': [], 'simple_polygons': []}, 'cancelled', datetime(2021, 6, 15, 12, 0), None, None, None) ] diff --git a/tests/app/db.py b/tests/app/db.py index eed02e534..58f50824f 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1146,7 +1146,7 @@ def create_broadcast_message( starts_at=starts_at, finishes_at=finishes_at, created_by_id=created_by.id if created_by else service.created_by_id, - areas=areas or {'areas': [], 'simple_polygons': []}, + areas=areas or {'areas': [], 'ids': [], 'simple_polygons': []}, content=content, stubbed=stubbed )