Update to strip down DAO and clarify tests

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()

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:

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

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()