Merge pull request #361 from alphagov/do-not-create-new-tempalte-version

Check for an update before saving template

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,14 +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())
-    current_data.update(request.get_json())
-    current_data['content'] = _strip_html(current_data['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 _template_has_not_changed(current_data, updated_template):
+        return jsonify(data=updated_template), 200
 
-    update_dict, errors = template_schema.load(current_data)
+    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(
-        current_data['content'],
+        updated_template['content'],
         fetched_template.template_type)
     if over_limit:
         return json_resp, 400
@@ -115,3 +118,10 @@ 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):
+    return all(
+        current_data[key] == updated_template[key]
+        for key in ('name', 'content', 'subject', 'archived')
+    )

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):
@@ -155,29 +156,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 <script type="text/javascript">alert("foo")</script>',
                 'created_by': str(sample_user.id)
@@ -186,7 +167,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 +175,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 +427,19 @@ 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
+    template = dao_get_template_by_id(sample_template.id)
+    assert template.version == 1

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