From 52e0367a13afd2e3e84b2aebdc88e864123c29fa Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 8 Nov 2017 10:32:45 +0000 Subject: [PATCH 01/10] Replace Notifications.template_history with Notifications.template Each notification instance is connected to a specific version of the template that was used to generate notification content. Template versions are stored in TemplateHistory, so that's the table we need to use for Notifications.template relationship. Currently, using Notifications.template returns the latest template version, which leads to incorrect content being returned by the API if the template has been updated. This change removes Notifications.template_history attribute and replaces template relationship to refer to TemplateHistory instance based on the notification's template ID and version. --- app/models.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/app/models.py b/app/models.py index 409fe3018..9d16c4968 100644 --- a/app/models.py +++ b/app/models.py @@ -909,9 +909,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) @@ -947,11 +947,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) @@ -963,6 +958,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: From a5ca66a01b38b7c4b517a23f0bf9f29c78f5a168 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Thu, 9 Nov 2017 14:25:47 +0000 Subject: [PATCH 02/10] Update notification queries with new TemplateHistory join Changes the queries to use `Notification.template` relationship instead of `.template_history` and TemplateHistory model insteaad of Template. --- app/dao/notifications_dao.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 From e73513f51994b97db1b390ef10c663217834d26b Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Thu, 9 Nov 2017 14:27:24 +0000 Subject: [PATCH 03/10] Update Notification schema to preserve template_history attribute `Notification.template_history` relationship has been removed but we want to keep the `template_history` key in existing notification serializations, so we serialize it from `Notifications.template`. This keeps the data format the same, but both `template` and `template_history` keys will now contain data from the `TemplateHistory` instance. --- app/schemas.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index e689bf83f..13bf10120 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -450,7 +450,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) From 94dae429028c470d7587cf6fb58934184f892064 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Thu, 9 Nov 2017 14:31:31 +0000 Subject: [PATCH 04/10] Avoid assigning notification.template when creating test objects `Notification.template` changed from being a Template relationship to TemplateHistory. When a relationship attribute is being assigned, SQLAlchemy checks that the assigned value type matches the relationship type. Since most tests at the moment create a notification using a Template instance this check fails. Rewriting the tests to use TemplateHistory objects would require changes to the majority of tests. Instead, when creating a notification objects we assign the foreign key attributes directly. This skips the SQLAlchemy type check, but we still get the constraint check on the foreign keys, so a matching TemplateHistory object needs to exist in the database. --- tests/app/conftest.py | 5 ++--- .../dao/notification_dao/test_notification_dao.py | 13 ++++++------- tests/app/db.py | 3 +-- .../test_process_letter_notifications.py | 3 ++- tests/app/test_model.py | 2 +- 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 43b17490d..356c951e5 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -544,8 +544,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 +623,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, 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/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/test_model.py b/tests/app/test_model.py index 7062e7a30..09f1325f8 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -202,8 +202,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' From c62f2a3f7cb88cc0587cb7d9526c2cc7e4ee0cc5 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Thu, 9 Nov 2017 14:42:43 +0000 Subject: [PATCH 05/10] Update create_custom_template test helper to create history object `create_custom_template` is not using `dao_create_template` since it explicitly sets a template id. Notifications.template relationship now refers to a TemplateHistory objects, so we need to make sure that `create_custom_template` creates a matching TemplateHistory record when it creates a Template. --- tests/app/celery/test_scheduled_tasks.py | 16 +++++++--------- tests/app/conftest.py | 3 +++ 2 files changed, 10 insertions(+), 9 deletions(-) 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 356c951e5..f2c2cd41f 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, @@ -974,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 From 79d417f13b38fbaaed8be03e9bbe4ebb7e0e2680 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Thu, 9 Nov 2017 14:48:27 +0000 Subject: [PATCH 06/10] Add TemplateHistory.get_link method Notifications.serialize calls `Notifications.template.get_link`, so we need TemplateHistory objects to generate their API URLs. This generates a link to the `/v2/` version of the endpoint. --- app/models.py | 8 ++++++++ tests/app/v2/notifications/test_get_notifications.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 9d16c4968..f209dbb13 100644 --- a/app/models.py +++ b/app/models.py @@ -645,6 +645,14 @@ class TemplateHistory(db.Model): nullable=False, default=NORMAL) + 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) 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" From bdcc89b4038e770e4d9e9b7a59734305f6b4eb5d Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Thu, 9 Nov 2017 14:50:18 +0000 Subject: [PATCH 07/10] Add TemplateHistory.redact_personalisation attribute TemplateHistory objects need to be connected to the template's TemplateRedacted record. This requires setting up a new SQLAlchemy relationship to allow accessing redact_personalisation from TemplateHistory instances. We can avoid creating a foreign key in this case by setting up an explicit `primaryjoin` expression. Since TemplateHistory.id is created from Template.id and TemplateRedacted.template_id is already a foreign key to Template.id the foreign key should always be valid even without a DB constraint. --- app/models.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/models.py b/app/models.py index f209dbb13..7b7e012aa 100644 --- a/app/models.py +++ b/app/models.py @@ -645,6 +645,11 @@ 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", From db1c6478738112980726eae4bdb3d643db51138e Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Thu, 9 Nov 2017 16:04:43 +0000 Subject: [PATCH 08/10] Remove NotificationHistory.template relationship Relationship attribute is not used by the application code, so we can remove it without replacing it with a TemplateHistory one. This also updates the foreign key constraint to refer to the composite primary key for the TemplateHistory records. --- app/models.py | 11 +++++++++-- tests/app/conftest.py | 2 +- tests/app/dao/test_provider_statistics_dao.py | 2 +- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/app/models.py b/app/models.py index 7b7e012aa..3032a10ad 100644 --- a/app/models.py +++ b/app/models.py @@ -1181,8 +1181,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') @@ -1212,6 +1211,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/tests/app/conftest.py b/tests/app/conftest.py index f2c2cd41f..cecfb5a27 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -679,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, 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(), From 0825177f2d3c911cc6e94d2877203fa5e348a353 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 10 Nov 2017 11:33:42 +0000 Subject: [PATCH 09/10] Add tests for notification.template returning the right version Adds test for: * checking the template version foreign key constraint * checking that template changes don't affect existing notifications * notification statistics aren't affected by different template versions * notification stats always return the current template name --- tests/app/service/test_rest.py | 18 +++++++++++++ tests/app/template_statistics/test_rest.py | 30 ++++++++++++++-------- tests/app/test_model.py | 19 ++++++++++++++ 3 files changed, 57 insertions(+), 10 deletions(-) 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 09f1325f8..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 ( @@ -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) From 9d4b8961cb1129cf02dffdbe987babe0942a4f9b Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 8 Nov 2017 10:36:11 +0000 Subject: [PATCH 10/10] Add a migration to replace notifications_template foreign key Removes notifications.template_id foreign key and replaces it with a composite foreign key constraint to TemplateHistory using the existing notification columns for template ID and template version. Foreign key constraint is created as NOT VALID to avoid locking the notifications table while postgres verifies that existing records don't break the constraint. From postgres docs: > If the constraint is marked NOT VALID, the potentially-lengthy initial > check to verify that all rows in the table satisfy the constraint is > skipped. The constraint will still be enforced against subsequent > inserts or updates (that is, they'll fail unless there is a matching > row in the referenced table, in the case of foreign keys; and they'll > fail unless the new row matches the specified check constraints). But > the database will not assume that the constraint holds for all rows > in the table, until it is validated by using the VALIDATE CONSTRAINT > option. VALIDATE CONSTRAINT will be issued as a separate migration in a follow-up PR. --- .../0136_notification_template_hist.py | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 migrations/versions/0136_notification_template_hist.py 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'])