From 5cf6e1cf72777bbd51bac7b9312840d6306f907f Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 2 Sep 2020 14:55:23 +0100 Subject: [PATCH 1/6] Persist simple polygons in the db. They are being sent over from admin, and persisted in the db so we can send them on to the broadcast provider later on. --- app/broadcast_message/broadcast_message_schema.py | 2 ++ app/broadcast_message/rest.py | 6 +++--- app/models.py | 2 +- tests/app/broadcast_message/test_rest.py | 10 ++++++++-- tests/app/celery/test_broadcast_message_tasks.py | 12 ++++++++++-- tests/app/db.py | 2 +- 6 files changed, 25 insertions(+), 9 deletions(-) diff --git a/app/broadcast_message/broadcast_message_schema.py b/app/broadcast_message/broadcast_message_schema.py index 743ca54d7..3d617533a 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"}}, + 'simple_polygons': {"type": "array", "items": {"type": "array"}}, }, 'required': ['template_id', 'service_id', 'created_by'], 'additionalProperties': False @@ -29,6 +30,7 @@ update_broadcast_message_schema = { 'starts_at': {'type': 'string', 'format': 'datetime'}, 'finishes_at': {'type': 'string', 'format': 'datetime'}, 'areas': {"type": "array", "items": {"type": "string"}}, + 'simple_polygons': {"type": "array", "items": {"type": "array"}}, }, 'required': [], 'additionalProperties': False diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index db7f459c8..1438b29f9 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -101,7 +101,7 @@ def create_broadcast_message(service_id): template_id=template.id, template_version=template.version, personalisation=data.get('personalisation', {}), - areas=data.get('areas', []), + areas=data.get('areas', {}), status=BroadcastStatusType.DRAFT, starts_at=_parse_nullable_datetime(data.get('starts_at')), finishes_at=_parse_nullable_datetime(data.get('finishes_at')), @@ -133,8 +133,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: - broadcast_message.areas = data['areas'] + if 'areas' in data and 'simple_polygons' in data: + broadcast_message.areas = {"areas": data["areas"], "simple_polygons": data["simple_polygons"]} dao_save_object(broadcast_message) diff --git a/app/models.py b/app/models.py index 72e900af4..cde9874a7 100644 --- a/app/models.py +++ b/app/models.py @@ -2263,7 +2263,7 @@ class BroadcastMessage(db.Model): 'template_name': self.template.name, 'personalisation': self.personalisation, - 'areas': self.areas, + 'areas': self.areas.get("areas", []), 'status': self.status, diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index d9c04cc9d..2a17451b2 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -11,7 +11,10 @@ from tests.app.db import create_broadcast_message, create_template, create_servi def test_get_broadcast_message(admin_request, sample_service): t = create_template(sample_service, BROADCAST_TYPE) - bm = create_broadcast_message(t, areas=['place A', 'region B']) + bm = create_broadcast_message(t, areas={ + "areas": ['place A', 'region B'], + "simple_polygons": [[50.1, 1.2], [50.12, 1.2]] + }) response = admin_request.get( 'broadcast_message.get_broadcast_message', @@ -177,7 +180,10 @@ def test_update_broadcast_message_doesnt_allow_edits_after_broadcast_goes_live(a def test_update_broadcast_message_sets_finishes_at_separately(admin_request, sample_service): t = create_template(sample_service, BROADCAST_TYPE) - bm = create_broadcast_message(t, areas=['manchester']) + bm = create_broadcast_message( + t, + areas={"areas": ['london'], "simple_polygons": [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]} + ) response = admin_request.post( 'broadcast_message.update_broadcast_message', diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index b39744e2f..e6c82c35a 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -11,7 +11,11 @@ from tests.app.db import create_template, create_broadcast_message, create_broad def test_send_broadcast_message_sends_data_correctly(sample_service): template = create_template(sample_service, BROADCAST_TYPE) - broadcast_message = create_broadcast_message(template, areas=['london'], status=BroadcastStatusType.BROADCASTING) + broadcast_message = create_broadcast_message( + template, + areas={"areas": ['london'], "simple_polygons": [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]}, + status=BroadcastStatusType.BROADCASTING + ) with requests_mock.Mocker() as request_mock: request_mock.post("http://test-cbc-proxy/broadcasts/stub-1", json={'valid': 'true'}, status_code=200) @@ -29,7 +33,11 @@ def test_send_broadcast_message_sends_data_correctly(sample_service): def test_send_broadcast_message_sends_old_version_of_template(sample_service): template = create_template(sample_service, BROADCAST_TYPE, content='first content') - broadcast_message = create_broadcast_message(template, areas=['london'], status=BroadcastStatusType.BROADCASTING) + broadcast_message = create_broadcast_message( + template, + areas={"areas": ['london'], "simple_polygons": [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]}, + status=BroadcastStatusType.BROADCASTING + ) template.content = 'second content' dao_update_template(template) diff --git a/tests/app/db.py b/tests/app/db.py index 75d4edeaa..89e42ec15 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1006,7 +1006,7 @@ def create_broadcast_message( status=BroadcastStatusType.DRAFT, starts_at=None, finishes_at=None, - areas=[], + areas={}, ): broadcast_message = BroadcastMessage( service_id=template.service_id, From bceb5d8fde72edb560efa9eaac0a3d96fce9c27c Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 2 Sep 2020 17:23:38 +0100 Subject: [PATCH 2/6] Purge broadcast data To avoid problems after changing data type for areas column from array to dictionary. --- .../versions/0329_purge_broadcast_data.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 migrations/versions/0329_purge_broadcast_data.py diff --git a/migrations/versions/0329_purge_broadcast_data.py b/migrations/versions/0329_purge_broadcast_data.py new file mode 100644 index 000000000..b8c698c53 --- /dev/null +++ b/migrations/versions/0329_purge_broadcast_data.py @@ -0,0 +1,24 @@ +""" + +Revision ID: 0329_purge_broadcast_data +Revises: 0328_international_letters_perm +Create Date: 2020-09-07 16:00:27.545673 + +""" +from alembic import op + + +revision = '0329_purge_broadcast_data' +down_revision = '0328_international_letters_perm' + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.execute("TRUNCATE broadcast_event, broadcast_message;") + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + pass + # ### end Alembic commands ### From 2f6b96dafc0782a6528572d2439dc4f101da57f7 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 2 Sep 2020 18:10:56 +0100 Subject: [PATCH 3/6] Ensure broadcast with no areas cannot be approved This is already checked by admin app, but we want to have a fallback safety measure as a precaution. --- app/broadcast_message/rest.py | 5 +++ tests/app/broadcast_message/test_rest.py | 55 +++++++++++++++++++++--- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 1438b29f9..98e831e24 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -59,6 +59,11 @@ def _update_broadcast_message(broadcast_message, new_status, updating_user): f'User {updating_user.id} cannot approve their own broadcast_message {broadcast_message.id}', status_code=400 ) + elif not broadcast_message.areas: + raise InvalidRequest( + f'broadcast_message {broadcast_message.id} has no selected areas and so cannot be broadcasted.', + status_code=400 + ) else: broadcast_message.approved_at = datetime.utcnow() broadcast_message.approved_by = updating_user diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 2a17451b2..f54030152 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -143,11 +143,19 @@ def test_create_broadcast_message_400s_if_json_schema_fails_validation( ]) def test_update_broadcast_message_allows_edit_while_not_yet_live(admin_request, sample_service, status): t = create_template(sample_service, BROADCAST_TYPE) - bm = create_broadcast_message(t, areas=['manchester'], status=status) + bm = create_broadcast_message( + t, + areas={"areas": ['manchester'], "simple_polygons": [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]}, + status=status + ) response = admin_request.post( 'broadcast_message.update_broadcast_message', - _data={'starts_at': '2020-06-01 20:00:01', 'areas': ['london', 'glasgow']}, + _data={ + 'starts_at': '2020-06-01 20:00:01', + 'areas': ['london', 'glasgow'], + "simple_polygons": [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]] + }, service_id=t.service_id, broadcast_message_id=bm.id, _expected_status=200 @@ -310,7 +318,11 @@ def test_update_broadcast_message_status_stores_approved_by_and_approved_at_and_ mocker ): t = create_template(sample_service, BROADCAST_TYPE, content='emergency broadcast') - bm = create_broadcast_message(t, status=BroadcastStatusType.PENDING_APPROVAL) + bm = create_broadcast_message( + t, + status=BroadcastStatusType.PENDING_APPROVAL, + areas={"areas": ["london"], "simple_polygons": [[50.21, 1.12], [50.22, 1.12], [50.23, 1.13]]} + ) approver = create_user(email='approver@gov.uk') sample_service.users.append(approver) mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async') @@ -360,6 +372,31 @@ def test_update_broadcast_message_status_rejects_approval_from_creator( assert f'cannot approve their own broadcast' in response['message'] +def test_update_broadcast_message_status_rejects_approval_of_broadcast_with_no_areas( + admin_request, + sample_service, + mocker +): + template = create_template(sample_service, BROADCAST_TYPE) + broadcast = create_broadcast_message(template, status=BroadcastStatusType.PENDING_APPROVAL) + approver = create_user(email='approver@gov.uk') + sample_service.users.append(approver) + mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async') + + response = admin_request.post( + 'broadcast_message.update_broadcast_message_status', + _data={'status': BroadcastStatusType.BROADCASTING, 'created_by': str(approver.id)}, + service_id=template.service_id, + broadcast_message_id=broadcast.id, + _expected_status=400 + ) + + assert mock_task.called is False + assert response[ + 'message' + ] == f'broadcast_message {broadcast.id} has no selected areas and so cannot be broadcasted.' + + def test_update_broadcast_message_status_allows_platform_admin_to_approve_own_message( notify_db, admin_request, @@ -369,7 +406,11 @@ def test_update_broadcast_message_status_allows_platform_admin_to_approve_own_me user = sample_service.created_by user.platform_admin = True t = create_template(sample_service, BROADCAST_TYPE) - bm = create_broadcast_message(t, status=BroadcastStatusType.PENDING_APPROVAL) + bm = create_broadcast_message( + t, + status=BroadcastStatusType.PENDING_APPROVAL, + areas={"areas": ["london"], "simple_polygons": [[50.21, 1.12], [50.22, 1.12], [50.23, 1.13]]} + ) mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async') response = admin_request.post( @@ -398,7 +439,11 @@ def test_update_broadcast_message_status_allows_trial_mode_services_to_approve_o ): sample_service.restricted = True t = create_template(sample_service, BROADCAST_TYPE) - bm = create_broadcast_message(t, status=BroadcastStatusType.PENDING_APPROVAL) + bm = create_broadcast_message( + t, + status=BroadcastStatusType.PENDING_APPROVAL, + areas={"areas": ["london"], "simple_polygons": [[50.21, 1.12], [50.22, 1.12], [50.23, 1.13]]} + ) mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async') response = admin_request.post( From 5927aae0198af641972cf424ef97e015ac17b502 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 4 Sep 2020 15:41:48 +0100 Subject: [PATCH 4/6] Serialise simple_polygons attribute of BroadcastMessage --- app/models.py | 1 + tests/app/broadcast_message/test_rest.py | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models.py b/app/models.py index cde9874a7..4d98f6545 100644 --- a/app/models.py +++ b/app/models.py @@ -2264,6 +2264,7 @@ class BroadcastMessage(db.Model): 'personalisation': self.personalisation, 'areas': self.areas.get("areas", []), + 'simple_polygons': self.areas.get("simple_polygons", []), 'status': self.status, diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index f54030152..52af219c9 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -154,7 +154,7 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live(admin_request, _data={ 'starts_at': '2020-06-01 20:00:01', 'areas': ['london', 'glasgow'], - "simple_polygons": [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]] + "simple_polygons": [[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]] }, service_id=t.service_id, broadcast_message_id=bm.id, @@ -163,6 +163,7 @@ 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['simple_polygons'] == [[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]] assert response['updated_at'] is not None @@ -321,7 +322,7 @@ 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": [[50.21, 1.12], [50.22, 1.12], [50.23, 1.13]]} + areas={"areas": ["london"], "simple_polygons": [[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]} ) approver = create_user(email='approver@gov.uk') sample_service.users.append(approver) @@ -409,7 +410,7 @@ def test_update_broadcast_message_status_allows_platform_admin_to_approve_own_me bm = create_broadcast_message( t, status=BroadcastStatusType.PENDING_APPROVAL, - areas={"areas": ["london"], "simple_polygons": [[50.21, 1.12], [50.22, 1.12], [50.23, 1.13]]} + areas={"areas": ["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') @@ -442,7 +443,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": [[50.21, 1.12], [50.22, 1.12], [50.23, 1.13]]} + areas={"areas": ["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') From b3ff172780d5135ef5b8ae005c49f27ec957f13b Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 7 Sep 2020 15:10:04 +0100 Subject: [PATCH 5/6] Validate that both polygons and areas have to be updated Because areas and polygons for broadcast messages should be in sync. --- app/broadcast_message/rest.py | 6 +++++ tests/app/broadcast_message/test_rest.py | 28 +++++++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 98e831e24..e0f211462 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -132,6 +132,12 @@ 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): + raise InvalidRequest( + f'Cannot update broadcast_message {broadcast_message.id}, areas or polygons are missing.', + status_code=400 + ) + if 'personalisation' in data: broadcast_message.personalisation = data['personalisation'] if 'starts_at' in data: diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 52af219c9..29cd892af 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -235,7 +235,10 @@ def test_update_broadcast_message_doesnt_let_you_update_status(admin_request, sa response = admin_request.post( 'broadcast_message.update_broadcast_message', - _data={'areas': ['glasgow'], 'status': BroadcastStatusType.BROADCASTING}, + _data={ + 'areas': ['glasgow'], + "simple_polygons": [[55.86, -4.25], [55.85, -4.25], [55.87, -4.24]], + 'status': BroadcastStatusType.BROADCASTING}, service_id=t.service_id, broadcast_message_id=bm.id, _expected_status=400 @@ -247,6 +250,29 @@ 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]]}, +]) +def test_update_broadcast_message_doesnt_let_you_update_areas_but_not_polygons( + admin_request, sample_service, incomplete_area_data +): + template = create_template(sample_service, BROADCAST_TYPE) + broadcast_message = create_broadcast_message(template) + + response = admin_request.post( + 'broadcast_message.update_broadcast_message', + _data=incomplete_area_data, + service_id=template.service_id, + broadcast_message_id=broadcast_message.id, + _expected_status=400 + ) + + assert response[ + 'message' + ] == f'Cannot update broadcast_message {broadcast_message.id}, areas or polygons are missing.' + + def test_update_broadcast_message_status(admin_request, sample_service): t = create_template(sample_service, BROADCAST_TYPE) bm = create_broadcast_message(t, status=BroadcastStatusType.DRAFT) From b86ce6844f6879b6d818d19ebde4e2881e023794 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 8 Sep 2020 11:04:13 +0100 Subject: [PATCH 6/6] Ensure data model the same at creation as when updating broadcast message --- app/broadcast_message/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index e0f211462..b034b08b3 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -106,7 +106,7 @@ def create_broadcast_message(service_id): template_id=template.id, template_version=template.version, personalisation=data.get('personalisation', {}), - areas=data.get('areas', {}), + areas={"areas": data.get("areas", []), "simple_polygons": data.get("simple_polygons", [])}, status=BroadcastStatusType.DRAFT, starts_at=_parse_nullable_datetime(data.get('starts_at')), finishes_at=_parse_nullable_datetime(data.get('finishes_at')),