Fix the template history when updating the reply_to for a template.

It seems selecting the service_letter_contact in the validation method was causing SQLAlchemy to persist the object. When the dao was called to save the object nothing was different so we didn't persist the history object.

It may be time to take another look at how we version. :(

app/template/rest.py

@@ -84,11 +84,13 @@ def update_template(service_id, template_id):
     current_data = dict(template_schema.dump(fetched_template).data.items())
     updated_template = dict(template_schema.dump(fetched_template).data.items())
     updated_template.update(data)
+
     # Check if there is a change to make.
     if _template_has_not_changed(current_data, updated_template):
         return jsonify(data=updated_template), 200
 
-    update_dict = template_schema.load(updated_template).data
+    check_reply_to(service_id, updated_template.get('reply_to', None), fetched_template.template_type)
+
     over_limit = _content_count_greater_than_limit(updated_template['content'], fetched_template.template_type)
     if over_limit:
         char_count_limit = current_app.config.get('SMS_CHAR_COUNT_LIMIT')
@@ -96,7 +98,7 @@ def update_template(service_id, template_id):
         errors = {'content': [message]}
         raise InvalidRequest(errors, status_code=400)
 
-    check_reply_to(service_id, update_dict.reply_to, fetched_template.template_type)
+    update_dict = template_schema.load(updated_template).data
 
     dao_update_template(update_dict)
     return jsonify(data=template_schema.dump(update_dict).data), 200

tests/app/celery/test_tasks.py

@@ -1040,6 +1040,55 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session):
     assert notification_db.reply_to_text == contact_block.contact_block
 
 
+def test_save_letter_saves_letter_to_database_right_reply_to(mocker, notify_db_session):
+    service = create_service()
+    create_letter_contact(service=service, contact_block="Address contact", is_default=True)
+    template = create_template(service=service, template_type=LETTER_TYPE, reply_to=None)
+    job = create_job(template=template)
+
+    mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life")
+
+    personalisation = {
+        'addressline1': 'Foo',
+        'addressline2': 'Bar',
+        'addressline3': 'Baz',
+        'addressline4': 'Wibble',
+        'addressline5': 'Wobble',
+        'addressline6': 'Wubble',
+        'postcode': 'Flob',
+    }
+    notification_json = _notification_json(
+        template=job.template,
+        to='Foo',
+        personalisation=personalisation,
+        job_id=job.id,
+        row_number=1
+    )
+    notification_id = uuid.uuid4()
+    created_at = datetime.utcnow()
+
+    save_letter(
+        job.service_id,
+        notification_id,
+        encryption.encrypt(notification_json),
+    )
+
+    notification_db = Notification.query.one()
+    assert notification_db.id == notification_id
+    assert notification_db.to == 'Foo'
+    assert notification_db.job_id == job.id
+    assert notification_db.template_id == job.template.id
+    assert notification_db.template_version == job.template.version
+    assert notification_db.status == 'created'
+    assert notification_db.created_at >= created_at
+    assert notification_db.notification_type == 'letter'
+    assert notification_db.sent_at is None
+    assert notification_db.sent_by is None
+    assert notification_db.personalisation == personalisation
+    assert notification_db.reference == "this-is-random-in-real-life"
+    assert not notification_db.reply_to_text
+
+
 def test_save_letter_uses_template_reply_to_text(mocker, notify_db_session):
     service = create_service()
     create_letter_contact(service=service, contact_block="Address contact", is_default=True)

tests/app/dao/test_templates_dao.py

@@ -111,6 +111,29 @@ def test_update_template_reply_to(sample_service, sample_user):
     assert template_history.service_letter_contact_id
 
 
+def test_update_template_reply_to_updates_history(sample_service, sample_user):
+    letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA')
+
+    data = {
+        'name': 'Sample Template',
+        'template_type': "letter",
+        'content': "Template content",
+        'service': sample_service,
+        'created_by': sample_user,
+    }
+    template = Template(**data)
+    dao_create_template(template)
+    created = dao_get_all_templates_for_service(sample_service.id)[0]
+    assert created.reply_to is None
+
+    created.reply_to = letter_contact.id
+    dao_update_template(created)
+    assert dao_get_all_templates_for_service(sample_service.id)[0].reply_to == letter_contact.id
+
+    template_history = TemplateHistory.query.filter_by(id=created.id, version=2).one()
+    assert template_history.service_letter_contact_id == letter_contact.id
+
+
 def test_redact_template(sample_template):
     redacted = TemplateRedacted.query.one()
     assert redacted.template_id == sample_template.id

tests/app/template/test_rest.py

@@ -6,7 +6,7 @@ from datetime import datetime, timedelta
 import pytest
 from freezegun import freeze_time
 
-from app.models import Template, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE
+from app.models import Template, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, TemplateHistory
 from app.dao.templates_dao import dao_get_template_by_id, dao_redact_template
 
 from tests import create_authorization_header
@@ -587,6 +587,8 @@ def test_create_a_template_with_reply_to(admin_request, sample_user):
     template = Template.query.get(json_resp['data']['id'])
     from app.schemas import template_schema
     assert sorted(json_resp['data']) == sorted(template_schema.dump(template).data)
+    th = TemplateHistory.query.filter_by(id=template.id, version=1).one()
+    assert th.service_letter_contact_id == letter_contact.id
 
 
 def test_create_a_template_with_foreign_service_reply_to(admin_request, sample_user):
@@ -644,7 +646,6 @@ def test_get_template_reply_to(client, sample_service, template_default, service
 def test_update_template_reply_to(client, sample_letter_template):
     auth_header = create_authorization_header()
     letter_contact = create_letter_contact(sample_letter_template.service, "Edinburgh, ED1 1AA")
-
     data = {
         'reply_to': str(letter_contact.id),
     }
@@ -656,7 +657,9 @@ def test_update_template_reply_to(client, sample_letter_template):
     assert resp.status_code == 200, resp.get_data(as_text=True)
 
     template = dao_get_template_by_id(sample_letter_template.id)
-    assert template.reply_to == letter_contact.id
+    assert template.service_letter_contact_id == letter_contact.id
+    th = TemplateHistory.query.filter_by(id=sample_letter_template.id, version=2).one()
+    assert th.service_letter_contact_id == letter_contact.id
 
 
 def test_update_template_with_foreign_service_reply_to(client, sample_letter_template):