From 936c8489b39209d0731c0bb9519d3a16d800e93f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 26 Oct 2018 16:01:31 +0100 Subject: [PATCH 1/5] add TemplateFolder model a TemplateFolder has a service, a name, and a parent. Parent is a nullable foreign key pointing to another TemplateFolder instance. We don't do any checks here for cyclical or otherwise invalid folder structures so keep your data clean, folks! Unsurprisingly, a Template can be part of a TemplateFolder - there's a mapping class (template_folder_map to avoid giving it a dumb name) - this mapping table shouldn't be interacted with directly - rather, you should use the `Template.folder` or `TemplateFolder.templates` relationship. --- app/models.py | 28 ++++++++++++++ migrations/versions/0242_template_folders.py | 39 ++++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 migrations/versions/0242_template_folders.py diff --git a/app/models.py b/app/models.py index 72a0e3f7d..f075c9f43 100644 --- a/app/models.py +++ b/app/models.py @@ -706,6 +706,27 @@ class TemplateProcessTypes(db.Model): name = db.Column(db.String(255), primary_key=True) +class TemplateFolder(db.Model): + __tablename__ = 'template_folder' + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), nullable=False) + name = db.Column(db.String, nullable=False) + parent_id = db.Column(UUID(as_uuid=True), db.ForeignKey('template_folder.id'), nullable=True) + + service = db.relationship('Service') + parent = db.relationship('TemplateFolder', remote_side=[id], backref='children') + + +template_folder_map = db.Table( + 'template_folder_map', + db.Model.metadata, + # template_id is a primary key as a template can only belong in one folder + db.Column('template_id', UUID(as_uuid=True), db.ForeignKey('templates.id'), primary_key=True, nullable=False), + db.Column('template_folder_id', UUID(as_uuid=True), db.ForeignKey('template_folder.id'), nullable=False), +) + + PRECOMPILED_TEMPLATE_NAME = 'Pre-compiled PDF' @@ -837,6 +858,13 @@ class Template(TemplateBase): service = db.relationship('Service', backref='templates') version = db.Column(db.Integer, default=0, nullable=False) + folder = db.relationship( + 'TemplateFolder', + secondary=template_folder_map, + uselist=False, + backref=db.backref('templates', lazy='dynamic') + ) + def get_link(self): # TODO: use "/v2/" route once available return url_for( diff --git a/migrations/versions/0242_template_folders.py b/migrations/versions/0242_template_folders.py new file mode 100644 index 000000000..187ae8ec5 --- /dev/null +++ b/migrations/versions/0242_template_folders.py @@ -0,0 +1,39 @@ +""" + +Revision ID: 0242_template_folders +Revises: 0241_another_letter_org +Create Date: 2018-10-26 16:00:40.173840 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0242_template_folders' +down_revision = '0241_another_letter_org' + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('template_folder', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('name', sa.String(), nullable=False), + sa.Column('parent_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.ForeignKeyConstraint(['parent_id'], ['template_folder.id'], ), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_table('template_folder_map', + sa.Column('template_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('template_folder_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.ForeignKeyConstraint(['template_folder_id'], ['template_folder.id'], ), + sa.ForeignKeyConstraint(['template_id'], ['templates.id'], ), + sa.PrimaryKeyConstraint('template_id') + ) + + +def downgrade(): + op.drop_table('template_folder_map') + op.drop_table('template_folder') + # ### end Alembic commands ### From 5693d1181e157464dd454e7f10bd5ccac8ee466e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 26 Oct 2018 17:51:50 +0100 Subject: [PATCH 2/5] add notify_db_session to test funcs that hit db --- tests/app/service/test_archived_service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/service/test_archived_service.py b/tests/app/service/test_archived_service.py index b06e7ba01..abd179c24 100644 --- a/tests/app/service/test_archived_service.py +++ b/tests/app/service/test_archived_service.py @@ -17,13 +17,13 @@ from tests.app.conftest import ( ) -def test_archive_only_allows_post(client): +def test_archive_only_allows_post(client, notify_db_session): auth_header = create_authorization_header() response = client.get('/service/{}/archive'.format(uuid.uuid4()), headers=[auth_header]) assert response.status_code == 405 -def test_archive_service_errors_with_bad_service_id(client): +def test_archive_service_errors_with_bad_service_id(client, notify_db_session): auth_header = create_authorization_header() response = client.post('/service/{}/archive'.format(uuid.uuid4()), headers=[auth_header]) assert response.status_code == 404 From ac5c19c3d5b0e8f2c087a6112daf52710cb42eb0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 26 Oct 2018 17:54:53 +0100 Subject: [PATCH 3/5] remove some pytest warnings --- app/schemas.py | 4 ++-- tests/app/service/test_archived_service.py | 23 +++++++++++----------- tests/conftest.py | 2 +- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index f4730afa5..460b52240 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -205,12 +205,12 @@ class ServiceSchema(BaseSchema): created_by = field_for(models.Service, 'created_by', required=True) organisation_type = field_for(models.Service, 'organisation_type') dvla_organisation = field_for(models.Service, 'dvla_organisation') - letter_logo_filename = fields.Method(method_name='get_letter_logo_filename') + letter_logo_filename = fields.Method(serialize='get_letter_logo_filename') permissions = fields.Method("service_permissions") email_branding = field_for(models.Service, 'email_branding') organisation = field_for(models.Service, 'organisation') override_flag = False - letter_contact_block = fields.Method(method_name="get_letter_contact") + letter_contact_block = fields.Method(serialize="get_letter_contact") def get_letter_logo_filename(self, service): return service.dvla_organisation.filename diff --git a/tests/app/service/test_archived_service.py b/tests/app/service/test_archived_service.py index abd179c24..2b9d59979 100644 --- a/tests/app/service/test_archived_service.py +++ b/tests/app/service/test_archived_service.py @@ -11,10 +11,7 @@ from app.dao.api_key_dao import expire_api_key from app.dao.templates_dao import dao_update_template from tests import create_authorization_header, unwrap_function -from tests.app.conftest import ( - sample_template as create_template, - sample_api_key as create_api_key -) +from tests.app.db import create_template, create_api_key def test_archive_only_allows_post(client, notify_db_session): @@ -38,11 +35,13 @@ def test_deactivating_inactive_service_does_nothing(client, sample_service): @pytest.fixture -def archived_service(client, notify_db, notify_db_session, sample_service): - create_template(notify_db, notify_db_session, template_name='a') - create_template(notify_db, notify_db_session, template_name='b') - create_api_key(notify_db, notify_db_session) - create_api_key(notify_db, notify_db_session) +def archived_service(client, notify_db, sample_service): + create_template(sample_service, template_name='a') + create_template(sample_service, template_name='b') + create_api_key(sample_service) + create_api_key(sample_service) + + notify_db.session.commit() auth_header = create_authorization_header() response = client.post('/service/{}/archive'.format(sample_service.id), headers=[auth_header]) @@ -83,10 +82,10 @@ def test_deactivating_service_creates_history(archived_service): @pytest.fixture -def archived_service_with_deleted_stuff(client, notify_db, notify_db_session, sample_service): +def archived_service_with_deleted_stuff(client, sample_service): with freeze_time('2001-01-01'): - template = create_template(notify_db, notify_db_session, template_name='a') - api_key = create_api_key(notify_db, notify_db_session) + template = create_template(sample_service, template_name='a') + api_key = create_api_key(sample_service) expire_api_key(sample_service.id, api_key.id) diff --git a/tests/conftest.py b/tests/conftest.py index e3762afd9..f50f97565 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -132,7 +132,7 @@ def os_environ(): def pytest_generate_tests(metafunc): # Copied from https://gist.github.com/pfctdayelise/5719730 - idparametrize = getattr(metafunc.function, 'idparametrize', None) + idparametrize = metafunc.definition.get_closest_marker('idparametrize') if idparametrize: argnames, testdata = idparametrize.args ids, argvalues = zip(*sorted(testdata.items())) From 0d5f8cc7df9050db8c3bcb5b912c48de21d19cae Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 26 Oct 2018 18:02:41 +0100 Subject: [PATCH 4/5] add status endpoint to '/' paas were trying to ascertain if notify was up by looking at '/', for cert renewal. This commit adds the status endpoint to '/', so we're not mistakenly left for our cert to expire --- app/status/healthcheck.py | 1 + tests/app/status/test_status.py | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/status/healthcheck.py b/app/status/healthcheck.py index da305c9e2..b0c451d0f 100644 --- a/app/status/healthcheck.py +++ b/app/status/healthcheck.py @@ -9,6 +9,7 @@ from app import db, version status = Blueprint('status', __name__) +@status.route('/', methods=['GET']) @status.route('/_status', methods=['GET', 'POST']) def show_status(): if request.args.get('simple', None): diff --git a/tests/app/status/test_status.py b/tests/app/status/test_status.py index 4203a4e4d..f96289988 100644 --- a/tests/app/status/test_status.py +++ b/tests/app/status/test_status.py @@ -1,8 +1,9 @@ +import pytest from flask import json -def test_get_status_all_ok(client): - path = '/_status' +@pytest.mark.parametrize('path', ['/', '/_status']) +def test_get_status_all_ok(client, notify_db_session, path): response = client.get(path) assert response.status_code == 200 resp_json = json.loads(response.get_data(as_text=True)) From 7d0fa279a3fc4d6c85de250b6b9ebb0aaff270d7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 29 Oct 2018 11:57:24 +0000 Subject: [PATCH 5/5] eager join folders from the Template object to keep versioning working The `@version_class` decorator looks at every dirty (modified) model in the session to work out which new history models to create. However, if there are dirty items in the session, sqlalchemy might flush to the database, clearing the whole session. We ran into problems with the archive service function, which is versioned for api keys, templates and services. When constructing the TemplateHistory objects, `history_meta.py::create_history` would call getattr on `Template.folders`, which would make a database call to join across to the TemplateFolder objects - this would then flush the dirty Service object from the session before the ServiceHistory object was created. To get around this, we eager load the Template.folder object, joining on to it automatically when the Template is fetched. That way, it doesn't make a SELECT mid-way through the version decorator, and the history is preserved. Note: This relationship is only on Template, not TemplateHistory - so we're not doing this join every single time we send a message. --- app/models.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models.py b/app/models.py index f075c9f43..29f842c96 100644 --- a/app/models.py +++ b/app/models.py @@ -862,6 +862,8 @@ class Template(TemplateBase): 'TemplateFolder', secondary=template_folder_map, uselist=False, + # eagerly load the folder whenever the template object is fetched + lazy='joined', backref=db.backref('templates', lazy='dynamic') )