From 3ac76192d0626cd31277d16d0c9525c92543014a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 27 May 2016 16:21:29 +0100 Subject: [PATCH 1/5] 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 %} + (({{ placeholder }})){% if not loop.last and loop.length != 2 %},{% endif %} + {% endfor %} +{% endmacro %} + +{% block page_title %} + GOV.UK Notify +{% endblock %} + +{% block maincolumn_content %} + +

Confirm changes

+ +
+ {% if template_change.placeholders_removed %} +

+ You removed + {{ list_of_placeholders(template_change.placeholders_removed) }} +

+ {% endif %} + {% if template_change.placeholders_added %} +

+ You added {{ list_of_placeholders(template_change.placeholders_added) }} +

+ {% endif %} +
+ +

+ When you send messages using this template you’ll need + {{ new_template.placeholders|length + 1 }} columns of data: +

+ +
+ {% 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 %} +
+ +

Developers, you’ll need to update your API calls

+ +
+ + + + + {{ 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" + ) }} +
+ +{% 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, From 6d3fbb1f51b3ffbed776e3db62536f9fec247adb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 May 2016 14:18:35 +0100 Subject: [PATCH 2/5] Make example on send look more like a spreadsheet Now that the example on the breaking changes page looks more like a spreadsheet, we should do the same thing for the downloadable example on the send page. --- app/main/views/send.py | 9 +++++++-- app/main/views/templates.py | 4 ++-- app/templates/views/send.html | 27 +++++++++++++-------------- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 0bf0f9270..7fba4f432 100644 --- a/app/main/views/send.py +++ b/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 ) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index fceec018a..b79a5dc03 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -1,4 +1,4 @@ -import string +from string import ascii_uppercase from flask import request, render_template, redirect, url_for, flash, abort, session from flask_login import login_required @@ -127,7 +127,7 @@ def edit_service_template(service_id, template_id): 'views/templates/breaking-change.html', template_change=template_change, new_template=new_template, - column_headings=list(string.ascii_uppercase[:len(new_template.placeholders) + 1]), + 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), diff --git a/app/templates/views/send.html b/app/templates/views/send.html index 1a58de28b..9e1409d56 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -63,20 +63,19 @@

Example file

- - {% call(item, row_number) list_table( - example, - caption="Example", - caption_visible=False, - field_headings=['1'] + [ - '{}'.format(recipient_column)|safe - ] + template.placeholders_as_markup|list - ) %} - {{ index_field(row_number) }} - {% for column in item %} - {{ text_field(column) }} - {% endfor %} - {% endcall %} +
+ {% 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 %} +

Download this example

From 012e43549d32205e3abb5db88796ad13d4ec6d16 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 31 May 2016 14:20:35 +0100 Subject: [PATCH 3/5] Remove blue from column headings on send page While the blue should help reinforce the relationship between the placeholders and the column headings, it makes the column headings very busy visually, and less like column headings. Which make the relationship harder to see. I think. --- app/templates/views/send-test.html | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index 0731ae2c4..95f298c30 100644 --- a/app/templates/views/send-test.html +++ b/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=[ - '{}'.format(recipient_column)|safe - ] + template.placeholders_as_markup|list + field_headings=[recipient_column] + template.placeholders|list ) %} {% for column in item %} {% call field() %} From acfe87e7888e02f5d661be8bacfc63f468d7362c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 2 Jun 2016 10:41:54 +0100 Subject: [PATCH 4/5] Use `hasattr` instead of `getattr` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t need the value returned by `getattr`, so using `hasattr` is cleaner. --- app/main/views/templates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index b79a5dc03..01230f27a 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -113,7 +113,7 @@ 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 + subject = form.subject.data if hasattr(form, 'subject') else None new_template = Template({ 'name': form.name.data, 'content': form.template_content.data, From 53ec44a2d2f2a15e009f4e2809caf7ead1832551 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 2 Jun 2016 10:56:04 +0100 Subject: [PATCH 5/5] Remove commas from list of placeholders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Visually, the space between them is enough. Also the oxford comma thing didn’t actuall work :| --- app/templates/views/templates/breaking-change.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/templates/views/templates/breaking-change.html b/app/templates/views/templates/breaking-change.html index b6bfd33b3..33ef94231 100644 --- a/app/templates/views/templates/breaking-change.html +++ b/app/templates/views/templates/breaking-change.html @@ -3,10 +3,10 @@ {% 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) %} +{% macro list_of_placeholders(placeholders) %} {% for placeholder in placeholders %} - {% if loop.last and loop.length > 1 %}{% if oxford_comma %},{% endif %} and {% endif %} - (({{ placeholder }})){% if not loop.last and loop.length != 2 %},{% endif %} + {% if loop.last and loop.length > 1 %} and {% endif %} + (({{ placeholder }})) {% endfor %} {% endmacro %}