From 3ac76192d0626cd31277d16d0c9525c92543014a Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 27 May 2016 16:21:29 +0100
Subject: [PATCH] Warn users a template change will break things
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When a user adds or removes placeholders in their template we should consider
this a ‘breaking change’ and warn them accordingly.

Implementing this mostly relies on using
https://github.com/alphagov/notifications-utils/pull/37

Temporarily storing the new template until the user confirms that they want to
make the changes in done using hidden fields. This is a bit hacky, but the
complexity of making sessions interact with WTForms was just too much to handle.

This commit also changes the example spreadsheet that we show on this page to
look more like a spreadsheet.
---
 app/assets/stylesheets/app.scss               | 21 ++++++
 app/main/views/templates.py                   | 32 ++++++++-
 .../views/templates/breaking-change.html      | 72 +++++++++++++++++++
 requirements.txt                              |  2 +-
 tests/app/main/views/test_templates.py        | 42 +++++++++++
 5 files changed, 165 insertions(+), 4 deletions(-)
 create mode 100644 app/templates/views/templates/breaking-change.html

diff --git a/app/assets/stylesheets/app.scss b/app/assets/stylesheets/app.scss
index b47b576be..9a1b05c57 100644
--- a/app/assets/stylesheets/app.scss
+++ b/app/assets/stylesheets/app.scss
@@ -153,3 +153,24 @@ details summary {
   text-decoration: underline;
   margin-bottom: $gutter-half;
 }
+
+.spreadsheet {
+
+  th,
+  .table-field-index {
+    background: $grey-4;
+    font-weight: bold;
+    text-align: center;
+  }
+
+  th, td {
+    padding-left: 10px;
+    padding-right: 10px;
+    border: 1px solid $border-colour;
+  }
+
+  td {
+    border-top: 0;
+  }
+
+}
diff --git a/app/main/views/templates.py b/app/main/views/templates.py
index 915206649..fceec018a 100644
--- a/app/main/views/templates.py
+++ b/app/main/views/templates.py
@@ -1,12 +1,16 @@
-from flask import request, render_template, redirect, url_for, flash, abort
+import string
+
+from flask import request, render_template, redirect, url_for, flash, abort, session
 from flask_login import login_required
 
-from notifications_utils.template import Template
+from notifications_utils.template import Template, TemplateChange
+from notifications_utils.recipients import first_column_heading
 from notifications_python_client.errors import HTTPError
 
 from app.main import main
 from app.utils import user_has_permissions
 from app.main.forms import SMSTemplateForm, EmailTemplateForm
+from app.main.views.send import get_example_csv_rows
 from app import service_api_client, current_service
 
 
@@ -109,6 +113,28 @@ def edit_service_template(service_id, template_id):
     form = form_objects[template['template_type']](**template)
 
     if form.validate_on_submit():
+        subject = form.subject.data if getattr(form, 'subject', None) else None
+        new_template = Template({
+            'name': form.name.data,
+            'content': form.template_content.data,
+            'subject': subject,
+            'template_type': template['template_type'],
+            'id': template['id']
+        })
+        template_change = Template(template).compare_to(new_template)
+        if template_change.has_different_placeholders and not request.form.get('confirm'):
+            return render_template(
+                'views/templates/breaking-change.html',
+                template_change=template_change,
+                new_template=new_template,
+                column_headings=list(string.ascii_uppercase[:len(new_template.placeholders) + 1]),
+                example_rows=[
+                    [first_column_heading[new_template.template_type]] + list(new_template.placeholders),
+                    get_example_csv_rows(new_template),
+                    get_example_csv_rows(new_template)
+                ],
+                form=form
+            )
         try:
             service_api_client.update_service_template(
                 template_id,
@@ -116,7 +142,7 @@ def edit_service_template(service_id, template_id):
                 template['template_type'],
                 form.template_content.data,
                 service_id,
-                form.subject.data if getattr(form, 'subject', None) else None
+                subject
             )
         except HTTPError as e:
             if e.status_code == 400:
diff --git a/app/templates/views/templates/breaking-change.html b/app/templates/views/templates/breaking-change.html
new file mode 100644
index 000000000..b6bfd33b3
--- /dev/null
+++ b/app/templates/views/templates/breaking-change.html
@@ -0,0 +1,72 @@
+{% extends "withnav_template.html" %}
+{% from "components/banner.html" import banner_wrapper %}
+{% from "components/page-footer.html" import page_footer %}
+{% from "components/table.html" import list_table, text_field, index_field, index_field_heading %}
+
+{% macro list_of_placeholders(placeholders, oxford_comma=False) %}
+  {% for placeholder in placeholders %}
+    {% if loop.last and loop.length > 1 %}{% if oxford_comma %},{% endif %} and {% endif %}
+    <span class="placeholder">(({{ placeholder }}))</span>{% if not loop.last and loop.length != 2 %},{% endif %}
+  {% endfor %}
+{% endmacro %}
+
+{% block page_title %}
+  GOV.UK Notify
+{% endblock %}
+
+{% block maincolumn_content %}
+
+  <h1 class="heading-large">Confirm changes</h1>
+
+  <div class="bottom-gutter">
+    {% if template_change.placeholders_removed %}
+      <p>
+        You removed
+        {{ list_of_placeholders(template_change.placeholders_removed) }}
+      </p>
+    {% endif %}
+    {% if template_change.placeholders_added %}
+      <p>
+        You added {{ list_of_placeholders(template_change.placeholders_added) }}
+      </p>
+    {% endif %}
+  </div>
+
+  <p>
+    When you send messages using this template you’ll need
+    {{ new_template.placeholders|length + 1 }} columns of data:
+  </p>
+
+  <div class="spreadsheet">
+    {% call(item, row_number) list_table(
+      example_rows,
+      caption="Example",
+      caption_visible=False,
+      field_headings=[''] + column_headings
+    ) %}
+      {% if 1 == row_number %}
+        {{ index_field('') }}
+      {% else %}
+        {{ index_field(row_number - 1) }}
+      {% endif %}
+      {% for column in item %}
+        {{ text_field(column) }}
+      {% endfor %}
+    {% endcall %}
+  </div>
+
+  <p>Developers, you’ll need to update your API calls</p>
+
+  <form method="post">
+    <input type="hidden" name="name" value="{{ new_template.name }}" />
+    <input type="hidden" name="subject" value="{{ new_template.subject or '' }}" />
+    <input type="hidden" name="template_content" value="{{ new_template.content }}" />
+    <input type="hidden" name="confirm" value="true" />
+    {{ page_footer(
+      'Save changes to template',
+      back_link=url_for(".edit_service_template", service_id=current_service.id, template_id=new_template.id),
+      back_link_text="Back"
+    ) }}
+  </form>
+
+{% endblock %}
diff --git a/requirements.txt b/requirements.txt
index acc96b2f0..b04c2c4ad 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -18,4 +18,4 @@ pytz==2016.4
 
 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@5.3.0#egg=notifications-utils==5.3.0
+git+https://github.com/alphagov/notifications-utils.git@5.4.0#egg=notifications-utils==5.4.0
diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py
index fb8166ab3..defd07af7 100644
--- a/tests/app/main/views/test_templates.py
+++ b/tests/app/main/views/test_templates.py
@@ -1,5 +1,6 @@
 import json
 import uuid
+from bs4 import BeautifulSoup
 from tests import validate_route_permission
 
 from flask import url_for
@@ -67,6 +68,47 @@ def test_should_redirect_when_saving_a_template(app_,
                 template_id, name, 'sms', content, service_id, None)
 
 
+def test_should_show_interstitial_when_making_breaking_change(
+    app_,
+    api_user_active,
+    mock_login,
+    mock_get_service_template,
+    mock_update_service_template,
+    mock_get_user,
+    mock_get_service,
+    mock_get_user_by_email,
+    mock_has_permissions,
+    fake_uuid
+):
+    with app_.test_request_context():
+        with app_.test_client() as client:
+            client.login(api_user_active)
+            service_id = fake_uuid
+            template_id = fake_uuid
+            response = client.post(
+                url_for('.edit_service_template', service_id=service_id, template_id=template_id),
+                data={
+                    'id': template_id,
+                    'name': "new name",
+                    'template_content': "hello ((name))",
+                    'template_type': 'sms',
+                    'service': service_id
+                }
+            )
+
+            assert response.status_code == 200
+            page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+            assert page.h1.string.strip() == "Confirm changes"
+
+            for key, value in {
+                'name': 'new name',
+                'subject': '',
+                'template_content': 'hello ((name))',
+                'confirm': 'true'
+            }.items():
+                assert page.find('input', {'name': key})['value'] == value
+
+
 def test_should_not_create_too_big_template(app_,
                                             api_user_active,
                                             mock_login,