From d7e03e00d32e2b71c55e3b44d2c740e0e3dec12b Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Tue, 19 Feb 2019 11:47:30 +0000
Subject: [PATCH 1/4] Storing more info about an organisation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently we have
- a thing in the database called an ‘organisation’ which we don’t use
- the idea of an organisation which we derive from the user’s email
  address and is used to set the default branding for their service and
  determine whether they’ve signed the MOU

We should make these two things into one thing, by storing everything
we know about an organisation against that organisation in the database.
This will be much less laborious than storing it in a YAML file that
needs a deploy every time it’s updated.

An organisation can now have:
- domains which we can use to automatically associate services with it
  (eg anyone whose email address ends in `dwp.gsi.gov.uk` gets services
  they create associated to the DWP organisation)
- default letter branding for any new services
- default email branding for any new services
---
 app/dao/organisation_dao.py                   | 19 ++++-
 app/models.py                                 | 47 +++++++++++-
 .../versions/0278_add_more_stuff_to_orgs.py   | 57 +++++++++++++++
 tests/app/dao/test_organisation_dao.py        | 38 ++++++++--
 tests/app/organisation/test_rest.py           | 73 ++++++++++++++++++-
 5 files changed, 221 insertions(+), 13 deletions(-)
 create mode 100644 migrations/versions/0278_add_more_stuff_to_orgs.py

diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py
index 94cdeb237..e5e1cbb62 100644
--- a/app/dao/organisation_dao.py
+++ b/app/dao/organisation_dao.py
@@ -2,6 +2,7 @@ from app import db
 from app.dao.dao_utils import transactional
 from app.models import (
     Organisation,
+    Domain,
     InvitedOrganisationUser,
     User
 )
@@ -34,10 +35,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 domains:
+
+        Domain.query.filter_by(organisation_id=organisation_id).delete()
+
+        db.session.bulk_save_objects([
+            Domain(domain=domain, organisation_id=organisation_id)
+            for domain in domains
+        ])
+
+        db.session.commit()
+
+    return organisation
+
 
 @transactional
 def dao_add_service_to_organisation(service, organisation_id):
diff --git a/app/models.py b/app/models.py
index b9c7799bd..a897bd761 100644
--- a/app/models.py
+++ b/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'
diff --git a/migrations/versions/0278_add_more_stuff_to_orgs.py b/migrations/versions/0278_add_more_stuff_to_orgs.py
new file mode 100644
index 000000000..f3bd91469
--- /dev/null
+++ b/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')
diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py
index 62c81041a..7e1f90e2f 100644
--- a/tests/app/dao/test_organisation_dao.py
+++ b/tests/app/dao/test_organisation_dao.py
@@ -1,3 +1,4 @@
+import datetime
 import uuid
 
 import pytest
@@ -16,7 +17,13 @@ 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_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 +54,38 @@ 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()
 
-    assert organisation.name != updated_name
+    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,
+    }
 
-    dao_update_organisation(organisation.id, **{'name': updated_name})
+    for attribute, value in data.items():
+        assert getattr(organisation, attribute) != value
+
+    dao_update_organisation(organisation.id, **data)
 
     organisation = Organisation.query.one()
 
-    assert organisation.name == updated_name
+    for attribute, value in data.items():
+        assert getattr(organisation, attribute) == value
 
 
 def test_add_service_to_organisation(notify_db, notify_db_session, sample_service, sample_organisation):
diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py
index dee403245..1bd0fecf7 100644
--- a/tests/app/organisation/test_rest.py
+++ b/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(

From c0fb9267bd531cd90750272c006bd0a35d37e2b6 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Tue, 19 Feb 2019 12:02:18 +0000
Subject: [PATCH 2/4] Automatically associate new service with an org

This is the same thing we do in the admin app at the moment with YAML:
https://github.com/alphagov/notifications-admin/blob/2f4e933b6595d384bebea253ba47f8091b462ca3/app/utils.py#L556-L562
---
 app/dao/organisation_dao.py            | 14 ++++++++
 app/dao/services_dao.py                | 26 +++++++++++++-
 tests/app/dao/test_organisation_dao.py | 29 +++++++++++++++
 tests/app/dao/test_services_dao.py     |  6 ++--
 tests/app/db.py                        | 13 ++++++-
 tests/app/service/test_rest.py         | 49 +++++++++++++++++++++++++-
 6 files changed, 131 insertions(+), 6 deletions(-)

diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py
index e5e1cbb62..eca2787b4 100644
--- a/app/dao/organisation_dao.py
+++ b/app/dao/organisation_dao.py
@@ -24,6 +24,20 @@ 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.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()
 
diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py
index eb766c06b..70d7a8a83 100644
--- a/app/dao/services_dao.py
+++ b/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)
 
 
diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py
index 7e1f90e2f..6375cf7b6 100644
--- a/tests/app/dao/test_organisation_dao.py
+++ b/tests/app/dao/test_organisation_dao.py
@@ -6,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,
@@ -18,6 +19,7 @@ from app.dao.organisation_dao import (
 from app.models import Organisation
 
 from tests.app.db import (
+    create_domain,
     create_email_branding,
     create_letter_branding,
     create_organisation,
@@ -197,3 +199,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
diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py
index 6461fd93c..cbc6cb24c 100644
--- a/tests/app/dao/test_services_dao.py
+++ b/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):
diff --git a/tests/app/db.py b/tests/app/db.py
index 1b301a2f2..0b3b24c2b 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -51,7 +51,8 @@ from app.models import (
     Complaint,
     InvitedUser,
     TemplateFolder,
-    LetterBranding
+    LetterBranding,
+    Domain,
 )
 
 
@@ -497,6 +498,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,
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index 22208dd52..e216f8801 100644
--- a/tests/app/service/test_rest.py
+++ b/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,51 @@ 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,
+):
+
+    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'

From 6e8ce786030f0bcea42ad8af7718551b6776b100 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 7 Mar 2019 11:23:42 +0000
Subject: [PATCH 3/4] Choose most specific domains first
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If we had organisations for GDS and Cabinet Office, then we’d always
want someone whose email address ends in `@cabinet-office.gov.uk` to
match to `cabinet-office.gov.uk` before matching to
`digital.cabinet-office.gov.uk`.

Sorting the list by shortest first addresses this.
---
 app/dao/organisation_dao.py    | 5 ++++-
 tests/app/service/test_rest.py | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py
index eca2787b4..bdd430c8c 100644
--- a/app/dao/organisation_dao.py
+++ b/app/dao/organisation_dao.py
@@ -1,3 +1,5 @@
+from sqlalchemy.sql.expression import func
+
 from app import db
 from app.dao.dao_utils import transactional
 from app.models import (
@@ -28,7 +30,8 @@ def dao_get_organisation_by_email_address(email_address):
 
     email_address = email_address.lower()
 
-    for domain in Domain().query.all():
+    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))
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index e216f8801..744f509e9 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -271,6 +271,10 @@ def test_create_service_with_domain_sets_organisation(
     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)

From 73ca8b73f92e6967e397ef5314a646538f30c4cc Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 8 Mar 2019 14:38:49 +0000
Subject: [PATCH 4/4] Make sure domains are always lowercased

Because otherwise we might get garbage duplicate data.
---
 app/dao/organisation_dao.py            |  4 ++--
 tests/app/dao/test_organisation_dao.py | 29 ++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py
index bdd430c8c..26b6dca1f 100644
--- a/app/dao/organisation_dao.py
+++ b/app/dao/organisation_dao.py
@@ -59,12 +59,12 @@ def dao_update_organisation(organisation_id, **kwargs):
         kwargs
     )
 
-    if domains:
+    if isinstance(domains, list):
 
         Domain.query.filter_by(organisation_id=organisation_id).delete()
 
         db.session.bulk_save_objects([
-            Domain(domain=domain, organisation_id=organisation_id)
+            Domain(domain=domain.lower(), organisation_id=organisation_id)
             for domain in domains
         ])
 
diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py
index 6375cf7b6..667fc1438 100644
--- a/tests/app/dao/test_organisation_dao.py
+++ b/tests/app/dao/test_organisation_dao.py
@@ -90,6 +90,35 @@ def test_update_organisation(
         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()
+
+    # Seed some domains
+    dao_update_organisation(organisation.id, domains=['123', '456'])
+
+    # This should overwrite the seeded domains
+    dao_update_organisation(organisation.id, domains=domain_list)
+
+    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):
     assert sample_organisation.services == []