From bab5c211484c0186a853a94bae10fac92139cf3c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 6 Sep 2021 11:12:06 +0100 Subject: [PATCH 1/5] Rename test to include method under test --- tests/app/models/test_broadcast_message.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/models/test_broadcast_message.py b/tests/app/models/test_broadcast_message.py index 6eacc3f25..136917d0d 100644 --- a/tests/app/models/test_broadcast_message.py +++ b/tests/app/models/test_broadcast_message.py @@ -49,7 +49,7 @@ def test_content_comes_from_attribute_not_template(fake_uuid): assert broadcast_message.content == 'This is a test' -def test_raises_for_missing_areas(fake_uuid): +def test_areas_raises_for_missing_areas(fake_uuid): broadcast_message = BroadcastMessage(broadcast_message_json( id_=fake_uuid, service_id=fake_uuid, From 47132d28d6ffe8207bf68b5cb3ba3884edc1b92d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 6 Sep 2021 11:13:14 +0100 Subject: [PATCH 2/5] Remove redundant arguments for broadcast JSON These are set automatically. --- tests/app/models/test_broadcast_message.py | 24 ++++------------------ 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/tests/app/models/test_broadcast_message.py b/tests/app/models/test_broadcast_message.py index 136917d0d..9b2dd3862 100644 --- a/tests/app/models/test_broadcast_message.py +++ b/tests/app/models/test_broadcast_message.py @@ -4,13 +4,8 @@ from app.models.broadcast_message import BroadcastMessage from tests import broadcast_message_json -def test_simple_polygons(fake_uuid): +def test_simple_polygons(): broadcast_message = BroadcastMessage(broadcast_message_json( - id_=fake_uuid, - service_id=fake_uuid, - template_id=fake_uuid, - status='draft', - created_by_id=fake_uuid, area_ids=[ # Hackney Central 'wd20-E05009372', @@ -38,24 +33,13 @@ def test_simple_polygons(fake_uuid): ] -def test_content_comes_from_attribute_not_template(fake_uuid): - broadcast_message = BroadcastMessage(broadcast_message_json( - id_=fake_uuid, - service_id=fake_uuid, - template_id=fake_uuid, - status='draft', - created_by_id=fake_uuid, - )) +def test_content_comes_from_attribute_not_template(): + broadcast_message = BroadcastMessage(broadcast_message_json()) assert broadcast_message.content == 'This is a test' -def test_areas_raises_for_missing_areas(fake_uuid): +def test_areas_raises_for_missing_areas(): broadcast_message = BroadcastMessage(broadcast_message_json( - id_=fake_uuid, - service_id=fake_uuid, - template_id=fake_uuid, - status='draft', - created_by_id=fake_uuid, area_ids=[ 'wd20-E05009372', 'something else', From 411fda81c0ff2d87c5553b1c5eb50c6023078797 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 6 Sep 2021 10:02:01 +0100 Subject: [PATCH 3/5] Support custom broadcasts (without area IDs) Custom broadcasts created directly via the API app won't have area IDs since [1], where we started to distinguish between "names" (all broadcasts have these) and IDs (for broadcasts created in this app). We forgot to propagate the distinction into the code here. This code fixes the bug for all broadcasts created after [1]. Any custom broadcasts created before [1] will have their "ids" field set instead of "names" so we won't show anything for them. This seems reasonable as we don't support custom broadcasts yet. [1]: https://github.com/alphagov/notifications-api/pull/3312/commits/023a06d5fb4c20e6bf5a1c9e65b473cb376b2b6b --- app/broadcast_areas/models.py | 4 ++-- app/models/broadcast_message.py | 9 ++++----- tests/__init__.py | 3 ++- tests/app/broadcast_areas/test_utils.py | 6 ++++-- tests/app/main/views/test_broadcast.py | 6 ++++-- tests/app/models/test_broadcast_message.py | 16 ++++++++++++++++ 6 files changed, 32 insertions(+), 12 deletions(-) diff --git a/app/broadcast_areas/models.py b/app/broadcast_areas/models.py index 8d364e3c5..ddd163a53 100644 --- a/app/broadcast_areas/models.py +++ b/app/broadcast_areas/models.py @@ -203,8 +203,8 @@ class CustomBroadcastArea(BaseBroadcastArea): class CustomBroadcastAreas(SerialisedModelCollection): model = CustomBroadcastArea - def __init__(self, *, area_ids, polygons): - self.items = area_ids + def __init__(self, *, names, polygons): + self.items = names self._polygons = polygons def __getitem__(self, index): diff --git a/app/models/broadcast_message.py b/app/models/broadcast_message.py index 5e1a5deb4..247edc478 100644 --- a/app/models/broadcast_message.py +++ b/app/models/broadcast_message.py @@ -89,10 +89,9 @@ class BroadcastMessage(JSONModel): @property def areas(self): - polygons = self._dict['areas']['simple_polygons'] - library_areas = self.get_areas(self.area_ids) + if 'ids' in self._dict['areas']: + library_areas = self.get_areas(self.area_ids) - if library_areas: if len(library_areas) != len(self.area_ids): raise RuntimeError( f'BroadcastMessage has {len(self.area_ids)} areas ' @@ -101,8 +100,8 @@ class BroadcastMessage(JSONModel): return library_areas return CustomBroadcastAreas( - area_ids=self.area_ids, - polygons=polygons, + names=self._dict['areas']['names'], + polygons=self._dict['areas']['simple_polygons'], ) @property diff --git a/tests/__init__.py b/tests/__init__.py index 95bd17233..8c9d5ad12 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -678,6 +678,7 @@ def broadcast_message_json( updated_at=None, approved_by_id=None, cancelled_by_id=None, + areas=None, area_ids=None, simple_polygons=None, content=None, @@ -696,7 +697,7 @@ def broadcast_message_json( 'reference': reference, 'personalisation': {}, - 'areas': { + 'areas': areas or { 'ids': area_ids or ['ctry19-E92000001', 'ctry19-S92000003'], 'simple_polygons': simple_polygons or [], }, diff --git a/tests/app/broadcast_areas/test_utils.py b/tests/app/broadcast_areas/test_utils.py index 8ed121995..c372aa2e4 100644 --- a/tests/app/broadcast_areas/test_utils.py +++ b/tests/app/broadcast_areas/test_utils.py @@ -175,8 +175,10 @@ def test_aggregate_areas_for_custom_polygons( ): broadcast_message = BroadcastMessage( broadcast_message_json( - area_ids=['derived from polygons'], - simple_polygons=simple_polygons + areas={ + 'names': [f'polygon {i}' for i, _ in enumerate(simple_polygons)], + 'simple_polygons': simple_polygons + } ) ) diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index e50b0a858..3c13b281c 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -974,8 +974,10 @@ def test_preview_broadcast_areas_page_with_custom_polygons( created_by_id=fake_uuid, service_id=SERVICE_ONE_ID, status='draft', - area_ids=['Area one', 'Area two', 'Area three'], - simple_polygons=polygons, + areas={ + 'names': ['Area one', 'Area two', 'Area three'], + 'simple_polygons': polygons, + } ), ) client_request.login(active_user_create_broadcasts_permission) diff --git a/tests/app/models/test_broadcast_message.py b/tests/app/models/test_broadcast_message.py index 9b2dd3862..423bf2db2 100644 --- a/tests/app/models/test_broadcast_message.py +++ b/tests/app/models/test_broadcast_message.py @@ -38,6 +38,22 @@ def test_content_comes_from_attribute_not_template(): assert broadcast_message.content == 'This is a test' +@pytest.mark.parametrize(('areas', 'expected_length'), [ + ({'ids': []}, 0), + ({'ids': ['wd20-E05009372']}, 1), + ({'names': ['somewhere'], 'simple_polygons': [[[3.5, 1.5]]]}, 1) +]) +def test_areas( + areas, + expected_length +): + broadcast_message = BroadcastMessage(broadcast_message_json( + areas=areas + )) + + assert len(list(broadcast_message.areas)) == expected_length + + def test_areas_raises_for_missing_areas(): broadcast_message = BroadcastMessage(broadcast_message_json( area_ids=[ From baf20e00751f6cfc6b84ce969988a85d04cc3a0d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 6 Sep 2021 11:57:32 +0100 Subject: [PATCH 4/5] Support broadcasts with no areas data Previously we used to return an empty CustomBroadcastAreas object, which doesn't make sense for broadcasts created in this app. --- app/models/broadcast_message.py | 13 +++++++++---- tests/app/models/test_broadcast_message.py | 1 + 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/models/broadcast_message.py b/app/models/broadcast_message.py index 247edc478..3a180dba2 100644 --- a/app/models/broadcast_message.py +++ b/app/models/broadcast_message.py @@ -99,10 +99,15 @@ class BroadcastMessage(JSONModel): ) return library_areas - return CustomBroadcastAreas( - names=self._dict['areas']['names'], - polygons=self._dict['areas']['simple_polygons'], - ) + polygons = self._dict['areas'].get('simple_polygons', []) + + if polygons: + return CustomBroadcastAreas( + names=self._dict['areas']['names'], + polygons=polygons, + ) + + return [] @property def area_ids(self): diff --git a/tests/app/models/test_broadcast_message.py b/tests/app/models/test_broadcast_message.py index 423bf2db2..025cd24f1 100644 --- a/tests/app/models/test_broadcast_message.py +++ b/tests/app/models/test_broadcast_message.py @@ -41,6 +41,7 @@ def test_content_comes_from_attribute_not_template(): @pytest.mark.parametrize(('areas', 'expected_length'), [ ({'ids': []}, 0), ({'ids': ['wd20-E05009372']}, 1), + ({'no data': 'just created'}, 0), ({'names': ['somewhere'], 'simple_polygons': [[[3.5, 1.5]]]}, 1) ]) def test_areas( From cf3f69199a06017bf84788236ab67841a84a2a6f Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 6 Sep 2021 12:34:38 +0100 Subject: [PATCH 5/5] Support new broadcasts (without area IDs) Previously we relied on the API defaulting this field to an empty array [1], but that conflicts with using it to decide whether a broadcast is custom or created in this app. [1]: https://github.com/alphagov/notifications-api/blob/3779146cc51385327cef90390af1f62db983919f/app/models.py#L2342 --- app/models/broadcast_message.py | 2 +- tests/app/models/test_broadcast_message.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/models/broadcast_message.py b/app/models/broadcast_message.py index 3a180dba2..b7e6c218e 100644 --- a/app/models/broadcast_message.py +++ b/app/models/broadcast_message.py @@ -111,7 +111,7 @@ class BroadcastMessage(JSONModel): @property def area_ids(self): - return self._dict['areas']['ids'] + return self._dict['areas'].get('ids', []) @area_ids.setter def area_ids(self, value): diff --git a/tests/app/models/test_broadcast_message.py b/tests/app/models/test_broadcast_message.py index 025cd24f1..4e2705b90 100644 --- a/tests/app/models/test_broadcast_message.py +++ b/tests/app/models/test_broadcast_message.py @@ -4,6 +4,21 @@ from app.models.broadcast_message import BroadcastMessage from tests import broadcast_message_json +@pytest.mark.parametrize('areas, expected_area_ids', [ + ({'simple_polygons': []}, []), + ({'ids': ['123'], 'simple_polygons': []}, ['123']) +]) +def test_area_ids( + areas, + expected_area_ids, +): + broadcast_message = BroadcastMessage(broadcast_message_json( + areas=areas + )) + + assert broadcast_message.area_ids == expected_area_ids + + def test_simple_polygons(): broadcast_message = BroadcastMessage(broadcast_message_json( area_ids=[