From 049514d4b2f02f8a5f191bd1c62229721ff20f9d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 2 Aug 2016 16:23:14 +0100 Subject: [PATCH 1/6] remove history-meta for templates, replace with hand-made history table history-meta's dynamic magic is insufficient for templates, where we need to be able to refer to the specific history table to take advantage of sqlalchemy's relationship management (2/3rds of an ORM). So replace it with a custom made version table. Had to change the version decorator slightly for this --- app/dao/dao_utils.py | 13 ++++++++----- app/dao/services_dao.py | 3 ++- app/dao/templates_dao.py | 21 ++++++++++++--------- app/history_meta.py | 14 +++++--------- app/models.py | 22 +++++++++++++++++++++- app/service/rest.py | 2 +- tests/app/dao/test_services_dao.py | 2 +- tests/app/dao/test_templates_dao.py | 14 +++++++------- 8 files changed, 57 insertions(+), 34 deletions(-) diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index f5179e8b5..d5ec27fb5 100644 --- a/app/dao/dao_utils.py +++ b/app/dao/dao_utils.py @@ -1,6 +1,7 @@ import itertools -from functools import wraps -from app.history_meta import versioned_objects, create_history +from functools import wraps, partial + +from app.history_meta import create_history def transactional(func): @@ -19,14 +20,16 @@ def transactional(func): return commit_or_rollback -def version_class(model_class): +def version_class(model_class, history_cls=None): + create_hist = partial(create_history, history_cls=history_cls) + def versioned(func): @wraps(func) def record_version(*args, **kwargs): from app import db func(*args, **kwargs) - history_objects = [create_history(obj) for obj in - versioned_objects(itertools.chain(db.session.new, db.session.dirty)) + history_objects = [create_hist(obj) for obj in + itertools.chain(db.session.new, db.session.dirty) if isinstance(obj, model_class)] for h_obj in history_objects: db.session.add(h_obj) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 5bd2e975a..eff4679dd 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -16,6 +16,7 @@ from app.models import ( VerifyCode, ApiKey, Template, + TemplateHistory, Job, NotificationHistory, Notification, @@ -122,7 +123,7 @@ def delete_service_and_all_associated_db_objects(service): _delete_commit(Notification.query.filter_by(service=service)) _delete_commit(Job.query.filter_by(service=service)) _delete_commit(Template.query.filter_by(service=service)) - _delete_commit(Template.get_history_model().query.filter_by(service_id=service.id)) + _delete_commit(TemplateHistory.query.filter_by(service_id=service.id)) verify_codes = VerifyCode.query.join(User).filter(User.id.in_([x.id for x in service.users])) list(map(db.session.delete, verify_codes)) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index d69b530e0..f7f4c5e4d 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -1,8 +1,9 @@ import uuid -from app import db -from app.models import (Template, Service) + from sqlalchemy import (asc, desc) +from app import db +from app.models import (Template, TemplateHistory) from app.dao.dao_utils import ( transactional, version_class @@ -10,7 +11,7 @@ from app.dao.dao_utils import ( @transactional -@version_class(Template) +@version_class(Template, TemplateHistory) def dao_create_template(template): template.id = uuid.uuid4() # must be set now so version history model can use same id template.archived = False @@ -18,14 +19,14 @@ def dao_create_template(template): @transactional -@version_class(Template) +@version_class(Template, TemplateHistory) def dao_update_template(template): db.session.add(template) def dao_get_template_by_id_and_service_id(template_id, service_id, version=None): if version is not None: - return Template.get_history_model().query.filter_by( + return TemplateHistory.query.filter_by( id=template_id, service_id=service_id, version=version).one() @@ -34,7 +35,7 @@ def dao_get_template_by_id_and_service_id(template_id, service_id, version=None) def dao_get_template_by_id(template_id, version=None): if version is not None: - return Template.get_history_model().query.filter_by( + return TemplateHistory.query.filter_by( id=template_id, version=version).one() return Template.query.filter_by(id=template_id).one() @@ -50,6 +51,8 @@ def dao_get_all_templates_for_service(service_id): def dao_get_template_versions(service_id, template_id): - history_model = Template.get_history_model() - return history_model.query.filter_by(service_id=service_id, id=template_id).order_by( - desc(history_model.version)).all() + return TemplateHistory.query.filter_by( + service_id=service_id, id=template_id + ).order_by( + desc(TemplateHistory.version) + ).all() diff --git a/app/history_meta.py b/app/history_meta.py index 9cbbe07f1..49a420572 100644 --- a/app/history_meta.py +++ b/app/history_meta.py @@ -171,17 +171,13 @@ class Versioned(object): return history_mapper.class_ -def versioned_objects(iter): - for obj in iter: - if hasattr(obj, '__history_mapper__'): - yield obj +def create_history(obj, history_cls=None): + if not history_cls: + history_mapper = obj.__history_mapper__ + history_cls = history_mapper.class_ - -def create_history(obj): - obj_mapper = object_mapper(obj) - history_mapper = obj.__history_mapper__ - history_cls = history_mapper.class_ history = history_cls() + obj_mapper = object_mapper(obj) obj_state = attributes.instance_state(obj) data = {} diff --git a/app/models.py b/app/models.py index e3aeb3d13..09fcaf05a 100644 --- a/app/models.py +++ b/app/models.py @@ -206,7 +206,7 @@ TEMPLATE_TYPES = [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE] template_types = db.Enum(*TEMPLATE_TYPES, name='template_type') -class Template(db.Model, Versioned): +class Template(db.Model): __tablename__ = 'templates' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) @@ -231,6 +231,26 @@ class Template(db.Model, Versioned): 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=1) + + +class TemplateHistory(db.Model): + __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) + MMG_PROVIDER = "mmg" FIRETEXT_PROVIDER = "firetext" diff --git a/app/service/rest.py b/app/service/rest.py index bf84fb859..776ab6400 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -189,7 +189,7 @@ def get_service_history(service_id): api_key_history = ApiKey.get_history_model().query.filter_by(service_id=service_id).all() api_keys_data = api_key_history_schema.dump(api_key_history, many=True).data - template_history = Template.get_history_model().query.filter_by(service_id=service_id).all() + template_history = TemplateHistory.query.filter_by(service_id=service_id).all() template_data, errors = template_history_schema.dump(template_history, many=True) events = Event.query.all() diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 1af238b4d..26494dda9 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -331,7 +331,7 @@ def test_delete_service_and_associated_objects(notify_db, assert ApiKey.query.count() == 0 assert ApiKey.get_history_model().query.count() == 0 assert Template.query.count() == 0 - assert Template.get_history_model().query.count() == 0 + assert TemplateHistory.query.count() == 0 assert Job.query.count() == 0 assert Notification.query.count() == 0 assert Permission.query.count() == 0 diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 82b943963..b2a29e1b5 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -185,7 +185,7 @@ def test_get_template_by_id_and_service_returns_none_if_no_template(sample_servi def test_create_template_creates_a_history_record_with_current_data(sample_service, sample_user): assert Template.query.count() == 0 - assert Template.get_history_model().query.count() == 0 + assert TemplateHistory.query.count() == 0 data = { 'name': 'Sample Template', 'template_type': "email", @@ -200,7 +200,7 @@ def test_create_template_creates_a_history_record_with_current_data(sample_servi assert Template.query.count() == 1 template_from_db = Template.query.first() - template_history = Template.get_history_model().query.first() + template_history = TemplateHistory.query.first() assert template_from_db.id == template_history.id assert template_from_db.name == template_history.name @@ -212,7 +212,7 @@ def test_create_template_creates_a_history_record_with_current_data(sample_servi def test_update_template_creates_a_history_record_with_current_data(sample_service, sample_user): assert Template.query.count() == 0 - assert Template.get_history_model().query.count() == 0 + assert TemplateHistory.query.count() == 0 data = { 'name': 'Sample Template', 'template_type': "email", @@ -228,20 +228,20 @@ def test_update_template_creates_a_history_record_with_current_data(sample_servi assert created.name == 'Sample Template' assert Template.query.count() == 1 assert Template.query.first().version == 1 - assert Template.get_history_model().query.count() == 1 + assert TemplateHistory.query.count() == 1 created.name = 'new name' dao_update_template(created) assert Template.query.count() == 1 - assert Template.get_history_model().query.count() == 2 + assert TemplateHistory.query.count() == 2 template_from_db = Template.query.first() assert template_from_db.version == 2 - assert Template.get_history_model().query.filter_by(name='Sample Template').one().version == 1 - assert Template.get_history_model().query.filter_by(name='new name').one().version == 2 + assert TemplateHistory.query.filter_by(name='Sample Template').one().version == 1 + assert TemplateHistory.query.filter_by(name='new name').one().version == 2 def test_get_template_history_version(sample_user, sample_service, sample_template): From c820938ceda49d389254b10773b6dada441fa354 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 8 Aug 2016 16:57:39 +0100 Subject: [PATCH 2/6] fix schema and primary key * version is an additional primary key so we need to indicate that * schema no longer relies on Template model, and uses nested user --- app/models.py | 2 +- app/schemas.py | 12 +++--------- app/service/rest.py | 2 +- tests/app/dao/test_services_dao.py | 1 + tests/app/dao/test_templates_dao.py | 20 ++++++++++++-------- 5 files changed, 18 insertions(+), 19 deletions(-) diff --git a/app/models.py b/app/models.py index 09fcaf05a..cc68e1843 100644 --- a/app/models.py +++ b/app/models.py @@ -249,7 +249,7 @@ class TemplateHistory(db.Model): 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) + version = db.Column(db.Integer, primary_key=True) MMG_PROVIDER = "mmg" diff --git a/app/schemas.py b/app/schemas.py index 168faa02c..9c2454e34 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -172,17 +172,11 @@ class TemplateSchema(BaseTemplateSchema): class TemplateHistorySchema(BaseSchema): - class Meta: - # Use the base model class that the history class is created from - model = models.Template - # We have to use a method here because the relationship field on the - # history object is not created. - created_by = fields.Method("populate_created_by", dump_only=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 populate_created_by(self, data): - usr = models.User.query.filter_by(id=data.created_by_id).one() - return {'id': str(usr.id), 'name': usr.name, 'email_address': usr.email_address} + class Meta: + model = models.TemplateHistory class NotificationsStatisticsSchema(BaseSchema): diff --git a/app/service/rest.py b/app/service/rest.py index 776ab6400..6f45b6da8 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -176,7 +176,7 @@ def get_service_provider_aggregate_statistics(service_id): # tables. This is so product owner can pass stories as done @service.route('//history', methods=['GET']) def get_service_history(service_id): - from app.models import (Service, ApiKey, Template, Event) + from app.models import (Service, ApiKey, Template, TemplateHistory, Event) from app.schemas import ( service_history_schema, api_key_history_schema, diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 26494dda9..654c3b686 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -29,6 +29,7 @@ from app.models import ( VerifyCode, ApiKey, Template, + TemplateHistory, Job, Notification, NotificationHistory, diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index b2a29e1b5..a01126445 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -6,7 +6,7 @@ from app.dao.templates_dao import ( dao_update_template, dao_get_template_versions) from tests.app.conftest import sample_template as create_sample_template -from app.models import Template +from app.models import Template, TemplateHistory import pytest @@ -261,12 +261,16 @@ def test_get_template_versions(sample_template): sample_template.content = 'new version' dao_update_template(sample_template) versions = dao_get_template_versions(service_id=sample_template.service_id, template_id=sample_template.id) - assert versions.__len__() == 2 - for x in versions: - if x.version == 2: - assert x.content == 'new version' - else: - assert x.content == original_content + assert len(versions) == 2 + versions = sorted(versions, key=lambda x: x.version) + assert versions[0].content == original_content + assert versions[1].content == 'new version' + + assert versions[0].created_at == versions[1].created_at + + assert versions[0].updated_at is None + assert versions[1].updated_at is not None + from app.schemas import template_history_schema v = template_history_schema.load(versions, many=True) - assert v.__len__() == 2 + assert len(v) == 2 From 46c0728b12dd65a0617712d317f09c11e723d809 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Aug 2016 13:07:48 +0100 Subject: [PATCH 3/6] add actual_template relationship to notification also renamed the function to make it apparent that it'll join and grab personalisation --- app/dao/notifications_dao.py | 12 +++++++++--- app/models.py | 11 ++++++++++- app/notifications/rest.py | 11 +++++++---- tests/app/celery/test_provider_tasks.py | 6 +++--- tests/app/dao/test_notification_dao.py | 10 ++++++---- tests/app/notifications/test_rest.py | 15 ++++++++------- 6 files changed, 43 insertions(+), 22 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d25adac3a..b70d1fad7 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -8,6 +8,7 @@ from datetime import ( from flask import current_app from werkzeug.datastructures import MultiDict from sqlalchemy import (desc, func, Integer, or_, and_, asc) +from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import cast from notifications_utils.template import get_sms_fragment_count @@ -351,12 +352,12 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page @statsd(namespace="dao") -def get_notification(service_id, notification_id, key_type=None): +def get_notification_with_personalisation(service_id, notification_id, key_type): filter_dict = {'service_id': service_id, 'id': notification_id} if key_type: filter_dict['key_type'] = key_type - return Notification.query.filter_by(**filter_dict).one() + return Notification.query.filter_by(**filter_dict).options(joinedload('actual_template')).one() @statsd(namespace="dao") @@ -374,7 +375,8 @@ def get_notifications_for_service(service_id, page=1, page_size=None, limit_days=None, - key_type=None): + key_type=None, + personalisation=False): if page_size is None: page_size = current_app.config['PAGE_SIZE'] filters = [Notification.service_id == service_id] @@ -388,6 +390,10 @@ def get_notifications_for_service(service_id, query = Notification.query.filter(*filters) query = _filter_query(query, filter_dict) + if personalisation: + query.options( + joinedload('actual_template') + ) return query.order_by(desc(Notification.created_at)).paginate( page=page, per_page=page_size diff --git a/app/models.py b/app/models.py index cc68e1843..3d9e12cfa 100644 --- a/app/models.py +++ b/app/models.py @@ -5,7 +5,8 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint, text +from sqlalchemy import UniqueConstraint, text, ForeignKeyConstraint, and_ +from sqlalchemy.orm import foreign, remote from app.encryption import ( hashpw, @@ -447,6 +448,14 @@ class Notification(db.Model): reference = db.Column(db.String, nullable=True, index=True) _personalisation = db.Column(db.String, nullable=True) + # __table_args__ = ( + # ForeignKeyConstraint(['template_id', 'template_version'], ['template_history.id', 'template_history.version']), + # ) + actual_template = db.relationship('TemplateHistory', primaryjoin=and_( + foreign(template_id) == remote(TemplateHistory.id), + foreign(template_version) == remote(TemplateHistory.version) + )) + @property def personalisation(self): if self._personalisation: diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 5461c198e..b54831105 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,5 +1,6 @@ from datetime import datetime import itertools + from flask import ( Blueprint, jsonify, @@ -7,6 +8,7 @@ from flask import ( current_app, json ) + from notifications_utils.recipients import allowed_to_send_to, first_column_heading from notifications_utils.template import Template from notifications_utils.renderers import PassThrough @@ -170,10 +172,10 @@ def process_firetext_response(): @notifications.route('/notifications/', methods=['GET']) -def get_notifications(notification_id): - notification = notifications_dao.get_notification(str(api_user.service_id), - notification_id, - key_type=api_user.key_type) +def get_notification_by_id(notification_id): + notification = notifications_dao.get_notification_with_personalisation(str(api_user.service_id), + notification_id, + key_type=api_user.key_type) return jsonify(data={"notification": notification_with_personalisation_schema.dump(notification).data}), 200 @@ -186,6 +188,7 @@ def get_all_notifications(): pagination = notifications_dao.get_notifications_for_service( str(api_user.service_id), + personalisation=True, filter_dict=data, page=page, page_size=page_size, diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 648c4a33d..9ef5681b4 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -188,7 +188,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( sender=None ) - persisted_notification = notifications_dao.get_notification(sample_template.service_id, db_notification.id) + persisted_notification = notifications_dao.get_notification_by_id(db_notification.id) assert persisted_notification.to == db_notification.to assert persisted_notification.template_id == sample_template.id assert persisted_notification.template_version == version_on_notification @@ -223,7 +223,7 @@ def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_s ('mmg', str(sample_notification.id), sample_notification.to), queue='research-mode' ) - persisted_notification = notifications_dao.get_notification(sample_service.id, sample_notification.id) + persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) assert persisted_notification.to == sample_notification.to assert persisted_notification.template_id == sample_notification.template_id assert persisted_notification.status == 'sending' @@ -499,7 +499,7 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic sample_notification.id ) - persisted_notification = notifications_dao.get_notification(sample_service.id, sample_notification.id) + persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) assert persisted_notification.billable_units == 0 diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 40c5228a2..cbed327cb 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -23,7 +23,7 @@ from app.models import ( from app.dao.notifications_dao import ( dao_create_notification, dao_update_notification, - get_notification, + get_notification_with_personalisation, get_notification_for_job, get_notifications_for_job, dao_get_notification_statistics_for_service, @@ -57,7 +57,7 @@ def test_should_have_decorated_notifications_dao_functions(): assert update_notification_status_by_reference.__wrapped__.__name__ == 'update_notification_status_by_reference' # noqa assert get_notification_for_job.__wrapped__.__name__ == 'get_notification_for_job' # noqa assert get_notifications_for_job.__wrapped__.__name__ == 'get_notifications_for_job' # noqa - assert get_notification.__wrapped__.__name__ == 'get_notification' # noqa + assert get_notification_with_personalisation.__wrapped__.__name__ == 'get_notification_with_personalisation' # noqa assert get_notifications_for_service.__wrapped__.__name__ == 'get_notifications_for_service' # noqa assert get_notification_by_id.__wrapped__.__name__ == 'get_notification_by_id' # noqa assert delete_notifications_created_more_than_a_week_ago.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago' # noqa @@ -637,9 +637,11 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): def test_get_notification(sample_notification): - notification_from_db = get_notification( + notification_from_db = get_notification_with_personalisation( sample_notification.service.id, - sample_notification.id) + sample_notification.id, + key_type=None + ) assert sample_notification == notification_from_db diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 49813da1b..c5774c6a9 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -640,14 +640,15 @@ def test_get_notification_selects_correct_template_for_personalisation(notify_ap auth_header = create_authorization_header(service_id=sample_template.service_id) response = client.get(path='/notifications', headers=[auth_header]) - assert response.status_code == 200 - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == 2 - assert resp['notifications'][0]['template_version'] == 1 - assert resp['notifications'][0]['body'] == 'This is a template' - assert resp['notifications'][1]['template_version'] == 2 - assert resp['notifications'][1]['body'] == 'foo' + assert response.status_code == 200 + + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == 2 + assert resp['notifications'][0]['template_version'] == 1 + assert resp['notifications'][0]['body'] == 'This is a template' + assert resp['notifications'][1]['template_version'] == 2 + assert resp['notifications'][1]['body'] == 'foo' def _create_auth_header_from_key(api_key): From 4ba3745159e85e615caca40781b0c2c1e64807e6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Aug 2016 16:53:09 +0100 Subject: [PATCH 4/6] update schema to use template_history for accurate template details only in the public notification endpoint so far for fear of breaking things - in an ideal world i'd remove the template relationship from models entirely and replace that with actual_template --- app/dao/notifications_dao.py | 2 +- app/models.py | 3 --- app/schemas.py | 12 +++++++++- tests/app/notifications/test_rest.py | 34 +++++++++++++++++++--------- 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index b70d1fad7..1e85d6cab 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -391,7 +391,7 @@ def get_notifications_for_service(service_id, query = Notification.query.filter(*filters) query = _filter_query(query, filter_dict) if personalisation: - query.options( + query = query.options( joinedload('actual_template') ) return query.order_by(desc(Notification.created_at)).paginate( diff --git a/app/models.py b/app/models.py index 3d9e12cfa..ae52827c4 100644 --- a/app/models.py +++ b/app/models.py @@ -448,9 +448,6 @@ class Notification(db.Model): reference = db.Column(db.String, nullable=True, index=True) _personalisation = db.Column(db.String, nullable=True) - # __table_args__ = ( - # ForeignKeyConstraint(['template_id', 'template_version'], ['template_history.id', 'template_history.version']), - # ) actual_template = db.relationship('TemplateHistory', primaryjoin=and_( foreign(template_id) == remote(TemplateHistory.id), foreign(template_version) == remote(TemplateHistory.version) diff --git a/app/schemas.py b/app/schemas.py index 9c2454e34..6de8c5ed4 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -282,12 +282,21 @@ class NotificationWithTemplateSchema(BaseSchema): strict = True exclude = ('_personalisation',) - template = fields.Nested(TemplateSchema, only=["id", "name", "template_type", "content", "subject"], dump_only=True) + template = fields.Nested( + TemplateSchema, + only=['id', 'version', 'name', 'template_type', 'content', 'subject'], + dump_only=True + ) job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True) personalisation = fields.Dict(required=False) class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): + template = None + actual_template = fields.Nested(TemplateHistorySchema, + only=['id', 'name', 'template_type', 'content', 'subject', 'version'], + dump_only=True) + @pre_dump def handle_personalisation_property(self, in_data): self.personalisation = in_data.personalisation @@ -295,6 +304,7 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): @post_dump def handle_template_merge(self, in_data): + in_data['template'] = in_data.pop('actual_template') from notifications_utils.template import Template template = Template( in_data['template'], diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index c5774c6a9..df9277724 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -8,6 +8,7 @@ from freezegun import freeze_time from app.dao.notifications_dao import dao_update_notification from app.dao.api_key_dao import save_model_api_key +from app.dao.templates_dao import dao_update_template from app.models import ApiKey, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST from tests import create_authorization_header from tests.app.conftest import sample_notification as create_sample_notification @@ -29,7 +30,9 @@ def test_get_sms_notification_by_id(notify_api, sample_notification): assert notification['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, - 'template_type': sample_notification.template.template_type} + 'template_type': sample_notification.template.template_type, + 'version': 1 + } assert notification['job'] == { 'id': str(sample_notification.job.id), 'original_file_name': sample_notification.job.original_file_name @@ -62,7 +65,9 @@ def test_get_email_notification_by_id(notify_api, notify_db, notify_db_session, assert notification['template'] == { 'id': str(email_notification.template.id), 'name': email_notification.template.name, - 'template_type': email_notification.template.template_type} + 'template_type': email_notification.template.template_type, + 'version': 1 + } assert notification['job'] == { 'id': str(email_notification.job.id), 'original_file_name': email_notification.job.original_file_name @@ -166,7 +171,9 @@ def test_get_all_notifications(notify_api, sample_notification): assert notifications['notifications'][0]['template'] == { 'id': str(sample_notification.template.id), 'name': sample_notification.template.name, - 'template_type': sample_notification.template.template_type} + 'template_type': sample_notification.template.template_type, + 'version': 1 + } assert notifications['notifications'][0]['job'] == { 'id': str(sample_notification.job.id), 'original_file_name': sample_notification.job.original_file_name @@ -626,8 +633,9 @@ def test_get_notification_selects_correct_template_for_personalisation(notify_ap notify_db_session, service=sample_template.service, template=sample_template) - + original_content = sample_template.content sample_template.content = '((name))' + dao_update_template(sample_template) notify_db.session.commit() create_sample_notification(notify_db, @@ -641,14 +649,18 @@ def test_get_notification_selects_correct_template_for_personalisation(notify_ap response = client.get(path='/notifications', headers=[auth_header]) - assert response.status_code == 200 + assert response.status_code == 200 - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == 2 - assert resp['notifications'][0]['template_version'] == 1 - assert resp['notifications'][0]['body'] == 'This is a template' - assert resp['notifications'][1]['template_version'] == 2 - assert resp['notifications'][1]['body'] == 'foo' + resp = json.loads(response.get_data(as_text=True)) + notis = sorted(resp['notifications'], key=lambda x: x['template_version']) + assert len(notis) == 2 + assert notis[0]['template_version'] == 1 + assert notis[0]['body'] == original_content + assert notis[1]['template_version'] == 2 + assert notis[1]['body'] == 'foo' + + assert notis[0]['template_version'] == notis[0]['template']['version'] + assert notis[1]['template_version'] == notis[1]['template']['version'] def _create_auth_header_from_key(api_key): From 648b4a17c8761a083ff73f5eff684fda950c9cca Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Aug 2016 16:54:43 +0100 Subject: [PATCH 5/6] remove xfail flag - the test passes properly now! :tada: --- tests/app/notifications/test_rest.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index df9277724..3386f1d84 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -623,7 +623,6 @@ def test_get_notifications_for_service_returns_merged_template_content(notify_ap } -@pytest.mark.xfail(strict=True, raises=NeededByTemplateError) def test_get_notification_selects_correct_template_for_personalisation(notify_api, notify_db, notify_db_session, From 5b3a0f03d342dc3d6811789b8b6829ab996032c4 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 10 Aug 2016 16:57:27 +0100 Subject: [PATCH 6/6] rename actual_template to template_history it's slightly less emotionally charged --- app/dao/notifications_dao.py | 4 ++-- app/models.py | 2 +- app/schemas.py | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1e85d6cab..d944a3202 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -357,7 +357,7 @@ def get_notification_with_personalisation(service_id, notification_id, key_type) if key_type: filter_dict['key_type'] = key_type - return Notification.query.filter_by(**filter_dict).options(joinedload('actual_template')).one() + return Notification.query.filter_by(**filter_dict).options(joinedload('template_history')).one() @statsd(namespace="dao") @@ -392,7 +392,7 @@ def get_notifications_for_service(service_id, query = _filter_query(query, filter_dict) if personalisation: query = query.options( - joinedload('actual_template') + joinedload('template_history') ) return query.order_by(desc(Notification.created_at)).paginate( page=page, diff --git a/app/models.py b/app/models.py index ae52827c4..d391022c7 100644 --- a/app/models.py +++ b/app/models.py @@ -448,7 +448,7 @@ class Notification(db.Model): reference = db.Column(db.String, nullable=True, index=True) _personalisation = db.Column(db.String, nullable=True) - actual_template = db.relationship('TemplateHistory', primaryjoin=and_( + template_history = db.relationship('TemplateHistory', primaryjoin=and_( foreign(template_id) == remote(TemplateHistory.id), foreign(template_version) == remote(TemplateHistory.version) )) diff --git a/app/schemas.py b/app/schemas.py index 7cad5de42..977c378fc 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -292,9 +292,9 @@ class NotificationWithTemplateSchema(BaseSchema): class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): - actual_template = fields.Nested(TemplateHistorySchema, - only=['id', 'name', 'template_type', 'content', 'subject', 'version'], - dump_only=True) + template_history = fields.Nested(TemplateHistorySchema, + only=['id', 'name', 'template_type', 'content', 'subject', 'version'], + dump_only=True) class Meta(NotificationWithTemplateSchema.Meta): # mark as many fields as possible as required since this is a public api. @@ -307,7 +307,7 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): # computed fields 'personalisation', # relationships - 'service', 'job', 'api_key', 'actual_template' + 'service', 'job', 'api_key', 'template_history' ) @pre_dump @@ -317,7 +317,7 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): @post_dump def handle_template_merge(self, in_data): - in_data['template'] = in_data.pop('actual_template') + in_data['template'] = in_data.pop('template_history') from notifications_utils.template import Template template = Template( in_data['template'],