From fd7ebbebb053318a0e1bb89b2445fe385f82583c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 25 Aug 2021 13:49:18 +0100 Subject: [PATCH] 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