Merge pull request #2368 from alphagov/richer-orgs

Use ‘organisations‘ table to store info about organisations (not YAML)

app/dao/organisation_dao.py

@@ -1,7 +1,10 @@
+from sqlalchemy.sql.expression import func
+
 from app import db
 from app.dao.dao_utils import transactional
 from app.models import (
     Organisation,
+    Domain,
     InvitedOrganisationUser,
     User
 )
@@ -23,6 +26,21 @@ def dao_get_organisation_by_id(organisation_id):
     return Organisation.query.filter_by(id=organisation_id).one()
 
 
+def dao_get_organisation_by_email_address(email_address):
+
+    email_address = email_address.lower()
+
+    for domain in Domain.query.order_by(func.char_length(Domain.domain).desc()).all():
+
+        if (
+            email_address.endswith("@{}".format(domain.domain)) or
+            email_address.endswith(".{}".format(domain.domain))
+        ):
+            return Organisation.query.filter_by(id=domain.organisation_id).one()
+
+    return None
+
+
 def dao_get_organisation_by_service_id(service_id):
     return Organisation.query.join(Organisation.services).filter_by(id=service_id).first()
 
@@ -34,10 +52,26 @@ def dao_create_organisation(organisation):
 
 @transactional
 def dao_update_organisation(organisation_id, **kwargs):
-    return Organisation.query.filter_by(id=organisation_id).update(
+
+    domains = kwargs.pop('domains', [])
+
+    organisation = Organisation.query.filter_by(id=organisation_id).update(
         kwargs
     )
 
+    if isinstance(domains, list):
+
+        Domain.query.filter_by(organisation_id=organisation_id).delete()
+
+        db.session.bulk_save_objects([
+            Domain(domain=domain.lower(), organisation_id=organisation_id)
+            for domain in domains
+        ])
+
+        db.session.commit()
+
+    return organisation
+
 
 @transactional
 def dao_add_service_to_organisation(service, organisation_id):

app/dao/services_dao.py

@@ -11,6 +11,7 @@ from app.dao.dao_utils import (
     transactional,
     version_class
 )
+from app.dao.organisation_dao import dao_get_organisation_by_email_address
 from app.dao.service_sms_sender_dao import insert_service_sms_sender
 from app.dao.service_user_dao import dao_get_service_user
 from app.models import (
@@ -151,14 +152,25 @@ def dao_fetch_service_by_id_and_user(service_id, user_id):
 
 @transactional
 @version_class(Service)
-def dao_create_service(service, user, service_id=None, service_permissions=None, letter_branding=None):
+def dao_create_service(
+    service,
+    user,
+    service_id=None,
+    service_permissions=None,
+    letter_branding=None,
+):
     # the default property does not appear to work when there is a difference between the sqlalchemy schema and the
     # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender
     # migration is completed, this code should be able to be removed.
 
+    if not user:
+        raise ValueError("Can't create a service without a user")
+
     if service_permissions is None:
         service_permissions = DEFAULT_SERVICE_PERMISSIONS
 
+    organisation = dao_get_organisation_by_email_address(user.email_address)
+
     from app.dao.permissions_dao import permission_dao
     service.users.append(user)
     permission_dao.add_default_service_permissions_for_user(user, service)
@@ -173,8 +185,20 @@ def dao_create_service(service, user, service_id=None, service_permissions=None,
 
     # do we just add the default - or will we get a value from FE?
     insert_service_sms_sender(service, current_app.config['FROM_NUMBER'])
+
     if letter_branding:
         service.letter_branding = letter_branding
+
+    if organisation:
+
+        service.organisation = organisation
+
+        if organisation.email_branding_id:
+            service.email_branding = organisation.email_branding_id
+
+        if organisation.letter_branding_id and not service.letter_branding:
+            service.letter_branding = organisation.letter_branding_id
+
     db.session.add(service)
 
 

app/models.py

@@ -316,6 +316,12 @@ organisation_to_service = db.Table(
 )
 
 
+class Domain(db.Model):
+    __tablename__ = "domain"
+    domain = db.Column(db.String(255), primary_key=True)
+    organisation_id = db.Column('organisation_id', UUID(as_uuid=True), db.ForeignKey('organisation.id'), nullable=False)
+
+
 class Organisation(db.Model):
     __tablename__ = "organisation"
     id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4, unique=False)
@@ -329,15 +335,50 @@ class Organisation(db.Model):
         secondary='organisation_to_service',
         uselist=True)
 
+    agreement_signed = db.Column(db.Boolean, nullable=True)
+    agreement_signed_at = db.Column(db.DateTime, nullable=True)
+    agreement_signed_by_id = db.Column(
+        UUID(as_uuid=True),
+        db.ForeignKey('users.id'),
+        nullable=True,
+    )
+    agreement_signed_version = db.Column(db.Float, nullable=True)
+    crown = db.Column(db.Boolean, nullable=True)
+    organisation_type = db.Column(db.String(255), nullable=True)
+
+    domains = db.relationship(
+        'Domain',
+    )
+
+    email_branding = db.relationship('EmailBranding')
+    email_branding_id = db.Column(
+        UUID(as_uuid=True),
+        db.ForeignKey('email_branding.id'),
+        nullable=True,
+    )
+
+    letter_branding = db.relationship('LetterBranding')
+    letter_branding_id = db.Column(
+        UUID(as_uuid=True),
+        db.ForeignKey('letter_branding.id'),
+        nullable=True,
+    )
+
     def serialize(self):
-        serialized = {
+        return {
             "id": str(self.id),
             "name": self.name,
             "active": self.active,
+            "crown": self.crown,
+            "organisation_type": self.organisation_type,
+            "letter_branding_id": self.letter_branding_id,
+            "email_branding_id": self.email_branding_id,
+            "agreement_signed": self.agreement_signed,
+            "agreement_signed_at": self.agreement_signed_at,
+            "agreement_signed_by_id": self.agreement_signed_by_id,
+            "agreement_signed_version": self.agreement_signed_version,
         }
 
-        return serialized
-
 
 class Service(db.Model, Versioned):
     __tablename__ = 'services'

migrations/versions/0278_add_more_stuff_to_orgs.py

@@ -0,0 +1,57 @@
+"""
+
+Revision ID: 0278_add_more_stuff_to_orgs
+Revises: 0277_consent_to_research_null
+Create Date: 2019-02-26 10:15:22.430340
+
+"""
+from alembic import op
+import sqlalchemy as sa
+from sqlalchemy.dialects import postgresql
+
+revision = '0278_add_more_stuff_to_orgs'
+down_revision = '0277_consent_to_research_null'
+
+
+def upgrade():
+    op.create_table(
+        'domain',
+        sa.Column('domain', sa.String(length=255), nullable=False),
+        sa.Column('organisation_id', postgresql.UUID(as_uuid=True), nullable=False),
+        sa.ForeignKeyConstraint(['organisation_id'], ['organisation.id'], ),
+        sa.PrimaryKeyConstraint('domain')
+    )
+    op.create_index(op.f('ix_domain_domain'), 'domain', ['domain'], unique=True)
+
+    op.add_column('organisation', sa.Column('email_branding_id', postgresql.UUID(as_uuid=True), nullable=True))
+    op.create_foreign_key('fk_organisation_email_branding_id', 'organisation', 'email_branding', ['email_branding_id'], ['id'])
+
+    op.add_column('organisation', sa.Column('letter_branding_id', postgresql.UUID(as_uuid=True), nullable=True))
+    op.create_foreign_key('fk_organisation_letter_branding_id', 'organisation', 'letter_branding', ['letter_branding_id'], ['id'])
+
+    op.add_column('organisation', sa.Column('agreement_signed', sa.Boolean(), nullable=True))
+    op.add_column('organisation', sa.Column('agreement_signed_at', sa.DateTime(), nullable=True))
+    op.add_column('organisation', sa.Column('agreement_signed_by_id', postgresql.UUID(as_uuid=True), nullable=True))
+    op.add_column('organisation', sa.Column('agreement_signed_version', sa.Float(), nullable=True))
+    op.add_column('organisation', sa.Column('crown', sa.Boolean(), nullable=True))
+    op.add_column('organisation', sa.Column('organisation_type', sa.String(length=255), nullable=True))
+    op.create_foreign_key('fk_organisation_agreement_user_id', 'organisation', 'users', ['agreement_signed_by_id'], ['id'])
+
+
+def downgrade():
+    op.drop_constraint('fk_organisation_agreement_user_id', 'organisation', type_='foreignkey')
+    op.drop_column('organisation', 'organisation_type')
+    op.drop_column('organisation', 'crown')
+    op.drop_column('organisation', 'agreement_signed_version')
+    op.drop_column('organisation', 'agreement_signed_by_id')
+    op.drop_column('organisation', 'agreement_signed_at')
+    op.drop_column('organisation', 'agreement_signed')
+
+    op.drop_column('organisation', 'email_branding_id')
+    op.drop_constraint('fk_organisation_email_branding_id', 'organisation', type_='foreignkey')
+
+    op.drop_column('organisation', 'letter_branding_id')
+    op.drop_constraint('fk_organisation_letter_branding_id', 'organisation', type_='foreignkey')
+
+    op.drop_index(op.f('ix_domain_domain'), table_name='domain')
+    op.drop_table('domain')

tests/app/dao/test_organisation_dao.py

@@ -1,3 +1,4 @@
+import datetime
 import uuid
 
 import pytest
@@ -5,6 +6,7 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError
 
 from app.dao.organisation_dao import (
     dao_get_organisations,
+    dao_get_organisation_by_email_address,
     dao_get_organisation_by_id,
     dao_get_organisation_by_service_id,
     dao_get_organisation_services,
@@ -16,7 +18,14 @@ from app.dao.organisation_dao import (
 )
 from app.models import Organisation
 
-from tests.app.db import create_organisation, create_service, create_user
+from tests.app.db import (
+    create_domain,
+    create_email_branding,
+    create_letter_branding,
+    create_organisation,
+    create_service,
+    create_user,
+)
 
 
 def test_get_organisations_gets_all_organisations_alphabetically_with_active_organisations_first(
@@ -47,19 +56,67 @@ def test_get_organisation_by_id_gets_correct_organisation(notify_db, notify_db_s
     assert organisation_from_db == organisation
 
 
-def test_update_organisation(notify_db, notify_db_session):
-    updated_name = 'new name'
+def test_update_organisation(
+    notify_db,
+    notify_db_session,
+):
+    create_organisation()
+
+    organisation = Organisation.query.one()
+    user = create_user()
+    email_branding = create_email_branding()
+    letter_branding = create_letter_branding()
+
+    data = {
+        'name': 'new name',
+        "crown": True,
+        "organisation_type": 'local',
+        "agreement_signed": True,
+        "agreement_signed_at": datetime.datetime.utcnow(),
+        "agreement_signed_by_id": user.id,
+        "agreement_signed_version": 999.99,
+        "letter_branding_id": letter_branding.id,
+        "email_branding_id": email_branding.id,
+    }
+
+    for attribute, value in data.items():
+        assert getattr(organisation, attribute) != value
+
+    dao_update_organisation(organisation.id, **data)
+
+    organisation = Organisation.query.one()
+
+    for attribute, value in data.items():
+        assert getattr(organisation, attribute) == value
+
+
+@pytest.mark.parametrize('domain_list, expected_domains', (
+    (['abc', 'def'], {'abc', 'def'}),
+    (['ABC', 'DEF'], {'abc', 'def'}),
+    ([], set()),
+    (None, {'123', '456'}),
+    pytest.param(
+        ['abc', 'ABC'], {'abc'},
+        marks=pytest.mark.xfail(raises=IntegrityError)
+    ),
+))
+def test_update_organisation_domains_lowercases(
+    notify_db,
+    notify_db_session,
+    domain_list,
+    expected_domains,
+):
     create_organisation()
 
     organisation = Organisation.query.one()
 
-    assert organisation.name != updated_name
+    # Seed some domains
+    dao_update_organisation(organisation.id, domains=['123', '456'])
 
-    dao_update_organisation(organisation.id, **{'name': updated_name})
+    # This should overwrite the seeded domains
+    dao_update_organisation(organisation.id, domains=domain_list)
 
-    organisation = Organisation.query.one()
-
-    assert organisation.name == updated_name
+    assert {domain.domain for domain in organisation.domains} == expected_domains
 
 
 def test_add_service_to_organisation(notify_db, notify_db_session, sample_service, sample_organisation):
@@ -171,3 +228,30 @@ def test_add_user_to_organisation_when_user_does_not_exist(sample_organisation):
 def test_add_user_to_organisation_when_organisation_does_not_exist(sample_user):
     with pytest.raises(expected_exception=SQLAlchemyError):
         dao_add_user_to_organisation(organisation_id=uuid.uuid4(), user_id=sample_user.id)
+
+
+@pytest.mark.parametrize('domain, expected_org', (
+    ('unknown.gov.uk', False),
+    ('example.gov.uk', True),
+))
+def test_get_organisation_by_email_address(
+    admin_request,
+    sample_user,
+    domain,
+    expected_org,
+):
+
+    org = create_organisation()
+    create_domain('example.gov.uk', org.id)
+    create_domain('test.gov.uk', org.id)
+
+    another_org = create_organisation(name='Another')
+    create_domain('cabinet-office.gov.uk', another_org.id)
+    create_domain('cabinetoffice.gov.uk', another_org.id)
+
+    found_org = dao_get_organisation_by_email_address('test@{}'.format(domain))
+
+    if expected_org:
+        assert found_org is org
+    else:
+        assert found_org is None

tests/app/dao/test_services_dao.py

@@ -4,7 +4,7 @@ from datetime import datetime
 import pytest
 from freezegun import freeze_time
 from sqlalchemy.exc import IntegrityError
-from sqlalchemy.orm.exc import FlushError, NoResultFound
+from sqlalchemy.orm.exc import NoResultFound
 
 from app import db
 from app.dao.inbound_numbers_dao import (
@@ -167,9 +167,9 @@ def test_cannot_create_service_with_no_user(notify_db_session):
                       message_limit=1000,
                       restricted=False,
                       created_by=user)
-    with pytest.raises(FlushError) as excinfo:
+    with pytest.raises(ValueError) as excinfo:
         dao_create_service(service, None)
-    assert "Can't flush None value found in collection Service.users" in str(excinfo.value)
+    assert "Can't create a service without a user" in str(excinfo.value)
 
 
 def test_should_add_user_to_service(notify_db_session):

tests/app/db.py

@@ -52,7 +52,8 @@ from app.models import (
     Complaint,
     InvitedUser,
     TemplateFolder,
-    LetterBranding
+    LetterBranding,
+    Domain,
 )
 
 
@@ -506,6 +507,16 @@ def create_annual_billing(
     return annual_billing
 
 
+def create_domain(domain, organisation_id):
+
+    domain = Domain(domain=domain, organisation_id=organisation_id)
+
+    db.session.add(domain)
+    db.session.commit()
+
+    return domain
+
+
 def create_organisation(name='test_org_1', active=True):
     data = {
         'name': name,

tests/app/organisation/test_rest.py

@@ -32,10 +32,29 @@ def test_get_organisation_by_id(admin_request, notify_db_session):
         organisation_id=org.id
     )
 
-    assert set(response.keys()) == {'id', 'name', 'active'}
+    assert set(response.keys()) == {
+        'id',
+        'name',
+        'active',
+        'crown',
+        'organisation_type',
+        'agreement_signed',
+        'agreement_signed_at',
+        'agreement_signed_by_id',
+        'agreement_signed_version',
+        'letter_branding_id',
+        'email_branding_id',
+    }
     assert response['id'] == str(org.id)
     assert response['name'] == 'test_org_1'
     assert response['active'] is True
+    assert response['crown'] is None
+    assert response['organisation_type'] is None
+    assert response['agreement_signed'] is None
+    assert response['agreement_signed_by_id'] is None
+    assert response['agreement_signed_version'] is None
+    assert response['letter_branding_id'] is None
+    assert response['email_branding_id'] is None
 
 
 def test_post_create_organisation(admin_request, notify_db_session):
@@ -91,12 +110,27 @@ def test_post_create_organisation_with_missing_name_gives_validation_error(admin
     assert response['errors'][0]['message'] == 'name is a required property'
 
 
-def test_post_update_organisation_updates_fields(admin_request, notify_db_session):
+@pytest.mark.parametrize('agreement_signed', (
+    None, True, False
+))
+@pytest.mark.parametrize('crown', (
+    None, True, False
+))
+def test_post_update_organisation_updates_fields(
+    admin_request,
+    notify_db_session,
+    agreement_signed,
+    crown,
+):
     org = create_organisation()
     data = {
         'name': 'new organisation name',
-        'active': False
+        'active': False,
+        'agreement_signed': agreement_signed,
+        'crown': crown,
     }
+    assert org.agreement_signed is None
+    assert org.crown is None
 
     admin_request.post(
         'organisation.update_organisation',
@@ -111,6 +145,39 @@ def test_post_update_organisation_updates_fields(admin_request, notify_db_sessio
     assert organisation[0].id == org.id
     assert organisation[0].name == data['name']
     assert organisation[0].active == data['active']
+    assert organisation[0].agreement_signed == agreement_signed
+    assert organisation[0].crown == crown
+    assert organisation[0].domains == []
+
+
+@pytest.mark.parametrize('domain_list', (
+    ['example.com'],
+    ['example.com', 'example.org', 'example.net'],
+    [],
+))
+def test_post_update_organisation_updates_domains(
+    admin_request,
+    notify_db_session,
+    domain_list,
+):
+    org = create_organisation(name='test_org_2')
+    data = {
+        'domains': domain_list,
+    }
+
+    admin_request.post(
+        'organisation.update_organisation',
+        _data=data,
+        organisation_id=org.id,
+        _expected_status=204
+    )
+
+    organisation = Organisation.query.all()
+
+    assert len(organisation) == 1
+    assert [
+        domain.domain for domain in organisation[0].domains
+    ] == domain_list
 
 
 def test_post_update_organisation_raises_400_on_existing_org_name(

tests/app/service/test_rest.py

@@ -44,7 +44,9 @@ from tests.app.db import (
     create_inbound_number,
     create_service_sms_sender,
     create_service_with_defined_sms_sender,
-    create_letter_branding
+    create_letter_branding,
+    create_organisation,
+    create_domain,
 )
 from tests.app.db import create_user
 
@@ -253,6 +255,55 @@ def test_create_service(admin_request, sample_user):
     assert service_sms_senders[0].sms_sender == current_app.config['FROM_NUMBER']
 
 
+@pytest.mark.parametrize('domain, expected_org', (
+    (None, False),
+    ('', False),
+    ('unknown.gov.uk', False),
+    ('unknown-example.gov.uk', False),
+    ('example.gov.uk', True),
+    ('test.gov.uk', True),
+    ('test.example.gov.uk', True),
+))
+def test_create_service_with_domain_sets_organisation(
+    admin_request,
+    sample_user,
+    domain,
+    expected_org,
+):
+
+    red_herring_org = create_organisation(name='Sub example')
+    create_domain('specific.example.gov.uk', red_herring_org.id)
+    create_domain('aaaaaaaa.example.gov.uk', red_herring_org.id)
+
+    org = create_organisation()
+    create_domain('example.gov.uk', org.id)
+    create_domain('test.gov.uk', org.id)
+
+    another_org = create_organisation(name='Another')
+    create_domain('cabinet-office.gov.uk', another_org.id)
+    create_domain('cabinetoffice.gov.uk', another_org.id)
+
+    sample_user.email_address = 'test@{}'.format(domain)
+
+    data = {
+        'name': 'created service',
+        'user_id': str(sample_user.id),
+        'message_limit': 1000,
+        'restricted': False,
+        'active': False,
+        'email_from': 'created.service',
+        'created_by': str(sample_user.id),
+        'service_domain': domain,
+    }
+
+    json_resp = admin_request.post('service.create_service', _data=data, _expected_status=201)
+
+    if expected_org:
+        assert json_resp['data']['organisation'] == str(org.id)
+    else:
+        assert json_resp['data']['organisation'] is None
+
+
 def test_create_service_with_domain_sets_letter_branding(admin_request, sample_user):
     letter_branding = create_letter_branding(
         name='test domain', filename='test-domain', domain='test.domain'