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/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/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/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 diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 8b859c10c..28b39a270 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', @@ -208,49 +203,8 @@ 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_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 +241,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 +253,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 +286,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 +297,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 +338,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 +349,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 +397,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 +439,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 +457,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 +490,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..d51a9effb 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -282,45 +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' - - -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 + assert json_resp['data']['allowed_broadcast_provider'] == allowed_broadcast_provider + assert json_resp['data']['broadcast_channel'] == broadcast_channel @pytest.mark.parametrize('detailed', [True, False])