From 194f54c38fc13f23e182e31b301452551f9bda59 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 27 Aug 2021 13:07:22 +0100 Subject: [PATCH 1/4] Add missing tests for old format areas This was (sort of) missed in [1], but it hasn't caused a problem because the code to create/update broadcasts will populate areas in the old ("areas") and the new ("ids") formats anyway [2]. However, we're about to remove create/update support code, so we need to have something in place to cope with old and new format data until we backfill old broadcasts with the new format [3]. [1]: https://github.com/alphagov/notifications-api/pull/3312 [2]: https://github.com/alphagov/notifications-api/pull/3312/files#diff-6be38b59e387630a0c6b8fc60312b7ba53ba9f36c54594fa5690646f286dd2b9R141 [3]: https://github.com/alphagov/notifications-admin/pull/4004/commits/9667433b7e2e23540eaeb25edf1a558955e97d7e --- app/models.py | 6 ++--- tests/app/broadcast_message/test_rest.py | 28 ++++++++++++++++++++---- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/app/models.py b/app/models.py index 6895cd029..3908a69aa 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["ids"] = areas_2.pop("areas", []) + areas_2["ids"] = areas_2.pop("areas", areas_2.get("ids", [])) return { 'id': str(self.id), @@ -2355,8 +2355,8 @@ class BroadcastMessage(db.Model): # 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", []), + 'areas': areas_2["ids"], + 'simple_polygons': areas_2["simple_polygons"], 'status': self.status, diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index d2f50776e..cfe6bc3ff 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -19,7 +19,16 @@ from tests.app.db import ( ) -def test_get_broadcast_message(admin_request, sample_broadcast_service): +# TEMPORARY: while we repurpose "areas" +@pytest.mark.parametrize("area_data", [ + {"areas": ["place A", "region B"]}, + {"ids": ["place A", "region B"]}, +]) +def test_get_broadcast_message( + area_data, + admin_request, + sample_broadcast_service +): t = create_template( sample_broadcast_service, BROADCAST_TYPE, @@ -28,7 +37,7 @@ def test_get_broadcast_message(admin_request, sample_broadcast_service): bm = create_broadcast_message( t, areas={ - "areas": ['place A', 'region B'], + **area_data, "simple_polygons": [[[50.1, 1.2], [50.12, 1.2], [50.13, 1.2]]], }, personalisation={ @@ -51,16 +60,27 @@ 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['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'} -def test_get_broadcast_message_without_template(admin_request, sample_broadcast_service): +# TEMPORARY: while we repurpose "areas" +@pytest.mark.parametrize("area_data", [ + {"areas": ["place A", "region B"]}, + {"ids": ["place A", "region B"]}, +]) +def test_get_broadcast_message_without_template( + area_data, + admin_request, + sample_broadcast_service +): bm = create_broadcast_message( service=sample_broadcast_service, content='emergency broadcast content', areas={ - "areas": ['place A', 'region B'], + **area_data, "simple_polygons": [[[50.1, 1.2], [50.12, 1.2], [50.13, 1.2]]], }, ) From a7d92b9058bc888282232408e16dfdbd33a23494 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 27 Aug 2021 13:13:04 +0100 Subject: [PATCH 2/4] Replace / remove redundant uses of "areas" In one case ("areas=['manchester']") the format was even invalid, but in general the original value of the column is pretty much irrelevant for tests that involve updating it (it's highly unlikely the column would default to the same value as the test data). --- app/broadcast_message/rest.py | 2 +- tests/app/broadcast_message/test_rest.py | 8 +++----- tests/app/celery/test_broadcast_message_tasks.py | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 02b734ec2..df814de9a 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -212,7 +212,7 @@ 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 areas and 'simple_polygons' in areas: + if 'ids' in areas and 'simple_polygons' in areas: broadcast_message.areas = areas dao_save_object(broadcast_message) diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index cfe6bc3ff..ff08e121d 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -449,7 +449,7 @@ def test_update_broadcast_message_doesnt_allow_edits_after_broadcast_goes_live( status ): t = create_template(sample_broadcast_service, BROADCAST_TYPE) - bm = create_broadcast_message(t, areas=['manchester'], status=status) + bm = create_broadcast_message(t, status=status) response = admin_request.post( 'broadcast_message.update_broadcast_message', @@ -658,7 +658,6 @@ def test_update_broadcast_message_status_stores_approved_by_and_approved_at_and_ t, status=BroadcastStatusType.PENDING_APPROVAL, areas={ - "areas": ["london"], "ids": ["london"], "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]] } @@ -708,7 +707,7 @@ def test_update_broadcast_message_status_updates_details_but_does_not_queue_task 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={"ids": ["london"], "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]}, stubbed=broadcast_message_stubbed ) approver = create_user(email='approver@gov.uk') @@ -761,7 +760,6 @@ def test_update_broadcast_message_status_creates_event_with_correct_content_if_b content='tailor made emergency broadcast content', status=BroadcastStatusType.PENDING_APPROVAL, areas={ - "areas": ["london"], "ids": ["london"], "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]] } @@ -849,7 +847,7 @@ def test_update_broadcast_message_status_allows_trial_mode_services_to_approve_o 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={"ids": ["london"], "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]} ) mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async') diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index fce2d2e91..e10f6cbf8 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -100,7 +100,7 @@ def test_send_broadcast_event_creates_zendesk_p1(mocker, notify_api, sample_broa broadcast_message = create_broadcast_message( template, status=BroadcastStatusType.BROADCASTING, - areas={'areas': ['wd20-S13002775', 'wd20-S13002773'], 'simple_polygons': []}, + areas={'ids': ['wd20-S13002775', 'wd20-S13002773'], 'simple_polygons': []}, ) event = create_broadcast_event(broadcast_message) mock_create_ticket = mocker.patch("app.celery.broadcast_message_tasks.zendesk_client.create_ticket") From ec1171f85cfac62318de9e69b0ee8a39b9c05949 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 27 Aug 2021 13:22:54 +0100 Subject: [PATCH 3/4] 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] From bf0bf4e31cb909623341939c00ddb1742c7a96cb Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 27 Aug 2021 14:11:59 +0100 Subject: [PATCH 4/4] Favour new "areas" format for PagerDuty alerts Broadcasts created via the API [1] and the Admin app [2] should both now have this field set. It's also more informative to show this, and broadcasts created via the API don't have IDs anyway. There's a small risk that an old broadcast that gets approved won't have this data, but it's for information only and we intend to backfill all old broadcasts in the near future. [1]: https://github.com/alphagov/notifications-api/pull/3312/commits/023a06d5fb4c20e6bf5a1c9e65b473cb376b2b6b [2]: https://github.com/alphagov/notifications-admin/pull/4004/commits/7dbe3afa19e29ff793213ced0d1360d0e677f70c --- app/celery/broadcast_message_tasks.py | 2 +- tests/app/celery/test_broadcast_message_tasks.py | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 6a0b06886..641e63f21 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("ids", broadcast_message.areas.get("areas"))}.', # noqa + f'This broadcast is targeted at areas {broadcast_message.areas.get("names", [])}.', # noqa '' f'This broadcast\'s content starts "{broadcast_message.content[:100]}"' '', diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index e10f6cbf8..5411419a5 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -95,12 +95,22 @@ def test_send_broadcast_event_does_nothing_if_provider_set_on_service_isnt_enabl assert mock_send_broadcast_provider_message.apply_async.called is False -def test_send_broadcast_event_creates_zendesk_p1(mocker, notify_api, sample_broadcast_service): +@pytest.mark.parametrize('area_data,expected_message', [ + ({'names': ['England', 'Scotland']}, ['England', 'Scotland']), + ({}, []) +]) +def test_send_broadcast_event_creates_zendesk_p1( + area_data, + expected_message, + mocker, + notify_api, + sample_broadcast_service +): template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( template, status=BroadcastStatusType.BROADCASTING, - areas={'ids': ['wd20-S13002775', 'wd20-S13002773'], 'simple_polygons': []}, + areas={**area_data, 'simple_polygons': []}, ) event = create_broadcast_event(broadcast_message) mock_create_ticket = mocker.patch("app.celery.broadcast_message_tasks.zendesk_client.create_ticket") @@ -117,7 +127,7 @@ def test_send_broadcast_event_creates_zendesk_p1(mocker, notify_api, sample_broa assert str(broadcast_message.id) in zendesk_args['message'] assert 'channel severe' in zendesk_args['message'] - assert "areas ['wd20-S13002775', 'wd20-S13002773']" in zendesk_args['message'] + assert f"areas {expected_message}" in zendesk_args['message'] # the start of the content from the broadcast template assert "Dear Sir/Madam" in zendesk_args['message']