From ce2f55038711a234ad236ab24f4bb1d84ac1a9d0 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 18 Feb 2021 18:20:05 +0000 Subject: [PATCH 1/3] Backfill services_broadcast_settings table At the moment, we currently have broadcast services that have the 'broadcast' service permission in the DB but don't have a corresponding row in the service_broadcast_settings table. It's important to give them a row in this table because this will control which broadcast channel their messages will go out on. All broadcast services will be expected to have this row so we can then remove hardcoded defaults from our code that currently set what channel a message should go out on. Whilst, when this gets deployed, we will have released https://github.com/alphagov/notifications-admin/pull/3794, which means that when a service setting is changed via that form, they will get the corresponding row in the service_broadcast_settings form, we don't want to ask every developer to go and fill in this form for every service on their local dev environment and also do the same to every service we have in preview, staging and production. Therefore a migration is the best way to backfill the data. Note, I made the decision that a service that is in trial mode will be given the 'severe' channel. This is because this is what most trial mode services should have. Only the MNOs would ever really have a trial mode service on the test channel. And given they are in trial mode, it is not too risky to give them the 'severe' channel. For services that are live though, although there are no services in production that have the broadcast permission and are live, it is not worth taking that risk and accidently setting that service to send an alert out on the 'severe' channel. We play it say by choosing 'test'. --- ...ast_settings_migrate_broadcast_settings.py | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 migrations/versions/0348_migrate_broadcast_settings_migrate_broadcast_settings.py diff --git a/migrations/versions/0348_migrate_broadcast_settings_migrate_broadcast_settings.py b/migrations/versions/0348_migrate_broadcast_settings_migrate_broadcast_settings.py new file mode 100644 index 000000000..90577f58b --- /dev/null +++ b/migrations/versions/0348_migrate_broadcast_settings_migrate_broadcast_settings.py @@ -0,0 +1,48 @@ +""" + +Revision ID: 0348_migrate_broadcast_settings +Revises: 0347_add_dvla_volumes_template +Create Date: 2021-02-18 15:25:30.667098 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0348_migrate_broadcast_settings' +down_revision = '0347_add_dvla_volumes_template' + + +def upgrade(): + # For every service that has the broadcast permission we want it to have + # a row in the broadcast_service_settings table + # + # If it doesnt have a row already, then: + # - if the service is in trial mode, add a row and set the channel as 'severe' + # - if the service is in live mode, add a row and set the channel as 'test' + # + # If it does have a row already no action needed + conn = op.get_bind() + + find_services_sql = """ + SELECT services.id, services.restricted + FROM services + LEFT JOIN service_permissions + ON services.id = service_permissions.service_id + WHERE service_permissions.permission = 'broadcast' + """ + + services = conn.execute(find_services_sql) + for service in services: + setting = conn.execute(f"SELECT service_id, channel, provider FROM service_broadcast_settings WHERE service_id = '{service.id}';").first() + if setting: + print(f"Service {service.id} already has service_broadcast_settings. No action required") + else: + channel = "severe" if service.restricted else "test" + print(f"Service {service.id} does not have service_broadcast_settings. Will insert one with channel {channel}") + conn.execute(f"INSERT INTO service_broadcast_settings (service_id, channel, created_at) VALUES ('{service.id}', '{channel}', now());") + + +def downgrade(): + # No downgrade as we do not know what the state of the table was before that it should return to + pass From 9b21e6b04f94813984c6f1a049159799f22b728f Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 19 Feb 2021 11:33:05 +0000 Subject: [PATCH 2/3] Use sample_broadcast_service fixture Now that every service has a row in the service_broadcast_settings table, we want all our tests to use the `sample_broadcast_service` fixture as this ensures it has a row in that table and is correctly representitive of what a real broadcast service looks like. --- app/models.py | 11 +-- .../celery/test_broadcast_message_tasks.py | 69 +++++++++---------- tests/app/service/test_rest.py | 25 ++++--- 3 files changed, 45 insertions(+), 60 deletions(-) diff --git a/app/models.py b/app/models.py index 226715fb4..b2f55eec3 100644 --- a/app/models.py +++ b/app/models.py @@ -2533,16 +2533,7 @@ class BroadcastProviderMessageNumber(db.Model): class ServiceBroadcastSettings(db.Model): """ - For the moment, broadcasts services CAN have a row in this table which will configure which broadcast - channel they will send to. If they don't then we will assume they should send to the test channel. - - There should only be one row per service in this table, and this is enforced by - the service_id being a primary key. - - TODO: We should enforce that every broadcast service will have a row in this table. We will need to do - this when the admin turns a service into a broadcast service, it inserts a row into this table and adds - the service permission for broadcasts for the service. Once that is up and running, we then should write - a DB migration to create rows for all broadcast services that do not have one yet in this table. + Every broadcast service should have one and only one row in this table. """ __tablename__ = "service_broadcast_settings" diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 8b859c10c..5d7cf0f39 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -10,8 +10,7 @@ from app.models import ( BROADCAST_TYPE, BroadcastStatusType, BroadcastEventMessageType, - BroadcastProviderMessageStatus, - ServiceBroadcastSettings, + BroadcastProviderMessageStatus ) from app.clients.cbc_proxy import CBCProxyRetryableException, CBCProxyFatalException from app.celery.broadcast_message_tasks import ( @@ -31,8 +30,8 @@ from tests.app.db import ( from tests.conftest import set_config -def test_send_broadcast_event_queues_up_for_active_providers(mocker, notify_api, sample_service): - template = create_template(sample_service, BROADCAST_TYPE) +def test_send_broadcast_event_queues_up_for_active_providers(mocker, notify_api, sample_broadcast_service): + template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message(template, status=BroadcastStatusType.BROADCASTING) event = create_broadcast_event(broadcast_message) @@ -53,12 +52,10 @@ def test_send_broadcast_event_only_sends_to_one_provider_if_set_on_service( mocker, notify_db, notify_api, - sample_service + sample_broadcast_service ): - settings = ServiceBroadcastSettings(service=sample_service, channel="test", provider="vodafone") - notify_db.session.add(settings) - - template = create_template(sample_service, BROADCAST_TYPE) + sample_broadcast_service.allowed_broadcast_provider = "vodafone" + template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message(template, status=BroadcastStatusType.BROADCASTING) event = create_broadcast_event(broadcast_message) @@ -78,12 +75,10 @@ def test_send_broadcast_event_does_nothing_if_provider_set_on_service_isnt_enabl mocker, notify_db, notify_api, - sample_service + sample_broadcast_service ): - settings = ServiceBroadcastSettings(service=sample_service, channel="test", provider="three") - notify_db.session.add(settings) - - template = create_template(sample_service, BROADCAST_TYPE) + sample_broadcast_service.allowed_broadcast_provider = "three" + template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message(template, status=BroadcastStatusType.BROADCASTING) event = create_broadcast_event(broadcast_message) @@ -117,9 +112,9 @@ def test_send_broadcast_event_does_nothing_if_cbc_proxy_disabled(mocker, notify_ ['vodafone', 'Vodafone'], ]) def test_send_broadcast_provider_message_sends_data_correctly( - mocker, sample_service, provider, provider_capitalised + mocker, sample_broadcast_service, provider, provider_capitalised ): - template = create_template(sample_service, BROADCAST_TYPE) + template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( template, areas={ @@ -160,7 +155,7 @@ def test_send_broadcast_provider_message_sends_data_correctly( }], sent=event.sent_at_as_cap_datetime_string, expires=event.transmitted_finishes_at_as_cap_datetime_string, - channel="test", + channel="severe", ) @@ -173,9 +168,10 @@ def test_send_broadcast_provider_message_sends_data_correctly( ]) @pytest.mark.parametrize('channel', ['test', 'severe']) def test_send_broadcast_provider_message_uses_channel_set_on_broadcast_service( - notify_db, mocker, sample_service, provider, provider_capitalised, channel + notify_db, mocker, sample_broadcast_service, provider, provider_capitalised, channel ): - template = create_template(sample_service, BROADCAST_TYPE) + sample_broadcast_service.broadcast_channel = channel + template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( template, areas={ @@ -188,7 +184,6 @@ def test_send_broadcast_provider_message_uses_channel_set_on_broadcast_service( status=BroadcastStatusType.BROADCASTING ) event = create_broadcast_event(broadcast_message) - notify_db.session.add(ServiceBroadcastSettings(service=sample_service, channel=channel)) mock_create_broadcast = mocker.patch( f'app.clients.cbc_proxy.CBCProxy{provider_capitalised}.create_and_send_broadcast', @@ -249,8 +244,8 @@ def test_send_broadcast_provider_message_defaults_to_test_channel_if_no_service_ ) -def test_send_broadcast_provider_message_works_if_we_retried_previously(mocker, sample_service): - template = create_template(sample_service, BROADCAST_TYPE) +def test_send_broadcast_provider_message_works_if_we_retried_previously(mocker, sample_broadcast_service): + template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( template, areas={'areas': [], 'simple_polygons': [], }, @@ -287,7 +282,7 @@ def test_send_broadcast_provider_message_works_if_we_retried_previously(mocker, areas=[], sent=event.sent_at_as_cap_datetime_string, expires=event.transmitted_finishes_at_as_cap_datetime_string, - channel='test', + channel='severe', ) @@ -299,10 +294,10 @@ def test_send_broadcast_provider_message_works_if_we_retried_previously(mocker, ['vodafone', 'Vodafone'], ]) def test_send_broadcast_provider_message_sends_data_correctly_when_broadcast_message_has_no_template( - mocker, sample_service, provider, provider_capitalised + mocker, sample_broadcast_service, provider, provider_capitalised ): broadcast_message = create_broadcast_message( - service=sample_service, + service=sample_broadcast_service, template=None, content='this is an emergency broadcast message', areas={ @@ -332,7 +327,7 @@ def test_send_broadcast_provider_message_sends_data_correctly_when_broadcast_mes areas=mocker.ANY, sent=mocker.ANY, expires=mocker.ANY, - channel="test" + channel="severe" ) @@ -343,9 +338,9 @@ def test_send_broadcast_provider_message_sends_data_correctly_when_broadcast_mes ['vodafone', 'Vodafone'], ]) def test_send_broadcast_provider_message_sends_update_with_references( - mocker, sample_service, provider, provider_capitalised + mocker, sample_broadcast_service, provider, provider_capitalised ): - template = create_template(sample_service, BROADCAST_TYPE, content='content') + template = create_template(sample_broadcast_service, BROADCAST_TYPE, content='content') broadcast_message = create_broadcast_message( template, @@ -384,7 +379,7 @@ def test_send_broadcast_provider_message_sends_update_with_references( ], sent=update_event.sent_at_as_cap_datetime_string, expires=update_event.transmitted_finishes_at_as_cap_datetime_string, - channel="test" + channel="severe" ) @@ -395,9 +390,9 @@ def test_send_broadcast_provider_message_sends_update_with_references( ['vodafone', 'Vodafone'], ]) def test_send_broadcast_provider_message_sends_cancel_with_references( - mocker, sample_service, provider, provider_capitalised + mocker, sample_broadcast_service, provider, provider_capitalised ): - template = create_template(sample_service, BROADCAST_TYPE, content='content') + template = create_template(sample_broadcast_service, BROADCAST_TYPE, content='content') broadcast_message = create_broadcast_message( template, @@ -443,8 +438,8 @@ def test_send_broadcast_provider_message_sends_cancel_with_references( ['o2', 'O2'], ['vodafone', 'Vodafone'], ]) -def test_send_broadcast_provider_message_errors(mocker, sample_service, provider, provider_capitalised): - template = create_template(sample_service, BROADCAST_TYPE) +def test_send_broadcast_provider_message_errors(mocker, sample_broadcast_service, provider, provider_capitalised): + template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( template, @@ -485,7 +480,7 @@ def test_send_broadcast_provider_message_errors(mocker, sample_service, provider }], sent=event.sent_at_as_cap_datetime_string, expires=event.transmitted_finishes_at_as_cap_datetime_string, - channel="test" + channel="severe" ) mock_retry.assert_called_once_with( countdown=1, @@ -503,11 +498,11 @@ def test_send_broadcast_provider_message_errors(mocker, sample_service, provider ]) def test_send_broadcast_provider_message_delays_retry_exponentially( mocker, - sample_service, + sample_broadcast_service, num_retries, expected_countdown ): - template = create_template(sample_service, BROADCAST_TYPE) + template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message(template, status=BroadcastStatusType.BROADCASTING) event = create_broadcast_event(broadcast_message) @@ -536,7 +531,7 @@ def test_send_broadcast_provider_message_delays_retry_exponentially( areas=[], sent=event.sent_at_as_cap_datetime_string, expires=event.transmitted_finishes_at_as_cap_datetime_string, - channel='test', + channel='severe', ) mock_retry.assert_called_once_with( countdown=expected_countdown, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 66712b2c6..f64dd4613 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -282,23 +282,22 @@ def test_get_service_by_id(admin_request, sample_service): } -def test_get_service_by_id_returns_allowed_broadcast_provider(notify_db, admin_request, sample_service): - settings = ServiceBroadcastSettings(service=sample_service, channel="severe", provider="ee") - notify_db.session.add(settings) - - json_resp = admin_request.get('service.get_service_by_id', service_id=sample_service.id) - assert json_resp['data']['id'] == str(sample_service.id) - assert json_resp['data']['allowed_broadcast_provider'] == 'ee' - - -def test_get_service_by_id_for_broadcast_service_takes_channel_from_service_broadcast_settings( - admin_request, sample_broadcast_service +@pytest.mark.parametrize('broadcast_channel,allowed_broadcast_provider', ( + ('test', None), + ('severe', None), + ('test', 'ee'), + ('severe', 'three'), +)) +def test_get_service_by_id_for_broadcast_service_returns_broadcast_keys( + notify_db, admin_request, sample_broadcast_service, broadcast_channel, allowed_broadcast_provider ): - assert sample_broadcast_service.broadcast_channel == 'severe' + sample_broadcast_service.broadcast_channel = broadcast_channel + sample_broadcast_service.allowed_broadcast_provider = allowed_broadcast_provider json_resp = admin_request.get('service.get_service_by_id', service_id=sample_broadcast_service.id) assert json_resp['data']['id'] == str(sample_broadcast_service.id) - assert json_resp['data']['broadcast_channel'] == 'severe' + assert json_resp['data']['allowed_broadcast_provider'] == allowed_broadcast_provider + assert json_resp['data']['broadcast_channel'] == broadcast_channel def test_get_service_by_id_for_service_with_broadcast_permission_sets_channel_as_test_if_no_service_broadcast_settings( From 3ea86bfb484dc811839c93d3e2693693592433b3 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 19 Feb 2021 11:38:10 +0000 Subject: [PATCH 3/3] Remove hardcoded default to use test channel There is no need for a default now as every broadcast service has set on it which broadcast channel to use. --- app/celery/broadcast_message_tasks.py | 8 +--- app/schemas.py | 11 +---- .../celery/test_broadcast_message_tasks.py | 41 ------------------- tests/app/service/test_rest.py | 22 ---------- 4 files changed, 4 insertions(+), 78 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 0713ec381..adce949d3 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -140,10 +140,6 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): for polygon in broadcast_event.transmitted_areas["simple_polygons"] ] - channel = "test" - if broadcast_event.service.broadcast_channel: - channel = broadcast_event.service.broadcast_channel - cbc_proxy_provider_client = cbc_proxy_client.get_proxy(provider) try: @@ -156,7 +152,7 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): areas=areas, sent=broadcast_event.sent_at_as_cap_datetime_string, expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string, - channel=channel + channel=broadcast_event.service.broadcast_channel ) elif broadcast_event.message_type == BroadcastEventMessageType.UPDATE: cbc_proxy_provider_client.update_and_send_broadcast( @@ -174,7 +170,7 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): # but we are relying on service channels changing almost never, and not mid incident # We may consider in the future, changing this such that we store the channel a broadcast was # sent on on the broadcast message itself and pick the value from there instead of the service - channel=channel + channel=broadcast_event.service.broadcast_channel ) elif broadcast_event.message_type == BroadcastEventMessageType.CANCEL: cbc_proxy_provider_client.cancel_broadcast( diff --git a/app/schemas.py b/app/schemas.py index f8b510f7b..41d487608 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -25,7 +25,7 @@ from notifications_utils.recipients import ( from app import ma from app import models -from app.models import ServicePermission, BROADCAST_TYPE +from app.models import ServicePermission from app.dao.permissions_dao import permission_dao from app.utils import DATETIME_FORMAT_NO_TIMEZONE, get_template_instance @@ -242,14 +242,7 @@ class ServiceSchema(BaseSchema, UUIDsAsStringsMixin): return service.allowed_broadcast_provider def _get_broadcast_channel(self, service): - # TODO: Once we've migrated data so that all broadcast services have `service.broadcast_channel` - # set then we can remove this logic and related tests and instead just return - # `service.broadcast_channel`. For the moment though, as we have some services with the broadcast - # permission that do not have a row in the service_broadcast_settings table, we need to hardcode - # this in here to give them a default that the admin app can use - if BROADCAST_TYPE in self.service_permissions(service): - return service.broadcast_channel if service.broadcast_channel else "test" - return None + return service.broadcast_channel def get_letter_logo_filename(self, service): return service.letter_branding and service.letter_branding.filename diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 5d7cf0f39..28b39a270 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -203,47 +203,6 @@ def test_send_broadcast_provider_message_uses_channel_set_on_broadcast_service( ) -@freeze_time('2020-08-01 12:00') -@pytest.mark.parametrize('provider,provider_capitalised', [ - ['ee', 'EE'], - ['three', 'Three'], - ['o2', 'O2'], - ['vodafone', 'Vodafone'], -]) -def test_send_broadcast_provider_message_defaults_to_test_channel_if_no_service_broadcast_settings( - notify_db, mocker, sample_service, provider, provider_capitalised -): - template = create_template(sample_service, BROADCAST_TYPE) - broadcast_message = create_broadcast_message( - template, - areas={ - 'areas': ['london', 'glasgow'], - 'simple_polygons': [ - [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]], - [[-4.53, 55.72], [-3.88, 55.72], [-3.88, 55.96], [-4.53, 55.96]], - ], - }, - status=BroadcastStatusType.BROADCASTING - ) - event = create_broadcast_event(broadcast_message) - mock_create_broadcast = mocker.patch( - f'app.clients.cbc_proxy.CBCProxy{provider_capitalised}.create_and_send_broadcast', - ) - - send_broadcast_provider_message(provider=provider, broadcast_event_id=str(event.id)) - - mock_create_broadcast.assert_called_once_with( - identifier=mocker.ANY, - message_number=mocker.ANY, - headline='GOV.UK Notify Broadcast', - description='this is an emergency broadcast message', - areas=mocker.ANY, - sent=mocker.ANY, - expires=mocker.ANY, - channel="test", - ) - - def test_send_broadcast_provider_message_works_if_we_retried_previously(mocker, sample_broadcast_service): template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f64dd4613..d51a9effb 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -300,28 +300,6 @@ def test_get_service_by_id_for_broadcast_service_returns_broadcast_keys( assert json_resp['data']['broadcast_channel'] == broadcast_channel -def test_get_service_by_id_for_service_with_broadcast_permission_sets_channel_as_test_if_no_service_broadcast_settings( - admin_request, notify_db_session -): - service = create_service(service_permissions=[BROADCAST_TYPE]) - assert BROADCAST_TYPE in [p.permission for p in service.permissions] - assert service.broadcast_channel is None - - json_resp = admin_request.get('service.get_service_by_id', service_id=service.id) - assert json_resp['data']['id'] == str(service.id) - assert json_resp['data']['broadcast_channel'] == 'test' - - -def test_get_service_by_id_for_non_broadcast_service_sets_channel_as_none( - admin_request, sample_service -): - assert BROADCAST_TYPE not in [p.permission for p in sample_service.permissions] - - json_resp = admin_request.get('service.get_service_by_id', service_id=sample_service.id) - assert json_resp['data']['id'] == str(sample_service.id) - assert json_resp['data']['broadcast_channel'] is None - - @pytest.mark.parametrize('detailed', [True, False]) def test_get_service_by_id_returns_organisation_type(admin_request, sample_service, detailed): json_resp = admin_request.get('service.get_service_by_id', service_id=sample_service.id, detailed=detailed)