From 114d4d84d497c1b057b875f019efc831ab7aba6c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 11 May 2017 15:22:58 +0100 Subject: [PATCH 1/6] Add service permissions DAO and refactor user service permission mock --- app/dao/service_permissions_dao.py | 41 ++++++++++ migrations/README | 8 +- .../0083_add_perm_types_and_svc_perm.py | 8 +- tests/app/conftest.py | 10 +-- tests/app/dao/test_service_permissions_dao.py | 82 +++++++++++++++++++ tests/app/db.py | 14 +++- tests/app/service/test_rest.py | 16 ++-- tests/app/user/test_rest.py | 6 +- tests/conftest.py | 3 +- 9 files changed, 165 insertions(+), 23 deletions(-) create mode 100644 app/dao/service_permissions_dao.py create mode 100644 tests/app/dao/test_service_permissions_dao.py diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py new file mode 100644 index 000000000..7ea9eb0d3 --- /dev/null +++ b/app/dao/service_permissions_dao.py @@ -0,0 +1,41 @@ +from sqlalchemy import exc + +from app import db +from app.dao.dao_utils import transactional +from app.models import Service, ServicePermission, SERVICE_PERMISSION_TYPES + + +def dao_fetch_service_permissions(service_id): + return ServicePermission.query.filter( + ServicePermission.service_id == service_id).all() + + +def make_service_permissions_list(service_id, permissions): + arr = [] + for permission in permissions: + if permission not in SERVICE_PERMISSION_TYPES: + raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) + + service_permission = ServicePermission(service_id=service_id, permission=permission) + arr.append(service_permission) + + return arr + + +@transactional +def dao_add_and_commit_service_permissions(service_id, permissions): + service_permissions = make_service_permissions_list(service_id, permissions) + + try: + db.session.add_all(service_permissions) + db.session.commit() + except exc.IntegrityError as e: + if "duplicate key value violates unique constraint" in str(e.orig): + raise ValueError(e.orig) + raise + + +def dao_remove_service_permission(service_id, permission=None): + return ServicePermission.query.filter( + ServicePermission.service_id == service_id, + ServicePermission.permission == permission if permission else None).delete() diff --git a/migrations/README b/migrations/README index 98e4f9c44..6c36a3e0e 100644 --- a/migrations/README +++ b/migrations/README @@ -1 +1,7 @@ -Generic single-database configuration. \ No newline at end of file +Generic single-database configuration. + +python application.py db migration to generate migration script. + +python application.py db upgrade to upgrade db with script. + +python application.py db downgrade to rollback db changes. diff --git a/migrations/versions/0083_add_perm_types_and_svc_perm.py b/migrations/versions/0083_add_perm_types_and_svc_perm.py index 485ddef5d..2bebb273e 100644 --- a/migrations/versions/0083_add_perm_types_and_svc_perm.py +++ b/migrations/versions/0083_add_perm_types_and_svc_perm.py @@ -15,10 +15,10 @@ import sqlalchemy as sa from sqlalchemy.dialects import postgresql def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### - service_permission_types = op.create_table('service_permission_types', - sa.Column('name', sa.String(length=255), nullable=False), - sa.PrimaryKeyConstraint('name')) + ### commands auto generated by Alembic - please adjust! ### + service_permission_types=op.create_table('service_permission_types', + sa.Column('name', sa.String(length=255), nullable=False), + sa.PrimaryKeyConstraint('name')) op.bulk_insert(service_permission_types, [ diff --git a/tests/app/conftest.py b/tests/app/conftest.py index fc9748870..00b4604b1 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -666,11 +666,11 @@ def sample_permission(notify_db, @pytest.fixture(scope='function') -def sample_service_permission(notify_db, - notify_db_session, - service=None, - user=None, - permission="manage_settings"): +def sample_user_service_permission(notify_db, + notify_db_session, + service=None, + user=None, + permission="manage_settings"): if user is None: user = create_user() if service is None: diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py new file mode 100644 index 000000000..b6130f0da --- /dev/null +++ b/tests/app/dao/test_service_permissions_dao.py @@ -0,0 +1,82 @@ +import pytest + +from app.dao.service_permissions_dao import ( + dao_fetch_service_permissions, dao_remove_service_permission) +from app.models import ( + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) + +from tests.app.db import create_service_permissions, create_service + + +def test_create_service_permissions(sample_service): + service_permission_types = [SMS_TYPE, INTERNATIONAL_SMS_TYPE] + + service_permissions = create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + + assert len(service_permissions) == len(service_permission_types) + assert all(sp.service_id == sample_service.id for sp in service_permissions) + assert all(sp.permission in service_permission_types for sp in service_permissions) + + +def test_fetch_service_permissions_gets_service_permissions(sample_service): + service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] + create_service_permissions(service_id=sample_service.id, permissions=service_permission_types) + service_permissions = dao_fetch_service_permissions(sample_service.id) + + assert len(service_permission_types) == len(service_permission_types) + assert all(sp.service_id == sample_service.id for sp in service_permissions) + assert all(sp.permission in service_permission_types for sp in service_permissions) + + +def test_add_service_permissions_to_existing_permissions(sample_service): + service_permission_types_1 = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_types_2 = [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] + + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types_1) + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types_2) + + permissions = dao_fetch_service_permissions(sample_service.id) + + assert len(permissions) == len(service_permission_types_1 + service_permission_types_2) + + +def test_create_invalid_service_permissions_raises_error(sample_service): + service_permission_types = ['invalid'] + + with pytest.raises(ValueError) as e: + service_permissions = create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + + assert "'invalid' not of service permission type: " + str(SERVICE_PERMISSION_TYPES) in str(e.value) + + +def test_remove_service_permission(sample_service): + service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_type_to_remove = EMAIL_TYPE + service_permission_type_remaining = INCOMING_SMS_TYPE + + service_permissions = create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + + dao_remove_service_permission(sample_service.id, service_permission_type_to_remove) + + permissions = dao_fetch_service_permissions(sample_service.id) + assert len(permissions) == 1 + assert permissions[0].permission == service_permission_type_remaining + assert permissions[0].service_id == sample_service.id + + +def test_adding_duplicate_service_id_permission_raises_value_error(sample_service): + service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_types_with_duplicate_email_type = [LETTER_TYPE, EMAIL_TYPE] + + with pytest.raises(ValueError) as e: + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types) + create_service_permissions( + service_id=sample_service.id, permissions=service_permission_types_with_duplicate_email_type) + + assert "duplicate key value violates unique constraint \"service_permissions_pkey\"" in str(e.value) diff --git a/tests/app/db.py b/tests/app/db.py index b5838d693..c193fd4a9 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -2,11 +2,14 @@ from datetime import datetime import uuid from app.dao.jobs_dao import dao_create_job -from app.models import Service, User, Template, Notification, SMS_TYPE, KEY_TYPE_NORMAL, Job +from app.models import (Service, User, Template, Notification, EMAIL_TYPE, LETTER_TYPE, + SMS_TYPE, KEY_TYPE_NORMAL, Job, ServicePermission) from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service +from app.dao.service_permissions_dao import ( + dao_add_and_commit_service_permissions, make_service_permissions_list) def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -141,3 +144,12 @@ def create_job(template, job = Job(**data) dao_create_job(job) return job + + +def create_service_permissions(service_id, permissions=[EMAIL_TYPE, LETTER_TYPE]): + dao_add_and_commit_service_permissions( + service_id if service_id else create_service().id, permissions) + + service_permissions = ServicePermission.query.all() + + return service_permissions diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 91c928751..2f4732473 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -15,7 +15,7 @@ from tests import create_authorization_header from tests.app.db import create_template from tests.app.conftest import ( sample_service as create_service, - sample_service_permission as create_service_permission, + sample_user_service_permission as create_user_service_permission, sample_notification as create_sample_notification, sample_notification_history as create_notification_history, sample_notification_with_job @@ -941,12 +941,12 @@ def test_add_unknown_user_to_service_returns404(notify_api, notify_db, notify_db assert result['message'] == expected_message -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_service_permission): +def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): with notify_api.test_request_context(): with notify_api.test_client() as client: second_user = create_user(email="new@digital.cabinet-office.gov.uk") # Simulates successfully adding a user to the service - second_permission = create_service_permission( + second_permission = create_user_service_permission( notify_db, notify_db_session, user=second_user) @@ -961,13 +961,13 @@ def test_remove_user_from_service(notify_api, notify_db, notify_db_session, samp assert resp.status_code == 204 -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_service_permission): +def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): with notify_api.test_request_context(): with notify_api.test_client() as client: second_user = create_user(email="new@digital.cabinet-office.gov.uk") endpoint = url_for( 'service.remove_user_from_service', - service_id=str(sample_service_permission.service.id), + service_id=str(sample_user_service_permission.service.id), user_id=str(second_user.id)) auth_header = create_authorization_header() resp = client.delete( @@ -979,13 +979,13 @@ def test_remove_user_from_service(notify_api, notify_db, notify_db_session, samp def test_cannot_remove_only_user_from_service(notify_api, notify_db, notify_db_session, - sample_service_permission): + sample_user_service_permission): with notify_api.test_request_context(): with notify_api.test_client() as client: endpoint = url_for( 'service.remove_user_from_service', - service_id=str(sample_service_permission.service.id), - user_id=str(sample_service_permission.user.id)) + service_id=str(sample_user_service_permission.service.id), + user_id=str(sample_user_service_permission.user.id)) auth_header = create_authorization_header() resp = client.delete( endpoint, diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index e476b2ebb..ddbb3eaae 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -290,13 +290,13 @@ def test_get_user_by_email_bad_url_returns_404(client, sample_user): assert json_resp['message'] == 'Invalid request. Email query string param required' -def test_get_user_with_permissions(client, sample_service_permission): +def test_get_user_with_permissions(client, sample_user_service_permission): header = create_authorization_header() - response = client.get(url_for('user.get_user', user_id=str(sample_service_permission.user.id)), + response = client.get(url_for('user.get_user', user_id=str(sample_user_service_permission.user.id)), headers=[header]) assert response.status_code == 200 permissions = json.loads(response.get_data(as_text=True))['data']['permissions'] - assert sample_service_permission.permission in permissions[str(sample_service_permission.service.id)] + assert sample_user_service_permission.permission in permissions[str(sample_user_service_permission.service.id)] def test_set_user_permissions(client, sample_user, sample_service): diff --git a/tests/conftest.py b/tests/conftest.py index 61cd17ce9..bf9823331 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -77,7 +77,8 @@ def notify_db_session(notify_db): "provider_details_history", "template_process_type", "dvla_organisation", - "notification_status_types"]: + "notification_status_types", + "service_permission_types"]: notify_db.engine.execute(tbl.delete()) notify_db.session.commit() From 54d801979caf0bb0c25711b2b29e4df3e586b416 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 10:57:57 +0100 Subject: [PATCH 2/6] Refactored to handle single service permission --- app/dao/service_permissions_dao.py | 27 ++----- tests/app/dao/test_service_permissions_dao.py | 71 ++++++------------- tests/app/db.py | 9 ++- 3 files changed, 32 insertions(+), 75 deletions(-) diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py index 7ea9eb0d3..26b1f6f9e 100644 --- a/app/dao/service_permissions_dao.py +++ b/app/dao/service_permissions_dao.py @@ -10,29 +10,14 @@ def dao_fetch_service_permissions(service_id): ServicePermission.service_id == service_id).all() -def make_service_permissions_list(service_id, permissions): - arr = [] - for permission in permissions: - if permission not in SERVICE_PERMISSION_TYPES: - raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) - - service_permission = ServicePermission(service_id=service_id, permission=permission) - arr.append(service_permission) - - return arr - - @transactional -def dao_add_and_commit_service_permissions(service_id, permissions): - service_permissions = make_service_permissions_list(service_id, permissions) +def dao_add_and_commit_service_permission(service_id, permission): + if permission not in SERVICE_PERMISSION_TYPES: + raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) - try: - db.session.add_all(service_permissions) - db.session.commit() - except exc.IntegrityError as e: - if "duplicate key value violates unique constraint" in str(e.orig): - raise ValueError(e.orig) - raise + service_permission = ServicePermission(service_id=service_id, permission=permission) + + db.session.add(service_permission) def dao_remove_service_permission(service_id, permission=None): diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index b6130f0da..cf3768306 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -5,61 +5,47 @@ from app.dao.service_permissions_dao import ( from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) -from tests.app.db import create_service_permissions, create_service +from tests.app.db import create_service_permission, create_service -def test_create_service_permissions(sample_service): - service_permission_types = [SMS_TYPE, INTERNATIONAL_SMS_TYPE] +def test_create_service_permission(sample_service): + service_permission_type = SMS_TYPE - service_permissions = create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) + service_permission = create_service_permission( + service_id=sample_service.id, permission=service_permission_type) + + assert len(service_permission) == 1 + assert all(sp.service_id == sample_service.id for sp in service_permission) + assert all(sp.permission in service_permission_type for sp in service_permission) + + +def test_fetch_service_permissions_gets_service_permissions(sample_service): + service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] + for spt in service_permission_types: + create_service_permission(service_id=sample_service.id, permission=spt) + service_permissions = dao_fetch_service_permissions(sample_service.id) assert len(service_permissions) == len(service_permission_types) assert all(sp.service_id == sample_service.id for sp in service_permissions) assert all(sp.permission in service_permission_types for sp in service_permissions) -def test_fetch_service_permissions_gets_service_permissions(sample_service): - service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] - create_service_permissions(service_id=sample_service.id, permissions=service_permission_types) - service_permissions = dao_fetch_service_permissions(sample_service.id) - - assert len(service_permission_types) == len(service_permission_types) - assert all(sp.service_id == sample_service.id for sp in service_permissions) - assert all(sp.permission in service_permission_types for sp in service_permissions) - - -def test_add_service_permissions_to_existing_permissions(sample_service): - service_permission_types_1 = [EMAIL_TYPE, INCOMING_SMS_TYPE] - service_permission_types_2 = [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] - - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types_1) - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types_2) - - permissions = dao_fetch_service_permissions(sample_service.id) - - assert len(permissions) == len(service_permission_types_1 + service_permission_types_2) - - def test_create_invalid_service_permissions_raises_error(sample_service): - service_permission_types = ['invalid'] + service_permission_type = 'invalid' with pytest.raises(ValueError) as e: - service_permissions = create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) + create_service_permission(service_id=sample_service.id, permission=service_permission_type) - assert "'invalid' not of service permission type: " + str(SERVICE_PERMISSION_TYPES) in str(e.value) + assert "'invalid' not of service permission type: {}".format(str(SERVICE_PERMISSION_TYPES)) in str(e.value) def test_remove_service_permission(sample_service): - service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] + service_permission_types_to_create = [EMAIL_TYPE, INCOMING_SMS_TYPE] service_permission_type_to_remove = EMAIL_TYPE service_permission_type_remaining = INCOMING_SMS_TYPE - service_permissions = create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) + for spt in service_permission_types_to_create: + create_service_permission(service_id=sample_service.id, permission=spt) dao_remove_service_permission(sample_service.id, service_permission_type_to_remove) @@ -67,16 +53,3 @@ def test_remove_service_permission(sample_service): assert len(permissions) == 1 assert permissions[0].permission == service_permission_type_remaining assert permissions[0].service_id == sample_service.id - - -def test_adding_duplicate_service_id_permission_raises_value_error(sample_service): - service_permission_types = [EMAIL_TYPE, INCOMING_SMS_TYPE] - service_permission_types_with_duplicate_email_type = [LETTER_TYPE, EMAIL_TYPE] - - with pytest.raises(ValueError) as e: - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types) - create_service_permissions( - service_id=sample_service.id, permissions=service_permission_types_with_duplicate_email_type) - - assert "duplicate key value violates unique constraint \"service_permissions_pkey\"" in str(e.value) diff --git a/tests/app/db.py b/tests/app/db.py index c193fd4a9..e4cded3cc 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -8,8 +8,7 @@ from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service -from app.dao.service_permissions_dao import ( - dao_add_and_commit_service_permissions, make_service_permissions_list) +from app.dao.service_permissions_dao import dao_add_and_commit_service_permission def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -146,9 +145,9 @@ def create_job(template, return job -def create_service_permissions(service_id, permissions=[EMAIL_TYPE, LETTER_TYPE]): - dao_add_and_commit_service_permissions( - service_id if service_id else create_service().id, permissions) +def create_service_permission(service_id, permission=EMAIL_TYPE): + dao_add_and_commit_service_permission( + service_id if service_id else create_service().id, permission) service_permissions = ServicePermission.query.all() From 733c16b2bbf20247947565c11594e685326da1f5 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 12:33:27 +0100 Subject: [PATCH 3/6] Update to strip down DAO and clarify tests --- app/dao/service_permissions_dao.py | 13 ++---- tests/app/conftest.py | 8 ++-- tests/app/dao/test_service_permissions_dao.py | 42 +++++++------------ tests/app/db.py | 4 +- 4 files changed, 23 insertions(+), 44 deletions(-) diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py index 26b1f6f9e..5c38ad6a2 100644 --- a/app/dao/service_permissions_dao.py +++ b/app/dao/service_permissions_dao.py @@ -1,8 +1,6 @@ -from sqlalchemy import exc - from app import db from app.dao.dao_utils import transactional -from app.models import Service, ServicePermission, SERVICE_PERMISSION_TYPES +from app.models import ServicePermission, SERVICE_PERMISSION_TYPES def dao_fetch_service_permissions(service_id): @@ -11,16 +9,13 @@ def dao_fetch_service_permissions(service_id): @transactional -def dao_add_and_commit_service_permission(service_id, permission): - if permission not in SERVICE_PERMISSION_TYPES: - raise ValueError("'{}' not of service permission type: {}".format(permission, SERVICE_PERMISSION_TYPES)) - +def dao_create_service_permission(service_id, permission): service_permission = ServicePermission(service_id=service_id, permission=permission) db.session.add(service_permission) -def dao_remove_service_permission(service_id, permission=None): +def dao_remove_service_permission(service_id, permission): return ServicePermission.query.filter( ServicePermission.service_id == service_id, - ServicePermission.permission == permission if permission else None).delete() + ServicePermission.permission == permission).delete() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 00b4604b1..c10c053cb 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -666,11 +666,9 @@ def sample_permission(notify_db, @pytest.fixture(scope='function') -def sample_user_service_permission(notify_db, - notify_db_session, - service=None, - user=None, - permission="manage_settings"): +def sample_user_service_permission( + notify_db, notify_db_session, service=None, user=None, permission="manage_settings" +): if user is None: user = create_user() if service is None: diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index cf3768306..248dd6ae6 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -5,51 +5,37 @@ from app.dao.service_permissions_dao import ( from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) -from tests.app.db import create_service_permission, create_service +from tests.app.db import create_service_permission def test_create_service_permission(sample_service): - service_permission_type = SMS_TYPE - service_permission = create_service_permission( - service_id=sample_service.id, permission=service_permission_type) + service_id=sample_service.id, permission=SMS_TYPE) assert len(service_permission) == 1 - assert all(sp.service_id == sample_service.id for sp in service_permission) - assert all(sp.permission in service_permission_type for sp in service_permission) + assert service_permission[0].service_id == sample_service.id + assert service_permission[0].permission == SMS_TYPE def test_fetch_service_permissions_gets_service_permissions(sample_service): - service_permission_types = [LETTER_TYPE, EMAIL_TYPE, SMS_TYPE] - for spt in service_permission_types: - create_service_permission(service_id=sample_service.id, permission=spt) + create_service_permission(service_id=sample_service.id, permission=LETTER_TYPE) + create_service_permission(service_id=sample_service.id, permission=INTERNATIONAL_SMS_TYPE) + create_service_permission(service_id=sample_service.id, permission=SMS_TYPE) + service_permissions = dao_fetch_service_permissions(sample_service.id) - assert len(service_permissions) == len(service_permission_types) + assert len(service_permissions) == 3 assert all(sp.service_id == sample_service.id for sp in service_permissions) - assert all(sp.permission in service_permission_types for sp in service_permissions) - - -def test_create_invalid_service_permissions_raises_error(sample_service): - service_permission_type = 'invalid' - - with pytest.raises(ValueError) as e: - create_service_permission(service_id=sample_service.id, permission=service_permission_type) - - assert "'invalid' not of service permission type: {}".format(str(SERVICE_PERMISSION_TYPES)) in str(e.value) + assert all(sp.permission in [LETTER_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE] for sp in service_permissions) def test_remove_service_permission(sample_service): - service_permission_types_to_create = [EMAIL_TYPE, INCOMING_SMS_TYPE] - service_permission_type_to_remove = EMAIL_TYPE - service_permission_type_remaining = INCOMING_SMS_TYPE + create_service_permission(service_id=sample_service.id, permission=EMAIL_TYPE) + create_service_permission(service_id=sample_service.id, permission=INCOMING_SMS_TYPE) - for spt in service_permission_types_to_create: - create_service_permission(service_id=sample_service.id, permission=spt) - - dao_remove_service_permission(sample_service.id, service_permission_type_to_remove) + dao_remove_service_permission(sample_service.id, EMAIL_TYPE) permissions = dao_fetch_service_permissions(sample_service.id) assert len(permissions) == 1 - assert permissions[0].permission == service_permission_type_remaining + assert permissions[0].permission == INCOMING_SMS_TYPE assert permissions[0].service_id == sample_service.id diff --git a/tests/app/db.py b/tests/app/db.py index e4cded3cc..228d49ec4 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -8,7 +8,7 @@ from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service -from app.dao.service_permissions_dao import dao_add_and_commit_service_permission +from app.dao.service_permissions_dao import dao_create_service_permission def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk"): @@ -146,7 +146,7 @@ def create_job(template, def create_service_permission(service_id, permission=EMAIL_TYPE): - dao_add_and_commit_service_permission( + dao_create_service_permission( service_id if service_id else create_service().id, permission) service_permissions = ServicePermission.query.all() From b233ae46f3899680fbfb0b641353e27076acde64 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 12:53:46 +0100 Subject: [PATCH 4/6] Tidy up test code for service permissions --- tests/app/dao/test_service_permissions_dao.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index 248dd6ae6..8005e2af2 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,20 +1,17 @@ import pytest -from app.dao.service_permissions_dao import ( - dao_fetch_service_permissions, dao_remove_service_permission) -from app.models import ( - EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE, SERVICE_PERMISSION_TYPES) +from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission +from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE from tests.app.db import create_service_permission def test_create_service_permission(sample_service): - service_permission = create_service_permission( - service_id=sample_service.id, permission=SMS_TYPE) + service_permissions = create_service_permission(service_id=sample_service.id, permission=SMS_TYPE) - assert len(service_permission) == 1 - assert service_permission[0].service_id == sample_service.id - assert service_permission[0].permission == SMS_TYPE + assert len(service_permissions) == 1 + assert service_permissions[0].service_id == sample_service.id + assert service_permissions[0].permission == SMS_TYPE def test_fetch_service_permissions_gets_service_permissions(sample_service): From 2a488910255848eb0559fad7986b1faac29dd807 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 12:54:32 +0100 Subject: [PATCH 5/6] Removed unused pytest from test --- tests/app/dao/test_service_permissions_dao.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/app/dao/test_service_permissions_dao.py b/tests/app/dao/test_service_permissions_dao.py index 8005e2af2..a098d1b0f 100644 --- a/tests/app/dao/test_service_permissions_dao.py +++ b/tests/app/dao/test_service_permissions_dao.py @@ -1,5 +1,3 @@ -import pytest - from app.dao.service_permissions_dao import dao_fetch_service_permissions, dao_remove_service_permission from app.models import EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INCOMING_SMS_TYPE From 3602431c2a7212c8c1b89cfe5d2c5c598fa743fe Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 16 May 2017 13:41:54 +0100 Subject: [PATCH 6/6] Renamed test and refactored fixtures --- tests/app/service/test_rest.py | 62 +++++++++++++++++----------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 2f4732473..93621668f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -941,39 +941,39 @@ def test_add_unknown_user_to_service_returns404(notify_api, notify_db, notify_db assert result['message'] == expected_message -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - second_user = create_user(email="new@digital.cabinet-office.gov.uk") - # Simulates successfully adding a user to the service - second_permission = create_user_service_permission( - notify_db, - notify_db_session, - user=second_user) - endpoint = url_for( - 'service.remove_user_from_service', - service_id=str(second_permission.service.id), - user_id=str(second_permission.user.id)) - auth_header = create_authorization_header() - resp = client.delete( - endpoint, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 204 +def test_remove_user_from_service( + notify_db, notify_db_session, client, sample_user_service_permission +): + second_user = create_user(email="new@digital.cabinet-office.gov.uk") + # Simulates successfully adding a user to the service + second_permission = create_user_service_permission( + notify_db, + notify_db_session, + user=second_user) + endpoint = url_for( + 'service.remove_user_from_service', + service_id=str(second_permission.service.id), + user_id=str(second_permission.user.id)) + auth_header = create_authorization_header() + resp = client.delete( + endpoint, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 204 -def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_user_service_permission): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - second_user = create_user(email="new@digital.cabinet-office.gov.uk") - endpoint = url_for( - 'service.remove_user_from_service', - service_id=str(sample_user_service_permission.service.id), - user_id=str(second_user.id)) - auth_header = create_authorization_header() - resp = client.delete( - endpoint, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 404 +def test_remove_non_existant_user_from_service( + client, sample_user_service_permission +): + second_user = create_user(email="new@digital.cabinet-office.gov.uk") + endpoint = url_for( + 'service.remove_user_from_service', + service_id=str(sample_user_service_permission.service.id), + user_id=str(second_user.id)) + auth_header = create_authorization_header() + resp = client.delete( + endpoint, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 404 def test_cannot_remove_only_user_from_service(notify_api,