From fd7ebbebb053318a0e1bb89b2445fe385f82583c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 25 Aug 2021 13:49:18 +0100 Subject: [PATCH 1/3] Introduce "areas_2" so we can repurpose "areas" Currently we have: - An "areas" column in the DB that stores a JSON blob. - An "areas" field inside the "areas" JSON that stores area IDs. - Each field has to be manually copied into the JSON column. We want to move to: - An "areas" column in the DB (unchanged). - An "ids" field inside the "areas" JSON (to replace "areas"). - The Admin app sending other data inside an "areas" JSON field. The API design for areas is confusing and difficult to extend. Here we duplicate the current API functionality using an "areas_2" field. Once the Admin app is using this field, we'll be able to rename it to just "areas", which is where we want to get to. In the next commits we'll build on this to support the migration from "areas"."areas" to "areas"."ids". --- .../broadcast_message_schema.py | 2 + app/broadcast_message/rest.py | 34 +++++++- app/models.py | 7 ++ tests/app/broadcast_message/test_rest.py | 81 +++++++++++++++---- tests/app/v2/broadcast/test_post_broadcast.py | 13 +++ 5 files changed, 119 insertions(+), 18 deletions(-) diff --git a/app/broadcast_message/broadcast_message_schema.py b/app/broadcast_message/broadcast_message_schema.py index fc4527511..dc7996707 100644 --- a/app/broadcast_message/broadcast_message_schema.py +++ b/app/broadcast_message/broadcast_message_schema.py @@ -14,6 +14,7 @@ create_broadcast_message_schema = { 'starts_at': {'type': 'string', 'format': 'datetime'}, 'finishes_at': {'type': 'string', 'format': 'datetime'}, 'areas': {"type": "array", "items": {"type": "string"}}, + 'areas_2': {'type': 'object'}, 'simple_polygons': {"type": "array", "items": {"type": "array"}}, 'content': {'type': 'string', 'minLength': 1}, 'reference': {'type': 'string', 'minLength': 1, 'maxLength': 255}, @@ -42,6 +43,7 @@ update_broadcast_message_schema = { 'starts_at': {'type': 'string', 'format': 'datetime'}, 'finishes_at': {'type': 'string', 'format': 'datetime'}, 'areas': {"type": "array", "items": {"type": "string"}}, + 'areas_2': {'type': 'object'}, 'simple_polygons': {"type": "array", "items": {"type": "array"}}, }, 'required': [], diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 9a50c5cfe..50cf6908c 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -133,12 +133,25 @@ def create_broadcast_message(service_id): content = str(temporary_template) reference = data['reference'] + # TEMPORARY: while we repurpose "areas" + areas = {} + areas_2 = data.get("areas_2", {}) + + if "areas" in data: + areas["areas"] = data["areas"] + if "simple_polygons" in data: + areas["simple_polygons"] = data["simple_polygons"] + if "areas" in areas_2: + areas["areas"] = areas_2["areas"] + if "simple_polygons" in areas_2: + areas["simple_polygons"] = areas_2["simple_polygons"] + broadcast_message = BroadcastMessage( service_id=service.id, template_id=template_id, template_version=template.version if template else None, personalisation=personalisation, - areas={"areas": data.get("areas", []), "simple_polygons": data.get("simple_polygons", [])}, + areas=areas, status=BroadcastStatusType.DRAFT, starts_at=_parse_nullable_datetime(data.get('starts_at')), finishes_at=_parse_nullable_datetime(data.get('finishes_at')), @@ -170,7 +183,20 @@ def update_broadcast_message(service_id, broadcast_message_id): status_code=400 ) - if ('areas' in data and 'simple_polygons' not in data) or ('areas' not in data and 'simple_polygons' in data): + # TEMPORARY: while we repurpose "areas" + areas = {} + areas_2 = data.get("areas_2", {}) + + if "areas" in data: + areas["areas"] = data["areas"] + if "simple_polygons" in data: + areas["simple_polygons"] = data["simple_polygons"] + if "areas" in areas_2: + areas["areas"] = areas_2["areas"] + 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): raise InvalidRequest( f'Cannot update broadcast_message {broadcast_message.id}, areas or polygons are missing.', status_code=400 @@ -182,8 +208,8 @@ def update_broadcast_message(service_id, broadcast_message_id): broadcast_message.starts_at = _parse_nullable_datetime(data['starts_at']) if 'finishes_at' in data: broadcast_message.finishes_at = _parse_nullable_datetime(data['finishes_at']) - if 'areas' in data and 'simple_polygons' in data: - broadcast_message.areas = {"areas": data["areas"], "simple_polygons": data["simple_polygons"]} + if 'areas' in areas and 'simple_polygons' in areas: + broadcast_message.areas = areas dao_save_object(broadcast_message) diff --git a/app/models.py b/app/models.py index cda2b3509..e94277e58 100644 --- a/app/models.py +++ b/app/models.py @@ -2336,6 +2336,11 @@ class BroadcastMessage(db.Model): self._personalisation = encryption.encrypt(personalisation or {}) def serialize(self): + # 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", []) + return { 'id': str(self.id), 'reference': self.reference, @@ -2348,6 +2353,8 @@ class BroadcastMessage(db.Model): 'personalisation': self.personalisation if self.template else None, 'content': self.content, + # TEMPORARY: switch to this so we can repurpose "areas" + 'areas_2': areas_2, 'areas': self.areas.get("areas", []), 'simple_polygons': self.areas.get("simple_polygons", []), diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 5c94fd6e1..1e927d5de 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -51,6 +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['personalisation'] == {'thing': 'test'} @@ -80,6 +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['personalisation'] is None @@ -153,6 +155,8 @@ 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']['simple_polygons'] == [] assert response['content'] == 'Some content\n€ŷŵ~\n\'\'""---' broadcast_message = dao_get_broadcast_message_by_id_and_service_id(response["id"], sample_broadcast_service.id) @@ -354,12 +358,29 @@ def test_create_broadcast_message_400s_if_no_content_or_template( ) +@pytest.mark.parametrize('area_data', [ # TEMPORARY: while we repurpose "areas" + { + "areas": ["london", "glasgow"], + "simple_polygons": [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] + }, + { + "areas_2": { + "areas": ["london", "glasgow"], + "simple_polygons": [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] + }, + } +]) @pytest.mark.parametrize('status', [ BroadcastStatusType.DRAFT, BroadcastStatusType.PENDING_APPROVAL, BroadcastStatusType.REJECTED, ]) -def test_update_broadcast_message_allows_edit_while_not_yet_live(admin_request, sample_broadcast_service, status): +def test_update_broadcast_message_allows_edit_while_not_yet_live( + area_data, + admin_request, + sample_broadcast_service, + status +): t = create_template(sample_broadcast_service, BROADCAST_TYPE) bm = create_broadcast_message( t, @@ -371,8 +392,7 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live(admin_request, 'broadcast_message.update_broadcast_message', _data={ 'starts_at': '2020-06-01 20:00:01', - 'areas': ['london', 'glasgow'], - "simple_polygons": [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] + **area_data, }, service_id=t.service_id, broadcast_message_id=bm.id, @@ -381,7 +401,9 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live(admin_request, assert response['starts_at'] == '2020-06-01T20:00:01.000000Z' assert response['areas'] == ['london', 'glasgow'] + assert response['areas_2']['areas'] == ['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 @@ -389,6 +411,10 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live(admin_request, False, pytest.param(True, marks=pytest.mark.xfail) ]) +@pytest.mark.parametrize('area_data', [ # TEMPORARY: while we repurpose "areas" + {'areas': ['london', 'glasgow']}, + {'areas_2': {'areas': ['london', 'glasgow']}}, +]) @pytest.mark.parametrize('status', [ BroadcastStatusType.BROADCASTING, BroadcastStatusType.CANCELLED, @@ -396,8 +422,9 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live(admin_request, BroadcastStatusType.TECHNICAL_FAILURE, ]) def test_update_broadcast_message_doesnt_allow_edits_after_broadcast_goes_live( - admin_request, force_override, + area_data, + admin_request, sample_broadcast_service, status ): @@ -406,7 +433,7 @@ def test_update_broadcast_message_doesnt_allow_edits_after_broadcast_goes_live( response = admin_request.post( 'broadcast_message.update_broadcast_message', - _data={'areas': ['london', 'glasgow'], 'force_override': force_override}, + _data={'force_override': force_override, **area_data}, service_id=t.service_id, broadcast_message_id=bm.id, _expected_status=400 @@ -418,7 +445,7 @@ def test_update_broadcast_message_sets_finishes_at_separately(admin_request, sam t = create_template(sample_broadcast_service, BROADCAST_TYPE) bm = create_broadcast_message( t, - areas={"areas": ['london'], "simple_polygons": [[[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]]} + areas={"areas": ["london"], "simple_polygons": [[[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]]} ) response = admin_request.post( @@ -456,15 +483,26 @@ def test_update_broadcast_message_allows_sensible_datetime_formats(admin_request assert response['updated_at'] is not None -def test_update_broadcast_message_doesnt_let_you_update_status(admin_request, sample_broadcast_service): +@pytest.mark.parametrize('area_data', [ # TEMPORARY: while we repurpose "areas" + { + "areas": ["glasgow"], + "simple_polygons": [[[55.86, -4.25], [55.85, -4.25], [55.87, -4.24]]], + }, + { + "areas_2": { + "areas": ["glasgow"], + "simple_polygons": [[[55.86, -4.25], [55.85, -4.25], [55.87, -4.24]]], + } + }, +]) +def test_update_broadcast_message_doesnt_let_you_update_status(area_data, admin_request, sample_broadcast_service): t = create_template(sample_broadcast_service, BROADCAST_TYPE) bm = create_broadcast_message(t) response = admin_request.post( 'broadcast_message.update_broadcast_message', _data={ - 'areas': ['glasgow'], - "simple_polygons": [[[55.86, -4.25], [55.85, -4.25], [55.87, -4.24]]], + **area_data, 'status': BroadcastStatusType.BROADCASTING}, service_id=t.service_id, broadcast_message_id=bm.id, @@ -478,8 +516,10 @@ def test_update_broadcast_message_doesnt_let_you_update_status(admin_request, sa @pytest.mark.parametrize("incomplete_area_data", [ - {"areas": ["cardiff"]}, - {"simple_polygons": [[[51.28, -3.11], [51.29, -3.12], [51.27, -3.10]]]}, + {"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": {"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( admin_request, sample_broadcast_service, incomplete_area_data @@ -516,13 +556,26 @@ def test_update_broadcast_message_status(admin_request, sample_broadcast_service assert response['updated_at'] is not None -def test_update_broadcast_message_status_doesnt_let_you_update_other_things(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') +]) +def test_update_broadcast_message_status_doesnt_let_you_update_other_things( + area_data, + error, + admin_request, + sample_broadcast_service +): t = create_template(sample_broadcast_service, BROADCAST_TYPE) bm = create_broadcast_message(t) response = admin_request.post( 'broadcast_message.update_broadcast_message_status', - _data={'areas': ['glasgow'], 'status': BroadcastStatusType.BROADCASTING, 'created_by': str(t.created_by_id)}, + _data={ + **area_data, + 'status': BroadcastStatusType.BROADCASTING, + 'created_by': str(t.created_by_id) + }, service_id=t.service_id, broadcast_message_id=bm.id, _expected_status=400 @@ -530,7 +583,7 @@ def test_update_broadcast_message_status_doesnt_let_you_update_other_things(admi assert response['errors'] == [{ 'error': 'ValidationError', - 'message': 'Additional properties are not allowed (areas was unexpected)' + 'message': f'Additional properties are not allowed ({error})', }] diff --git a/tests/app/v2/broadcast/test_post_broadcast.py b/tests/app/v2/broadcast/test_post_broadcast.py index 0fd8de0c2..ddbedaba0 100644 --- a/tests/app/v2/broadcast/test_post_broadcast.py +++ b/tests/app/v2/broadcast/test_post_broadcast.py @@ -84,9 +84,13 @@ def test_valid_post_cap_xml_broadcast_returns_201( assert response_json['approved_at'] is None assert response_json['approved_by_id'] is None + # TEMPORARY: while we repurpose "areas" assert response_json['areas'] == [ 'River Steeping in Wainfleet All Saints' ] + assert response_json['areas_2']['areas'] == [ + 'River Steeping in Wainfleet All Saints' + ] assert response_json['cancelled_at'] is None assert response_json['cancelled_by_id'] is None assert response_json['content'].startswith( @@ -102,10 +106,19 @@ def test_valid_post_cap_xml_broadcast_returns_201( assert response_json['id'] == ANY assert response_json['personalisation'] is None assert response_json['service_id'] == str(sample_broadcast_service.id) + + # TEMPORARY: while we repurpose "areas" assert len(response_json['simple_polygons']) == 1 assert len(response_json['simple_polygons'][0]) == 23 assert response_json['simple_polygons'][0][0] == [53.10562, 0.244127] assert response_json['simple_polygons'][0][-1] == [53.10562, 0.244127] + assert response_json['simple_polygons'][0][-1] == [53.10562, 0.244127] + assert len(response_json['areas_2']['simple_polygons']) == 1 + assert len(response_json['areas_2']['simple_polygons'][0]) == 23 + assert response_json['areas_2']['simple_polygons'][0][0] == [53.10562, 0.244127] + assert response_json['areas_2']['simple_polygons'][0][-1] == [53.10562, 0.244127] + assert response_json['areas_2']['simple_polygons'][0][-1] == [53.10562, 0.244127] + assert response_json['starts_at'] is None assert response_json['status'] == 'pending-approval' assert response_json['template_id'] is None From 8f39d476bd1296c124f991bcede1b2c41a77e395 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 25 Aug 2021 15:48:09 +0100 Subject: [PATCH 2/3] 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 ) From 023a06d5fb4c20e6bf5a1c9e65b473cb376b2b6b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 25 Aug 2021 15:53:05 +0100 Subject: [PATCH 3/3] Start dual running with "areas" and "names" For the public API we actually receive a "name" instead of an ID, which we also want to start sending from the Admin app. Unlike IDs, which aren't really used anywhere, we want the names to display the alerts on gov.uk/alerts. --- app/v2/broadcast/post_broadcast.py | 3 +++ tests/app/v2/broadcast/test_post_broadcast.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/v2/broadcast/post_broadcast.py b/app/v2/broadcast/post_broadcast.py index c5fae20a8..afebc0265 100644 --- a/app/v2/broadcast/post_broadcast.py +++ b/app/v2/broadcast/post_broadcast.py @@ -70,6 +70,9 @@ def create_broadcast(): 'areas': [ area['name'] for area in broadcast_json['areas'] ], + 'names': [ + area['name'] for area in broadcast_json['areas'] + ], 'simple_polygons': polygons.smooth.simplify.as_coordinate_pairs_long_lat, }, status=BroadcastStatusType.PENDING_APPROVAL, diff --git a/tests/app/v2/broadcast/test_post_broadcast.py b/tests/app/v2/broadcast/test_post_broadcast.py index ddbedaba0..6b3b57b05 100644 --- a/tests/app/v2/broadcast/test_post_broadcast.py +++ b/tests/app/v2/broadcast/test_post_broadcast.py @@ -88,7 +88,7 @@ def test_valid_post_cap_xml_broadcast_returns_201( assert response_json['areas'] == [ 'River Steeping in Wainfleet All Saints' ] - assert response_json['areas_2']['areas'] == [ + assert response_json['areas_2']['names'] == [ 'River Steeping in Wainfleet All Saints' ] assert response_json['cancelled_at'] is None