Refactored the services dao to be a little cleaner

- some things we don't need
	- bulk update of users
	- delete service

Now returns None if can't find an object

app/dao/services_dao.py

@@ -1,48 +1,44 @@
 import json
-from datetime import datetime
-from sqlalchemy.orm import load_only
 from . import DAOException
 from app import db
-from app.models import (Service, User)
+from app.models import Service
+from sqlalchemy import asc
 
 
-def save_model_service(service, update_dict=None):
-    users_list = update_dict.get('users', []) if update_dict else getattr(service, 'users', [])
-    if not users_list:
-        error_msg = {'users': ['Missing data for required attribute']}
-        raise DAOException(json.dumps(error_msg))
-    if update_dict:
-        # Make sure the update_dict doesn't contain conflicting
-        update_dict.pop('id', None)
-        update_dict.pop('users', None)
-        # TODO optimize this algorithm
-        for i, x in enumerate(service.users):
-            if x not in users_list:
-                service.users.remove(x)
-            else:
-                users_list.remove(x)
-        for x in users_list:
-            service.users.append(x)
-        Service.query.filter_by(id=service.id).update(update_dict)
-    else:
+def dao_fetch_all_services():
+    return Service.query.order_by(asc(Service.created_at)).all()
+
+
+def dao_fetch_service_by_id(service_id):
+    return Service.query.filter_by(id=service_id).first()
+
+
+def dao_fetch_all_services_by_user(user_id):
+    return Service.query.filter(Service.users.any(id=user_id)).order_by(asc(Service.created_at)).all()
+
+
+def dao_fetch_service_by_id_and_user(service_id, user_id):
+    return Service.query.filter(Service.users.any(id=user_id)).filter_by(id=service_id).first()
+
+
+def dao_create_service(service, user):
+    service.users.append(user)
     db.session.add(service)
     db.session.commit()
 
 
-def delete_model_service(service):
-    db.session.delete(service)
+def dao_update_service(service):
+    db.session.add(service)
     db.session.commit()
 
 
-def get_model_services(service_id=None, user_id=None, _raise=True):
-    # TODO need better mapping from function params to sql query.
-    if user_id and service_id:
-        return Service.query.filter(
-            Service.users.any(id=user_id)).filter_by(id=service_id).one()
-    elif service_id:
-        result = Service.query.filter_by(id=service_id).one() if _raise else Service.query.filter_by(
-            id=service_id).first()
-        return result
-    elif user_id:
-        return Service.query.filter(Service.users.any(id=user_id)).all()
-    return Service.query.all()
+def dao_add_user_to_service(service, user):
+    service.users.append(user)
+    db.session.add(service)
+    db.session.commit()
+
+
+def dao_remove_user_from_service(service, user):
+    service.users.remove(user)
+    db.session.add(service)
+    db.session.commit()

tests/app/dao/test_services_dao.py

@@ -1,88 +1,172 @@
+import uuid
 import pytest
 from app.dao.services_dao import (
-    save_model_service, get_model_services, DAOException, delete_model_service)
-from tests.app.conftest import sample_service as create_sample_service
-from tests.app.conftest import sample_user as create_sample_user
-from app.models import Service
+    dao_create_service,
+    dao_add_user_to_service,
+    dao_remove_user_from_service,
+    dao_fetch_all_services,
+    dao_fetch_service_by_id,
+    dao_fetch_all_services_by_user,
+    dao_fetch_service_by_id_and_user
+)
+from app.dao.users_dao import save_model_user
+from app.models import Service, User
+from sqlalchemy.orm.exc import FlushError
+from sqlalchemy.exc import IntegrityError
 
 
-def test_create_service(notify_api, notify_db, notify_db_session, sample_user):
+def test_create_service(sample_user):
     assert Service.query.count() == 0
-    service_name = 'Sample Service'
-    data = {
-        'name': service_name,
-        'users': [sample_user],
-        'limit': 1000,
-        'active': False,
-        'restricted': False}
-    service = Service(**data)
-    save_model_service(service)
+    service = Service(name="service_name", email_from="email_from", limit=1000, active=True, restricted=False)
+    dao_create_service(service, sample_user)
     assert Service.query.count() == 1
-    assert Service.query.first().name == service_name
+    assert Service.query.first().name == "service_name"
     assert Service.query.first().id == service.id
+    assert sample_user in Service.query.first().users
 
 
-def test_get_services(notify_api, notify_db, notify_db_session, sample_user):
-    sample_service = create_sample_service(notify_db,
-                                           notify_db_session,
-                                           user=sample_user)
-    assert Service.query.count() == 1
-    assert len(get_model_services()) == 1
-    service_name = "Another service"
-    sample_service = create_sample_service(notify_db,
-                                           notify_db_session,
-                                           service_name=service_name,
-                                           user=sample_user)
-    assert Service.query.count() == 2
-    assert len(get_model_services()) == 2
-
-
-def test_get_user_service(notify_api, notify_db, notify_db_session, sample_user):
+def test_cannot_create_two_services_with_same_name(sample_user):
     assert Service.query.count() == 0
-    service_name = "Random service"
-    sample_service = create_sample_service(notify_db,
-                                           notify_db_session,
-                                           service_name=service_name,
-                                           user=sample_user)
-    assert get_model_services(service_id=sample_service.id).name == service_name
-    assert Service.query.count() == 1
+    service1 = Service(name="service_name", email_from="email_from1", limit=1000, active=True, restricted=False)
+    service2 = Service(name="service_name", email_from="email_from2", limit=1000, active=True, restricted=False)
+    with pytest.raises(IntegrityError) as excinfo:
+        dao_create_service(service1, sample_user)
+        dao_create_service(service2, sample_user)
+    assert 'duplicate key value violates unique constraint "services_name_key"' in str(excinfo.value)
 
 
-def test_get_services_for_user(notify_api, notify_db, notify_db_session, sample_service):
-    assert Service.query.count() == 1
-    service_name = "Random service"
-    second_user = create_sample_user(notify_db, notify_db_session, 'an@other.gov.uk')
-    create_sample_service(notify_db, notify_db_session, service_name='another service', user=second_user)
-
-    sample_service = create_sample_service(notify_db,
-                                           notify_db_session,
-                                           service_name=service_name,
-                                           user=sample_service.users[0])
-    assert Service.query.count() == 3
-    services = get_model_services(user_id=sample_service.users[0].id)
-    assert len(services) == 2
-    assert service_name in [x.name for x in services]
-    assert 'Sample service' in [x.name for x in services]
-
-
-def test_missing_user_attribute(notify_api, notify_db, notify_db_session):
+def test_cannot_create_two_services_with_same_email_from(sample_user):
     assert Service.query.count() == 0
-    try:
-        service_name = 'Sample Service'
-        data = {
-            'name': service_name,
-            'limit': 1000,
-            'active': False,
-            'restricted': False}
-
-        service = Service(**data)
-        save_model_service(service)
-        pytest.fail("DAOException not thrown")
-    except DAOException as e:
-        assert "Missing data for required attribute" in str(e)
+    service1 = Service(name="service_name1", email_from="email_from", limit=1000, active=True, restricted=False)
+    service2 = Service(name="service_name2", email_from="email_from", limit=1000, active=True, restricted=False)
+    with pytest.raises(IntegrityError) as excinfo:
+        dao_create_service(service1, sample_user)
+        dao_create_service(service2, sample_user)
+    assert 'duplicate key value violates unique constraint "services_email_from_key"' in str(excinfo.value)
 
 
-def test_delete_service(notify_api, notify_db, notify_db_session, sample_service):
-    assert Service.query.count() == 1
-    delete_model_service(sample_service)
+def test_cannot_create_service_with_no_user(notify_db_session):
     assert Service.query.count() == 0
+    service = Service(name="service_name", email_from="email_from", limit=1000, active=True, restricted=False)
+    with pytest.raises(FlushError) as excinfo:
+        dao_create_service(service, None)
+    assert "Can't flush None value found in collection Service.users" in str(excinfo.value)
+
+
+def test_should_add_user_to_service(sample_user):
+    service = Service(name="service_name", email_from="email_from", limit=1000, active=True, restricted=False)
+    dao_create_service(service, sample_user)
+    assert sample_user in Service.query.first().users
+    new_user = User(
+        name='Test User',
+        email_address='new_user@digital.cabinet-office.gov.uk',
+        password='password',
+        mobile_number='+447700900986'
+    )
+    save_model_user(new_user)
+    dao_add_user_to_service(service, new_user)
+    assert new_user in Service.query.first().users
+
+
+def test_should_remove_user_from_service(sample_user):
+    service = Service(name="service_name", email_from="email_from", limit=1000, active=True, restricted=False)
+    dao_create_service(service, sample_user)
+    new_user = User(
+        name='Test User',
+        email_address='new_user@digital.cabinet-office.gov.uk',
+        password='password',
+        mobile_number='+447700900986'
+    )
+    save_model_user(new_user)
+    dao_add_user_to_service(service, new_user)
+    assert new_user in Service.query.first().users
+    dao_remove_user_from_service(service, new_user)
+    assert new_user not in Service.query.first().users
+
+
+def test_get_all_services(service_factory):
+    service_factory.get('service 1')
+    assert len(dao_fetch_all_services()) == 1
+    assert dao_fetch_all_services()[0].name == 'service 1'
+
+    service_factory.get('service 2')
+    assert len(dao_fetch_all_services()) == 2
+    assert dao_fetch_all_services()[1].name == 'service 2'
+
+
+def test_get_all_services_should_return_in_created_order(service_factory):
+    service_factory.get('service 1')
+    service_factory.get('service 2')
+    service_factory.get('service 3')
+    service_factory.get('service 4')
+    assert len(dao_fetch_all_services()) == 4
+    assert dao_fetch_all_services()[0].name == 'service 1'
+    assert dao_fetch_all_services()[1].name == 'service 2'
+    assert dao_fetch_all_services()[2].name == 'service 3'
+    assert dao_fetch_all_services()[3].name == 'service 4'
+
+
+def test_get_all_services_should_return_empty_list_if_no_services():
+    assert len(dao_fetch_all_services()) == 0
+
+
+def test_get_all_services_for_user(service_factory, sample_user):
+    service_factory.get('service 1', sample_user)
+    service_factory.get('service 2', sample_user)
+    service_factory.get('service 3', sample_user)
+    assert len(dao_fetch_all_services_by_user(sample_user.id)) == 3
+    assert dao_fetch_all_services_by_user(sample_user.id)[0].name == 'service 1'
+    assert dao_fetch_all_services_by_user(sample_user.id)[1].name == 'service 2'
+    assert dao_fetch_all_services_by_user(sample_user.id)[2].name == 'service 3'
+
+
+def test_get_all_only_services_user_has_access_to(service_factory, sample_user):
+    service_factory.get('service 1', sample_user)
+    service_factory.get('service 2', sample_user)
+    service_3 = service_factory.get('service 3', sample_user)
+    new_user = User(
+        name='Test User',
+        email_address='new_user@digital.cabinet-office.gov.uk',
+        password='password',
+        mobile_number='+447700900986'
+    )
+    save_model_user(new_user)
+    dao_add_user_to_service(service_3, new_user)
+    assert len(dao_fetch_all_services_by_user(sample_user.id)) == 3
+    assert dao_fetch_all_services_by_user(sample_user.id)[0].name == 'service 1'
+    assert dao_fetch_all_services_by_user(sample_user.id)[1].name == 'service 2'
+    assert dao_fetch_all_services_by_user(sample_user.id)[2].name == 'service 3'
+    assert len(dao_fetch_all_services_by_user(new_user.id)) == 1
+    assert dao_fetch_all_services_by_user(new_user.id)[0].name == 'service 3'
+
+
+def test_get_all_user_services_should_return_empty_list_if_no_services_for_user(sample_user):
+    assert len(dao_fetch_all_services_by_user(sample_user.id)) == 0
+
+
+def test_get_service_by_id_returns_none_if_no_service(notify_db):
+    assert not dao_fetch_service_by_id(str(uuid.uuid4()))
+
+
+def test_get_service_by_id_returns_service(service_factory):
+    service = service_factory.get('testing')
+    assert dao_fetch_service_by_id(service.id).name == 'testing'
+
+
+def test_can_get_service_by_id_and_user(service_factory, sample_user):
+    service = service_factory.get('service 1', sample_user)
+    assert dao_fetch_service_by_id_and_user(service.id, sample_user.id).name == 'service 1'
+
+
+def test_cannot_get_service_by_id_and_owned_by_different_user(service_factory, sample_user):
+    service1 = service_factory.get('service 1', sample_user)
+    new_user = User(
+        name='Test User',
+        email_address='new_user@digital.cabinet-office.gov.uk',
+        password='password',
+        mobile_number='+447700900986'
+    )
+    save_model_user(new_user)
+    service2 = service_factory.get('service 2', new_user)
+    assert dao_fetch_service_by_id_and_user(service1.id, sample_user.id).name == 'service 1'
+    assert not dao_fetch_service_by_id_and_user(service2.id, sample_user.id)