From c12594949f3c7f9fa8dbbe66a557f22c244a3af2 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 28 Dec 2018 16:15:12 +0000 Subject: [PATCH 01/12] Refactor service_factory --- tests/app/conftest.py | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 6b04e83fe..38b483f0e 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -76,20 +76,26 @@ def service_factory(notify_db, notify_db_session): user = create_user() if not email_from: email_from = service_name - service = sample_service(notify_db, notify_db_session, service_name, user, email_from=email_from) + service = Service.query.filter_by(name=service_name).first() + if not service: + service = create_service( + email_from=email_from, + service_name=service_name, + service_permissions=None, + user=user, + ) if template_type == 'email': - sample_template( - notify_db, - notify_db_session, + create_template( + service, + template_name="Template Name", template_type=template_type, - subject_line=service.email_from, - service=service + subject=service.email_from, ) else: - sample_template( - notify_db, - notify_db_session, - service=service + create_template( + service, + template_name="Template Name", + template_type='sms', ) return service From 923703120b3dc5a21992717443cc35053d6b51ba Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 13:01:24 +0000 Subject: [PATCH 02/12] Check if test service exists before it gets created --- tests/app/conftest.py | 15 +++++++-------- tests/app/db.py | 28 +++++++++++++++------------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 38b483f0e..e77deb4a1 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -76,14 +76,13 @@ def service_factory(notify_db, notify_db_session): user = create_user() if not email_from: email_from = service_name - service = Service.query.filter_by(name=service_name).first() - if not service: - service = create_service( - email_from=email_from, - service_name=service_name, - service_permissions=None, - user=user, - ) + + service = create_service( + email_from=email_from, + service_name=service_name, + service_permissions=None, + user=user, + ) if template_type == 'email': create_template( service, diff --git a/tests/app/db.py b/tests/app/db.py index 02a2df85c..3783026e6 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -83,21 +83,23 @@ def create_service( organisation_type='central', postage='second' ): - service = Service( - name=service_name, - message_limit=message_limit, - restricted=restricted, - email_from=email_from if email_from else service_name.lower().replace(' ', '.'), - created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())), - prefix_sms=prefix_sms, - organisation_type=organisation_type, - postage=postage - ) + service = Service.query.filter_by(name=service_name).first() + if not service: + service = Service( + name=service_name, + message_limit=message_limit, + restricted=restricted, + email_from=email_from if email_from else service_name.lower().replace(' ', '.'), + created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())), + prefix_sms=prefix_sms, + organisation_type=organisation_type, + postage=postage + ) - dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) + dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) - service.active = active - service.research_mode = research_mode + service.active = active + service.research_mode = research_mode return service From 3306b9fc97e44b91caad8086e831e85f047bbb4f Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 14:18:52 +0000 Subject: [PATCH 03/12] use conditional in create_service to establish user --- tests/app/db.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/app/db.py b/tests/app/db.py index 3783026e6..e0ca9bddd 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -90,12 +90,11 @@ def create_service( message_limit=message_limit, restricted=restricted, email_from=email_from if email_from else service_name.lower().replace(' ', '.'), - created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())), + created_by=user if user else create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())), prefix_sms=prefix_sms, organisation_type=organisation_type, postage=postage ) - dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) service.active = active From 95115e7ae6ae3eefcb49196e015e728ee3e02409 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 14:34:02 +0000 Subject: [PATCH 04/12] Use create_service instead of sample_service when creating service permission for tests --- tests/app/conftest.py | 2 +- tests/app/db.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index e77deb4a1..ff817501e 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -802,7 +802,7 @@ def sample_user_service_permission( if user is None: user = create_user() if service is None: - service = sample_service(notify_db, notify_db_session, user=user) + service = create_service(user=user) data = { 'user': user, 'service': service, diff --git a/tests/app/db.py b/tests/app/db.py index e0ca9bddd..ee17c48cd 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -17,7 +17,7 @@ from app.dao.service_data_retention_dao import insert_service_data_retention from app.dao.service_inbound_api_dao import save_service_inbound_api from app.dao.service_permissions_dao import dao_add_service_permission from app.dao.service_sms_sender_dao import update_existing_sms_sender_with_inbound_number, dao_update_service_sms_sender -from app.dao.services_dao import dao_create_service +from app.dao.services_dao import dao_create_service, dao_add_user_to_service from app.dao.templates_dao import dao_create_template, dao_update_template from app.dao.users_dao import save_model_user from app.models import ( @@ -99,6 +99,9 @@ def create_service( service.active = active service.research_mode = research_mode + else: + if user not in service.users: + dao_add_user_to_service(service, user) return service From e8ce669b723e8c3a11ef322739b2278dc2fe4895 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 15:04:01 +0000 Subject: [PATCH 05/12] Use create_service instead of sample_service when creating sample_email_template for tests --- tests/app/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index ff817501e..bca4f3776 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -295,7 +295,7 @@ def sample_email_template( if user is None: user = create_user() if service is None: - service = sample_service(notify_db, notify_db_session, permissions=permissions) + service = create_service(user=user, service_permissions=permissions) data = { 'name': template_name, 'template_type': template_type, From 0bcf13d85cf1f5a22cc08810e231de4df803d6d6 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 15:16:00 +0000 Subject: [PATCH 06/12] sample_api_key uses create_service instead of sample_service --- tests/app/conftest.py | 2 +- tests/app/db.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index bca4f3776..cadced70d 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -350,7 +350,7 @@ def sample_api_key(notify_db, key_type=KEY_TYPE_NORMAL, name=None): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service() data = {'service': service, 'name': name or uuid.uuid4(), 'created_by': service.created_by, 'key_type': key_type} api_key = ApiKey(**data) save_model_api_key(api_key) diff --git a/tests/app/db.py b/tests/app/db.py index ee17c48cd..d6d4b9553 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -100,7 +100,7 @@ def create_service( service.active = active service.research_mode = research_mode else: - if user not in service.users: + if user and user not in service.users: dao_add_user_to_service(service, user) return service From a3310c2da6d7f7bb8cfcc6e2bf439b6bebb1cefd Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 15:20:30 +0000 Subject: [PATCH 07/12] sample_job uses create_service instead of sample_service --- tests/app/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index cadced70d..48fe278d2 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -382,7 +382,7 @@ def sample_job( archived=False ): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service() if template is None: template = sample_template(notify_db, notify_db_session, service=service) From d367daaf6edc684637dfb088015baba4e42cbd12 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 15:36:25 +0000 Subject: [PATCH 08/12] Some more conftest fixtures use create_service instead of sample_service --- tests/app/conftest.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 48fe278d2..8ecb0ae20 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -442,7 +442,7 @@ def sample_email_job(notify_db, service=None, template=None): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service() if template is None: template = sample_email_template( notify_db, @@ -550,7 +550,7 @@ def sample_notification( if created_at is None: created_at = datetime.utcnow() if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service() if template is None: template = sample_template(notify_db, notify_db_session, service=service) @@ -639,7 +639,7 @@ def sample_notification_with_api_key(notify_db, notify_db_session): @pytest.fixture(scope='function') def sample_email_notification(notify_db, notify_db_session): created_at = datetime.utcnow() - service = sample_service(notify_db, notify_db_session) + service = create_service() template = sample_email_template(notify_db, notify_db_session, service=service) job = sample_job(notify_db, notify_db_session, service=service, template=template) @@ -741,7 +741,7 @@ def sample_invited_user(notify_db, to_email_address=None): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service() if to_email_address is None: to_email_address = 'invited_user@digital.gov.uk' @@ -781,7 +781,7 @@ def sample_permission(notify_db, 'permission': permission } if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service() if service: data['service'] = service p_model = Permission.query.filter_by( From 154257027f8c69dd7e6df5c2fe8b272190d1259e Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 15:53:56 +0000 Subject: [PATCH 09/12] Nothing in conftest uses sample_service now :) --- tests/app/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 8ecb0ae20..abcb0f64a 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1036,7 +1036,7 @@ def notify_service(notify_db, notify_db_session): @pytest.fixture(scope='function') def sample_service_whitelist(notify_db, notify_db_session, service=None, email_address=None, mobile_number=None): if service is None: - service = sample_service(notify_db, notify_db_session) + service = create_service() if email_address: whitelisted_user = ServiceWhitelist.from_string(service.id, EMAIL_TYPE, email_address) From cde30de100bcf86b8558f3c4ea0ac8330f002783 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 16:14:09 +0000 Subject: [PATCH 10/12] Use create_template instead of sample_template in sample_notification --- tests/app/conftest.py | 2 +- .../notification_dao/test_notification_dao_template_usage.py | 2 +- tests/app/notifications/test_rest.py | 2 +- tests/app/template_statistics/test_rest.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index abcb0f64a..306eb2637 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -552,7 +552,7 @@ def sample_notification( if service is None: service = create_service() if template is None: - template = sample_template(notify_db, notify_db_session, service=service) + template = create_template(service=service) if job is None and api_key is None: # we didn't specify in test - lets create it diff --git a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py index 76800694f..88aaa783d 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py +++ b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py @@ -22,7 +22,7 @@ from tests.app.db import ( def test_last_template_usage_should_get_right_data(sample_notification): results = dao_get_last_template_usage(sample_notification.template_id, 'sms', sample_notification.service_id) - assert results.template.name == 'Template Name' + assert results.template.name == 'sms Template Name' assert results.template.template_type == 'sms' assert results.created_at == sample_notification.created_at assert results.template_id == sample_notification.template_id diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 0c9051d90..a035500cb 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -153,7 +153,7 @@ def test_get_all_notifications(client, sample_notification): assert notifications['notifications'][0]['to'] == '+447700900855' assert notifications['notifications'][0]['service'] == str(sample_notification.service_id) - assert notifications['notifications'][0]['body'] == "This is a template:\nwith a newline" + assert notifications['notifications'][0]['body'] == 'Dear Sir/Madam, Hello. Yours Truly, The Government.' def test_normal_api_key_returns_notifications_created_from_jobs_and_from_api( diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index b9559e134..1a37cc6e7 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -59,7 +59,7 @@ def test_get_template_statistics_for_service_by_day_returns_template_info(admin_ assert json_resp['data'][0]['count'] == 1 assert json_resp['data'][0]['template_id'] == str(sample_notification.template_id) - assert json_resp['data'][0]['template_name'] == 'Template Name' + assert json_resp['data'][0]['template_name'] == 'sms Template Name' assert json_resp['data'][0]['template_type'] == 'sms' assert json_resp['data'][0]['is_precompiled_letter'] is False From f79c0b03e7ca00c02f8a4d9f417e673883f8cc98 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 31 Dec 2018 16:19:41 +0000 Subject: [PATCH 11/12] Sample job uses create_template instead of sample template --- tests/app/conftest.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 306eb2637..407372d64 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -384,8 +384,7 @@ def sample_job( if service is None: service = create_service() if template is None: - template = sample_template(notify_db, notify_db_session, - service=service) + template = create_template(service=service) data = { 'id': uuid.uuid4(), 'service_id': service.id, From eea324a19d0e1b10a44700977ddadaa8a6724d85 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 2 Jan 2019 17:15:27 +0000 Subject: [PATCH 12/12] Add flag on create_service to decide whether we should also check if service exists --- tests/app/conftest.py | 28 +++++++++++++++------------- tests/app/db.py | 8 +++++--- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 407372d64..5b6aa2234 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -82,6 +82,7 @@ def service_factory(notify_db, notify_db_session): service_name=service_name, service_permissions=None, user=user, + check_if_service_exists=True, ) if template_type == 'email': create_template( @@ -200,7 +201,8 @@ def sample_service( def _sample_service_full_permissions(notify_db_session): service = create_service( service_name="sample service full permissions", - service_permissions=set(SERVICE_PERMISSION_TYPES) + service_permissions=set(SERVICE_PERMISSION_TYPES), + check_if_service_exists=True ) create_inbound_number('12345', service_id=service.id) return service @@ -233,7 +235,7 @@ def sample_template( if service is None: service = Service.query.filter_by(name='Sample service').first() if not service: - service = create_service(service_permissions=permissions) + service = create_service(service_permissions=permissions, check_if_service_exists=True) if created_by is None: created_by = create_user() @@ -295,7 +297,7 @@ def sample_email_template( if user is None: user = create_user() if service is None: - service = create_service(user=user, service_permissions=permissions) + service = create_service(user=user, service_permissions=permissions, check_if_service_exists=True) data = { 'name': template_name, 'template_type': template_type, @@ -350,7 +352,7 @@ def sample_api_key(notify_db, key_type=KEY_TYPE_NORMAL, name=None): if service is None: - service = create_service() + service = create_service(check_if_service_exists=True) data = {'service': service, 'name': name or uuid.uuid4(), 'created_by': service.created_by, 'key_type': key_type} api_key = ApiKey(**data) save_model_api_key(api_key) @@ -382,7 +384,7 @@ def sample_job( archived=False ): if service is None: - service = create_service() + service = create_service(check_if_service_exists=True) if template is None: template = create_template(service=service) data = { @@ -441,7 +443,7 @@ def sample_email_job(notify_db, service=None, template=None): if service is None: - service = create_service() + service = create_service(check_if_service_exists=True) if template is None: template = sample_email_template( notify_db, @@ -549,7 +551,7 @@ def sample_notification( if created_at is None: created_at = datetime.utcnow() if service is None: - service = create_service() + service = create_service(check_if_service_exists=True) if template is None: template = create_template(service=service) @@ -638,7 +640,7 @@ def sample_notification_with_api_key(notify_db, notify_db_session): @pytest.fixture(scope='function') def sample_email_notification(notify_db, notify_db_session): created_at = datetime.utcnow() - service = create_service() + service = create_service(check_if_service_exists=True) template = sample_email_template(notify_db, notify_db_session, service=service) job = sample_job(notify_db, notify_db_session, service=service, template=template) @@ -740,7 +742,7 @@ def sample_invited_user(notify_db, to_email_address=None): if service is None: - service = create_service() + service = create_service(check_if_service_exists=True) if to_email_address is None: to_email_address = 'invited_user@digital.gov.uk' @@ -780,7 +782,7 @@ def sample_permission(notify_db, 'permission': permission } if service is None: - service = create_service() + service = create_service(check_if_service_exists=True) if service: data['service'] = service p_model = Permission.query.filter_by( @@ -801,7 +803,7 @@ def sample_user_service_permission( if user is None: user = create_user() if service is None: - service = create_service(user=user) + service = create_service(user=user, check_if_service_exists=True) data = { 'user': user, 'service': service, @@ -1035,7 +1037,7 @@ def notify_service(notify_db, notify_db_session): @pytest.fixture(scope='function') def sample_service_whitelist(notify_db, notify_db_session, service=None, email_address=None, mobile_number=None): if service is None: - service = create_service() + service = create_service(check_if_service_exists=True) if email_address: whitelisted_user = ServiceWhitelist.from_string(service.id, EMAIL_TYPE, email_address) @@ -1060,7 +1062,7 @@ def sample_provider_rate(notify_db, notify_db_session, valid_from=None, rate=Non @pytest.fixture def sample_inbound_numbers(notify_db, notify_db_session, sample_service): - service = create_service(service_name='sample service 2') + service = create_service(service_name='sample service 2', check_if_service_exists=True) inbound_numbers = list() inbound_numbers.append(create_inbound_number(number='1', provider='mmg')) inbound_numbers.append(create_inbound_number(number='2', provider='mmg', active=False, service_id=service.id)) diff --git a/tests/app/db.py b/tests/app/db.py index d6d4b9553..5c4889057 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -81,10 +81,12 @@ def create_service( prefix_sms=True, message_limit=1000, organisation_type='central', - postage='second' + postage='second', + check_if_service_exists=False ): - service = Service.query.filter_by(name=service_name).first() - if not service: + if check_if_service_exists: + service = Service.query.filter_by(name=service_name).first() + if (not check_if_service_exists) or (check_if_service_exists and not service): service = Service( name=service_name, message_limit=message_limit,