Merge pull request #631 from alphagov/breaking-change

Warn users when a template change is going to break things

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;
+  }
+
+}

app/main/views/send.py

@@ -1,5 +1,7 @@
 import json
 import itertools
+from string import ascii_uppercase
+
 from contextlib import suppress
 from zipfile import BadZipFile
 from xlrd.biffh import XLRDError
@@ -127,8 +129,11 @@ def send_messages(service_id, template_id):
     return render_template(
         'views/send.html',
         template=template,
-        recipient_column=first_column_heading[template.template_type],
-        example=[get_example_csv_rows(template)],
+        column_headings=list(ascii_uppercase[:len(template.placeholders) + 1]),
+        example=[
+            [first_column_heading[template.template_type]] + list(template.placeholders),
+            get_example_csv_rows(template)
+        ],
         form=form
     )
 

app/main/views/templates.py

@@ -1,12 +1,16 @@
-from flask import request, render_template, redirect, url_for, flash, abort
+from string import ascii_uppercase
+
+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 hasattr(form, 'subject') 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(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:

app/templates/views/send-test.html

@@ -40,9 +40,7 @@
     {% call(item, row_number) list_table(
       example,
       caption="Fill in the {}".format('field' if template.placeholders|length == 1 else 'fields'),
-      field_headings=[
-        '<span class="placeholder">{}</span>'.format(recipient_column)|safe
-      ] + template.placeholders_as_markup|list
+      field_headings=[recipient_column] + template.placeholders|list
     ) %}
       {% for column in item %}
         {% call field() %}

app/templates/views/send.html

@@ -63,20 +63,19 @@
       <h2 class="heading-small">
         Example file
       </h2>
-
-      {% call(item, row_number) list_table(
-        example,
-        caption="Example",
-        caption_visible=False,
-        field_headings=['1'] + [
-          '<span class="placeholder">{}</span>'.format(recipient_column)|safe
-        ] + template.placeholders_as_markup|list
-      ) %}
-        {{ index_field(row_number) }}
-        {% for column in item %}
-          {{ text_field(column) }}
-        {% endfor %}
-      {% endcall %}
+      <div class="spreadsheet">
+        {% call(item, row_number) list_table(
+          example,
+          caption="Example",
+          caption_visible=False,
+          field_headings=[''] + column_headings
+        ) %}
+          {{ index_field(row_number - 1) }}
+          {% for column in item %}
+            {{ text_field(column) }}
+          {% endfor %}
+        {% endcall %}
+      </div>
       <p class="table-show-more-link">
         <a href="{{ url_for('.get_example_csv', service_id=current_service.id, template_id=template.id) }}">Download this example</a>
       </p>

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) %}
+  {% for placeholder in placeholders %}
+    {% if loop.last and loop.length > 1 %} and {% endif %}
+    <span class="placeholder">(({{ placeholder }}))</span>
+  {% 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 %}

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

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,