From cc839562dae499389777fdfdbc6095edf81affad Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 9 Jan 2018 16:41:58 +0000 Subject: [PATCH 1/3] 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 | 6 ++-- tests/app/celery/test_tasks.py | 49 +++++++++++++++++++++++++++++ tests/app/dao/test_templates_dao.py | 23 ++++++++++++++ tests/app/template/test_rest.py | 9 ++++-- 4 files changed, 82 insertions(+), 5 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 2dba8f961..d3f65d9e7 100644 --- a/app/template/rest.py +++ b/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 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 4e1937370..fd87850ff 100644 --- a/tests/app/celery/test_tasks.py +++ b/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) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 9d999c006..1f30c78d7 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/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 diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index ffa2147f5..21100af2a 100644 --- a/tests/app/template/test_rest.py +++ b/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): From e59d6d470ee98a8493c97deb6ebeb8eeb8d21755 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 10 Jan 2018 12:40:14 +0000 Subject: [PATCH 2/3] 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() From 03deb804b053b2deb338ee249ea2c4079fcf1d1d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 10 Jan 2018 13:32:54 +0000 Subject: [PATCH 3/3] -Fix typo - return template from dao not the history --- app/dao/templates_dao.py | 2 +- tests/app/dao/test_templates_dao.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index f42428440..16979bd30 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -60,7 +60,7 @@ def dao_update_template_reply_to(template_id, reply_to): "service_letter_contact_id": template.service_letter_contact_id }) db.session.add(history) - return history + return template @transactional diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 5e0ceb88b..561875b46 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -144,7 +144,7 @@ def test_dao_update_tempalte_reply_to_some_to_some(sample_service, sample_user): assert updated_history.updated_at == updated_history.updated_at -def test_dao_update_tempalte_reply_to_some_to_none(sample_service, sample_user): +def test_dao_update_template_reply_to_some_to_none(sample_service, sample_user): letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA') data = { 'name': 'Sample Template',