From d7e03e00d32e2b71c55e3b44d2c740e0e3dec12b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 19 Feb 2019 11:47:30 +0000 Subject: [PATCH] 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(