Merge pull request #1384 from alphagov/notifications-template-history

Fix notifications template history relationship

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
 

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_id = db.Column(UUID(as_uuid=True), index=True, unique=False)
-    template = db.relationship('Template')
     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_id = db.Column(UUID(as_uuid=True), index=True, unique=False)
-    template = db.relationship('Template')
     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)

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)
 

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'])

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(
-        template = create_custom_template(
+        service=service,
-            service=service,
+        user=service.users[0],
-            user=service.users[0],
+        template_config_name='FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID',
-            template_config_name='FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID',
+        template_type='sms'
-            template_type='sms'
+    )
-        )
 
     create_notification(
         template=template,

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_id': template.id,
-        'template': template,
         '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
 

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(),

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(),

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_id': template.id,
-        'template': template,
         'template_version': template.version,
         'status': status,
         'reference': reference,

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

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',
     [

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
 
 

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)

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"