From 288e3ae81147ca92ff10a4fc2c0730ff72ec247d Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 10 Sep 2020 14:32:02 +0100 Subject: [PATCH 1/4] Bring in latest version of utils to fix link breaking bug Fixes https://github.com/alphagov/notifications-utils/pull/784 Note, also is a breaking change of the utils, see https://github.com/alphagov/notifications-utils/pull/769 but I don't believe it requires any changes on the API because of this --- requirements-app.txt | 2 +- requirements.txt | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index b95414b1f..58776861b 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -29,7 +29,7 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@40.9.0#egg=notifications-utils==40.9.0 +git+https://github.com/alphagov/notifications-utils.git@41.3.1#egg=notifications-utils==41.3.1 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 diff --git a/requirements.txt b/requirements.txt index ddd014c51..576cf4c87 100644 --- a/requirements.txt +++ b/requirements.txt @@ -31,7 +31,7 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@40.9.0#egg=notifications-utils==40.9.0 +git+https://github.com/alphagov/notifications-utils.git@41.3.1#egg=notifications-utils==41.3.1 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 @@ -42,14 +42,14 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==20.2.0 -awscli==1.18.133 +awscli==1.18.135 bcrypt==3.2.0 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.56 +botocore==1.17.58 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 From 5aee6269e04daa033a79128e24381ae41252c848 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 14 Sep 2020 14:55:01 +0100 Subject: [PATCH 2/4] Fix format of simple_polygons It is not of the form [[lat, long][lat, long]] as this would only hold a single polygon. It instead needs to handle multiple polygons so instead is of the form [[[lat, long][lat, long]]]. --- tests/app/broadcast_message/test_rest.py | 18 +++++++++--------- .../app/celery/test_broadcast_message_tasks.py | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 29cd892af..c5564df17 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -13,7 +13,7 @@ def test_get_broadcast_message(admin_request, sample_service): t = create_template(sample_service, BROADCAST_TYPE) bm = create_broadcast_message(t, areas={ "areas": ['place A', 'region B'], - "simple_polygons": [[50.1, 1.2], [50.12, 1.2]] + "simple_polygons": [[[50.1, 1.2], [50.12, 1.2], [50.13, 1.2]]] }) response = admin_request.get( @@ -145,7 +145,7 @@ def test_update_broadcast_message_allows_edit_while_not_yet_live(admin_request, t = create_template(sample_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={"areas": ['manchester'], "simple_polygons": [[[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]]}, status=status ) @@ -163,7 +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['simple_polygons'] == [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] assert response['updated_at'] is not None @@ -191,7 +191,7 @@ def test_update_broadcast_message_sets_finishes_at_separately(admin_request, sam t = create_template(sample_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( @@ -237,7 +237,7 @@ def test_update_broadcast_message_doesnt_let_you_update_status(admin_request, sa 'broadcast_message.update_broadcast_message', _data={ 'areas': ['glasgow'], - "simple_polygons": [[55.86, -4.25], [55.85, -4.25], [55.87, -4.24]], + "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, @@ -252,7 +252,7 @@ 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]]}, + {"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 @@ -348,7 +348,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": [[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]} + 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) @@ -436,7 +436,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": [[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]} + 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') @@ -469,7 +469,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={"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') diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index e6c82c35a..b9c0642da 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -13,7 +13,7 @@ def test_send_broadcast_message_sends_data_correctly(sample_service): template = create_template(sample_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( template, - 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]]]}, status=BroadcastStatusType.BROADCASTING ) @@ -35,7 +35,7 @@ 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={"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]]]}, status=BroadcastStatusType.BROADCASTING ) From d352c9914256be4ec4b616430f59cf3c0fe3b30f Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 14 Sep 2020 15:16:59 +0100 Subject: [PATCH 3/4] Remove unused send_broadcast_message task We only call send_broadcast_event now --- app/celery/broadcast_message_tasks.py | 30 --------- .../celery/test_broadcast_message_tasks.py | 63 +------------------ 2 files changed, 1 insertion(+), 92 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 97cc11b9a..9539fb7cb 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -7,36 +7,6 @@ from app import notify_celery from app.dao.broadcast_message_dao import dao_get_broadcast_message_by_id, dao_get_broadcast_event_by_id -@notify_celery.task(name="send-broadcast-message") -@statsd(namespace="tasks") -def send_broadcast_message(broadcast_message_id, provider='stub-1'): - # imports of schemas from tasks have to happen within functions to prevent - # `AttributeError: 'DummySession' object has no attribute 'query'` errors in unrelated tests - from app.schemas import template_schema - - broadcast_message = dao_get_broadcast_message_by_id(broadcast_message_id) - - current_app.logger.info( - f'sending broadcast_message {broadcast_message_id} ' - f'status {broadcast_message.status} to {provider}' - ) - - payload = { - "template": template_schema.dump(broadcast_message.template).data, - "broadcast_message": broadcast_message.serialize(), - } - resp = requests.post( - f'{current_app.config["CBC_PROXY_URL"]}/broadcasts/{provider}', - json=payload - ) - resp.raise_for_status() - - current_app.logger.info( - f'broadcast_message {broadcast_message.id} ' - f'status {broadcast_message.status} sent to {provider}' - ) - - @notify_celery.task(name="send-broadcast-event") @statsd(namespace="tasks") def send_broadcast_event(broadcast_event_id, provider='stub-1'): diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index b9c0642da..3436dce9b 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -5,71 +5,10 @@ from requests import RequestException from app.dao.templates_dao import dao_update_template from app.models import BROADCAST_TYPE, BroadcastStatusType, BroadcastEventMessageType -from app.celery.broadcast_message_tasks import send_broadcast_message, send_broadcast_event +from app.celery.broadcast_message_tasks import send_broadcast_event from tests.app.db import create_template, create_broadcast_message, create_broadcast_event -def test_send_broadcast_message_sends_data_correctly(sample_service): - template = create_template(sample_service, BROADCAST_TYPE) - 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) - send_broadcast_message(broadcast_message_id=str(broadcast_message.id)) - - assert request_mock.call_count == 1 - assert request_mock.request_history[0].method == 'POST' - assert request_mock.request_history[0].headers["Content-type"] == "application/json" - - cbc_json = request_mock.request_history[0].json() - assert cbc_json['template']['id'] == str(template.id) - assert cbc_json['template']['template_type'] == BROADCAST_TYPE - assert cbc_json['broadcast_message']['areas'] == ['london'] - - -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={"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) - assert template.version == 2 - - with requests_mock.Mocker() as request_mock: - request_mock.post("http://test-cbc-proxy/broadcasts/stub-1", json={'valid': 'true'}, status_code=200) - send_broadcast_message(broadcast_message_id=str(broadcast_message.id)) - - assert request_mock.call_count == 1 - assert request_mock.request_history[0].method == 'POST' - assert request_mock.request_history[0].headers["Content-type"] == "application/json" - - cbc_json = request_mock.request_history[0].json() - assert cbc_json['template']['id'] == str(template.id) - assert cbc_json['template']['version'] == 1 - assert cbc_json['template']['content'] == 'first content' - - -def test_send_broadcast_message_errors(sample_service): - template = create_template(sample_service, BROADCAST_TYPE) - broadcast_message = create_broadcast_message(template, status=BroadcastStatusType.BROADCASTING) - - with requests_mock.Mocker() as request_mock: - request_mock.post("http://test-cbc-proxy/broadcasts/stub-1", text='503 bad gateway', status_code=503) - # we're not retrying or anything for the moment - but this'll ensure any exception gets logged - with pytest.raises(RequestException) as ex: - send_broadcast_message(broadcast_message_id=str(broadcast_message.id)) - - assert ex.value.response.status_code == 503 - - @freeze_time('2020-08-01 12:00') def test_send_broadcast_event_sends_data_correctly(sample_service): template = create_template(sample_service, BROADCAST_TYPE) From 5b2dee5ddb6b87e8b636d2a4481228703357292a Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 14 Sep 2020 15:21:55 +0100 Subject: [PATCH 4/4] Bump utils to 42.0.0 Requires unit test updating as we now expect broadcast event areas to be a dict containing a list of areas and simple polygons. --- app/celery/broadcast_message_tasks.py | 2 +- requirements-app.txt | 2 +- requirements.txt | 10 +++++----- tests/app/broadcast_message/test_rest.py | 2 +- tests/app/celery/test_broadcast_message_tasks.py | 11 ++++++++--- tests/app/db.py | 4 +++- 6 files changed, 19 insertions(+), 12 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 9539fb7cb..f73705f5a 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -4,7 +4,7 @@ from notifications_utils.statsd_decorators import statsd from app import notify_celery -from app.dao.broadcast_message_dao import dao_get_broadcast_message_by_id, dao_get_broadcast_event_by_id +from app.dao.broadcast_message_dao import dao_get_broadcast_event_by_id @notify_celery.task(name="send-broadcast-event") diff --git a/requirements-app.txt b/requirements-app.txt index 58776861b..16cf4e6f7 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -29,7 +29,7 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@41.3.1#egg=notifications-utils==41.3.1 +git+https://github.com/alphagov/notifications-utils.git@42.0.0#egg=notifications-utils==42.0.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 diff --git a/requirements.txt b/requirements.txt index 576cf4c87..b6efb7e08 100644 --- a/requirements.txt +++ b/requirements.txt @@ -31,25 +31,25 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@41.3.1#egg=notifications-utils==41.3.1 +git+https://github.com/alphagov/notifications-utils.git@42.0.0#egg=notifications-utils==42.0.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 gds-metrics==0.2.4 ## The following requirements were added by pip freeze: -alembic==1.4.2 +alembic==1.4.3 amqp==1.4.9 anyjson==0.3.3 attrs==20.2.0 -awscli==1.18.135 +awscli==1.18.137 bcrypt==3.2.0 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.58 +botocore==1.17.60 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 @@ -76,7 +76,7 @@ phonenumbers==8.11.2 pyasn1==0.4.8 pycparser==2.20 PyPDF2==1.26.0 -pyrsistent==0.16.0 +pyrsistent==0.17.3 python-dateutil==2.8.1 python-editor==1.0.4 python-json-logger==0.1.11 diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index c5564df17..ef3e6f031 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": [[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]] + "simple_polygons": [[[51.12, 0.2], [50.13, 0.4], [50.14, 0.45]]] }, service_id=t.service_id, broadcast_message_id=bm.id, diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 3436dce9b..76d5e2001 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -3,7 +3,6 @@ import pytest import requests_mock from requests import RequestException -from app.dao.templates_dao import dao_update_template from app.models import BROADCAST_TYPE, BroadcastStatusType, BroadcastEventMessageType from app.celery.broadcast_message_tasks import send_broadcast_event from tests.app.db import create_template, create_broadcast_message, create_broadcast_event @@ -12,7 +11,11 @@ from tests.app.db import create_template, create_broadcast_message, create_broad @freeze_time('2020-08-01 12:00') def test_send_broadcast_event_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 + ) event = create_broadcast_event(broadcast_message) with requests_mock.Mocker() as request_mock: @@ -28,7 +31,9 @@ def test_send_broadcast_event_sends_data_correctly(sample_service): assert cbc_json['broadcast_message_id'] == str(broadcast_message.id) assert cbc_json['sent_at'] == '2020-08-01T12:00:00.000000Z' assert cbc_json['transmitted_starts_at'] is None - assert cbc_json['transmitted_areas'] == ['london'] + assert cbc_json['transmitted_areas'] == { + "areas": ['london'], "simple_polygons": [[[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]] + } def test_send_broadcast_event_sends_references(sample_service): diff --git a/tests/app/db.py b/tests/app/db.py index 89e42ec15..4fe5018dd 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1040,7 +1040,9 @@ def create_broadcast_event( sent_at=sent_at or datetime.utcnow(), message_type=message_type, transmitted_content=transmitted_content or {'body': 'this is an emergency broadcast message'}, - transmitted_areas=transmitted_areas or ['london'], + transmitted_areas=transmitted_areas or { + 'areas': ['london'], 'simple_polygons': [[[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]]] + }, transmitted_sender=transmitted_sender or 'www.notifications.service.gov.uk', transmitted_starts_at=transmitted_starts_at, transmitted_finishes_at=transmitted_finishes_at or datetime.utcnow(),