From ec1171f85cfac62318de9e69b0ee8a39b9c05949 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 27 Aug 2021 13:22:54 +0100 Subject: [PATCH] Switch "areas" field to "areas_2" format The Admin app is now temporarily using the "areas_2" field while we migrate "areas" to the new format [1]. [1]: https://github.com/alphagov/notifications-admin/pull/4004 --- .../broadcast_message_schema.py | 4 +- app/broadcast_message/rest.py | 31 +------------ app/models.py | 11 ++--- app/v2/broadcast/post_broadcast.py | 3 -- tests/app/broadcast_message/test_rest.py | 46 ++++++++++++------- tests/app/dao/test_broadcast_message_dao.py | 4 +- tests/app/db.py | 2 +- tests/app/v2/broadcast/test_post_broadcast.py | 14 +++--- 8 files changed, 49 insertions(+), 66 deletions(-) diff --git a/app/broadcast_message/broadcast_message_schema.py b/app/broadcast_message/broadcast_message_schema.py index dc7996707..4b67e0ee7 100644 --- a/app/broadcast_message/broadcast_message_schema.py +++ b/app/broadcast_message/broadcast_message_schema.py @@ -13,7 +13,7 @@ create_broadcast_message_schema = { 'personalisation': {'type': 'object'}, 'starts_at': {'type': 'string', 'format': 'datetime'}, 'finishes_at': {'type': 'string', 'format': 'datetime'}, - 'areas': {"type": "array", "items": {"type": "string"}}, + 'areas': {'type': 'object'}, 'areas_2': {'type': 'object'}, 'simple_polygons': {"type": "array", "items": {"type": "array"}}, 'content': {'type': 'string', 'minLength': 1}, @@ -42,7 +42,7 @@ update_broadcast_message_schema = { 'personalisation': {'type': 'object'}, 'starts_at': {'type': 'string', 'format': 'datetime'}, 'finishes_at': {'type': 'string', 'format': 'datetime'}, - 'areas': {"type": "array", "items": {"type": "string"}}, + 'areas': {'type': 'object'}, 'areas_2': {'type': 'object'}, 'simple_polygons': {"type": "array", "items": {"type": "array"}}, }, diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index df814de9a..bf1c30d49 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -133,27 +133,12 @@ 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"] - areas["ids"] = data["areas"] - if "simple_polygons" in data: - areas["simple_polygons"] = data["simple_polygons"] - 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"] - broadcast_message = BroadcastMessage( service_id=service.id, template_id=template_id, template_version=template.version if template else None, personalisation=personalisation, - areas=areas, + areas=data.get("areas", data.get("areas_2", {})), # TEMPORARY: while we repurpose "areas" status=BroadcastStatusType.DRAFT, starts_at=_parse_nullable_datetime(data.get('starts_at')), finishes_at=_parse_nullable_datetime(data.get('finishes_at')), @@ -186,19 +171,7 @@ def update_broadcast_message(service_id, broadcast_message_id): ) # TEMPORARY: while we repurpose "areas" - areas = {} - areas_2 = data.get("areas_2", {}) - - if "areas" in data: - areas["areas"] = data["areas"] - areas["ids"] = data["areas"] - if "simple_polygons" in data: - areas["simple_polygons"] = data["simple_polygons"] - 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"] + areas = data.get("areas", data.get("areas_2", {})) if ('ids' in areas and 'simple_polygons' not in areas) or ('ids' not in areas and 'simple_polygons' in areas): raise InvalidRequest( diff --git a/app/models.py b/app/models.py index 3908a69aa..f24aa23a8 100644 --- a/app/models.py +++ b/app/models.py @@ -2337,9 +2337,9 @@ class BroadcastMessage(db.Model): def serialize(self): # TEMPORARY: while we repurpose "areas" - areas_2 = dict(self.areas) - areas_2["simple_polygons"] = areas_2.get("simple_polygons", []) - areas_2["ids"] = areas_2.pop("areas", areas_2.get("ids", [])) + areas = dict(self.areas) + areas["simple_polygons"] = areas.get("simple_polygons", []) + areas["ids"] = areas.pop("areas", areas.get("ids", [])) return { 'id': str(self.id), @@ -2354,9 +2354,8 @@ class BroadcastMessage(db.Model): 'content': self.content, # TEMPORARY: switch to this so we can repurpose "areas" - 'areas_2': areas_2, - 'areas': areas_2["ids"], - 'simple_polygons': areas_2["simple_polygons"], + 'areas_2': areas, + 'areas': areas, 'status': self.status, diff --git a/app/v2/broadcast/post_broadcast.py b/app/v2/broadcast/post_broadcast.py index afebc0265..ae5f0823d 100644 --- a/app/v2/broadcast/post_broadcast.py +++ b/app/v2/broadcast/post_broadcast.py @@ -67,9 +67,6 @@ def create_broadcast(): content=broadcast_json['content'], reference=broadcast_json['reference'], areas={ - 'areas': [ - area['name'] for area in broadcast_json['areas'] - ], 'names': [ area['name'] for area in broadcast_json['areas'] ], diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index ff08e121d..2715361d9 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -59,8 +59,8 @@ def test_get_broadcast_message( assert response['status'] == BroadcastStatusType.DRAFT assert response['created_at'] is not None assert response['starts_at'] is None - assert response['areas'] == ['place A', 'region B'] - assert response['simple_polygons'] == [[[50.1, 1.2], [50.12, 1.2], [50.13, 1.2]]] + assert response['areas']['ids'] == ['place A', 'region B'] + assert response['areas']['simple_polygons'] == [[[50.1, 1.2], [50.12, 1.2], [50.13, 1.2]]] assert response['areas_2']['ids'] == ['place A', 'region B'] assert response['areas_2']['simple_polygons'] == [[[50.1, 1.2], [50.12, 1.2], [50.13, 1.2]]] assert response['personalisation'] == {'thing': 'test'} @@ -100,8 +100,10 @@ def test_get_broadcast_message_without_template( assert response['status'] == BroadcastStatusType.DRAFT assert response['created_at'] is not None assert response['starts_at'] is None - assert response['areas'] == ['place A', 'region B'] + assert response['areas']['ids'] == ['place A', 'region B'] + assert response['areas']['simple_polygons'] == [[[50.1, 1.2], [50.12, 1.2], [50.13, 1.2]]] assert response['areas_2']['ids'] == ['place A', 'region B'] + assert response['areas_2']['simple_polygons'] == [[[50.1, 1.2], [50.12, 1.2], [50.13, 1.2]]] assert response['personalisation'] is None @@ -174,7 +176,8 @@ def test_create_broadcast_message(admin_request, sample_broadcast_service, train assert response['created_at'] is not None assert response['created_by_id'] == str(t.created_by_id) assert response['personalisation'] == {} - assert response['areas'] == [] + assert response['areas']['ids'] == [] + assert response['areas']['simple_polygons'] == [] assert response['areas_2']['ids'] == [] assert response['areas_2']['simple_polygons'] == [] assert response['content'] == 'Some content\n€ŷŵ~\n\'\'""---' @@ -380,8 +383,10 @@ 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": { + "ids": ["london", "glasgow"], + "simple_polygons": [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] + }, }, { "areas_2": { @@ -404,7 +409,10 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live( t = create_template(sample_broadcast_service, BROADCAST_TYPE) bm = create_broadcast_message( t, - areas={"areas": ['manchester'], "simple_polygons": [[[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]]}, + areas={ + "ids": ['manchester'], + "simple_polygons": [[[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]] + }, status=status ) @@ -420,9 +428,9 @@ 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']['ids'] == ['london', 'glasgow'] + assert response['areas']['simple_polygons'] == [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] 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 @@ -432,7 +440,7 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live( pytest.param(True, marks=pytest.mark.xfail) ]) @pytest.mark.parametrize('area_data', [ # TEMPORARY: while we repurpose "areas" - {'areas': ['london', 'glasgow']}, + {'areas': {'ids': ['london', 'glasgow']}}, {'areas_2': {'ids': ['london', 'glasgow']}}, ]) @pytest.mark.parametrize('status', [ @@ -465,7 +473,10 @@ 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={ + "ids": ["london"], + "simple_polygons": [[[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]] + } ) response = admin_request.post( @@ -505,8 +516,10 @@ def test_update_broadcast_message_allows_sensible_datetime_formats(admin_request @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": { + "ids": ["glasgow"], + "simple_polygons": [[[55.86, -4.25], [55.85, -4.25], [55.87, -4.24]]], + } }, { "areas_2": { @@ -536,8 +549,9 @@ 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": {"ids": ["cardiff"]}}, + {"areas": {"simple_polygons": [[[51.28, -3.11], [51.29, -3.12], [51.27, -3.10]]]}}, + # TEMPORARY: while we repurpose "areas" {"areas_2": {"ids": ["cardiff"]}}, {"areas_2": {"simple_polygons": [[[51.28, -3.11], [51.29, -3.12], [51.27, -3.10]]]}}, ]) @@ -577,7 +591,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': {'ids': ['glasgow']}}, 'areas was unexpected'), ({'areas_2': {'ids': ['glasgow']}}, 'areas_2 was unexpected') ]) def test_update_broadcast_message_status_doesnt_let_you_update_other_things( diff --git a/tests/app/dao/test_broadcast_message_dao.py b/tests/app/dao/test_broadcast_message_dao.py index 144e896a2..fa52cb066 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': [], 'ids': [], 'simple_polygons': []}, 'broadcasting', datetime(2021, 6, 20, 12, 0), + {'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': [], 'ids': [], 'simple_polygons': []}, 'cancelled', datetime(2021, 6, 15, 12, 0), + {'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 58f50824f..a71abc313 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': [], 'ids': [], 'simple_polygons': []}, + areas=areas or {'ids': [], 'simple_polygons': []}, content=content, stubbed=stubbed ) diff --git a/tests/app/v2/broadcast/test_post_broadcast.py b/tests/app/v2/broadcast/test_post_broadcast.py index 6b3b57b05..5f6dd10d1 100644 --- a/tests/app/v2/broadcast/test_post_broadcast.py +++ b/tests/app/v2/broadcast/test_post_broadcast.py @@ -84,10 +84,10 @@ 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'] == [ + assert response_json['areas']['names'] == [ 'River Steeping in Wainfleet All Saints' ] + # TEMPORARY: while we repurpose "areas" assert response_json['areas_2']['names'] == [ 'River Steeping in Wainfleet All Saints' ] @@ -108,11 +108,11 @@ def test_valid_post_cap_xml_broadcast_returns_201( 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']['simple_polygons']) == 1 + assert len(response_json['areas']['simple_polygons'][0]) == 23 + assert response_json['areas']['simple_polygons'][0][0] == [53.10562, 0.244127] + assert response_json['areas']['simple_polygons'][0][-1] == [53.10562, 0.244127] + assert response_json['areas']['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]