From e59d6d470ee98a8493c97deb6ebeb8eeb8d21755 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 10 Jan 2018 12:40:14 +0000 Subject: [PATCH] Fix the problem with updating the reply_to or service_letter_contact_id for templates. The history was not being updated properly, we think this is because the declaritive attribute is not being set propery by the property. When reply_to: None it will update the service_letter_contact_id, but not the service_letter_contact, we think when the history_meta is build the history class and checking if the value is updated it depends which attribute it is checking first. In order to fix this issue, there is a new dao method to update the reply_to on the Template and insert a new Template history. --- app/dao/templates_dao.py | 30 ++++++++++ app/template/rest.py | 12 ++-- tests/app/dao/test_templates_dao.py | 90 ++++++++++++++++++++--------- tests/app/template/test_rest.py | 22 +++++++ 4 files changed, 122 insertions(+), 32 deletions(-) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 5343f511c..f42428440 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -33,6 +33,36 @@ def dao_update_template(template): db.session.add(template) +@transactional +def dao_update_template_reply_to(template_id, reply_to): + Template.query.filter_by(id=template_id).update( + {"service_letter_contact_id": reply_to, + "updated_at": datetime.utcnow(), + "version": Template.version + 1, + } + ) + template = Template.query.filter_by(id=template_id).one() + + history = TemplateHistory(** + { + "id": template.id, + "name": template.name, + "template_type": template.template_type, + "created_at": template.created_at, + "updated_at": template.updated_at, + "content": template.content, + "service_id": template.service_id, + "subject": template.subject, + "created_by_id": template.created_by_id, + "version": template.version, + "archived": template.archived, + "process_type": template.process_type, + "service_letter_contact_id": template.service_letter_contact_id + }) + db.session.add(history) + return history + + @transactional def dao_redact_template(template, user_id): template.template_redacted.redact_personalisation = True diff --git a/app/template/rest.py b/app/template/rest.py index d3f65d9e7..038f249bf 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -11,7 +11,8 @@ from app.dao.templates_dao import ( dao_redact_template, dao_get_template_by_id_and_service_id, dao_get_all_templates_for_service, - dao_get_template_versions + dao_get_template_versions, + dao_update_template_reply_to ) from notifications_utils.template import SMSMessageTemplate from app.dao.services_dao import dao_fetch_service_by_id @@ -81,6 +82,11 @@ def update_template(service_id, template_id): if data.get('redact_personalisation') is True: return redact_template(fetched_template, data) + if "reply_to" in data: + check_reply_to(service_id, data.get("reply_to"), fetched_template.template_type) + updated = dao_update_template_reply_to(template_id=template_id, reply_to=data.get("reply_to")) + return jsonify(data=template_schema.dump(updated).data), 200 + current_data = dict(template_schema.dump(fetched_template).data.items()) updated_template = dict(template_schema.dump(fetched_template).data.items()) updated_template.update(data) @@ -89,8 +95,6 @@ def update_template(service_id, template_id): if _template_has_not_changed(current_data, updated_template): return jsonify(data=updated_template), 200 - 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') @@ -160,7 +164,7 @@ def get_template_versions(service_id, template_id): def _template_has_not_changed(current_data, updated_template): return all( current_data[key] == updated_template[key] - for key in ('name', 'content', 'subject', 'archived', 'process_type', 'reply_to') + for key in ('name', 'content', 'subject', 'archived', 'process_type') ) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 1f30c78d7..5e0ceb88b 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -11,7 +11,8 @@ from app.dao.templates_dao import ( dao_update_template, dao_get_template_versions, dao_get_templates_for_cache, - dao_redact_template) + dao_redact_template, dao_update_template_reply_to +) from app.models import Template, TemplateHistory, TemplateRedacted from tests.app.conftest import sample_template as create_sample_template @@ -88,7 +89,7 @@ def test_update_template(sample_service, sample_user): assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'new name' -def test_update_template_reply_to(sample_service, sample_user): +def test_dao_update_template_reply_to_none_to_some(sample_service, sample_user): letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA') data = { @@ -100,38 +101,71 @@ def test_update_template_reply_to(sample_service, sample_user): } template = Template(**data) dao_create_template(template) - created = dao_get_all_templates_for_service(sample_service.id)[0] + created = Template.query.get(template.id) assert created.reply_to is None + assert created.service_letter_contact_id 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 + dao_update_template_reply_to(template_id=template.id, + reply_to=letter_contact.id) - template_history = TemplateHistory.query.filter_by(id=created.id, version=2).one() - 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 + updated = Template.query.get(template.id) + assert updated.reply_to == letter_contact.id + assert updated.version == 2 + assert updated.updated_at template_history = TemplateHistory.query.filter_by(id=created.id, version=2).one() assert template_history.service_letter_contact_id == letter_contact.id + assert template_history.updated_at == updated.updated_at + + +def test_dao_update_tempalte_reply_to_some_to_some(sample_service, sample_user): + letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA') + letter_contact_2 = create_letter_contact(sample_service, 'London, N1 1DE') + + data = { + 'name': 'Sample Template', + 'template_type': "letter", + 'content': "Template content", + 'service': sample_service, + 'created_by': sample_user, + 'service_letter_contact_id': letter_contact.id + } + template = Template(**data) + dao_create_template(template) + created = Template.query.get(template.id) + dao_update_template_reply_to(template_id=created.id, reply_to=letter_contact_2.id) + updated = Template.query.get(template.id) + assert updated.reply_to == letter_contact_2.id + assert updated.version == 2 + assert updated.updated_at + + updated_history = TemplateHistory.query.filter_by(id=created.id, version=2).one() + assert updated_history.service_letter_contact_id == letter_contact_2.id + assert updated_history.updated_at == updated_history.updated_at + + +def test_dao_update_tempalte_reply_to_some_to_none(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, + 'service_letter_contact_id': letter_contact.id + } + template = Template(**data) + dao_create_template(template) + created = Template.query.get(template.id) + dao_update_template_reply_to(template_id=created.id, reply_to=None) + updated = Template.query.get(template.id) + assert updated.reply_to is None + assert updated.version == 2 + assert updated.updated_at + + history = TemplateHistory.query.filter_by(id=created.id, version=2).one() + assert history.service_letter_contact_id is None + assert history.updated_at == updated.updated_at def test_redact_template(sample_template): diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 21100af2a..1324a9915 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -662,6 +662,28 @@ def test_update_template_reply_to(client, sample_letter_template): assert th.service_letter_contact_id == letter_contact.id +def test_update_template_reply_to_set_to_blank(client, notify_db_session): + auth_header = create_authorization_header() + service = create_service(service_permissions=['letter']) + letter_contact = create_letter_contact(service, "Edinburgh, ED1 1AA") + template = create_template(service=service, template_type='letter', reply_to=letter_contact.id) + + data = { + 'reply_to': None, + } + + resp = client.post('/service/{}/template/{}'.format(template.service_id, template.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert resp.status_code == 200, resp.get_data(as_text=True) + + template = dao_get_template_by_id(template.id) + assert template.service_letter_contact_id is None + th = TemplateHistory.query.filter_by(id=template.id, version=2).one() + assert th.service_letter_contact_id is None + + def test_update_template_with_foreign_service_reply_to(client, sample_letter_template): auth_header = create_authorization_header()