From 0bb671df456f2a6a37ffdd97eb024690d7bc112d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 12 Feb 2021 17:36:25 +0000 Subject: [PATCH 01/15] Validate content length on broadcast API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The maximum content count of a broadcast varies depending on its encoding, so we can’t simply validate it against a schema. This commit moves to using the validation from `notifications-utils`, and raising a custom error response. --- app/v2/broadcast/broadcast_schemas.py | 1 - app/v2/broadcast/post_broadcast.py | 19 +++++++++- requirements-app.txt | 2 +- requirements.txt | 8 ++-- .../v2/broadcast/sample_cap_xml_documents.py | 37 +++++++++++++++++++ tests/app/v2/broadcast/test_post_broadcast.py | 31 ++++++++++++++++ 6 files changed, 91 insertions(+), 7 deletions(-) diff --git a/app/v2/broadcast/broadcast_schemas.py b/app/v2/broadcast/broadcast_schemas.py index 83fc65607..67886507e 100644 --- a/app/v2/broadcast/broadcast_schemas.py +++ b/app/v2/broadcast/broadcast_schemas.py @@ -40,7 +40,6 @@ post_broadcast_schema = { "content": { "type": "string", "minLength": 1, - "maxLength": 1395, }, "web": { "type": "string", diff --git a/app/v2/broadcast/post_broadcast.py b/app/v2/broadcast/post_broadcast.py index 6833babde..fd8e00b2c 100644 --- a/app/v2/broadcast/post_broadcast.py +++ b/app/v2/broadcast/post_broadcast.py @@ -1,6 +1,7 @@ from itertools import chain from flask import current_app, jsonify, request from notifications_utils.polygons import Polygons +from notifications_utils.template import BroadcastMessageTemplate from app import authenticated_service, api_user from app.broadcast_message.translators import cap_xml_to_dict from app.dao.dao_utils import dao_save_object @@ -9,7 +10,7 @@ from app.models import BROADCAST_TYPE, BroadcastMessage, BroadcastStatusType from app.schema_validation import validate from app.v2.broadcast import v2_broadcast_blueprint from app.v2.broadcast.broadcast_schemas import post_broadcast_schema -from app.v2.errors import BadRequestError +from app.v2.errors import BadRequestError, ValidationError from app.xml_schemas import validate_xml @@ -43,6 +44,22 @@ def create_broadcast(): area['polygons'] for area in broadcast_json['areas'] )))) + template = BroadcastMessageTemplate.from_content( + broadcast_json['content'] + ) + + if template.content_too_long: + raise ValidationError( + message=( + f'description must be {template.max_content_count:,.0f} ' + f'characters or fewer' + ) + ( + ' (because it could not be GSM7 encoded)' + if template.non_gsm_characters else '' + ), + status_code=400, + ) + broadcast_message = BroadcastMessage( service_id=authenticated_service.id, content=broadcast_json['content'], diff --git a/requirements-app.txt b/requirements-app.txt index b358e3c3c..8d99c0d8f 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -33,7 +33,7 @@ notifications-python-client==5.7.1 # PaaS awscli-cwlogs==1.4.6 -git+https://github.com/alphagov/notifications-utils.git@43.8.3#egg=notifications-utils==43.8.3 +git+https://github.com/alphagov/notifications-utils.git@43.9.0#egg=notifications-utils==43.9.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.9.0 diff --git a/requirements.txt b/requirements.txt index 9ec6beb50..41242987a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -35,7 +35,7 @@ notifications-python-client==5.7.1 # PaaS awscli-cwlogs==1.4.6 -git+https://github.com/alphagov/notifications-utils.git@43.8.3#egg=notifications-utils==43.8.3 +git+https://github.com/alphagov/notifications-utils.git@43.9.0#egg=notifications-utils==43.9.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.9.0 @@ -46,14 +46,14 @@ alembic==1.5.4 amqp==1.4.9 anyjson==0.3.3 attrs==20.3.0 -awscli==1.19.5 +awscli==1.19.8 bcrypt==3.2.0 billiard==3.3.0.23 bleach==3.3.0 blinker==1.4 boto==2.49.0 -boto3==1.17.5 -botocore==1.20.5 +boto3==1.17.8 +botocore==1.20.8 certifi==2020.12.5 chardet==4.0.0 click==7.1.2 diff --git a/tests/app/v2/broadcast/sample_cap_xml_documents.py b/tests/app/v2/broadcast/sample_cap_xml_documents.py index 51244e379..a8a0b8cab 100644 --- a/tests/app/v2/broadcast/sample_cap_xml_documents.py +++ b/tests/app/v2/broadcast/sample_cap_xml_documents.py @@ -198,3 +198,40 @@ CANCEL = """ """ + +WITH_PLACEHOLDER_FOR_CONTENT = """ + + 50385fcb0ab7aa447bbd46d848ce8466E + www.gov.uk/environment-agency + 2020-02-16T23:01:13-00:00 + Actual + Alert + Flood warning service + Public + www.gov.uk/environment-agency,4f6d28b10ab7aa447bbd46d85f1e9effE,2020-02-16T19:20:03+00:00 + + en-GB + Met + 053/055 Issue Severe Flood Warning EA + Immediate + Severe + Likely + 2020-02-26T23:01:14-00:00 + Environment Agency + {} + https://flood-warning-information.service.gov.uk + 0345 988 1188 + + River Steeping in Wainfleet All Saints + 53.10569,0.24453 53.10593,0.24430 53.10601,0.24375 53.10615,0.24349 53.10629,0.24356 53.10656,0.24336 53.10697,0.24354 53.10684,0.24298 53.10694,0.24264 53.10721,0.24302 53.10752,0.24310 53.10777,0.24308 53.10805,0.24320 53.10803,0.24187 53.10776,0.24085 53.10774,0.24062 53.10702,0.24056 53.10679,0.24088 53.10658,0.24071 53.10651,0.24049 53.10656,0.24022 53.10642,0.24022 53.10632,0.24052 53.10629,0.24082 53.10612,0.24093 53.10583,0.24133 53.10564,0.24178 53.10541,0.24282 53.10569,0.24453 + + TargetAreaCode + 053FWFSTEEP4 + + + + +""" + +LONG_GSM7 = WITH_PLACEHOLDER_FOR_CONTENT.format('a' * 1396) +LONG_UCS2 = WITH_PLACEHOLDER_FOR_CONTENT.format('ŵ' * 616) diff --git a/tests/app/v2/broadcast/test_post_broadcast.py b/tests/app/v2/broadcast/test_post_broadcast.py index 8cd0256d5..0b9de61a4 100644 --- a/tests/app/v2/broadcast/test_post_broadcast.py +++ b/tests/app/v2/broadcast/test_post_broadcast.py @@ -191,3 +191,34 @@ def test_unsupported_message_types_400( } in ( json.loads(response.get_data(as_text=True))['errors'] ) + + +@pytest.mark.parametrize('xml_document, expected_error', ( + (sample_cap_xml_documents.LONG_UCS2, ( + 'description must be 615 characters or fewer (because it ' + 'could not be GSM7 encoded)' + )), + (sample_cap_xml_documents.LONG_GSM7, ( + 'description must be 1,395 characters or fewer' + )), +)) +def test_content_too_long_returns_400( + client, + sample_broadcast_service, + xml_document, + expected_error, +): + auth_header = create_authorization_header(service_id=sample_broadcast_service.id) + response = client.post( + path='/v2/broadcast', + data=xml_document, + headers=[('Content-Type', 'application/cap+xml'), auth_header], + ) + + assert json.loads(response.get_data(as_text=True)) == { + 'errors': [{ + 'error': 'ValidationError', + 'message': expected_error, + }], + 'status_code': 400, + } From 5d62647b9d4391af267a77e0a13707e0d7e585b1 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 3 Feb 2021 16:43:01 +0000 Subject: [PATCH 02/15] Add broadcast channel to service schema This will show which channel is configured, if any, for a service. It mimics what we are doing for the `allowed_broadcast_provider`. --- app/schemas.py | 4 ++++ tests/app/service/test_rest.py | 2 ++ 2 files changed, 6 insertions(+) diff --git a/app/schemas.py b/app/schemas.py index 7d9a59d06..41d487608 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -236,10 +236,14 @@ class ServiceSchema(BaseSchema, UUIDsAsStringsMixin): override_flag = False go_live_at = field_for(models.Service, 'go_live_at', format=DATETIME_FORMAT_NO_TIMEZONE) allowed_broadcast_provider = fields.Method(dump_only=True, serialize='_get_allowed_broadcast_provider') + broadcast_channel = fields.Method(dump_only=True, serialize='_get_broadcast_channel') def _get_allowed_broadcast_provider(self, service): return service.allowed_broadcast_provider + def _get_broadcast_channel(self, service): + 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/service/test_rest.py b/tests/app/service/test_rest.py index efcb45ff7..6f6b02ed4 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -243,6 +243,7 @@ def test_get_service_by_id(admin_request, sample_service): assert json_resp['data']['email_branding'] is None assert json_resp['data']['prefix_sms'] is True assert json_resp['data']['allowed_broadcast_provider'] is None + assert json_resp['data']['broadcast_channel'] is None assert set(json_resp['data'].keys()) == { 'active', @@ -250,6 +251,7 @@ def test_get_service_by_id(admin_request, sample_service): 'billing_contact_email_addresses', 'billing_contact_names', 'billing_reference', + 'broadcast_channel', 'consent_to_research', 'contact_link', 'count_as_live', From 3b5d86c85461309531e2ee2d9107e815d3bad575 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 4 Feb 2021 13:20:42 +0000 Subject: [PATCH 03/15] Add endpoint to set broadcast service channel --- app/dao/service_broadcast_settings_dao.py | 15 ++++ app/service/rest.py | 25 ++++++- .../service_broadcast_settings_schema.py | 10 +++ tests/app/service/test_rest.py | 74 +++++++++++++++++++ 4 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 app/dao/service_broadcast_settings_dao.py create mode 100644 app/service/service_broadcast_settings_schema.py diff --git a/app/dao/service_broadcast_settings_dao.py b/app/dao/service_broadcast_settings_dao.py new file mode 100644 index 000000000..768b0d194 --- /dev/null +++ b/app/dao/service_broadcast_settings_dao.py @@ -0,0 +1,15 @@ +from app import db +from app.models import ServiceBroadcastSettings +from app.dao.dao_utils import transactional + + +@transactional +def insert_or_update_service_broadcast_settings(service, channel, provider_restriction=None): + if not service.service_broadcast_settings: + settings = ServiceBroadcastSettings() + settings.service = service + settings.channel = channel + db.session.add(settings) + else: + service.service_broadcast_settings.channel = channel + db.session.add(service.service_broadcast_settings) diff --git a/app/service/rest.py b/app/service/rest.py index 2f04ed2d1..ef68e6dc2 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -73,6 +73,9 @@ from app.dao.services_dao import ( dao_update_service, get_services_by_partial_name, ) +from app.dao.service_broadcast_settings_dao import ( + insert_or_update_service_broadcast_settings +) from app.dao.service_guest_list_dao import ( dao_fetch_service_guest_list, dao_add_and_commit_guest_list_contacts, @@ -100,9 +103,15 @@ from app.errors import ( ) from app.letters.utils import letter_print_day from app.models import ( - KEY_TYPE_NORMAL, LETTER_TYPE, NOTIFICATION_CANCELLED, Permission, Service, - EmailBranding, LetterBranding, - ServiceContactList + KEY_TYPE_NORMAL, + LETTER_TYPE, + NOTIFICATION_CANCELLED, + Permission, + Service, + EmailBranding, + LetterBranding, + ServiceContactList, + ServiceBroadcastSettings, ) from app.notifications.process_notifications import persist_notification, send_notification_to_queue from app.schema_validation import validate @@ -118,6 +127,7 @@ from app.service.service_senders_schema import ( add_service_letter_contact_block_request, add_service_sms_sender_request ) +from app.service.service_broadcast_settings_schema import service_broadcast_settings_schema from app.service.utils import get_guest_list_objects from app.service.sender import send_notification_to_service_users from app.service.send_notification import send_one_off_notification, send_pdf_letter_notification @@ -1070,3 +1080,12 @@ def create_contact_list(service_id): save_service_contact_list(list_to_save) return jsonify(list_to_save.serialize()), 201 + + +@service_blueprint.route('//set-as-broadcast-service', methods=['POST']) +def set_as_broadcast_service(service_id): + data = validate(request.get_json(), service_broadcast_settings_schema) + service = dao_fetch_service_by_id(service_id) + insert_or_update_service_broadcast_settings(service, channel=data["broadcast_channel"]) + data = service_schema.dump(service).data + return jsonify(data=data) diff --git a/app/service/service_broadcast_settings_schema.py b/app/service/service_broadcast_settings_schema.py new file mode 100644 index 000000000..63946e860 --- /dev/null +++ b/app/service/service_broadcast_settings_schema.py @@ -0,0 +1,10 @@ +service_broadcast_settings_schema = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "Set a services broadcast settings", + "type": "object", + "title": "Set a services broadcast settings", + "properties": { + "broadcast_channel": {"enum": ["test", "severe"]} + }, + "required": ["broadcast_channel"] +} diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6f6b02ed4..f144f71ac 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -31,11 +31,13 @@ from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, + BROADCAST_TYPE, INTERNATIONAL_LETTERS, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE, NOTIFICATION_RETURNED_LETTER, UPLOAD_LETTERS, + ServiceBroadcastSettings, ) from tests import create_authorization_header from tests.app.db import ( @@ -3647,3 +3649,75 @@ def test_get_returned_letter(admin_request, sample_letter_template): assert not response[4]['original_file_name'] assert not response[4]['job_row_number'] assert response[4]['uploaded_letter_file_name'] == 'filename.pdf' + + +@pytest.mark.parametrize('channel', ["test", "severe"]) +def test_set_as_broadcast_service_sets_broadcast_channel(client, notify_db, sample_service, channel): + assert sample_service.service_broadcast_settings is None + data = { + 'broadcast_channel': channel, + } + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['name'] == 'Sample service' + assert result['data']['broadcast_channel'] == channel + + records = ServiceBroadcastSettings.query.filter_by(service_id=sample_service.id).all() + assert len(records) == 1 + assert records[0].service_id == sample_service.id + assert records[0].channel == channel + + +def test_set_as_broadcast_service_updates_channel(client, notify_db, sample_service): + settings = ServiceBroadcastSettings(channel="test", service=sample_service) + notify_db.session.add(settings) + notify_db.session.commit() + assert sample_service.broadcast_channel == "test" + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps({ + 'broadcast_channel': "severe", + }), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['name'] == 'Sample service' + assert result['data']['broadcast_channel'] == "severe" + + records = ServiceBroadcastSettings.query.filter_by(service_id=sample_service.id).all() + assert len(records) == 1 + assert records[0].service_id == sample_service.id + assert records[0].channel == "severe" + + +@pytest.mark.parametrize('channel', ["government", "extreme", "exercise", "random", ""]) +def test_set_as_broadcast_service_rejects_unknown_channels(client, notify_db, sample_service, channel): + data = { + 'broadcast_channel': channel, + } + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert resp.status_code == 400 + + +def test_set_as_broadcast_service_rejects_if_no_channel(client, notify_db, sample_service): + data = {} + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert resp.status_code == 400 From 16ee040923adf4c501df5012d5223fd87a544001 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 4 Feb 2021 15:08:50 +0000 Subject: [PATCH 04/15] Add service broadcast settings to `sample_broadcast_service` This will make our `sample_broadcast_service` look more realistic. The one downside to this, is that there will be a short amount of time where we have broadcast services that do not have a row in the service_broadcast_settings table until we have backfilled this data. Our unit tests therefore won't have coverage for this case but I think the risk is small and acceptable for the moment as this will no longer be the case in say a weeks time. --- tests/app/conftest.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index dd02564a1..9ea4e70cd 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -16,6 +16,7 @@ from app.dao.jobs_dao import dao_create_job from app.dao.notifications_dao import dao_create_notification from app.dao.organisation_dao import dao_create_organisation from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) +from app.dao.service_broadcast_settings_dao import insert_or_update_service_broadcast_settings from app.dao.templates_dao import dao_create_template from app.dao.users_dao import create_secret_code, create_user_code from app.history_meta import create_history @@ -42,7 +43,7 @@ from app.models import ( LETTER_TYPE, SERVICE_PERMISSION_TYPES, ServiceEmailReplyTo, - BROADCAST_TYPE + BROADCAST_TYPE, ) from tests import create_authorization_header from tests.app.db import ( @@ -167,6 +168,7 @@ def sample_broadcast_service(notify_db_session): if not service: service = Service(**data) dao_create_service(service, user, service_permissions=[BROADCAST_TYPE]) + insert_or_update_service_broadcast_settings(service, channel="severe") else: if user not in service.users: dao_add_user_to_service(service, user) From 3f16549f64f192da3bdd222ed2b31cadace65944 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 4 Feb 2021 15:15:01 +0000 Subject: [PATCH 05/15] Use `sample_broadcast_service` for update test We can use the `sample_broadcast_service` as this gives us a broadcast service with service broadcast settings already for us to update rather than needing to create our own settings db row --- tests/app/service/test_rest.py | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f144f71ac..461d56801 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3674,28 +3674,25 @@ def test_set_as_broadcast_service_sets_broadcast_channel(client, notify_db, samp assert records[0].channel == channel -def test_set_as_broadcast_service_updates_channel(client, notify_db, sample_service): - settings = ServiceBroadcastSettings(channel="test", service=sample_service) - notify_db.session.add(settings) - notify_db.session.commit() - assert sample_service.broadcast_channel == "test" +def test_set_as_broadcast_service_updates_channel_for_broadcast_service(client, notify_db, sample_broadcast_service): + assert sample_broadcast_service.broadcast_channel == "severe" resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), + '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), data=json.dumps({ - 'broadcast_channel': "severe", + 'broadcast_channel': "test", }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) result = resp.json assert resp.status_code == 200 - assert result['data']['name'] == 'Sample service' - assert result['data']['broadcast_channel'] == "severe" + assert result['data']['name'] == 'Sample broadcast service' + assert result['data']['broadcast_channel'] == "test" - records = ServiceBroadcastSettings.query.filter_by(service_id=sample_service.id).all() + records = ServiceBroadcastSettings.query.filter_by(service_id=sample_broadcast_service.id).all() assert len(records) == 1 - assert records[0].service_id == sample_service.id - assert records[0].channel == "severe" + assert records[0].service_id == sample_broadcast_service.id + assert records[0].channel == "test" @pytest.mark.parametrize('channel', ["government", "extreme", "exercise", "random", ""]) From 54b9d20f73a772ccb1da793d470338f9a192a4db Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 4 Feb 2021 15:15:58 +0000 Subject: [PATCH 06/15] Give broadcast permission to broadcast services --- app/service/rest.py | 20 +++++++++++++++++ tests/app/service/test_rest.py | 41 ++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/app/service/rest.py b/app/service/rest.py index ef68e6dc2..c71f9ed02 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -42,6 +42,11 @@ from app.dao.service_contact_list_dao import ( dao_get_contact_list_by_id, save_service_contact_list, ) +from app.dao.service_permissions_dao import ( + dao_add_service_permission, + dao_fetch_service_permissions, + dao_remove_service_permission, +) from app.dao.service_data_retention_dao import ( fetch_service_data_retention, fetch_service_data_retention_by_id, @@ -105,6 +110,7 @@ from app.letters.utils import letter_print_day from app.models import ( KEY_TYPE_NORMAL, LETTER_TYPE, + BROADCAST_TYPE, NOTIFICATION_CANCELLED, Permission, Service, @@ -112,6 +118,7 @@ from app.models import ( LetterBranding, ServiceContactList, ServiceBroadcastSettings, + ServicePermission ) from app.notifications.process_notifications import persist_notification, send_notification_to_queue from app.schema_validation import validate @@ -1084,8 +1091,21 @@ def create_contact_list(service_id): @service_blueprint.route('//set-as-broadcast-service', methods=['POST']) def set_as_broadcast_service(service_id): + """ + This route does three things + - adds a service broadcast settings to define which channel broadcasts should go out on + - removes all current service permissions + - adds the broadcast service permission + """ data = validate(request.get_json(), service_broadcast_settings_schema) service = dao_fetch_service_by_id(service_id) + insert_or_update_service_broadcast_settings(service, channel=data["broadcast_channel"]) + + current_service_permissions = dao_fetch_service_permissions(service.id) + for permission in current_service_permissions: + dao_remove_service_permission(service.id, permission.permission) + dao_add_service_permission(service.id, BROADCAST_TYPE) + data = service_schema.dump(service).data return jsonify(data=data) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 461d56801..ec87873d0 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3718,3 +3718,44 @@ def test_set_as_broadcast_service_rejects_if_no_channel(client, notify_db, sampl headers=[('Content-Type', 'application/json'), create_authorization_header()] ) assert resp.status_code == 400 + + +def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_permissions(client, notify_db, sample_service): + current_permissions = [p.permission for p in sample_service.permissions] + assert len(current_permissions) > 0 + assert current_permissions != [BROADCAST_TYPE] + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps({ + 'broadcast_channel': "severe", + }), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['permissions'] == [BROADCAST_TYPE] + + permissions = ServicePermission.query.filter_by(service_id=sample_service.id).all() + assert [p.permission for p in permissions] == [BROADCAST_TYPE] + + +def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_broadcast_service( + client, notify_db, sample_broadcast_service +): + current_permissions = [p.permission for p in sample_broadcast_service.permissions] + assert current_permissions == [BROADCAST_TYPE] + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), + data=json.dumps({ + 'broadcast_channel': "severe", + }), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['permissions'] == [BROADCAST_TYPE] + + permissions = ServicePermission.query.filter_by(service_id=sample_broadcast_service.id).all() + assert [p.permission for p in permissions] == [BROADCAST_TYPE] From cdcbd1e23811cfea9ae82cdb7d52668e82283398 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 4 Feb 2021 18:06:08 +0000 Subject: [PATCH 07/15] Set count as live to false for broadcast services We think it would be a security risk to show the name of services involved in emergency alerts as they be responsible for things such as counter terrorism. On top of that, showing broadcast services in the list of all services could enable someone to use that information to try and trick an admin into letting them access of a particular service given the fact they know the name of it --- app/service/rest.py | 6 +++++- tests/app/conftest.py | 3 ++- tests/app/service/test_rest.py | 18 ++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index c71f9ed02..c1f9d22ad 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1092,10 +1092,11 @@ def create_contact_list(service_id): @service_blueprint.route('//set-as-broadcast-service', methods=['POST']) def set_as_broadcast_service(service_id): """ - This route does three things + This route does four things - adds a service broadcast settings to define which channel broadcasts should go out on - removes all current service permissions - adds the broadcast service permission + - sets the services `count_as_live` to false """ data = validate(request.get_json(), service_broadcast_settings_schema) service = dao_fetch_service_by_id(service_id) @@ -1107,5 +1108,8 @@ def set_as_broadcast_service(service_id): dao_remove_service_permission(service.id, permission.permission) dao_add_service_permission(service.id, BROADCAST_TYPE) + service.count_as_live = False + dao_update_service(service) + data = service_schema.dump(service).data return jsonify(data=data) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 9ea4e70cd..698d59e3e 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -162,7 +162,8 @@ def sample_broadcast_service(notify_db_session): 'restricted': False, 'email_from': email_from, 'created_by': user, - 'crown': True + 'crown': True, + 'count_as_live': False, } service = Service.query.filter_by(name=service_name).first() if not service: diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index ec87873d0..16fa1488e 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3759,3 +3759,21 @@ def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_br permissions = ServicePermission.query.filter_by(service_id=sample_broadcast_service.id).all() assert [p.permission for p in permissions] == [BROADCAST_TYPE] + + +def test_set_as_broadcast_service_sets_count_as_live_to_false(client, notify_db, sample_service): + assert sample_service.count_as_live == True + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps({ + 'broadcast_channel': "severe", + }), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['count_as_live'] == False + + service_from_db = Service.query.filter_by(id=sample_service.id).all()[0] + assert service_from_db.count_as_live == False From 9f4b82f07476a1206c192535ec52225f1da33921 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 5 Feb 2021 17:10:41 +0000 Subject: [PATCH 08/15] Make service a member of the broadcast organisation We will use this to easily identify all our broadcast services. There could be other ways to deal with finding and seeing all broadcast services but this is a good and easy way to start. --- app/config.py | 3 ++ app/service/rest.py | 11 +++++-- tests/app/conftest.py | 15 ++++++++-- tests/app/service/test_rest.py | 55 +++++++++++++++++++++++++++++----- 4 files changed, 72 insertions(+), 12 deletions(-) diff --git a/app/config.py b/app/config.py index 30acf16d7..e7ff9869d 100644 --- a/app/config.py +++ b/app/config.py @@ -384,6 +384,9 @@ class Config(object): ENABLED_CBCS = {BroadcastProvider.EE, BroadcastProvider.THREE, BroadcastProvider.O2, BroadcastProvider.VODAFONE} + # as defined in api db migration 0331_add_broadcast_org.py + BROADCAST_ORGANISATION_ID = '38e4bf69-93b0-445d-acee-53ea53fe02df' + ###################### # Config overrides ### diff --git a/app/service/rest.py b/app/service/rest.py index c1f9d22ad..756828cb5 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -29,7 +29,10 @@ from app.dao.fact_notification_status_dao import ( fetch_stats_for_all_services_by_date_range, fetch_monthly_template_usage_for_service ) from app.dao.inbound_numbers_dao import dao_allocate_number_for_service -from app.dao.organisation_dao import dao_get_organisation_by_service_id +from app.dao.organisation_dao import ( + dao_get_organisation_by_service_id, + dao_add_service_to_organisation, +) from app.dao.returned_letters_dao import ( fetch_most_recent_returned_letter, fetch_recent_returned_letter_count, @@ -1094,9 +1097,9 @@ def set_as_broadcast_service(service_id): """ This route does four things - adds a service broadcast settings to define which channel broadcasts should go out on - - removes all current service permissions - - adds the broadcast service permission + - removes all current service permissions and adds the broadcast service permission - sets the services `count_as_live` to false + - adds the service to the broadcast organisation """ data = validate(request.get_json(), service_broadcast_settings_schema) service = dao_fetch_service_by_id(service_id) @@ -1111,5 +1114,7 @@ def set_as_broadcast_service(service_id): service.count_as_live = False dao_update_service(service) + dao_add_service_to_organisation(service, current_app.config['BROADCAST_ORGANISATION_ID']) + data = service_schema.dump(service).data return jsonify(data=data) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 698d59e3e..33e68b630 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -14,7 +14,7 @@ from app.dao.api_key_dao import save_model_api_key from app.dao.invited_user_dao import save_invited_user from app.dao.jobs_dao import dao_create_job from app.dao.notifications_dao import dao_create_notification -from app.dao.organisation_dao import dao_create_organisation +from app.dao.organisation_dao import dao_create_organisation, dao_add_service_to_organisation from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) from app.dao.service_broadcast_settings_dao import insert_or_update_service_broadcast_settings from app.dao.templates_dao import dao_create_template @@ -151,7 +151,7 @@ def sample_service(notify_db_session): @pytest.fixture(scope='function') -def sample_broadcast_service(notify_db_session): +def sample_broadcast_service(notify_db_session, broadcast_organisation): user = create_user() service_name = 'Sample broadcast service' email_from = service_name.lower().replace(' ', '.') @@ -170,6 +170,7 @@ def sample_broadcast_service(notify_db_session): service = Service(**data) dao_create_service(service, user, service_permissions=[BROADCAST_TYPE]) insert_or_update_service_broadcast_settings(service, channel="severe") + dao_add_service_to_organisation(service, current_app.config['BROADCAST_ORGANISATION_ID']) else: if user not in service.users: dao_add_user_to_service(service, user) @@ -877,6 +878,16 @@ def sample_organisation(notify_db_session): return org +@pytest.fixture +def broadcast_organisation(notify_db_session): + org = Organisation.query.get(current_app.config['BROADCAST_ORGANISATION_ID']) + if not org: + org = Organisation(id=current_app.config['BROADCAST_ORGANISATION_ID'], name='broadcast organisation') + dao_create_organisation(org) + + return org + + @pytest.fixture def restore_provider_details(notify_db, notify_db_session): """ diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 16fa1488e..aab284edc 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3652,7 +3652,9 @@ def test_get_returned_letter(admin_request, sample_letter_template): @pytest.mark.parametrize('channel', ["test", "severe"]) -def test_set_as_broadcast_service_sets_broadcast_channel(client, notify_db, sample_service, channel): +def test_set_as_broadcast_service_sets_broadcast_channel( + client, notify_db, sample_service, broadcast_organisation, channel +): assert sample_service.service_broadcast_settings is None data = { 'broadcast_channel': channel, @@ -3674,7 +3676,9 @@ def test_set_as_broadcast_service_sets_broadcast_channel(client, notify_db, samp assert records[0].channel == channel -def test_set_as_broadcast_service_updates_channel_for_broadcast_service(client, notify_db, sample_broadcast_service): +def test_set_as_broadcast_service_updates_channel_for_broadcast_service( + client, notify_db, sample_broadcast_service, broadcast_organisation +): assert sample_broadcast_service.broadcast_channel == "severe" resp = client.post( @@ -3696,7 +3700,9 @@ def test_set_as_broadcast_service_updates_channel_for_broadcast_service(client, @pytest.mark.parametrize('channel', ["government", "extreme", "exercise", "random", ""]) -def test_set_as_broadcast_service_rejects_unknown_channels(client, notify_db, sample_service, channel): +def test_set_as_broadcast_service_rejects_unknown_channels( + client, notify_db, sample_service, broadcast_organisation, channel +): data = { 'broadcast_channel': channel, } @@ -3709,7 +3715,7 @@ def test_set_as_broadcast_service_rejects_unknown_channels(client, notify_db, sa assert resp.status_code == 400 -def test_set_as_broadcast_service_rejects_if_no_channel(client, notify_db, sample_service): +def test_set_as_broadcast_service_rejects_if_no_channel(client, notify_db, sample_service, broadcast_organisation): data = {} resp = client.post( @@ -3720,7 +3726,9 @@ def test_set_as_broadcast_service_rejects_if_no_channel(client, notify_db, sampl assert resp.status_code == 400 -def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_permissions(client, notify_db, sample_service): +def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_permissions( + client, notify_db, sample_service, broadcast_organisation +): current_permissions = [p.permission for p in sample_service.permissions] assert len(current_permissions) > 0 assert current_permissions != [BROADCAST_TYPE] @@ -3741,7 +3749,7 @@ def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_p def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_broadcast_service( - client, notify_db, sample_broadcast_service + client, notify_db, sample_broadcast_service, broadcast_organisation ): current_permissions = [p.permission for p in sample_broadcast_service.permissions] assert current_permissions == [BROADCAST_TYPE] @@ -3761,7 +3769,7 @@ def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_br assert [p.permission for p in permissions] == [BROADCAST_TYPE] -def test_set_as_broadcast_service_sets_count_as_live_to_false(client, notify_db, sample_service): +def test_set_as_broadcast_service_sets_count_as_live_to_false(client, notify_db, sample_service, broadcast_organisation): assert sample_service.count_as_live == True resp = client.post( @@ -3777,3 +3785,36 @@ def test_set_as_broadcast_service_sets_count_as_live_to_false(client, notify_db, service_from_db = Service.query.filter_by(id=sample_service.id).all()[0] assert service_from_db.count_as_live == False + + +def test_set_as_broadcast_service_sets_service_org_to_broadcast_org(client, notify_db, sample_service, broadcast_organisation): + assert sample_service.organisation_id != current_app.config['BROADCAST_ORGANISATION_ID'] + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps({ + 'broadcast_channel': "severe", + }), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['organisation'] == current_app.config['BROADCAST_ORGANISATION_ID'] + + service_from_db = Service.query.filter_by(id=sample_service.id).all()[0] + assert str(service_from_db.organisation_id) == current_app.config['BROADCAST_ORGANISATION_ID'] + + +def test_set_as_broadcast_service_does_not_error_if_run_on_a_service_that_is_already_a_broadcast_service( + client, notify_db, sample_service, broadcast_organisation +): + for _ in range(2): + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps({ + 'broadcast_channel': "severe", + }), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 From cb70b81ea45851892084e103842b7272fdb88ed0 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 8 Feb 2021 16:56:34 +0000 Subject: [PATCH 09/15] make service live or training --- app/service/rest.py | 8 +- .../service_broadcast_settings_schema.py | 5 +- tests/app/service/test_rest.py | 85 ++++++++++++++++++- 3 files changed, 94 insertions(+), 4 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 756828cb5..91a3951b9 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1095,11 +1095,12 @@ def create_contact_list(service_id): @service_blueprint.route('//set-as-broadcast-service', methods=['POST']) def set_as_broadcast_service(service_id): """ - This route does four things + This route does the following - adds a service broadcast settings to define which channel broadcasts should go out on - removes all current service permissions and adds the broadcast service permission - sets the services `count_as_live` to false - adds the service to the broadcast organisation + - puts the service into training mode or live mode """ data = validate(request.get_json(), service_broadcast_settings_schema) service = dao_fetch_service_by_id(service_id) @@ -1112,6 +1113,11 @@ def set_as_broadcast_service(service_id): dao_add_service_permission(service.id, BROADCAST_TYPE) service.count_as_live = False + + service.restricted = True + if data["service_mode"] == "live": + service.restricted = False + dao_update_service(service) dao_add_service_to_organisation(service, current_app.config['BROADCAST_ORGANISATION_ID']) diff --git a/app/service/service_broadcast_settings_schema.py b/app/service/service_broadcast_settings_schema.py index 63946e860..3ca492f4f 100644 --- a/app/service/service_broadcast_settings_schema.py +++ b/app/service/service_broadcast_settings_schema.py @@ -4,7 +4,8 @@ service_broadcast_settings_schema = { "type": "object", "title": "Set a services broadcast settings", "properties": { - "broadcast_channel": {"enum": ["test", "severe"]} + "broadcast_channel": {"enum": ["test", "severe"]}, + "service_mode": {"enum": ["training", "live"]} }, - "required": ["broadcast_channel"] + "required": ["broadcast_channel", "service_mode"] } diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index aab284edc..961d1567c 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3658,6 +3658,7 @@ def test_set_as_broadcast_service_sets_broadcast_channel( assert sample_service.service_broadcast_settings is None data = { 'broadcast_channel': channel, + 'service_mode': 'live', } resp = client.post( @@ -3685,6 +3686,7 @@ def test_set_as_broadcast_service_updates_channel_for_broadcast_service( '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), data=json.dumps({ 'broadcast_channel': "test", + 'service_mode': 'training', }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3705,6 +3707,7 @@ def test_set_as_broadcast_service_rejects_unknown_channels( ): data = { 'broadcast_channel': channel, + 'service_mode': 'live', } resp = client.post( @@ -3716,7 +3719,7 @@ def test_set_as_broadcast_service_rejects_unknown_channels( def test_set_as_broadcast_service_rejects_if_no_channel(client, notify_db, sample_service, broadcast_organisation): - data = {} + data = {'service_mode': 'training'} resp = client.post( '/service/{}/set-as-broadcast-service'.format(sample_service.id), @@ -3737,6 +3740,7 @@ def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_p '/service/{}/set-as-broadcast-service'.format(sample_service.id), data=json.dumps({ 'broadcast_channel': "severe", + 'service_mode': 'training', }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3758,6 +3762,7 @@ def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_br '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), data=json.dumps({ 'broadcast_channel': "severe", + 'service_mode': 'live', }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3776,6 +3781,7 @@ def test_set_as_broadcast_service_sets_count_as_live_to_false(client, notify_db, '/service/{}/set-as-broadcast-service'.format(sample_service.id), data=json.dumps({ 'broadcast_channel': "severe", + 'service_mode': 'live', }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3794,6 +3800,7 @@ def test_set_as_broadcast_service_sets_service_org_to_broadcast_org(client, noti '/service/{}/set-as-broadcast-service'.format(sample_service.id), data=json.dumps({ 'broadcast_channel': "severe", + 'service_mode': 'training', }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3813,8 +3820,84 @@ def test_set_as_broadcast_service_does_not_error_if_run_on_a_service_that_is_alr '/service/{}/set-as-broadcast-service'.format(sample_service.id), data=json.dumps({ 'broadcast_channel': "severe", + 'service_mode': "live" }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) result = resp.json assert resp.status_code == 200 + + +def test_set_as_broadcast_service_sets_service_to_live_mode( + client, notify_db, sample_service, broadcast_organisation +): + sample_service.restricted = True + notify_db.session.add(sample_service) + notify_db.session.commit() + assert sample_service.restricted == True + data = { + 'broadcast_channel': 'severe', + 'service_mode': 'live', + } + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['name'] == 'Sample service' + assert result['data']['restricted'] == False + + +def test_set_as_broadcast_service_sets_service_to_training_mode( + client, notify_db, sample_broadcast_service, broadcast_organisation +): + sample_broadcast_service.restricted = False + notify_db.session.add(sample_broadcast_service) + notify_db.session.commit() + assert sample_broadcast_service.restricted == False + + data = { + 'broadcast_channel': 'severe', + 'service_mode': 'training', + } + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['name'] == 'Sample broadcast service' + assert result['data']['restricted'] == True + + +@pytest.mark.parametrize('service_mode', ["testing", ""]) +def test_set_as_broadcast_service_rejects_unknown_service_mode( + client, notify_db, sample_service, broadcast_organisation, service_mode +): + data = { + 'broadcast_channel': 'severe', + 'service_mode': service_mode, + } + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert resp.status_code == 400 + + +def test_set_as_broadcast_service_rejects_if_no_service_mode(client, notify_db, sample_service, broadcast_organisation): + data = {'broadcast_channel': 'severe'} + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert resp.status_code == 400 From 4f7afa3fbe6b8e51123bb2bef02507d805082a2a Mon Sep 17 00:00:00 2001 From: David McDonald Date: Tue, 9 Feb 2021 14:47:36 +0000 Subject: [PATCH 10/15] Set provider restriction --- app/dao/service_broadcast_settings_dao.py | 2 + app/service/rest.py | 4 +- .../service_broadcast_settings_schema.py | 5 +- tests/app/service/test_rest.py | 116 +++++++++++++++++- 4 files changed, 121 insertions(+), 6 deletions(-) diff --git a/app/dao/service_broadcast_settings_dao.py b/app/dao/service_broadcast_settings_dao.py index 768b0d194..8835fbae9 100644 --- a/app/dao/service_broadcast_settings_dao.py +++ b/app/dao/service_broadcast_settings_dao.py @@ -9,7 +9,9 @@ def insert_or_update_service_broadcast_settings(service, channel, provider_restr settings = ServiceBroadcastSettings() settings.service = service settings.channel = channel + settings.provider = provider_restriction db.session.add(settings) else: service.service_broadcast_settings.channel = channel + service.service_broadcast_settings.provider = provider_restriction db.session.add(service.service_broadcast_settings) diff --git a/app/service/rest.py b/app/service/rest.py index 91a3951b9..5e3754e1b 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1105,7 +1105,9 @@ def set_as_broadcast_service(service_id): data = validate(request.get_json(), service_broadcast_settings_schema) service = dao_fetch_service_by_id(service_id) - insert_or_update_service_broadcast_settings(service, channel=data["broadcast_channel"]) + insert_or_update_service_broadcast_settings( + service, channel=data["broadcast_channel"], provider_restriction=data.get("provider_restriction") + ) current_service_permissions = dao_fetch_service_permissions(service.id) for permission in current_service_permissions: diff --git a/app/service/service_broadcast_settings_schema.py b/app/service/service_broadcast_settings_schema.py index 3ca492f4f..bc18af07d 100644 --- a/app/service/service_broadcast_settings_schema.py +++ b/app/service/service_broadcast_settings_schema.py @@ -5,7 +5,8 @@ service_broadcast_settings_schema = { "title": "Set a services broadcast settings", "properties": { "broadcast_channel": {"enum": ["test", "severe"]}, - "service_mode": {"enum": ["training", "live"]} + "service_mode": {"enum": ["training", "live"]}, + "provider_restriction": {"enum": [None, "three", "o2", "vodafone", "ee"]} }, - "required": ["broadcast_channel", "service_mode"] + "required": ["broadcast_channel", "service_mode", "provider_restriction"] } diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 961d1567c..6d20d1508 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3659,6 +3659,7 @@ def test_set_as_broadcast_service_sets_broadcast_channel( data = { 'broadcast_channel': channel, 'service_mode': 'live', + 'provider_restriction': None, } resp = client.post( @@ -3687,6 +3688,7 @@ def test_set_as_broadcast_service_updates_channel_for_broadcast_service( data=json.dumps({ 'broadcast_channel': "test", 'service_mode': 'training', + 'provider_restriction': None, }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3708,6 +3710,7 @@ def test_set_as_broadcast_service_rejects_unknown_channels( data = { 'broadcast_channel': channel, 'service_mode': 'live', + 'provider_restriction': None, } resp = client.post( @@ -3719,7 +3722,10 @@ def test_set_as_broadcast_service_rejects_unknown_channels( def test_set_as_broadcast_service_rejects_if_no_channel(client, notify_db, sample_service, broadcast_organisation): - data = {'service_mode': 'training'} + data = { + 'service_mode': 'training', + 'provider_restriction': None, + } resp = client.post( '/service/{}/set-as-broadcast-service'.format(sample_service.id), @@ -3741,6 +3747,7 @@ def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_p data=json.dumps({ 'broadcast_channel': "severe", 'service_mode': 'training', + 'provider_restriction': None, }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3763,6 +3770,7 @@ def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_br data=json.dumps({ 'broadcast_channel': "severe", 'service_mode': 'live', + 'provider_restriction': None, }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3782,6 +3790,7 @@ def test_set_as_broadcast_service_sets_count_as_live_to_false(client, notify_db, data=json.dumps({ 'broadcast_channel': "severe", 'service_mode': 'live', + 'provider_restriction': None, }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3801,6 +3810,7 @@ def test_set_as_broadcast_service_sets_service_org_to_broadcast_org(client, noti data=json.dumps({ 'broadcast_channel': "severe", 'service_mode': 'training', + 'provider_restriction': None, }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3820,7 +3830,8 @@ def test_set_as_broadcast_service_does_not_error_if_run_on_a_service_that_is_alr '/service/{}/set-as-broadcast-service'.format(sample_service.id), data=json.dumps({ 'broadcast_channel': "severe", - 'service_mode': "live" + 'service_mode': "live", + 'provider_restriction': None, }), headers=[('Content-Type', 'application/json'), create_authorization_header()] ) @@ -3838,6 +3849,7 @@ def test_set_as_broadcast_service_sets_service_to_live_mode( data = { 'broadcast_channel': 'severe', 'service_mode': 'live', + 'provider_restriction': None, } resp = client.post( @@ -3862,6 +3874,7 @@ def test_set_as_broadcast_service_sets_service_to_training_mode( data = { 'broadcast_channel': 'severe', 'service_mode': 'training', + 'provider_restriction': None, } resp = client.post( @@ -3882,6 +3895,7 @@ def test_set_as_broadcast_service_rejects_unknown_service_mode( data = { 'broadcast_channel': 'severe', 'service_mode': service_mode, + 'provider_restriction': None, } resp = client.post( @@ -3893,7 +3907,103 @@ def test_set_as_broadcast_service_rejects_unknown_service_mode( def test_set_as_broadcast_service_rejects_if_no_service_mode(client, notify_db, sample_service, broadcast_organisation): - data = {'broadcast_channel': 'severe'} + data = { + 'broadcast_channel': 'severe', + 'provider_restriction': None, + } + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 400 + + +@pytest.mark.parametrize('provider', [None, "three", "ee", "vodafone", "o2"]) +def test_set_as_broadcast_service_sets_mobile_provider_restriction( + client, notify_db, sample_service, broadcast_organisation, provider +): + assert sample_service.service_broadcast_settings is None + data = { + 'broadcast_channel': 'severe', + 'service_mode': 'live', + 'provider_restriction': provider + } + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['name'] == 'Sample service' + assert result['data']['allowed_broadcast_provider'] == provider + + records = ServiceBroadcastSettings.query.filter_by(service_id=sample_service.id).all() + assert len(records) == 1 + assert records[0].service_id == sample_service.id + assert records[0].provider == provider + + +@pytest.mark.parametrize('provider', [None, "vodafone"]) +def test_set_as_broadcast_service_updates_mobile_provider_restriction( + client, notify_db, sample_broadcast_service, broadcast_organisation, provider +): + sample_broadcast_service.service_broadcast_settings.provider = "o2" + notify_db.session.add(sample_broadcast_service) + notify_db.session.commit() + assert sample_broadcast_service.service_broadcast_settings.provider == "o2" + + data = { + 'broadcast_channel': 'severe', + 'service_mode': 'live', + 'provider_restriction': provider + } + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + result = resp.json + assert resp.status_code == 200 + assert result['data']['name'] == 'Sample broadcast service' + assert result['data']['allowed_broadcast_provider'] == provider + + records = ServiceBroadcastSettings.query.filter_by(service_id=sample_broadcast_service.id).all() + assert len(records) == 1 + assert records[0].service_id == sample_broadcast_service.id + assert records[0].provider == provider + + +@pytest.mark.parametrize('provider', ["three, o2", "giffgaff", "", "None"]) +def test_set_as_broadcast_service_rejects_unknown_provider_restriction( + client, notify_db, sample_service, broadcast_organisation, provider +): + data = { + 'broadcast_channel': 'test', + 'service_mode': 'live', + 'provider_restriction': provider + } + + resp = client.post( + '/service/{}/set-as-broadcast-service'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert resp.status_code == 400 + + +def test_set_as_broadcast_service_errors_if_no_mobile_provider_restriction( + client, notify_db, sample_service, broadcast_organisation +): + data = { + 'broadcast_channel': 'severe', + 'service_mode': 'live', + } resp = client.post( '/service/{}/set-as-broadcast-service'.format(sample_service.id), From d846ed79d2d6abb682c36663451c04700702721b Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 12 Feb 2021 09:51:05 +0000 Subject: [PATCH 11/15] Improve tests and remove unneeded code Some of the fixtures weren't needed so have been removed. I've also moved from using `client.post` to using `admin_request.post` which saves a bit of code too. Also one small assertion tidied up to make it a bit stronger regarding permissions. --- tests/app/service/test_rest.py | 273 ++++++++++++++++----------------- 1 file changed, 134 insertions(+), 139 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6d20d1508..128afb392 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3653,7 +3653,7 @@ def test_get_returned_letter(admin_request, sample_letter_template): @pytest.mark.parametrize('channel', ["test", "severe"]) def test_set_as_broadcast_service_sets_broadcast_channel( - client, notify_db, sample_service, broadcast_organisation, channel + admin_request, sample_service, broadcast_organisation, channel ): assert sample_service.service_broadcast_settings is None data = { @@ -3662,13 +3662,11 @@ def test_set_as_broadcast_service_sets_broadcast_channel( 'provider_restriction': None, } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 assert result['data']['name'] == 'Sample service' assert result['data']['broadcast_channel'] == channel @@ -3679,21 +3677,21 @@ def test_set_as_broadcast_service_sets_broadcast_channel( def test_set_as_broadcast_service_updates_channel_for_broadcast_service( - client, notify_db, sample_broadcast_service, broadcast_organisation + admin_request, sample_broadcast_service ): assert sample_broadcast_service.broadcast_channel == "severe" - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), - data=json.dumps({ - 'broadcast_channel': "test", - 'service_mode': 'training', - 'provider_restriction': None, - }), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + data = { + 'broadcast_channel': "test", + 'service_mode': 'training', + 'provider_restriction': None, + } + + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_broadcast_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 assert result['data']['name'] == 'Sample broadcast service' assert result['data']['broadcast_channel'] == "test" @@ -3705,7 +3703,7 @@ def test_set_as_broadcast_service_updates_channel_for_broadcast_service( @pytest.mark.parametrize('channel', ["government", "extreme", "exercise", "random", ""]) def test_set_as_broadcast_service_rejects_unknown_channels( - client, notify_db, sample_service, broadcast_organisation, channel + admin_request, sample_service, broadcast_organisation, channel ): data = { 'broadcast_channel': channel, @@ -3713,46 +3711,48 @@ def test_set_as_broadcast_service_rejects_unknown_channels( 'provider_restriction': None, } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, + _expected_status=400, ) - assert resp.status_code == 400 -def test_set_as_broadcast_service_rejects_if_no_channel(client, notify_db, sample_service, broadcast_organisation): +def test_set_as_broadcast_service_rejects_if_no_channel( + admin_request, notify_db, sample_service, broadcast_organisation +): data = { 'service_mode': 'training', 'provider_restriction': None, } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, + _expected_status=400, ) - assert resp.status_code == 400 def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_permissions( - client, notify_db, sample_service, broadcast_organisation + admin_request, sample_service, broadcast_organisation ): current_permissions = [p.permission for p in sample_service.permissions] assert len(current_permissions) > 0 - assert current_permissions != [BROADCAST_TYPE] + assert BROADCAST_TYPE not in current_permissions - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps({ - 'broadcast_channel': "severe", - 'service_mode': 'training', - 'provider_restriction': None, - }), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + data = { + 'broadcast_channel': "severe", + 'service_mode': 'training', + 'provider_restriction': None, + } + + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 assert result['data']['permissions'] == [BROADCAST_TYPE] permissions = ServicePermission.query.filter_by(service_id=sample_service.id).all() @@ -3760,62 +3760,64 @@ def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_p def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_broadcast_service( - client, notify_db, sample_broadcast_service, broadcast_organisation + admin_request, sample_broadcast_service ): current_permissions = [p.permission for p in sample_broadcast_service.permissions] assert current_permissions == [BROADCAST_TYPE] - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), - data=json.dumps({ - 'broadcast_channel': "severe", - 'service_mode': 'live', - 'provider_restriction': None, - }), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + data = { + 'broadcast_channel': "severe", + 'service_mode': 'live', + 'provider_restriction': None, + } + + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_broadcast_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 assert result['data']['permissions'] == [BROADCAST_TYPE] permissions = ServicePermission.query.filter_by(service_id=sample_broadcast_service.id).all() assert [p.permission for p in permissions] == [BROADCAST_TYPE] -def test_set_as_broadcast_service_sets_count_as_live_to_false(client, notify_db, sample_service, broadcast_organisation): +def test_set_as_broadcast_service_sets_count_as_live_to_false( + admin_request, sample_service, broadcast_organisation +): assert sample_service.count_as_live == True - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps({ - 'broadcast_channel': "severe", - 'service_mode': 'live', - 'provider_restriction': None, - }), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + data = { + 'broadcast_channel': "severe", + 'service_mode': 'live', + 'provider_restriction': None, + } + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 assert result['data']['count_as_live'] == False service_from_db = Service.query.filter_by(id=sample_service.id).all()[0] assert service_from_db.count_as_live == False -def test_set_as_broadcast_service_sets_service_org_to_broadcast_org(client, notify_db, sample_service, broadcast_organisation): +def test_set_as_broadcast_service_sets_service_org_to_broadcast_org( + admin_request, sample_service, broadcast_organisation +): assert sample_service.organisation_id != current_app.config['BROADCAST_ORGANISATION_ID'] - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps({ - 'broadcast_channel': "severe", - 'service_mode': 'training', - 'provider_restriction': None, - }), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + data = { + 'broadcast_channel': "severe", + 'service_mode': 'training', + 'provider_restriction': None, + } + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 assert result['data']['organisation'] == current_app.config['BROADCAST_ORGANISATION_ID'] service_from_db = Service.query.filter_by(id=sample_service.id).all()[0] @@ -3823,24 +3825,23 @@ def test_set_as_broadcast_service_sets_service_org_to_broadcast_org(client, noti def test_set_as_broadcast_service_does_not_error_if_run_on_a_service_that_is_already_a_broadcast_service( - client, notify_db, sample_service, broadcast_organisation + admin_request, sample_service, broadcast_organisation ): + data = { + 'broadcast_channel': "severe", + 'service_mode': "live", + 'provider_restriction': None, + } for _ in range(2): - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps({ - 'broadcast_channel': "severe", - 'service_mode': "live", - 'provider_restriction': None, - }), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 def test_set_as_broadcast_service_sets_service_to_live_mode( - client, notify_db, sample_service, broadcast_organisation + admin_request, notify_db, sample_service, broadcast_organisation ): sample_service.restricted = True notify_db.session.add(sample_service) @@ -3852,19 +3853,17 @@ def test_set_as_broadcast_service_sets_service_to_live_mode( 'provider_restriction': None, } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 assert result['data']['name'] == 'Sample service' assert result['data']['restricted'] == False def test_set_as_broadcast_service_sets_service_to_training_mode( - client, notify_db, sample_broadcast_service, broadcast_organisation + admin_request, notify_db, sample_broadcast_service ): sample_broadcast_service.restricted = False notify_db.session.add(sample_broadcast_service) @@ -3877,20 +3876,18 @@ def test_set_as_broadcast_service_sets_service_to_training_mode( 'provider_restriction': None, } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_broadcast_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 assert result['data']['name'] == 'Sample broadcast service' assert result['data']['restricted'] == True @pytest.mark.parametrize('service_mode', ["testing", ""]) def test_set_as_broadcast_service_rejects_unknown_service_mode( - client, notify_db, sample_service, broadcast_organisation, service_mode + admin_request, sample_service, broadcast_organisation, service_mode ): data = { 'broadcast_channel': 'severe', @@ -3898,32 +3895,33 @@ def test_set_as_broadcast_service_rejects_unknown_service_mode( 'provider_restriction': None, } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, + _expected_status=400, ) - assert resp.status_code == 400 -def test_set_as_broadcast_service_rejects_if_no_service_mode(client, notify_db, sample_service, broadcast_organisation): +def test_set_as_broadcast_service_rejects_if_no_service_mode( + admin_request, sample_service, broadcast_organisation +): data = { 'broadcast_channel': 'severe', 'provider_restriction': None, } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, + _expected_status=400, ) - result = resp.json - assert resp.status_code == 400 @pytest.mark.parametrize('provider', [None, "three", "ee", "vodafone", "o2"]) def test_set_as_broadcast_service_sets_mobile_provider_restriction( - client, notify_db, sample_service, broadcast_organisation, provider + admin_request, sample_service, broadcast_organisation, provider ): assert sample_service.service_broadcast_settings is None data = { @@ -3932,13 +3930,11 @@ def test_set_as_broadcast_service_sets_mobile_provider_restriction( 'provider_restriction': provider } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 assert result['data']['name'] == 'Sample service' assert result['data']['allowed_broadcast_provider'] == provider @@ -3950,7 +3946,7 @@ def test_set_as_broadcast_service_sets_mobile_provider_restriction( @pytest.mark.parametrize('provider', [None, "vodafone"]) def test_set_as_broadcast_service_updates_mobile_provider_restriction( - client, notify_db, sample_broadcast_service, broadcast_organisation, provider + admin_request, notify_db, sample_broadcast_service, provider ): sample_broadcast_service.service_broadcast_settings.provider = "o2" notify_db.session.add(sample_broadcast_service) @@ -3963,13 +3959,12 @@ def test_set_as_broadcast_service_updates_mobile_provider_restriction( 'provider_restriction': provider } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_broadcast_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_broadcast_service.id, + _data=data, ) - result = resp.json - assert resp.status_code == 200 + assert result['data']['name'] == 'Sample broadcast service' assert result['data']['allowed_broadcast_provider'] == provider @@ -3981,7 +3976,7 @@ def test_set_as_broadcast_service_updates_mobile_provider_restriction( @pytest.mark.parametrize('provider', ["three, o2", "giffgaff", "", "None"]) def test_set_as_broadcast_service_rejects_unknown_provider_restriction( - client, notify_db, sample_service, broadcast_organisation, provider + admin_request, sample_service, broadcast_organisation, provider ): data = { 'broadcast_channel': 'test', @@ -3989,25 +3984,25 @@ def test_set_as_broadcast_service_rejects_unknown_provider_restriction( 'provider_restriction': provider } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, + _expected_status=400, ) - assert resp.status_code == 400 def test_set_as_broadcast_service_errors_if_no_mobile_provider_restriction( - client, notify_db, sample_service, broadcast_organisation + admin_request, sample_service, broadcast_organisation ): data = { 'broadcast_channel': 'severe', 'service_mode': 'live', } - resp = client.post( - '/service/{}/set-as-broadcast-service'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()] + admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_service.id, + _data=data, + _expected_status=400, ) - assert resp.status_code == 400 From 42163813fe4fbfbf24bddfee7b69b2ed34e81041 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 12 Feb 2021 15:26:03 +0000 Subject: [PATCH 12/15] Hardcode service broadcast channel that API shows We are in a weird situation where at the moment, we have services with the broadcast permission that do not have a row in the service_broadcast_settings table and therefore do not have defined whether they should send messages on the 'test' or 'severe' channel. We currently get around this when we send broadcast messages out as such: https://github.com/alphagov/notifications-api/blob/master/app/celery/broadcast_message_tasks.py#L51 We need to something equivalent for the broadcast channel that the API says the service is on. In time, when we have added a row in the service_broadcast_settings table for every service with the broadcast permission then we can remove both of these two hardcodings. Note, one option would have been to move the default of `test` on to the `Service` model rather than having it in both the broadcast_message_tasks file and the `ServiceSchema` class. However, I went for the quickest thing which was to add it here. --- app/schemas.py | 11 +++++++++-- tests/app/service/test_rest.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 41d487608..f8b510f7b 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 +from app.models import ServicePermission, BROADCAST_TYPE from app.dao.permissions_dao import permission_dao from app.utils import DATETIME_FORMAT_NO_TIMEZONE, get_template_instance @@ -242,7 +242,14 @@ class ServiceSchema(BaseSchema, UUIDsAsStringsMixin): return service.allowed_broadcast_provider def _get_broadcast_channel(self, service): - return service.broadcast_channel + # 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 def get_letter_logo_filename(self, service): return service.letter_branding and service.letter_branding.filename diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 128afb392..88a714349 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -292,6 +292,38 @@ def test_get_service_by_id_returns_allowed_broadcast_provider(notify_db, admin_r 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 +): + assert sample_broadcast_service.broadcast_channel == 'severe' + + 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 == 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'] == 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) From f9c87bafa3aa1bfa2bac5a7c860c7e1f3b381ff8 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 12 Feb 2021 17:22:09 +0000 Subject: [PATCH 13/15] Add `go_live_at` timestamp to set_as_broadcast_service Note, I haven't added anything for the `go_live_user` because it doesn't quite make sense because here a user isn't requesting to go live. So there should be no reason to record this. We will in time though want to add audit events to capture every change to the service broadcast settings, that will actually capture who has done what. --- app/service/rest.py | 8 ++++++++ tests/app/service/test_rest.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/app/service/rest.py b/app/service/rest.py index 5e3754e1b..49b93e4ab 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1116,6 +1116,14 @@ def set_as_broadcast_service(service_id): service.count_as_live = False + if data["service_mode"] == "live": + if service.restricted == True: + # Only update the go live at timestamp if this if moving from training mode + # to live mode, not if it's moving from one type of live mode service to another + service.go_live_at = datetime.utcnow() + else: + service.go_live_at = None + service.restricted = True if data["service_mode"] == "live": service.restricted = False diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 88a714349..1be4e1fa3 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3872,6 +3872,7 @@ def test_set_as_broadcast_service_does_not_error_if_run_on_a_service_that_is_alr ) +@freeze_time('2021-02-02') def test_set_as_broadcast_service_sets_service_to_live_mode( admin_request, notify_db, sample_service, broadcast_organisation ): @@ -3879,6 +3880,7 @@ def test_set_as_broadcast_service_sets_service_to_live_mode( notify_db.session.add(sample_service) notify_db.session.commit() assert sample_service.restricted == True + assert sample_service.go_live_at == None data = { 'broadcast_channel': 'severe', 'service_mode': 'live', @@ -3892,15 +3894,43 @@ def test_set_as_broadcast_service_sets_service_to_live_mode( ) assert result['data']['name'] == 'Sample service' assert result['data']['restricted'] == False + assert result['data']['go_live_at'] == '2021-02-02 00:00:00.000000' + + +def test_set_as_broadcast_service_doesnt_override_existing_go_live_at( + admin_request, notify_db, sample_broadcast_service +): + sample_broadcast_service.restricted = False + sample_broadcast_service.go_live_at = datetime(2021, 1, 1) + notify_db.session.add(sample_broadcast_service) + notify_db.session.commit() + assert sample_broadcast_service.restricted == False + assert sample_broadcast_service.go_live_at is not None + data = { + 'broadcast_channel': 'severe', + 'service_mode': 'live', + 'provider_restriction': None, + } + + result = admin_request.post( + 'service.set_as_broadcast_service', + service_id=sample_broadcast_service.id, + _data=data, + ) + assert result['data']['name'] == 'Sample broadcast service' + assert result['data']['restricted'] == False + assert result['data']['go_live_at'] == '2021-01-01 00:00:00.000000' def test_set_as_broadcast_service_sets_service_to_training_mode( admin_request, notify_db, sample_broadcast_service ): sample_broadcast_service.restricted = False + sample_broadcast_service.go_live_at = datetime(2021, 1, 1) notify_db.session.add(sample_broadcast_service) notify_db.session.commit() assert sample_broadcast_service.restricted == False + assert sample_broadcast_service.go_live_at is not None data = { 'broadcast_channel': 'severe', @@ -3915,6 +3945,7 @@ def test_set_as_broadcast_service_sets_service_to_training_mode( ) assert result['data']['name'] == 'Sample broadcast service' assert result['data']['restricted'] == True + assert result['data']['go_live_at'] is None @pytest.mark.parametrize('service_mode', ["testing", ""]) From 6fcda6debb3aa901e8b2195c843174312a6a4481 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 15 Feb 2021 14:54:36 +0000 Subject: [PATCH 14/15] Make set_as_broadcast_service use a single DB commit We don't want things in a half state if there is an error during the method. Therefore, we move it all into a single function that is wrapped in a transaction. Note, we copy the approach of https://github.com/alphagov/notifications-api/blob/master/app/dao/services_dao.py#L293 by having a single new dao function that does all the DB work. --- app/dao/broadcast_service_dao.py | 65 +++++++++++++++++++++++ app/dao/service_broadcast_settings_dao.py | 17 ------ app/service/rest.py | 45 +++------------- tests/app/conftest.py | 2 +- tests/app/service/test_rest.py | 1 - 5 files changed, 73 insertions(+), 57 deletions(-) create mode 100644 app/dao/broadcast_service_dao.py delete mode 100644 app/dao/service_broadcast_settings_dao.py diff --git a/app/dao/broadcast_service_dao.py b/app/dao/broadcast_service_dao.py new file mode 100644 index 000000000..7ff40315e --- /dev/null +++ b/app/dao/broadcast_service_dao.py @@ -0,0 +1,65 @@ +from datetime import datetime + +from flask import current_app + +from app import db +from app.models import ServiceBroadcastSettings, ServicePermission, Organisation, BROADCAST_TYPE +from app.dao.dao_utils import transactional + + +@transactional +def set_broadcast_service_type(service, service_mode, broadcast_channel, provider_restriction): + insert_or_update_service_broadcast_settings( + service, channel=broadcast_channel, provider_restriction=provider_restriction + ) + + # Remove all permissions and add broadcast permission + if not service.has_permission(BROADCAST_TYPE): + service_permission = ServicePermission(service_id=service.id, permission=BROADCAST_TYPE) + db.session.add(service_permission) + + ServicePermission.query.filter( + ServicePermission.service_id == service.id, + ServicePermission.permission != BROADCAST_TYPE + ).delete() + + # Refresh the service object as it has references to the service permissions but we don't yet + # want to commit the permission changes incase all of this needs to rollback + db.session.refresh(service) + + # Set service count as live false always + service.count_as_live = False + + # Set service into training mode or live mode + if service_mode == "live": + if service.restricted == True: + # Only update the go live at timestamp if this if moving from training mode + # to live mode, not if it's moving from one type of live mode service to another + service.go_live_at = datetime.utcnow() + service.restricted = False + else: + service.restricted = True + service.go_live_at = None + + # Add service to organisation + organisation = Organisation.query.filter_by( + id=current_app.config['BROADCAST_ORGANISATION_ID'] + ).one() + service.organisation_id = organisation.id + service.organisation_type = organisation.organisation_type + service.crown = organisation.crown + + db.session.add(service) + + +def insert_or_update_service_broadcast_settings(service, channel, provider_restriction=None): + if not service.service_broadcast_settings: + settings = ServiceBroadcastSettings() + settings.service = service + settings.channel = channel + settings.provider = provider_restriction + db.session.add(settings) + else: + service.service_broadcast_settings.channel = channel + service.service_broadcast_settings.provider = provider_restriction + db.session.add(service.service_broadcast_settings) diff --git a/app/dao/service_broadcast_settings_dao.py b/app/dao/service_broadcast_settings_dao.py deleted file mode 100644 index 8835fbae9..000000000 --- a/app/dao/service_broadcast_settings_dao.py +++ /dev/null @@ -1,17 +0,0 @@ -from app import db -from app.models import ServiceBroadcastSettings -from app.dao.dao_utils import transactional - - -@transactional -def insert_or_update_service_broadcast_settings(service, channel, provider_restriction=None): - if not service.service_broadcast_settings: - settings = ServiceBroadcastSettings() - settings.service = service - settings.channel = channel - settings.provider = provider_restriction - db.session.add(settings) - else: - service.service_broadcast_settings.channel = channel - service.service_broadcast_settings.provider = provider_restriction - db.session.add(service.service_broadcast_settings) diff --git a/app/service/rest.py b/app/service/rest.py index 49b93e4ab..d0c6af347 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -31,7 +31,6 @@ from app.dao.fact_notification_status_dao import ( from app.dao.inbound_numbers_dao import dao_allocate_number_for_service from app.dao.organisation_dao import ( dao_get_organisation_by_service_id, - dao_add_service_to_organisation, ) from app.dao.returned_letters_dao import ( fetch_most_recent_returned_letter, @@ -45,11 +44,7 @@ from app.dao.service_contact_list_dao import ( dao_get_contact_list_by_id, save_service_contact_list, ) -from app.dao.service_permissions_dao import ( - dao_add_service_permission, - dao_fetch_service_permissions, - dao_remove_service_permission, -) +from app.dao.broadcast_service_dao import set_broadcast_service_type from app.dao.service_data_retention_dao import ( fetch_service_data_retention, fetch_service_data_retention_by_id, @@ -81,9 +76,6 @@ from app.dao.services_dao import ( dao_update_service, get_services_by_partial_name, ) -from app.dao.service_broadcast_settings_dao import ( - insert_or_update_service_broadcast_settings -) from app.dao.service_guest_list_dao import ( dao_fetch_service_guest_list, dao_add_and_commit_guest_list_contacts, @@ -113,15 +105,12 @@ from app.letters.utils import letter_print_day from app.models import ( KEY_TYPE_NORMAL, LETTER_TYPE, - BROADCAST_TYPE, NOTIFICATION_CANCELLED, Permission, Service, EmailBranding, LetterBranding, - ServiceContactList, - ServiceBroadcastSettings, - ServicePermission + ServiceContactList ) from app.notifications.process_notifications import persist_notification, send_notification_to_queue from app.schema_validation import validate @@ -1105,32 +1094,12 @@ def set_as_broadcast_service(service_id): data = validate(request.get_json(), service_broadcast_settings_schema) service = dao_fetch_service_by_id(service_id) - insert_or_update_service_broadcast_settings( - service, channel=data["broadcast_channel"], provider_restriction=data.get("provider_restriction") + set_broadcast_service_type( + service, + service_mode=data["service_mode"], + broadcast_channel=data["broadcast_channel"], + provider_restriction=data["provider_restriction"] ) - current_service_permissions = dao_fetch_service_permissions(service.id) - for permission in current_service_permissions: - dao_remove_service_permission(service.id, permission.permission) - dao_add_service_permission(service.id, BROADCAST_TYPE) - - service.count_as_live = False - - if data["service_mode"] == "live": - if service.restricted == True: - # Only update the go live at timestamp if this if moving from training mode - # to live mode, not if it's moving from one type of live mode service to another - service.go_live_at = datetime.utcnow() - else: - service.go_live_at = None - - service.restricted = True - if data["service_mode"] == "live": - service.restricted = False - - dao_update_service(service) - - dao_add_service_to_organisation(service, current_app.config['BROADCAST_ORGANISATION_ID']) - data = service_schema.dump(service).data return jsonify(data=data) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 33e68b630..ce91c2e5a 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -16,7 +16,7 @@ from app.dao.jobs_dao import dao_create_job from app.dao.notifications_dao import dao_create_notification from app.dao.organisation_dao import dao_create_organisation, dao_add_service_to_organisation from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) -from app.dao.service_broadcast_settings_dao import insert_or_update_service_broadcast_settings +from app.dao.broadcast_service_dao import insert_or_update_service_broadcast_settings from app.dao.templates_dao import dao_create_template from app.dao.users_dao import create_secret_code, create_user_code from app.history_meta import create_history diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 1be4e1fa3..34b116393 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -37,7 +37,6 @@ from app.models import ( INBOUND_SMS_TYPE, NOTIFICATION_RETURNED_LETTER, UPLOAD_LETTERS, - ServiceBroadcastSettings, ) from tests import create_authorization_header from tests.app.db import ( From abb3b3307c55c5d3a6ebff8f73818ed8ed47fca2 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Tue, 16 Feb 2021 10:27:10 +0000 Subject: [PATCH 15/15] Fix flake8 --- app/dao/broadcast_service_dao.py | 6 +++--- tests/app/service/test_rest.py | 24 ++++++++++++------------ 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/dao/broadcast_service_dao.py b/app/dao/broadcast_service_dao.py index 7ff40315e..d228bfc31 100644 --- a/app/dao/broadcast_service_dao.py +++ b/app/dao/broadcast_service_dao.py @@ -26,13 +26,13 @@ def set_broadcast_service_type(service, service_mode, broadcast_channel, provide # Refresh the service object as it has references to the service permissions but we don't yet # want to commit the permission changes incase all of this needs to rollback db.session.refresh(service) - + # Set service count as live false always service.count_as_live = False # Set service into training mode or live mode if service_mode == "live": - if service.restricted == True: + if service.restricted: # Only update the go live at timestamp if this if moving from training mode # to live mode, not if it's moving from one type of live mode service to another service.go_live_at = datetime.utcnow() @@ -41,7 +41,7 @@ def set_broadcast_service_type(service, service_mode, broadcast_channel, provide service.restricted = True service.go_live_at = None - # Add service to organisation + # Add service to organisation organisation = Organisation.query.filter_by( id=current_app.config['BROADCAST_ORGANISATION_ID'] ).one() diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 34b116393..66712b2c6 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -306,7 +306,7 @@ def test_get_service_by_id_for_service_with_broadcast_permission_sets_channel_as ): service = create_service(service_permissions=[BROADCAST_TYPE]) assert BROADCAST_TYPE in [p.permission for p in service.permissions] - assert service.broadcast_channel == None + 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) @@ -320,7 +320,7 @@ def test_get_service_by_id_for_non_broadcast_service_sets_channel_as_none( 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'] == None + assert json_resp['data']['broadcast_channel'] is None @pytest.mark.parametrize('detailed', [True, False]) @@ -3816,7 +3816,7 @@ def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_br def test_set_as_broadcast_service_sets_count_as_live_to_false( admin_request, sample_service, broadcast_organisation ): - assert sample_service.count_as_live == True + assert sample_service.count_as_live is True data = { 'broadcast_channel': "severe", @@ -3828,10 +3828,10 @@ def test_set_as_broadcast_service_sets_count_as_live_to_false( service_id=sample_service.id, _data=data, ) - assert result['data']['count_as_live'] == False + assert result['data']['count_as_live'] is False service_from_db = Service.query.filter_by(id=sample_service.id).all()[0] - assert service_from_db.count_as_live == False + assert service_from_db.count_as_live is False def test_set_as_broadcast_service_sets_service_org_to_broadcast_org( @@ -3878,8 +3878,8 @@ def test_set_as_broadcast_service_sets_service_to_live_mode( sample_service.restricted = True notify_db.session.add(sample_service) notify_db.session.commit() - assert sample_service.restricted == True - assert sample_service.go_live_at == None + assert sample_service.restricted is True + assert sample_service.go_live_at is None data = { 'broadcast_channel': 'severe', 'service_mode': 'live', @@ -3892,7 +3892,7 @@ def test_set_as_broadcast_service_sets_service_to_live_mode( _data=data, ) assert result['data']['name'] == 'Sample service' - assert result['data']['restricted'] == False + assert result['data']['restricted'] is False assert result['data']['go_live_at'] == '2021-02-02 00:00:00.000000' @@ -3903,7 +3903,7 @@ def test_set_as_broadcast_service_doesnt_override_existing_go_live_at( sample_broadcast_service.go_live_at = datetime(2021, 1, 1) notify_db.session.add(sample_broadcast_service) notify_db.session.commit() - assert sample_broadcast_service.restricted == False + assert sample_broadcast_service.restricted is False assert sample_broadcast_service.go_live_at is not None data = { 'broadcast_channel': 'severe', @@ -3917,7 +3917,7 @@ def test_set_as_broadcast_service_doesnt_override_existing_go_live_at( _data=data, ) assert result['data']['name'] == 'Sample broadcast service' - assert result['data']['restricted'] == False + assert result['data']['restricted'] is False assert result['data']['go_live_at'] == '2021-01-01 00:00:00.000000' @@ -3928,7 +3928,7 @@ def test_set_as_broadcast_service_sets_service_to_training_mode( sample_broadcast_service.go_live_at = datetime(2021, 1, 1) notify_db.session.add(sample_broadcast_service) notify_db.session.commit() - assert sample_broadcast_service.restricted == False + assert sample_broadcast_service.restricted is False assert sample_broadcast_service.go_live_at is not None data = { @@ -3943,7 +3943,7 @@ def test_set_as_broadcast_service_sets_service_to_training_mode( _data=data, ) assert result['data']['name'] == 'Sample broadcast service' - assert result['data']['restricted'] == True + assert result['data']['restricted'] is True assert result['data']['go_live_at'] is None