From 04d8678c27168cb3699505e464bbba6776e87db2 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 7 Sep 2021 13:24:09 +0100 Subject: [PATCH 1/4] Make it easy to develop with broadcast services Previously I had to handcraft some SQL to give myself access to a broadcast service I created locally. I've done this enough times that I think it's worth automating. --- app/commands.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/app/commands.py b/app/commands.py index c98ce9bf3..7844c942b 100644 --- a/app/commands.py +++ b/app/commands.py @@ -1,6 +1,7 @@ import csv import functools import itertools +import os import uuid from datetime import datetime, timedelta from decimal import Decimal @@ -43,6 +44,7 @@ from app.dao.organisation_dao import ( dao_add_service_to_organisation, dao_get_organisation_by_email_address, ) +from app.dao.permissions_dao import permission_dao from app.dao.provider_rates_dao import ( create_provider_rates as dao_create_provider_rates, ) @@ -75,6 +77,7 @@ from app.models import ( LetterBranding, Notification, Organisation, + Permission, Service, User, ) @@ -914,3 +917,32 @@ def populate_annual_billing_with_defaults(year, missing_services_only): for service in active_services: set_default_free_allowance_for_service(service, year) + + +@click.option('-u', '--user-id', required=True) +@notify_command(name='local-dev-broadcast-permissions') +def local_dev_broadcast_permissions(user_id): + if os.getenv('NOTIFY_ENVIRONMENT', '') not in ['development', 'test']: + current_app.logger.error('Can only be run in development') + return + + user = User.query.filter_by(id=user_id).one() + + user_broadcast_services = Service.query.filter( + Service.permissions.any(permission='broadcast'), + Service.users.any(id=user_id) + ) + + for service in user_broadcast_services: + permission_list = [ + Permission(service_id=service.id, user_id=user_id, permission=permission) + for permission in [ + 'reject_broadcasts', 'cancel_broadcasts', # required to create / approve + 'create_broadcasts', 'approve_broadcasts', # minimum for testing + 'manage_templates', # unlikely but might be useful + ] + ] + + permission_dao.set_user_service_permission( + user, service, permission_list, _commit=True, replace=True + ) From 922fd2f333f62bc7ce1069eae913ed43e9b63652 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 8 Sep 2021 09:45:32 +0100 Subject: [PATCH 2/4] Support testing commands and add first test We have a lot of commands and it's important we test the ones that are meant to be used in the future to ensure they work when they're needed. Testing Flask commands is usually easy as written in their docs [1], but I had to make some changes to the way we decorate the command functions so they can work with test DB objects - I couldn't find any example of someone else encountering the same problem. [1]: https://flask.palletsprojects.com/en/2.0.x/testing/#testing-cli-commands --- app/commands.py | 22 +++++++++++++++------- tests/app/test_commands.py | 26 ++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) create mode 100644 tests/app/test_commands.py diff --git a/app/commands.py b/app/commands.py index 7844c942b..2adfb94a4 100644 --- a/app/commands.py +++ b/app/commands.py @@ -94,17 +94,25 @@ class notify_command: self.name = name def __call__(self, func): - # we need to call the flask with_appcontext decorator to ensure the config is loaded, db connected etc etc. - # we also need to use functools.wraps to carry through the names and docstrings etc of the functions. - # Then we need to turn it into a click.Command - that's what command_group.add_command expects. - @click.command(name=self.name) - @functools.wraps(func) - @flask.cli.with_appcontext + decorators = [ + click.command(name=self.name), # turn it into a click.Command + functools.wraps(func) # carry through function name, docstrings, etc. + ] + + # in the test environment the app context is already provided and having + # another will lead to the test db connection being closed prematurely + if os.getenv('NOTIFY_ENVIRONMENT', '') != 'test': + # with_appcontext ensures the config is loaded, db connected, etc. + decorators.insert(0, flask.cli.with_appcontext) + def wrapper(*args, **kwargs): return func(*args, **kwargs) - command_group.add_command(wrapper) + for decorator in decorators: + # this syntax is equivalent to e.g. "@flask.cli.with_appcontext" + wrapper = decorator(wrapper) + command_group.add_command(wrapper) return wrapper diff --git a/tests/app/test_commands.py b/tests/app/test_commands.py new file mode 100644 index 000000000..adc3d76e4 --- /dev/null +++ b/tests/app/test_commands.py @@ -0,0 +1,26 @@ +import uuid + +from app.commands import local_dev_broadcast_permissions +from app.dao.services_dao import dao_add_user_to_service +from tests.app.db import create_user + + +def test_local_dev_broadcast_permissions( + sample_service, + sample_broadcast_service, + notify_api, +): + # create_user will pull existing unless email is unique + user = create_user(email=f'{uuid.uuid4()}@example.com') + dao_add_user_to_service(sample_service, user) + dao_add_user_to_service(sample_broadcast_service, user) + + assert len(user.get_permissions(sample_service.id)) == 0 + assert len(user.get_permissions(sample_broadcast_service.id)) == 0 + + notify_api.test_cli_runner().invoke( + local_dev_broadcast_permissions, ['-u', user.id] + ) + + assert len(user.get_permissions(sample_service.id)) == 0 + assert len(user.get_permissions(sample_broadcast_service.id)) > 0 From c9ea85938afc23b4de6752b9c6141185307c0c5c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 8 Sep 2021 11:16:46 +0100 Subject: [PATCH 3/4] Refactor "create_user" to actually create a user This switches a number of fixtures to use "sample_user", which is equivalent to calling the previous "create_user" function when it used to default the email to "notify@...". --- tests/app/conftest.py | 58 ++++++++++++++++++-------------------- tests/app/db.py | 4 +-- tests/app/test_commands.py | 5 +--- 3 files changed, 30 insertions(+), 37 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index bc2038886..0794000e2 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -71,11 +71,11 @@ def rmock(): @pytest.fixture(scope='function') -def service_factory(notify_db, notify_db_session): +def service_factory(notify_db, notify_db_session, sample_user): class ServiceFactory(object): def get(self, service_name, user=None, template_type=None, email_from=None): if not user: - user = create_user() + user = sample_user if not email_from: email_from = service_name @@ -106,7 +106,9 @@ def service_factory(notify_db, notify_db_session): @pytest.fixture(scope='function') def sample_user(notify_db_session): - return create_user() + return create_user( + email='notify@digital.cabinet-office.gov.uk' + ) @pytest.fixture(scope='function') @@ -131,8 +133,7 @@ def sample_sms_code(notify_db_session): @pytest.fixture(scope='function') -def sample_service(notify_db_session): - user = create_user() +def sample_service(notify_db_session, sample_user): service_name = 'Sample service' email_from = service_name.lower().replace(' ', '.') @@ -141,23 +142,22 @@ def sample_service(notify_db_session): 'message_limit': 1000, 'restricted': False, 'email_from': email_from, - 'created_by': user, + 'created_by': sample_user, 'crown': True } service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user, service_permissions=None) + dao_create_service(service, sample_user, service_permissions=None) else: - if user not in service.users: - dao_add_user_to_service(service, user) + if sample_user not in service.users: + dao_add_user_to_service(service, sample_user) return service @pytest.fixture(scope='function') -def sample_broadcast_service(notify_db_session, broadcast_organisation): - user = create_user() +def sample_broadcast_service(notify_db_session, broadcast_organisation, sample_user): service_name = 'Sample broadcast service' email_from = service_name.lower().replace(' ', '.') @@ -166,19 +166,19 @@ def sample_broadcast_service(notify_db_session, broadcast_organisation): 'message_limit': 1000, 'restricted': False, 'email_from': email_from, - 'created_by': user, + 'created_by': sample_user, 'crown': True, 'count_as_live': False, } service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user, service_permissions=[BROADCAST_TYPE]) + dao_create_service(service, sample_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) + if sample_user not in service.users: + dao_add_user_to_service(service, sample_user) return service @@ -201,8 +201,7 @@ def _sample_service_custom_letter_contact_block(sample_service): @pytest.fixture(scope='function') -def sample_template(notify_db_session): - user = create_user() +def sample_template(notify_db_session, sample_user): service = create_service(service_permissions=[EMAIL_TYPE, SMS_TYPE], check_if_service_exists=True) data = { @@ -210,7 +209,7 @@ def sample_template(notify_db_session): 'template_type': 'sms', 'content': 'This is a template:\nwith a newline', 'service': service, - 'created_by': user, + 'created_by': sample_user, 'archived': False, 'hidden': False, 'process_type': 'normal' @@ -240,15 +239,14 @@ def sample_sms_template_with_html(sample_service): @pytest.fixture(scope='function') -def sample_email_template(notify_db_session): - user = create_user() - service = create_service(user=user, service_permissions=[EMAIL_TYPE, SMS_TYPE], check_if_service_exists=True) +def sample_email_template(notify_db_session, sample_user): + service = create_service(user=sample_user, service_permissions=[EMAIL_TYPE, SMS_TYPE], check_if_service_exists=True) data = { 'name': 'Email Template Name', 'template_type': EMAIL_TYPE, 'content': 'This is a template', 'service': service, - 'created_by': user, + 'created_by': sample_user, 'subject': 'Email Subject' } template = Template(**data) @@ -551,18 +549,17 @@ def sample_invited_org_user(sample_user, sample_organisation): @pytest.fixture(scope='function') -def sample_user_service_permission(notify_db_session): - user = create_user() - service = create_service(user=user, check_if_service_exists=True) +def sample_user_service_permission(notify_db_session, sample_user): + service = create_service(user=sample_user, check_if_service_exists=True) permission = 'manage_settings' data = { - 'user': user, + 'user': sample_user, 'service': service, 'permission': permission } p_model = Permission.query.filter_by( - user=user, + user=sample_user, service=service, permission=permission).first() if not p_model: @@ -828,8 +825,7 @@ def letter_volumes_email_template(notify_service): @pytest.fixture -def notify_service(notify_db, notify_db_session): - user = create_user() +def notify_service(notify_db, notify_db_session, sample_user): service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) if not service: service = Service( @@ -837,13 +833,13 @@ def notify_service(notify_db, notify_db_session): message_limit=1000, restricted=False, email_from='notify.service', - created_by=user, + created_by=sample_user, prefix_sms=False, ) dao_create_service( service=service, service_id=current_app.config['NOTIFY_SERVICE_ID'], - user=user + user=sample_user ) data = { diff --git a/tests/app/db.py b/tests/app/db.py index a71abc313..227d305a2 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -81,7 +81,7 @@ from app.models import ( def create_user( *, mobile_number="+447700900986", - email="notify@digital.cabinet-office.gov.uk", + email=None, state='active', id_=None, name="Test User" @@ -89,7 +89,7 @@ def create_user( data = { 'id': id_ or uuid.uuid4(), 'name': name, - 'email_address': email, + 'email_address': email or f"{uuid.uuid4()}@digital.cabinet-office.gov.uk", 'password': 'password', 'mobile_number': mobile_number, 'state': state diff --git a/tests/app/test_commands.py b/tests/app/test_commands.py index adc3d76e4..dd8f1c15b 100644 --- a/tests/app/test_commands.py +++ b/tests/app/test_commands.py @@ -1,5 +1,3 @@ -import uuid - from app.commands import local_dev_broadcast_permissions from app.dao.services_dao import dao_add_user_to_service from tests.app.db import create_user @@ -10,8 +8,7 @@ def test_local_dev_broadcast_permissions( sample_broadcast_service, notify_api, ): - # create_user will pull existing unless email is unique - user = create_user(email=f'{uuid.uuid4()}@example.com') + user = create_user() dao_add_user_to_service(sample_service, user) dao_add_user_to_service(sample_broadcast_service, user) From 4d05b064fd32585a2e3b3acecc999cce9a5e4820 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 14 Sep 2021 09:37:26 +0100 Subject: [PATCH 4/4] Remove redundant DB fixtures in conftest.py These were only there to ensure a DB session existed for the test and are now included implicitly as one of the dependencies of the "sample_user" fixture. --- tests/app/conftest.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 0794000e2..808f4e1fc 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -71,7 +71,7 @@ def rmock(): @pytest.fixture(scope='function') -def service_factory(notify_db, notify_db_session, sample_user): +def service_factory(sample_user): class ServiceFactory(object): def get(self, service_name, user=None, template_type=None, email_from=None): if not user: @@ -133,7 +133,7 @@ def sample_sms_code(notify_db_session): @pytest.fixture(scope='function') -def sample_service(notify_db_session, sample_user): +def sample_service(sample_user): service_name = 'Sample service' email_from = service_name.lower().replace(' ', '.') @@ -157,7 +157,7 @@ def sample_service(notify_db_session, sample_user): @pytest.fixture(scope='function') -def sample_broadcast_service(notify_db_session, broadcast_organisation, sample_user): +def sample_broadcast_service(broadcast_organisation, sample_user): service_name = 'Sample broadcast service' email_from = service_name.lower().replace(' ', '.') @@ -201,7 +201,7 @@ def _sample_service_custom_letter_contact_block(sample_service): @pytest.fixture(scope='function') -def sample_template(notify_db_session, sample_user): +def sample_template(sample_user): service = create_service(service_permissions=[EMAIL_TYPE, SMS_TYPE], check_if_service_exists=True) data = { @@ -239,7 +239,7 @@ def sample_sms_template_with_html(sample_service): @pytest.fixture(scope='function') -def sample_email_template(notify_db_session, sample_user): +def sample_email_template(sample_user): service = create_service(user=sample_user, service_permissions=[EMAIL_TYPE, SMS_TYPE], check_if_service_exists=True) data = { 'name': 'Email Template Name', @@ -549,7 +549,7 @@ def sample_invited_org_user(sample_user, sample_organisation): @pytest.fixture(scope='function') -def sample_user_service_permission(notify_db_session, sample_user): +def sample_user_service_permission(sample_user): service = create_service(user=sample_user, check_if_service_exists=True) permission = 'manage_settings' @@ -825,7 +825,7 @@ def letter_volumes_email_template(notify_service): @pytest.fixture -def notify_service(notify_db, notify_db_session, sample_user): +def notify_service(sample_user): service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) if not service: service = Service(