diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index c28b2a686..2ea240f5b 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -29,6 +29,7 @@ from app.models import ( ScheduledNotification, ServiceEmailReplyTo, Template, + TemplateHistory, EMAIL_TYPE, SMS_TYPE, KEY_TYPE_NORMAL, @@ -225,7 +226,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('template_history')).one() + return Notification.query.filter_by(**filter_dict).options(joinedload('template')).one() @statsd(namespace="dao") @@ -281,7 +282,7 @@ def get_notifications_for_service( query = _filter_query(query, filter_dict) if personalisation: query = query.options( - joinedload('template_history') + joinedload('template') ) return query.order_by(desc(Notification.created_at)).paginate( @@ -305,7 +306,7 @@ def _filter_query(query, filter_dict=None): # filter by template template_types = multidict.getlist('template_type') if template_types: - query = query.join(Template).filter(Template.template_type.in_(template_types)) + query = query.join(TemplateHistory).filter(TemplateHistory.template_type.in_(template_types)) return query diff --git a/app/models.py b/app/models.py index a722e301a..a62de1562 100644 --- a/app/models.py +++ b/app/models.py @@ -648,6 +648,19 @@ class TemplateHistory(db.Model): 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') + + def get_link(self): + return url_for( + "v2_template.get_template_by_id", + template_id=self.id, + version=self.version, + _external=True + ) + def _as_utils_template(self): return Template._as_utils_template(self) @@ -912,9 +925,9 @@ class Notification(db.Model): job_row_number = db.Column(db.Integer, nullable=True) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False) service = db.relationship('Service') - template_id = db.Column(UUID(as_uuid=True), db.ForeignKey('templates.id'), index=True, unique=False) - template = db.relationship('Template') + template_id = db.Column(UUID(as_uuid=True), index=True, unique=False) template_version = db.Column(db.Integer, nullable=False) + template = db.relationship('TemplateHistory') api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), index=True, unique=False) api_key = db.relationship('ApiKey') key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False) @@ -950,11 +963,6 @@ class Notification(db.Model): client_reference = db.Column(db.String, index=True, nullable=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) - )) - scheduled_notification = db.relationship('ScheduledNotification', uselist=False) client_reference = db.Column(db.String, index=True, nullable=True) @@ -966,6 +974,14 @@ class Notification(db.Model): created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), nullable=True) + __table_args__ = ( + db.ForeignKeyConstraint( + ['template_id', 'template_version'], + ['templates_history.id', 'templates_history.version'], + ), + {} + ) + @property def personalisation(self): if self._personalisation: @@ -1168,8 +1184,7 @@ class NotificationHistory(db.Model, HistoryModel): job_row_number = db.Column(db.Integer, nullable=True) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False) service = db.relationship('Service') - template_id = db.Column(UUID(as_uuid=True), db.ForeignKey('templates.id'), index=True, unique=False) - template = db.relationship('Template') + template_id = db.Column(UUID(as_uuid=True), index=True, unique=False) template_version = db.Column(db.Integer, nullable=False) api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), index=True, unique=False) api_key = db.relationship('ApiKey') @@ -1199,6 +1214,14 @@ class NotificationHistory(db.Model, HistoryModel): created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), nullable=True) + __table_args__ = ( + db.ForeignKeyConstraint( + ['template_id', 'template_version'], + ['templates_history.id', 'templates_history.version'], + ), + {} + ) + @classmethod def from_original(cls, notification): history = super().from_original(notification) diff --git a/app/schemas.py b/app/schemas.py index fe9d29b0f..28abe8edc 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -471,7 +471,7 @@ class NotificationWithTemplateSchema(BaseSchema): class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): - template_history = fields.Nested(TemplateHistorySchema, + template_history = fields.Nested(TemplateHistorySchema, attribute="template", only=['id', 'name', 'template_type', 'content', 'subject', 'version'], dump_only=True) diff --git a/migrations/versions/0136_notification_template_hist.py b/migrations/versions/0136_notification_template_hist.py new file mode 100644 index 000000000..9af35974f --- /dev/null +++ b/migrations/versions/0136_notification_template_hist.py @@ -0,0 +1,36 @@ +""" + +Revision ID: 0136_notification_template_hist +Revises: 0135_stats_template_usage +Create Date: 2017-11-08 10:15:07.039227 + +""" +from alembic import op + +revision = '0136_notification_template_hist' +down_revision = '0135_stats_template_usage' + + +def upgrade(): + op.drop_constraint('notifications_template_id_fkey', 'notifications', type_='foreignkey') + op.execute(""" + ALTER TABLE notifications ADD CONSTRAINT "notifications_templates_history_fkey" + FOREIGN KEY ("template_id", "template_version") REFERENCES "templates_history" ("id", "version") + NOT VALID + """) + + op.drop_constraint('notification_history_template_id_fkey', 'notification_history', type_='foreignkey') + op.execute(""" + ALTER TABLE notification_history ADD CONSTRAINT "notification_history_templates_history_fkey" + FOREIGN KEY ("template_id", "template_version") REFERENCES "templates_history" ("id", "version") + NOT VALID + """) + + +def downgrade(): + op.drop_constraint('notifications_templates_history_fkey', 'notifications', type_='foreignkey') + op.create_foreign_key('notifications_template_id_fkey', 'notifications', 'templates', ['template_id'], ['id']) + + op.drop_constraint('notification_history_templates_history_fkey', 'notification_history', type_='foreignkey') + op.create_foreign_key('notification_history_template_id_fkey', 'notification_history', 'templates', + ['template_id'], ['id']) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 01e40dd95..6bdce0f6f 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -48,7 +48,6 @@ from app.models import ( NotificationHistory, Service, StatsTemplateUsageByMonth, - Template, JOB_STATUS_READY_TO_SEND, JOB_STATUS_IN_PROGRESS, JOB_STATUS_SENT_TO_DVLA, @@ -81,14 +80,13 @@ def _create_slow_delivery_notification(provider='mmg'): service = create_service( service_id=current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SERVICE_ID') ) - template = Template.query.get(current_app.config['FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID']) - if not template: - template = create_custom_template( - service=service, - user=service.users[0], - template_config_name='FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID', - template_type='sms' - ) + + template = create_custom_template( + service=service, + user=service.users[0], + template_config_name='FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID', + template_type='sms' + ) create_notification( template=template, diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 43b17490d..cecfb5a27 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -14,6 +14,7 @@ from app import db from app.models import ( Service, Template, + TemplateHistory, ApiKey, Job, Notification, @@ -38,6 +39,7 @@ from app.dao.notifications_dao import dao_create_notification from app.dao.invited_user_dao import save_invited_user from app.dao.provider_rates_dao import create_provider_rates from app.clients.sms.firetext import FiretextClient +from app.history_meta import create_history from tests import create_authorization_header from tests.app.db import ( create_user, @@ -544,8 +546,7 @@ def sample_notification( 'job': job, 'service_id': service.id, 'service': service, - 'template_id': template.id if template else None, - 'template': template, + 'template_id': template.id, 'template_version': template.version, 'status': status, 'reference': reference, @@ -624,7 +625,7 @@ def sample_email_notification(notify_db, notify_db_session): 'job': job, 'service_id': service.id, 'service': service, - 'template': template, + 'template_id': template.id, 'template_version': template.version, 'status': 'created', 'reference': None, @@ -678,7 +679,7 @@ def sample_notification_history( notification_history = NotificationHistory( id=uuid.uuid4(), service=sample_template.service, - template=sample_template, + template_id=sample_template.id, template_version=sample_template.version, status=status, created_at=created_at, @@ -975,6 +976,7 @@ def create_custom_template(service, user, template_config_name, template_type, c } template = Template(**data) db.session.add(template) + db.session.add(create_history(template, TemplateHistory)) db.session.commit() return template diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index cec15552d..d662b6e4c 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -615,7 +615,7 @@ def test_save_notification_creates_sms(sample_template, sample_job, mmg_provider assert data['to'] == notification_from_db.to assert data['job_id'] == notification_from_db.job_id assert data['service'] == notification_from_db.service - assert data['template'] == notification_from_db.template + assert data['template_id'] == notification_from_db.template_id assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert notification_from_db.status == 'created' @@ -635,7 +635,7 @@ def test_save_notification_and_create_email(sample_email_template, sample_job): assert data['to'] == notification_from_db.to assert data['job_id'] == notification_from_db.job_id assert data['service'] == notification_from_db.service - assert data['template'] == notification_from_db.template + assert data['template_id'] == notification_from_db.template_id assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert notification_from_db.status == 'created' @@ -761,7 +761,7 @@ def test_save_notification_and_increment_job(sample_template, sample_job, mmg_pr assert data['to'] == notification_from_db.to assert data['job_id'] == notification_from_db.job_id assert data['service'] == notification_from_db.service - assert data['template'] == notification_from_db.template + assert data['template_id'] == notification_from_db.template_id assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert notification_from_db.status == 'created' @@ -788,7 +788,7 @@ def test_save_notification_and_increment_correct_job(notify_db, notify_db_sessio assert data['to'] == notification_from_db.to assert data['job_id'] == notification_from_db.job_id assert data['service'] == notification_from_db.service - assert data['template'] == notification_from_db.template + assert data['template_id'] == notification_from_db.template_id assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert notification_from_db.status == 'created' @@ -807,7 +807,7 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): assert notification_from_db.id assert data['to'] == notification_from_db.to assert data['service'] == notification_from_db.service - assert data['template'] == notification_from_db.template + assert data['template_id'] == notification_from_db.template_id assert data['template_version'] == notification_from_db.template_version assert data['created_at'] == notification_from_db.created_at assert notification_from_db.status == 'created' @@ -852,7 +852,7 @@ def test_save_notification_no_job_id(sample_template, mmg_provider): assert notification_from_db.id assert data['to'] == notification_from_db.to assert data['service'] == notification_from_db.service - assert data['template'] == notification_from_db.template + assert data['template_id'] == notification_from_db.template_id assert data['template_version'] == notification_from_db.template_version assert notification_from_db.status == 'created' assert data.get('job_id') is None @@ -1251,7 +1251,6 @@ def _notification_json(sample_template, job_id=None, id=None, status=None): 'to': '+44709123456', 'service': sample_template.service, 'service_id': sample_template.service.id, - 'template': sample_template, 'template_id': sample_template.id, 'template_version': sample_template.version, 'created_at': datetime.utcnow(), diff --git a/tests/app/dao/test_provider_statistics_dao.py b/tests/app/dao/test_provider_statistics_dao.py index 54e429b0e..f23550b6b 100644 --- a/tests/app/dao/test_provider_statistics_dao.py +++ b/tests/app/dao/test_provider_statistics_dao.py @@ -73,7 +73,7 @@ def noti_hist(notify_db, template, status='delivered', billable_units=None, key_ notification_history = NotificationHistory( id=uuid.uuid4(), service=template.service, - template=template, + template_id=template.id, template_version=template.version, status=status, created_at=datetime.utcnow(), diff --git a/tests/app/db.py b/tests/app/db.py index c7c950544..d4f3a2afe 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -160,8 +160,7 @@ def create_notification( 'job': job, 'service_id': template.service.id, 'service': template.service, - 'template_id': template and template.id, - 'template': template, + 'template_id': template.id, 'template_version': template.version, 'status': status, 'reference': reference, diff --git a/tests/app/notifications/test_process_letter_notifications.py b/tests/app/notifications/test_process_letter_notifications.py index 24f37c06f..a607bf68b 100644 --- a/tests/app/notifications/test_process_letter_notifications.py +++ b/tests/app/notifications/test_process_letter_notifications.py @@ -18,7 +18,8 @@ def test_create_letter_notification_creates_notification(sample_letter_template, assert notification == Notification.query.one() assert notification.job is None assert notification.status == NOTIFICATION_CREATED - assert notification.template == sample_letter_template + assert notification.template_id == sample_letter_template.id + assert notification.template_version == sample_letter_template.version assert notification.api_key == sample_api_key assert notification.notification_type == LETTER_TYPE assert notification.key_type == sample_api_key.key_type diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 59a55da66..bd365b739 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1451,6 +1451,24 @@ def test_get_notification_for_service_includes_created_by(admin_request, sample_ } +def test_get_notification_for_service_returns_old_template_version(admin_request, sample_template): + sample_notification = create_notification(sample_template) + sample_notification.reference = 'modified-inplace' + sample_template.version = 2 + sample_template.content = 'New template content' + + resp = admin_request.get( + 'service.get_notification_for_service', + service_id=sample_notification.service_id, + notification_id=sample_notification.id + ) + + assert resp['reference'] == 'modified-inplace' + assert resp['template']['version'] == 1 + assert resp['template']['content'] == sample_notification.template.content + assert resp['template']['content'] != sample_template.content + + @pytest.mark.parametrize( 'include_from_test_key, expected_count_of_notifications', [ diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index f71bea3af..3aa6f9463 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -4,6 +4,8 @@ import json import pytest from freezegun import freeze_time +from app.dao.templates_dao import dao_update_template + from tests import create_authorization_header from tests.app.conftest import ( sample_template as create_sample_template, @@ -108,9 +110,9 @@ def test_get_template_statistics_for_service_limit_7_days(notify_db, notify_db_s json_resp = json.loads(response_for_a_week.get_data(as_text=True)) assert len(json_resp['data']) == 2 assert json_resp['data'][0]['count'] == 3 - assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][0]['template_name'] == 'New Email Template Name' assert json_resp['data'][1]['count'] == 3 - assert json_resp['data'][1]['template_name'] == 'Template Name' + assert json_resp['data'][1]['template_name'] == 'New SMS Template Name' mocked_redis_get.assert_called_once_with("{}-template-counter-limit-7-days".format(email.service_id)) if cache_values: @@ -138,9 +140,9 @@ def test_get_template_statistics_for_service_limit_30_days(notify_db, notify_db_ json_resp = json.loads(response_for_a_month.get_data(as_text=True)) assert len(json_resp['data']) == 2 assert json_resp['data'][0]['count'] == 3 - assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][0]['template_name'] == 'New Email Template Name' assert json_resp['data'][1]['count'] == 3 - assert json_resp['data'][1]['template_name'] == 'Template Name' + assert json_resp['data'][1]['template_name'] == 'New SMS Template Name' mock_redis.assert_not_called() @@ -159,9 +161,9 @@ def test_get_template_statistics_for_service_no_limit(notify_db, notify_db_sessi json_resp = json.loads(response_for_all.get_data(as_text=True)) assert len(json_resp['data']) == 2 assert json_resp['data'][0]['count'] == 3 - assert json_resp['data'][0]['template_name'] == 'Email Template Name' + assert json_resp['data'][0]['template_name'] == 'New Email Template Name' assert json_resp['data'][1]['count'] == 3 - assert json_resp['data'][1]['template_name'] == 'Template Name' + assert json_resp['data'][1]['template_name'] == 'New SMS Template Name' mock_redis.assert_not_called() @@ -172,12 +174,20 @@ def set_up_notifications(notify_db, notify_db_session): today = datetime.now() a_week_ago = datetime.now() - timedelta(days=7) a_month_ago = datetime.now() - timedelta(days=30) - sample_notification(notify_db, notify_db_session, created_at=today, template=sms) - sample_notification(notify_db, notify_db_session, created_at=today, template=email) - sample_notification(notify_db, notify_db_session, created_at=a_week_ago, template=sms) - sample_notification(notify_db, notify_db_session, created_at=a_week_ago, template=email) sample_notification(notify_db, notify_db_session, created_at=a_month_ago, template=sms) sample_notification(notify_db, notify_db_session, created_at=a_month_ago, template=email) + email.name = 'Updated Email Template Name' + dao_update_template(email) + sms.name = 'Updated SMS Template Name' + dao_update_template(sms) + sample_notification(notify_db, notify_db_session, created_at=a_week_ago, template=sms) + sample_notification(notify_db, notify_db_session, created_at=a_week_ago, template=email) + email.name = 'New Email Template Name' + dao_update_template(email) + sms.name = 'New SMS Template Name' + dao_update_template(sms) + sample_notification(notify_db, notify_db_session, created_at=today, template=sms) + sample_notification(notify_db, notify_db_session, created_at=today, template=email) return email, sms diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 7062e7a30..fe30b3006 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -1,6 +1,7 @@ import pytest from freezegun import freeze_time +from sqlalchemy.exc import IntegrityError from app import encryption from app.models import ( @@ -202,8 +203,8 @@ def test_notification_subject_fills_in_placeholders_for_email(sample_email_templ def test_notification_subject_fills_in_placeholders_for_letter(sample_letter_template): - sample_letter_template.subject = '((name))' noti = create_notification(sample_letter_template, personalisation={'name': 'hello'}) + noti.template.subject = '((name))' assert noti.subject == 'hello' @@ -239,6 +240,24 @@ def test_letter_notification_serializes_with_subject(client, sample_letter_templ assert res['subject'] == 'Template subject' +def test_notification_references_template_history(client, sample_template): + noti = create_notification(sample_template) + sample_template.version = 3 + sample_template.content = 'New template content' + + res = noti.serialize() + assert res['template']['version'] == 1 + + assert res['body'] == noti.template.content + assert noti.template.content != sample_template.content + + +def test_notification_requires_a_valid_template_version(client, sample_template): + sample_template.version = 2 + with pytest.raises(IntegrityError): + create_notification(sample_template) + + def test_inbound_number_serializes_with_service(client, notify_db_session): service = create_service() inbound_number = create_inbound_number(number='1', service_id=service.id) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index d4306ba93..fc6b091c9 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -335,7 +335,7 @@ def test_get_all_notifications_filter_by_template_type(client): assert json_response['notifications'][0]['status'] == "created" assert json_response['notifications'][0]['template'] == { 'id': str(email_template.id), - 'uri': email_template.get_link(), + 'uri': notification.template.get_link(), 'version': 1 } assert json_response['notifications'][0]['email_address'] == "don.draper@scdp.biz"