From 5b256fa64eb789606cc272a15648db83071bd61a Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 19 Jul 2019 14:51:06 +0100 Subject: [PATCH 1/3] Migrate all orgs and services onto new organisation types Remove all mentions of generic 'nhs' organisation type. --- app/dao/services_dao.py | 2 +- app/models.py | 5 +- migrations/versions/0300_migrate_org_types.py | 116 ++++++++++++++++++ tests/app/dao/test_services_dao.py | 4 +- tests/app/organisation/test_rest.py | 2 +- 5 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 migrations/versions/0300_migrate_org_types.py diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index bf20f6f4b..f901fce0a 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -309,7 +309,7 @@ def dao_create_service( if organisation.letter_branding and not service.letter_branding: service.letter_branding = organisation.letter_branding - elif service.organisation_type == 'nhs' or email_address_is_nhs(user.email_address): + elif service.organisation_type in ['nhs_central', 'nhs_local'] or email_address_is_nhs(user.email_address): service.email_branding = dao_get_email_branding_by_name('NHS') service.letter_branding = dao_get_letter_branding_by_name('NHS') diff --git a/app/models.py b/app/models.py index 3a40d8924..2361bc922 100644 --- a/app/models.py +++ b/app/models.py @@ -325,12 +325,11 @@ class Domain(db.Model): ORGANISATION_TYPES = [ - "central", "local", "nhs_central", "nhs", - "nhs_local", "emergency_service", "school_or_college", "other", + "central", "local", "nhs_central", "nhs_local", "emergency_service", "school_or_college", "other", ] CROWN_ORGANISATION_TYPES = ["nhs_central"] -NON_CROWN_ORGANISATION_TYPES = ["local", "nhs_local", "emergency_service", "school_or_college", "nhs"] +NON_CROWN_ORGANISATION_TYPES = ["local", "nhs_local", "emergency_service", "school_or_college"] class OrganisationTypes(db.Model): diff --git a/migrations/versions/0300_migrate_org_types.py b/migrations/versions/0300_migrate_org_types.py new file mode 100644 index 000000000..d75abe3c4 --- /dev/null +++ b/migrations/versions/0300_migrate_org_types.py @@ -0,0 +1,116 @@ +""" + +Revision ID: 0300_migrate_org_types +Revises: 0299_org_types_table +Create Date: 2019-07-19 11:13:41.286472 + +""" +from alembic import op + + +revision = '0300_migrate_org_types' +down_revision = '0299_org_types_table' + + +def upgrade(): + op.execute(""" + UPDATE + organisation + SET + organisation_type = 'nhs_local' + FROM + organisation_to_service, annual_billing + WHERE + organisation.organisation_type = 'nhs' + AND + annual_billing.service_id = organisation_to_service.service_id + AND + organisation_to_service.organisation_id = organisation.id + AND + annual_billing.free_sms_fragment_limit = 25000 + """) + + op.execute(""" + UPDATE + services + SET + organisation_type = 'nhs_local' + FROM + annual_billing + WHERE + services.organisation_type = 'nhs' + AND + annual_billing.service_id = services.id + AND + annual_billing.free_sms_fragment_limit = 25000 + """) + + op.execute(""" + UPDATE + organisation + SET + organisation_type = 'nhs_central' + FROM + organisation_to_service, annual_billing + WHERE + organisation.organisation_type = 'nhs' + AND + annual_billing.service_id = organisation_to_service.service_id + AND + organisation_to_service.organisation_id = organisation.id + AND + annual_billing.free_sms_fragment_limit = 250000 + """) + + op.execute(""" + UPDATE + services + SET + organisation_type = 'nhs_central' + FROM + annual_billing + WHERE + services.organisation_type = 'nhs' + AND + annual_billing.service_id = services.id + AND + annual_billing.free_sms_fragment_limit = 250000 + """) + + +def downgrade(): + op.execute(""" + UPDATE + organisation + SET + organisation_type = 'nhs' + WHERE + organisation_type = 'nhs_central' + """) + + op.execute(""" + UPDATE + services + SET + organisation_type = 'nhs' + WHERE + organisation_type = 'nhs_central' + """) + + op.execute(""" + UPDATE + organisation + SET + organisation_type = 'nhs' + WHERE + organisation_type = 'nhs_local' + """) + + op.execute(""" + UPDATE + services + SET + organisation_type = 'nhs' + WHERE + organisation_type = 'nhs_local' + """) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 72414c2ba..61c1fc7da 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -110,8 +110,8 @@ def test_create_service(notify_db_session): @pytest.mark.parametrize('email_address, organisation_type', ( - ("test@example.gov.uk", 'nhs'), - ("test@nhs.net", 'nhs'), + ("test@example.gov.uk", 'nhs_central'), + ("test@nhs.net", 'nhs_local'), ("test@nhs.net", 'local'), ("test@nhs.net", 'central'), ("test@nhs.uk", 'central'), diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index fa230e2c9..b6d0f0341 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -204,7 +204,7 @@ def test_post_create_organisation_existing_name_raises_400(admin_request, sample 'name': 'Service name', 'crown': False, 'organisation_type': 'foo', - }, 'organisation_type foo is not one of [central, local, nhs_central, nhs, nhs_local, emergency_service, school_or_college, other]'), # noqa + }, 'organisation_type foo is not one of [central, local, nhs_central, nhs_local, emergency_service, school_or_college, other]'), # noqa )) def test_post_create_organisation_with_missing_data_gives_validation_error( admin_request, From 38bb2c1cf6d71672d13b5a352fc335416694bdee Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 24 Jul 2019 16:37:23 +0100 Subject: [PATCH 2/3] Simpify org types migration script and introduce foreign key Don't transform org types on prod --- app/models.py | 9 +- app/schemas.py | 1 + migrations/versions/0300_migrate_org_types.py | 145 +++++++----------- tests/app/service/test_rest.py | 22 ++- tests/conftest.py | 1 + 5 files changed, 82 insertions(+), 96 deletions(-) diff --git a/app/models.py b/app/models.py index 2361bc922..9b2885f9c 100644 --- a/app/models.py +++ b/app/models.py @@ -365,7 +365,12 @@ class Organisation(db.Model): agreement_signed_on_behalf_of_email_address = db.Column(db.String(255), 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) + organisation_type = db.Column( + db.String(255), + db.ForeignKey('organisation_types.name'), + unique=False, + nullable=True, + ) request_to_go_live_notes = db.Column(db.Text) domains = db.relationship( @@ -456,6 +461,8 @@ class Service(db.Model, Versioned): prefix_sms = db.Column(db.Boolean, nullable=False, default=True) organisation_type = db.Column( db.String(255), + db.ForeignKey('organisation_types.name'), + unique=False, nullable=True, ) crown = db.Column(db.Boolean, index=False, nullable=True) diff --git a/app/schemas.py b/app/schemas.py index 455b23470..33f5cd634 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -266,6 +266,7 @@ class ServiceSchema(BaseSchema): class DetailedServiceSchema(BaseSchema): statistics = fields.Dict() + organisation_type = field_for(models.Service, 'organisation_type') class Meta: model = models.Service diff --git a/migrations/versions/0300_migrate_org_types.py b/migrations/versions/0300_migrate_org_types.py index d75abe3c4..239da1b96 100644 --- a/migrations/versions/0300_migrate_org_types.py +++ b/migrations/versions/0300_migrate_org_types.py @@ -1,116 +1,75 @@ +import os + """ Revision ID: 0300_migrate_org_types Revises: 0299_org_types_table -Create Date: 2019-07-19 11:13:41.286472 +Create Date: 2019-07-24 16:18:27.467361 """ from alembic import op +import sqlalchemy as sa revision = '0300_migrate_org_types' down_revision = '0299_org_types_table' +environment = os.environ['NOTIFY_ENVIRONMENT'] + def upgrade(): - op.execute(""" - UPDATE - organisation - SET - organisation_type = 'nhs_local' - FROM - organisation_to_service, annual_billing - WHERE - organisation.organisation_type = 'nhs' - AND - annual_billing.service_id = organisation_to_service.service_id - AND - organisation_to_service.organisation_id = organisation.id - AND - annual_billing.free_sms_fragment_limit = 25000 - """) + if environment != "production": + op.execute(""" + UPDATE + organisation + SET + organisation_type = 'nhs_local' + WHERE + organisation.organisation_type = 'nhs' + """) - op.execute(""" - UPDATE - services - SET - organisation_type = 'nhs_local' - FROM - annual_billing - WHERE - services.organisation_type = 'nhs' - AND - annual_billing.service_id = services.id - AND - annual_billing.free_sms_fragment_limit = 25000 - """) + op.execute(""" + UPDATE + services + SET + organisation_type = 'nhs_local' + WHERE + services.organisation_type = 'nhs' + """) - op.execute(""" - UPDATE - organisation - SET - organisation_type = 'nhs_central' - FROM - organisation_to_service, annual_billing - WHERE - organisation.organisation_type = 'nhs' - AND - annual_billing.service_id = organisation_to_service.service_id - AND - organisation_to_service.organisation_id = organisation.id - AND - annual_billing.free_sms_fragment_limit = 250000 - """) + op.alter_column('organisation_types', 'name', existing_type=sa.VARCHAR(), type_=sa.String(length=255)) - op.execute(""" - UPDATE - services - SET - organisation_type = 'nhs_central' - FROM - annual_billing - WHERE - services.organisation_type = 'nhs' - AND - annual_billing.service_id = services.id - AND - annual_billing.free_sms_fragment_limit = 250000 - """) + op.create_foreign_key( + 'organisation_organisation_type_fkey', 'organisation', 'organisation_types', ['organisation_type'], ['name'] + ) + + op.create_foreign_key( + 'services_organisation_type_fkey', 'services', 'organisation_types', ['organisation_type'], ['name'] + ) def downgrade(): - op.execute(""" - UPDATE - organisation - SET - organisation_type = 'nhs' - WHERE - organisation_type = 'nhs_central' - """) + op.drop_constraint('services_organisation_type_fkey', 'services', type_='foreignkey') - op.execute(""" - UPDATE - services - SET - organisation_type = 'nhs' - WHERE - organisation_type = 'nhs_central' - """) + op.drop_constraint('organisation_organisation_type_fkey', 'organisation', type_='foreignkey') - op.execute(""" - UPDATE - organisation - SET - organisation_type = 'nhs' - WHERE - organisation_type = 'nhs_local' - """) + op.alter_column('organisation_types', 'name', existing_type=sa.String(length=255), type_=sa.VARCHAR()) - op.execute(""" - UPDATE - services - SET - organisation_type = 'nhs' - WHERE - organisation_type = 'nhs_local' - """) + if environment != "production": + op.execute(""" + UPDATE + organisation + SET + organisation_type = 'nhs' + WHERE + organisation_type = 'nhs_local' + """) + + op.execute(""" + UPDATE + services + SET + organisation_type = 'nhs' + WHERE + organisation_type = 'nhs_local' + """) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 028584452..03cb5c4f6 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -574,7 +574,7 @@ def test_update_service(client, notify_db, sample_service): 'email_from': 'updated.service.name', 'created_by': str(sample_service.created_by.id), 'email_branding': str(brand.id), - 'organisation_type': 'foo', + 'organisation_type': 'school_or_college', } auth_header = create_authorization_header() @@ -589,7 +589,25 @@ def test_update_service(client, notify_db, sample_service): assert result['data']['name'] == 'updated service name' assert result['data']['email_from'] == 'updated.service.name' assert result['data']['email_branding'] == str(brand.id) - assert result['data']['organisation_type'] == 'foo' + assert result['data']['organisation_type'] == 'school_or_college' + + +def test_cant_update_service_org_type_to_random_value(client, sample_service): + data = { + 'name': 'updated service name', + 'email_from': 'updated.service.name', + 'created_by': str(sample_service.created_by.id), + 'organisation_type': 'foo', + } + + auth_header = create_authorization_header() + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + assert resp.status_code == 500 def test_update_service_letter_branding(client, notify_db, sample_service): diff --git a/tests/conftest.py b/tests/conftest.py index f8c6f798a..627250ca4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -102,6 +102,7 @@ def notify_db_session(notify_db): "provider_details_history", "template_process_type", "notification_status_types", + "organisation_types", "service_permission_types", "auth_type", "invite_status_type", From 5977adef1c792c1f7afe137eff3809ef4d57dfb5 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 25 Jul 2019 16:03:08 +0100 Subject: [PATCH 3/3] Add live to excluded environments in migration --- migrations/versions/0300_migrate_org_types.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/migrations/versions/0300_migrate_org_types.py b/migrations/versions/0300_migrate_org_types.py index 239da1b96..9a9d1b4b7 100644 --- a/migrations/versions/0300_migrate_org_types.py +++ b/migrations/versions/0300_migrate_org_types.py @@ -18,7 +18,7 @@ environment = os.environ['NOTIFY_ENVIRONMENT'] def upgrade(): - if environment != "production": + if environment not in ["live", "production"]: op.execute(""" UPDATE organisation @@ -55,7 +55,7 @@ def downgrade(): op.alter_column('organisation_types', 'name', existing_type=sa.String(length=255), type_=sa.VARCHAR()) - if environment != "production": + if environment not in ["live", "production"]: op.execute(""" UPDATE organisation