From 559639eb632580cfc21ad6a61e5679fa7cb0c269 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 21 Nov 2017 14:37:26 +0000 Subject: [PATCH 1/9] Verify that attribute exists on *History model when versioning objects When createing a history instance of the updated object `create_history` sets attributes using `setattr`. Since SQLAlchemy model instances are Python objects they don't prevent new attributes being created by setattr, which means that if history models are missing some of the columns the attributes will still be assigned, but their values will not be persisted by SQLAlchemy since database columns for them do not exist. To avoid this, we check that the attribute is defined on the `history_cls` and raise an error if it isn't. --- app/history_meta.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/history_meta.py b/app/history_meta.py index c89fe06ca..ff44255f9 100644 --- a/app/history_meta.py +++ b/app/history_meta.py @@ -219,6 +219,8 @@ def create_history(obj, history_cls=None): data['created_at'] = obj.created_at for key, value in data.items(): + if not hasattr(history_cls, key): + raise AttributeError("{} has no attribute '{}'".format(history_cls.__name__, key)) setattr(history, key, value) return history From 4c253bf3b949b14bf61091d49c3935c65415e875 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 21 Nov 2017 14:43:07 +0000 Subject: [PATCH 2/9] Move common Template/TemplateHistory attributes to a base class This allows us to avoid duplication between Template and TemplateHistory classes and makes it easier to ensure that all columns are copied to the TemplateHistory objects. --- app/models.py | 116 ++++++++++++++++++++++---------------------------- 1 file changed, 51 insertions(+), 65 deletions(-) diff --git a/app/models.py b/app/models.py index f921f2aca..8addb11c8 100644 --- a/app/models.py +++ b/app/models.py @@ -4,6 +4,7 @@ import uuid import datetime from flask import url_for, current_app +from sqlalchemy.ext.declarative import declared_attr from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy.dialects.postgresql import ( UUID, @@ -522,51 +523,42 @@ class TemplateProcessTypes(db.Model): name = db.Column(db.String(255), primary_key=True) -class Template(db.Model): - __tablename__ = 'templates' +class TemplateBase(db.Model): + __abstract__ = True id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) name = db.Column(db.String(255), nullable=False) template_type = db.Column(template_types, nullable=False) - created_at = db.Column( - db.DateTime, - index=False, - unique=False, - nullable=False, - default=datetime.datetime.utcnow) - updated_at = db.Column( - db.DateTime, - index=False, - unique=False, - nullable=True, - onupdate=datetime.datetime.utcnow) - content = db.Column(db.Text, index=False, unique=False, nullable=False) - archived = db.Column(db.Boolean, index=False, nullable=False, default=False) - service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False) - service = db.relationship('Service', backref='templates') - subject = db.Column(db.Text, index=False, unique=False, nullable=True) - created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) - created_by = db.relationship('User') - version = db.Column(db.Integer, default=0, nullable=False) - process_type = db.Column( - db.String(255), - db.ForeignKey('template_process_type.name'), - index=True, - nullable=False, - default=NORMAL - ) + created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) + updated_at = db.Column(db.DateTime, onupdate=datetime.datetime.utcnow) + content = db.Column(db.Text, nullable=False) + archived = db.Column(db.Boolean, nullable=False, default=False) + subject = db.Column(db.Text) + + @declared_attr + def service_id(cls): + return db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) + + @declared_attr + def created_by_id(cls): + return db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) + + @declared_attr + def created_by(cls): + return db.relationship('User') + + @declared_attr + def process_type(cls): + return db.Column( + db.String(255), + db.ForeignKey('template_process_type.name'), + index=True, + nullable=False, + default=NORMAL + ) redact_personalisation = association_proxy('template_redacted', 'redact_personalisation') - def get_link(self): - # TODO: use "/v2/" route once available - return url_for( - "template.get_template_by_id_and_service_id", - service_id=self.service_id, - template_id=self.id, - _external=True - ) - def _as_utils_template(self): if self.template_type == EMAIL_TYPE: return PlainTextEmailTemplate( @@ -605,6 +597,22 @@ class Template(db.Model): return serialized +class Template(TemplateBase): + __tablename__ = 'templates' + + service = db.relationship('Service', backref='templates') + version = db.Column(db.Integer, default=0, nullable=False) + + def get_link(self): + # TODO: use "/v2/" route once available + return url_for( + "template.get_template_by_id_and_service_id", + service_id=self.service_id, + template_id=self.id, + _external=True + ) + + class TemplateRedacted(db.Model): __tablename__ = 'template_redacted' @@ -618,32 +626,16 @@ class TemplateRedacted(db.Model): template = db.relationship('Template', uselist=False, backref=db.backref('template_redacted', uselist=False)) -class TemplateHistory(db.Model): +class TemplateHistory(TemplateBase): __tablename__ = 'templates_history' - id = db.Column(UUID(as_uuid=True), primary_key=True) - name = db.Column(db.String(255), nullable=False) - template_type = db.Column(template_types, nullable=False) - created_at = db.Column(db.DateTime, nullable=False) - updated_at = db.Column(db.DateTime) - content = db.Column(db.Text, nullable=False) - archived = db.Column(db.Boolean, nullable=False, default=False) - service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) service = db.relationship('Service') - subject = db.Column(db.Text) - created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) - created_by = db.relationship('User') version = db.Column(db.Integer, primary_key=True, nullable=False) - process_type = db.Column(db.String(255), - db.ForeignKey('template_process_type.name'), - index=True, - nullable=False, - default=NORMAL) - template_redacted = db.relationship('TemplateRedacted', foreign_keys=[id], - primaryjoin='TemplateRedacted.template_id == TemplateHistory.id') - - redact_personalisation = association_proxy('template_redacted', 'redact_personalisation') + @declared_attr + def template_redacted(cls): + return db.relationship('TemplateRedacted', foreign_keys=[cls.id], + primaryjoin='TemplateRedacted.template_id == TemplateHistory.id') def get_link(self): return url_for( @@ -653,12 +645,6 @@ class TemplateHistory(db.Model): _external=True ) - def _as_utils_template(self): - return Template._as_utils_template(self) - - def serialize(self): - return Template.serialize(self) - MMG_PROVIDER = "mmg" FIRETEXT_PROVIDER = "firetext" From cbce6100987df71930fc824c5678ed285e45f996 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 21 Nov 2017 14:46:08 +0000 Subject: [PATCH 3/9] Add template.service_letter_contact_id and reply_to wrapper property Adds a relationship between Template models and service letter contact blocks. Depending on template type, we can have a reference to either a letter contact block, email reply-to address or SMS sender record. This means that in order to enforce foreign key constraints we need to define three separate foreign key columns on the template model. To hide this implementation detail and make it easier to access the sender/reply-to information we define a wrapper property that returns the value from the correct column. The relationship and the property are only defined for letter templates at the moment. The setter raises an error when trying to assign a reply_to value for non-letter templates. The exception isn't raised if the value being assigned is `None` since it can get assigned by marshmallow schemas and as it matches the value returned for other template types it doesn't need to be written anywhere. --- app/models.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/app/models.py b/app/models.py index 8addb11c8..a2187fcb4 100644 --- a/app/models.py +++ b/app/models.py @@ -559,6 +559,26 @@ class TemplateBase(db.Model): redact_personalisation = association_proxy('template_redacted', 'redact_personalisation') + @declared_attr + def service_letter_contact_id(cls): + return db.Column(UUID(as_uuid=True), db.ForeignKey('service_letter_contacts.id'), nullable=True) + + @property + def reply_to(self): + if self.template_type == LETTER_TYPE: + return self.service_letter_contact_id + else: + return None + + @reply_to.setter + def reply_to(self, value): + if self.template_type == LETTER_TYPE: + self.service_letter_contact_id = value + elif value is None: + pass + else: + raise ValueError('Unable to set sender for {} template'.format(self.template_type)) + def _as_utils_template(self): if self.template_type == EMAIL_TYPE: return PlainTextEmailTemplate( From 0be39cc9f97f69d8841d0cb8bc402881858ac5d8 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 21 Nov 2017 14:50:21 +0000 Subject: [PATCH 4/9] Add a migration to add template service_letter_contact_id columns --- .../versions/0144_template_service_letter.py | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 migrations/versions/0144_template_service_letter.py diff --git a/migrations/versions/0144_template_service_letter.py b/migrations/versions/0144_template_service_letter.py new file mode 100644 index 000000000..31bc017cb --- /dev/null +++ b/migrations/versions/0144_template_service_letter.py @@ -0,0 +1,33 @@ +""" + +Revision ID: 0144_template_service_letter +Revises: 0143_remove_reply_to +Create Date: 2017-11-17 15:42:16.401229 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0144_template_service_letter' +down_revision = '0143_remove_reply_to' + + +def upgrade(): + op.add_column('templates', + sa.Column('service_letter_contact_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.create_foreign_key('templates_service_letter_contact_id_fkey', 'templates', + 'service_letter_contacts', ['service_letter_contact_id'], ['id']) + + op.add_column('templates_history', + sa.Column('service_letter_contact_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.create_foreign_key('templates_history_service_letter_contact_id_fkey', 'templates_history', + 'service_letter_contacts', ['service_letter_contact_id'], ['id']) + + +def downgrade(): + op.drop_constraint('templates_service_letter_contact_id_fkey', 'templates', type_='foreignkey') + op.drop_column('templates', 'service_letter_contact_id') + + op.drop_constraint('templates_history_service_letter_contact_id_fkey', 'templates_history', type_='foreignkey') + op.drop_column('templates_history', 'service_letter_contact_id') From f8e1fbe3e60d67ab50807b49bb364f5d81fdf87d Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 21 Nov 2017 14:51:12 +0000 Subject: [PATCH 5/9] Add reply_to fields to template schemas We're hiding the `service_letter_contact_id` column since it should only be readable and writable using the `.reply_to` wrapper. Schemas are defined using `fields.Method` since the fields are represented by a property on the Template model that requires template type to be set. When creating a template, if `reply_to` is defined using `fields.String` it gets assigned at the same time as `template_type` (or the order of assignments is not defined), so the schema loader attempts to set `.reply_to` on a Template object with a `None` `template_type`, which raises an exception. Using `fields.Method` seems to delay `.reply_to` assignment until the Template object is created, which means it already has a valid type. --- app/schemas.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index 5e020ddf1..b4d34ab62 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -314,9 +314,14 @@ class NotificationModelSchema(BaseSchema): class BaseTemplateSchema(BaseSchema): + reply_to = fields.Method("get_reply_to", allow_none=True) + + def get_reply_to(self, template): + return template.reply_to + class Meta: model = models.Template - exclude = ("service_id", "jobs") + exclude = ("service_id", "jobs", "service_letter_contact_id") strict = True @@ -339,9 +344,14 @@ class TemplateSchema(BaseTemplateSchema): class TemplateHistorySchema(BaseSchema): + reply_to = fields.Method("get_reply_to", allow_none=True) + created_by = fields.Nested(UserSchema, only=['id', 'name', 'email_address'], dump_only=True) created_at = field_for(models.Template, 'created_at', format='%Y-%m-%d %H:%M:%S.%f') + def get_reply_to(self, template): + return template.reply_to + class Meta: model = models.TemplateHistory From dc7bd216bf8a365b599e0b24ec9020ba6f4b51bd Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 21 Nov 2017 14:53:06 +0000 Subject: [PATCH 6/9] Add tests for setting reply_to in templates_dao --- tests/app/dao/test_templates_dao.py | 42 ++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 43cc88fd7..607330097 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -15,7 +15,7 @@ from app.dao.templates_dao import ( from app.models import Template, TemplateHistory, TemplateRedacted from tests.app.conftest import sample_template as create_sample_template -from tests.app.db import create_template +from tests.app.db import create_template, create_letter_contact @pytest.mark.parametrize('template_type, subject', [ @@ -53,6 +53,23 @@ def test_create_template_creates_redact_entry(sample_service): assert redacted.updated_by_id == sample_service.created_by_id +def test_create_template_with_reply_to(sample_service, sample_user): + letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA') + + data = { + 'name': 'Sample Template', + 'template_type': "letter", + 'content': "Template content", + 'service': sample_service, + 'created_by': sample_user, + 'reply_to': letter_contact.id, + } + template = Template(**data) + dao_create_template(template) + + assert dao_get_all_templates_for_service(sample_service.id)[0].reply_to == letter_contact.id + + def test_update_template(sample_service, sample_user): data = { 'name': 'Sample Template', @@ -71,6 +88,29 @@ def test_update_template(sample_service, sample_user): assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'new name' +def test_update_template_reply_to(sample_service, sample_user): + letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA') + + data = { + 'name': 'Sample Template', + 'template_type': "letter", + 'content': "Template content", + 'service': sample_service, + 'created_by': sample_user, + } + template = Template(**data) + dao_create_template(template) + created = dao_get_all_templates_for_service(sample_service.id)[0] + assert created.reply_to is None + + created.reply_to = letter_contact.id + dao_update_template(created) + assert dao_get_all_templates_for_service(sample_service.id)[0].reply_to == letter_contact.id + + template_history = TemplateHistory.query.filter_by(id=created.id, version=2).one() + assert template_history.service_letter_contact_id + + def test_redact_template(sample_template): redacted = TemplateRedacted.query.one() assert redacted.template_id == sample_template.id From 999afa7e0dc18d275584eebc42c81217f3ead2cd Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 22 Nov 2017 14:19:16 +0000 Subject: [PATCH 7/9] Add reply_to to the list of template fields that can change --- app/template/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/template/rest.py b/app/template/rest.py index 9a30aab4b..de42e723d 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -155,7 +155,7 @@ def get_template_versions(service_id, template_id): def _template_has_not_changed(current_data, updated_template): return all( current_data[key] == updated_template[key] - for key in ('name', 'content', 'subject', 'archived', 'process_type') + for key in ('name', 'content', 'subject', 'archived', 'process_type', 'reply_to') ) From 7b1f07dd31c3343cf24cf5d2bb7c33f7f7242b11 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 22 Nov 2017 14:19:49 +0000 Subject: [PATCH 8/9] Add tests for reading and updating template reply_to through the API --- tests/app/template/test_rest.py | 37 +++++++++++++++++++++++-- tests/app/template/test_rest_history.py | 19 +++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 2c0f8ea96..da72c3dff 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -17,9 +17,7 @@ from tests.app.conftest import ( sample_template_without_letter_permission, sample_template_without_sms_permission, ) -from tests.app.db import create_service - -from app.dao.templates_dao import dao_get_template_by_id +from tests.app.db import create_service, create_letter_contact @pytest.mark.parametrize('template_type, subject', [ @@ -618,6 +616,39 @@ def test_update_set_process_type_on_template(client, sample_template): assert template.process_type == 'priority' +def test_get_template_reply_to(client, sample_letter_template): + auth_header = create_authorization_header() + letter_contact = create_letter_contact(sample_letter_template.service, "Edinburgh, ED1 1AA") + sample_letter_template.reply_to = str(letter_contact.id) + + resp = client.get('/service/{}/template/{}'.format(sample_letter_template.service_id, sample_letter_template.id), + headers=[auth_header]) + + assert resp.status_code == 200, resp.get_data(as_text=True) + json_resp = json.loads(resp.get_data(as_text=True)) + + assert 'service_letter_contact_id' not in json_resp['data'] + assert json_resp['data']['reply_to'] == str(letter_contact.id) + + +def test_update_template_reply_to(client, sample_letter_template): + auth_header = create_authorization_header() + letter_contact = create_letter_contact(sample_letter_template.service, "Edinburgh, ED1 1AA") + + data = { + 'reply_to': str(letter_contact.id), + } + + resp = client.post('/service/{}/template/{}'.format(sample_letter_template.service_id, sample_letter_template.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert resp.status_code == 200, resp.get_data(as_text=True) + + template = dao_get_template_by_id(sample_letter_template.id) + assert template.reply_to == letter_contact.id + + def test_update_redact_template(admin_request, sample_template): assert sample_template.redact_personalisation is False diff --git a/tests/app/template/test_rest_history.py b/tests/app/template/test_rest_history.py index 482b04aad..c26c4abc8 100644 --- a/tests/app/template/test_rest_history.py +++ b/tests/app/template/test_rest_history.py @@ -3,6 +3,7 @@ from datetime import (datetime, date) from flask import url_for from app.dao.templates_dao import dao_update_template from tests import create_authorization_header +from tests.app.db import create_letter_contact def test_template_history_version(notify_api, sample_user, sample_template): @@ -93,3 +94,21 @@ def test_all_versions_of_template(notify_api, sample_template): assert json_resp['data'][1]['content'] == newer_content assert json_resp['data'][1]['updated_at'] assert json_resp['data'][2]['content'] == old_content + + +def test_update_template_reply_to_updates_history(client, sample_letter_template): + auth_header = create_authorization_header() + letter_contact = create_letter_contact(sample_letter_template.service, "Edinburgh, ED1 1AA") + + sample_letter_template.reply_to = letter_contact.id + dao_update_template(sample_letter_template) + + resp = client.get( + '/service/{}/template/{}/version/2'.format(sample_letter_template.service_id, sample_letter_template.id), + headers=[auth_header] + ) + assert resp.status_code == 200 + + hist_json_resp = json.loads(resp.get_data(as_text=True)) + assert 'service_letter_contact_id' not in hist_json_resp['data'] + assert hist_json_resp['data']['reply_to'] == str(letter_contact.id) From e8ce408f6a9a68149feabaf099bc8cf83c31559b Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 22 Nov 2017 15:55:11 +0000 Subject: [PATCH 9/9] Fix an intermittent test failure when creating a template with reply_to reply_to requires template_type to be already set, but the order of attribute assignment is not defined when a model object is created from a dictionary. This adds a constructor to Template model that makes sure that template_type is set first when multiple arguments are passed to the constructor at once. The problem might still exist when the template is created through the API, so this is a temporary fix to unblock the release. --- app/models.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/models.py b/app/models.py index a2187fcb4..a1322cf75 100644 --- a/app/models.py +++ b/app/models.py @@ -526,6 +526,12 @@ class TemplateProcessTypes(db.Model): class TemplateBase(db.Model): __abstract__ = True + def __init__(self, **kwargs): + if 'template_type' in kwargs: + self.template_type = kwargs.pop('template_type') + + super().__init__(**kwargs) + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) name = db.Column(db.String(255), nullable=False) template_type = db.Column(template_types, nullable=False)