Merge pull request #1174 from alphagov/unbreak-my-change

Fix breaking change page

app/__init__.py

@@ -30,6 +30,7 @@ from notifications_python_client.errors import HTTPError
 from notifications_utils import logging, request_id, formatters
 from notifications_utils.clients.statsd.statsd_client import StatsdClient
 from notifications_utils.recipients import validate_phone_number, InvalidPhoneError
+from notifications_utils.field import escape_html
 from pygments import highlight
 from pygments.formatters.html import HtmlFormatter
 from pygments.lexers.javascript import JavascriptLexer
@@ -363,17 +364,17 @@ def formatted_list(
     if prefix_plural:
         prefix_plural += ' '
 
-    items = list(items)
+    items = list(map(escape_html, items))
     if len(items) == 1:
-        return '{prefix}{before_each}{items[0]}{after_each}'.format(**locals())
+        return Markup('{prefix}{before_each}{items[0]}{after_each}'.format(**locals()))
     elif items:
         formatted_items = ['{}{}{}'.format(before_each, item, after_each) for item in items]
 
         first_items = separator.join(formatted_items[:-1])
         last_item = formatted_items[-1]
-        return (
+        return Markup((
             '{prefix_plural}{first_items} {conjunction} {last_item}'
-        ).format(**locals())
+        ).format(**locals()))
 
 
 def nl2br(value):

app/main/views/templates.py

@@ -262,12 +262,7 @@ def edit_service_template(service_id, template_id):
             return render_template(
                 'views/templates/breaking-change.html',
                 template_change=template_change,
-                new_template={
-                    'name': form.name.data,
-                    'subject': subject,
-                    'content': form.template_content.data,
-                    'id': new_template.id
-                },
+                new_template=new_template,
                 column_headings=list(ascii_uppercase[:len(new_template.placeholders) + 1]),
                 example_rows=[
                     first_column_headings[new_template.template_type] + list(new_template.placeholders),

app/templates/views/templates/breaking-change.html

@@ -41,7 +41,8 @@
 
   <p>
     When you send messages using this template you’ll need
-    {{ new_template.placeholders|length + 1 }} columns of data:
+    {{ new_template.placeholders|length + 1 }}
+    column{{ 's' if new_template.placeholders|length > 0 else '' }} of data:
   </p>
 
   <div class="spreadsheet">

tests/app/main/views/test_templates.py

@@ -9,6 +9,7 @@ from freezegun import freeze_time
 from notifications_python_client.errors import HTTPError
 from tests.conftest import service_one as create_sample_service
 from tests import validate_route_permission, template_json, single_notification_json
+from tests.app.test_utils import normalize_spaces
 
 from app.main.views.templates import get_last_use_message, get_human_readable_delta
 
@@ -262,7 +263,7 @@ def test_should_show_interstitial_when_making_breaking_change(
         data={
             'id': template_id,
             'name': "new name",
-            'template_content': "hello",
+            'template_content': "hello ((name)) lets talk about ((thing))",
             'template_type': 'email',
             'subject': 'reminder',
             'service': service_id,
@@ -276,10 +277,17 @@ def test_should_show_interstitial_when_making_breaking_change(
     assert page.find('a', {'class': 'page-footer-back-link'})['href'] == url_for(".edit_service_template",
                                                                                  service_id=service_id,
                                                                                  template_id=template_id)
+    for index, p in enumerate([
+        'You removed ((date))',
+        'You added ((name))',
+        'When you send messages using this template you’ll need 3 columns of data:',
+    ]):
+        assert normalize_spaces(page.select('main p')[index].text) == p
+
     for key, value in {
         'name': 'new name',
         'subject': 'reminder',
-        'template_content': 'hello',
+        'template_content': 'hello ((name)) lets talk about ((thing))',
         'confirm': 'true'
     }.items():
         assert page.find('input', {'name': key})['value'] == value

tests/app/test_jinja_filters.py

@@ -1,5 +1,6 @@
 import pytest
 
+from flask import Markup
 from app import formatted_list
 
 
@@ -11,6 +12,12 @@ from app import formatted_list
     ([1], {'prefix': 'foo', 'prefix_plural': 'bar'}, 'foo ‘1’'),
     ([1, 2, 3], {'before_each': 'a', 'after_each': 'b'}, 'a1b, a2b and a3b'),
     ([1, 2, 3], {'conjunction': 'foo'}, '‘1’, ‘2’ foo ‘3’'),
+    (['&'], {'before_each': '<i>', 'after_each': '</i>'}, '<i>&amp;</i>'),
+    ([1, 2, 3], {'before_each': '<i>', 'after_each': '</i>'}, '<i>1</i>, <i>2</i> and <i>3</i>'),
 ])
 def test_formatted_list(items, kwargs, expected_output):
     assert formatted_list(items, **kwargs) == expected_output
+
+
+def test_formatted_list_returns_markup():
+    assert isinstance(formatted_list([0]), Markup)