From 0a9cdbd75aab62ca38b8ff19d69e7389dfcac97d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 1 Jun 2016 10:53:03 +0100 Subject: [PATCH 1/4] Do not create a new version of the template if there is no change to the tempalte. --- app/template/rest.py | 12 ++++--- tests/app/template/test_rest.py | 45 ++++++++++++------------- tests/app/template/test_rest_history.py | 2 -- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 752417e0f..78696d2a1 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -59,14 +59,18 @@ def update_template(service_id, template_id): fetched_template = dao_get_template_by_id_and_service_id(template_id=template_id, service_id=service_id) current_data = dict(template_schema.dump(fetched_template).data.items()) - current_data.update(request.get_json()) - current_data['content'] = _strip_html(current_data['content']) + update_template = dict(template_schema.dump(fetched_template).data.items()) + update_template.update(request.get_json()) + update_template['content'] = _strip_html(update_template['content']) + # Check if there is a change to make. + if current_data == update_template: + return jsonify(data=update_template), 200 - update_dict, errors = template_schema.load(current_data) + update_dict, errors = template_schema.load(update_template) if errors: return jsonify(result="error", message=errors), 400 over_limit, json_resp = _content_count_greater_than_limit( - current_data['content'], + update_template['content'], fetched_template.template_type) if over_limit: return json_resp, 400 diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index b7ea2d47e..df17e2121 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -155,29 +155,9 @@ def test_must_have_a_subject_on_an_email_template(notify_api, sample_user, sampl assert json_resp['message'] == {'subject': ['Invalid template subject']} -def test_should_be_able_to_update_a_template(notify_api, sample_user, sample_service): +def test_update_should_update_a_template(notify_api, sample_user, sample_template): with notify_api.test_request_context(): with notify_api.test_client() as client: - data = { - 'name': 'my template', - 'template_type': 'email', - 'subject': 'subject', - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() - - create_response = client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - assert create_response.status_code == 201 - json_resp = json.loads(create_response.get_data(as_text=True)) - assert json_resp['data']['name'] == 'my template' - assert json_resp['data']['version'] == 1 data = { 'content': 'my template has new content ', 'created_by': str(sample_user.id) @@ -186,7 +166,7 @@ def test_should_be_able_to_update_a_template(notify_api, sample_user, sample_ser auth_header = create_authorization_header() update_response = client.post( - '/service/{}/template/{}'.format(sample_service.id, json_resp['data']['id']), + '/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), headers=[('Content-Type', 'application/json'), auth_header], data=data ) @@ -194,10 +174,12 @@ def test_should_be_able_to_update_a_template(notify_api, sample_user, sample_ser assert update_response.status_code == 200 update_json_resp = json.loads(update_response.get_data(as_text=True)) assert update_json_resp['data']['content'] == 'my template has new content alert("foo")' + assert update_json_resp['data']['name'] == sample_template.name + assert update_json_resp['data']['template_type'] == sample_template.template_type assert update_json_resp['data']['version'] == 2 -def test_should_be_able_to_archive_template(notify_api, sample_user, sample_service, sample_template): +def test_should_be_able_to_archive_template(notify_api, sample_template): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { @@ -444,3 +426,20 @@ def test_should_return_all_template_versions_for_service_and_template_id(notify_ assert x['content'] == original_content + '1' else: assert x['content'] == original_content + '2' + + +def test_update_does_not_create_new_version_when_there_is_no_change(notify_api, sample_template): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth_header = create_authorization_header() + data = { + 'template_type': sample_template.template_type, + 'content': sample_template.content, + } + resp = client.post('/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 200 + from app.dao.templates_dao import dao_get_template_by_id + template = dao_get_template_by_id(sample_template.id) + assert template.version == 1 diff --git a/tests/app/template/test_rest_history.py b/tests/app/template/test_rest_history.py index 5b0c919b6..482b04aad 100644 --- a/tests/app/template/test_rest_history.py +++ b/tests/app/template/test_rest_history.py @@ -1,8 +1,6 @@ import json from datetime import (datetime, date) from flask import url_for -from app.models import Template -from freezegun import freeze_time from app.dao.templates_dao import dao_update_template from tests import create_authorization_header From 05e72b07aec98132853de79a33203e3714e08ed9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 1 Jun 2016 11:50:45 +0100 Subject: [PATCH 2/4] Return status code 304 when template is not updated. Moved import --- app/template/rest.py | 2 +- tests/app/template/test_rest.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 78696d2a1..e010fbb03 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -64,7 +64,7 @@ def update_template(service_id, template_id): update_template['content'] = _strip_html(update_template['content']) # Check if there is a change to make. if current_data == update_template: - return jsonify(data=update_template), 200 + return jsonify(data=update_template), 304 update_dict, errors = template_schema.load(update_template) if errors: diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index df17e2121..6adef6af2 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -3,6 +3,7 @@ import random import string from app.models import Template from tests import create_authorization_header +from app.dao.templates_dao import dao_get_template_by_id def test_should_create_a_new_sms_template_for_a_service(notify_api, sample_user, sample_service): @@ -439,7 +440,6 @@ def test_update_does_not_create_new_version_when_there_is_no_change(notify_api, resp = client.post('/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 200 - from app.dao.templates_dao import dao_get_template_by_id + assert resp.status_code == 304 template = dao_get_template_by_id(sample_template.id) assert template.version == 1 From 047a7d548820e650330dfff3b50d6fc3dbca6b15 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 1 Jun 2016 12:19:59 +0100 Subject: [PATCH 3/4] Only test the items of the template that can change on an update request. Return 200 if no change is made. --- app/template/rest.py | 25 +++++++++++++++++-------- tests/app/template/test_rest.py | 2 +- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index e010fbb03..94a51a749 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -5,7 +5,6 @@ from flask import ( current_app ) import bleach -from sqlalchemy.exc import IntegrityError from app.dao.templates_dao import ( dao_update_template, @@ -59,18 +58,18 @@ def update_template(service_id, template_id): fetched_template = dao_get_template_by_id_and_service_id(template_id=template_id, service_id=service_id) current_data = dict(template_schema.dump(fetched_template).data.items()) - update_template = dict(template_schema.dump(fetched_template).data.items()) - update_template.update(request.get_json()) - update_template['content'] = _strip_html(update_template['content']) + updated_template = dict(template_schema.dump(fetched_template).data.items()) + updated_template.update(request.get_json()) + updated_template['content'] = _strip_html(updated_template['content']) # Check if there is a change to make. - if current_data == update_template: - return jsonify(data=update_template), 304 + if _template_has_not_changed(current_data, updated_template): + return jsonify(data=updated_template), 200 - update_dict, errors = template_schema.load(update_template) + update_dict, errors = template_schema.load(updated_template) if errors: return jsonify(result="error", message=errors), 400 over_limit, json_resp = _content_count_greater_than_limit( - update_template['content'], + updated_template['content'], fetched_template.template_type) if over_limit: return json_resp, 400 @@ -119,3 +118,13 @@ def get_template_versions(service_id, template_id): def _strip_html(content): return bleach.clean(content, tags=[], strip=True) + + +def _template_has_not_changed(current_data, updated_template): + if (current_data['name'] == updated_template['name'] and + current_data['content'] == updated_template['content'] and + current_data['subject'] == updated_template['subject']and + current_data['archived'] == updated_template['archived']): + return True + else: + return False diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 6adef6af2..45e7300e6 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -440,6 +440,6 @@ def test_update_does_not_create_new_version_when_there_is_no_change(notify_api, resp = client.post('/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 304 + assert resp.status_code == 200 template = dao_get_template_by_id(sample_template.id) assert template.version == 1 From ec7d9b1c6e49f427f5966a0a9187cbb5b4ca66e8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 1 Jun 2016 13:55:04 +0100 Subject: [PATCH 4/4] Use a more pythonic way to compare the dicts. --- app/template/rest.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 94a51a749..168d483ea 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -121,10 +121,7 @@ def _strip_html(content): def _template_has_not_changed(current_data, updated_template): - if (current_data['name'] == updated_template['name'] and - current_data['content'] == updated_template['content'] and - current_data['subject'] == updated_template['subject']and - current_data['archived'] == updated_template['archived']): - return True - else: - return False + return all( + current_data[key] == updated_template[key] + for key in ('name', 'content', 'subject', 'archived') + )