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/notifications_dao.py b/app/dao/notifications_dao.py index 16301e64f..86d5b986c 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 app import db @@ -376,12 +377,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('template_history')).one() @statsd(namespace="dao") @@ -399,7 +400,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] @@ -413,6 +415,10 @@ def get_notifications_for_service(service_id, query = Notification.query.filter(*filters) query = _filter_query(query, filter_dict) + if personalisation: + query = query.options( + joinedload('template_history') + ) return query.order_by(desc(Notification.created_at)).paginate( page=page, per_page=page_size 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 2cb9b709e..efe5bb7b7 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, @@ -206,7 +207,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 +232,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, primary_key=True) + MMG_PROVIDER = "mmg" FIRETEXT_PROVIDER = "firetext" @@ -427,6 +448,11 @@ class Notification(db.Model): reference = db.Column(db.String, nullable=True, index=True) _personalisation = db.Column(db.String, nullable=True) + template_history = 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 61bd66574..8e4af63b2 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/app/schemas.py b/app/schemas.py index a858a6cf2..a28c4aa94 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -173,17 +173,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): @@ -289,12 +283,20 @@ 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_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. # WARNING: Does _not_ reference fields computed in handle_template_merge, such as @@ -306,7 +308,7 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): # computed fields 'personalisation', # relationships - 'service', 'job', 'api_key', 'template' + 'service', 'job', 'api_key', 'template_history' ) @pre_dump @@ -316,6 +318,7 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): @post_dump def handle_template_merge(self, in_data): + in_data['template'] = in_data.pop('template_history') from notifications_utils.template import Template template = Template( in_data['template'], diff --git a/app/service/rest.py b/app/service/rest.py index bf84fb859..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, @@ -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/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 11d0839cd..5baca8f0d 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -197,7 +197,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 @@ -232,7 +232,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' @@ -508,7 +508,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 b75ec5ccf..7a0aa8692 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, @@ -58,7 +58,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 @@ -769,9 +769,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/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 1af238b4d..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, @@ -331,7 +332,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..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 @@ -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): @@ -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 diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index cbbaf5ca2..df611adca 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 @@ -656,7 +663,6 @@ def test_get_notification_public_api_format_is_not_changed(notify_api, sample_no } -@pytest.mark.xfail(strict=True, raises=NeededByTemplateError) def test_get_notification_selects_correct_template_for_personalisation(notify_api, notify_db, notify_db_session, @@ -666,8 +672,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, @@ -680,14 +687,19 @@ 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' + 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):