From a51d92c87a7f0d18f1223cefd111b06e47e4a902 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 Feb 2016 16:37:39 +0000 Subject: [PATCH 1/4] Consolidate page templates for emails + texts Use one page template for each of: - choosing a message template - adding recipients - checking an email message - looking at a job This commit consolidates the two templates into one, and adds logic to show the SMS message pattern or the email message pattern depending on the type of template. It also gives email templates a bit more width, because the body and the from address tend to be quite long. --- app/main/views/jobs.py | 5 ++- app/main/views/send.py | 19 +++++++--- app/templates/views/check-email.html | 18 ---------- .../views/{check-sms.html => check.html} | 28 +++++++++------ .../views/choose-email-template.html | 36 ------------------- ...sms-template.html => choose-template.html} | 17 ++++++--- app/templates/views/job.html | 21 +++++++---- app/templates/views/send.html | 25 +++++++------ tests/app/main/views/test_jobs.py | 1 + 9 files changed, 80 insertions(+), 90 deletions(-) delete mode 100644 app/templates/views/check-email.html rename app/templates/views/{check-sms.html => check.html} (74%) delete mode 100644 app/templates/views/choose-email-template.html rename app/templates/views/{choose-sms-template.html => choose-template.html} (69%) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index dd5e45e04..15c21e65c 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -13,6 +13,7 @@ from utils.template import Template from app import job_api_client from app.main import main from app.main.dao import templates_dao +from app.main.dao import services_dao now = time.strftime('%H:%M') @@ -37,6 +38,7 @@ def view_jobs(service_id): @main.route("/services//jobs/") @login_required def view_job(service_id, job_id): + service = services_dao.get_service_by_id_or_404(service_id) try: job = job_api_client.get_job(service_id, job_id)['data'] messages = [] @@ -58,7 +60,8 @@ def view_job(service_id, job_id): template=Template( templates_dao.get_service_template_or_404(service_id, job['template'])['data'] ), - service_id=service_id + service_id=service_id, + service=service ) except HTTPError as e: if e.status_code == 404: diff --git a/app/main/views/send.py b/app/main/views/send.py index e9dfeea44..67947df2b 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -28,11 +28,16 @@ from app.main.dao import services_dao from app import job_api_client from app.utils import validate_recipient, InvalidPhoneError, InvalidEmailError +page_headings = { + 'email': 'Send emails', + 'sms': 'Send text messages' +} + @main.route("/services//send/", methods=['GET']) def choose_template(service_id, template_type): - services_dao.get_service_by_id_or_404(service_id) + service = services_dao.get_service_by_id_or_404(service_id) if template_type not in ['email', 'sms']: abort(404) @@ -44,11 +49,14 @@ def choose_template(service_id, template_type): else: raise e return render_template( - 'views/choose-{}-template.html'.format(template_type), + 'views/choose-template.html', templates=[ Template(template) for template in templates_dao.get_service_templates(service_id)['data'] if template['template_type'] == template_type ], + template_type=template_type, + page_heading=page_headings[template_type], + service=service, has_jobs=len(jobs), service_id=service_id ) @@ -130,6 +138,7 @@ def check_messages(service_id, upload_id): upload_data = session['upload_data'] template_id = upload_data.get('template_id') + service = services_dao.get_service_by_id_or_404(service_id) if request.method == 'GET': contents = s3download(service_id, upload_id) @@ -144,14 +153,16 @@ def check_messages(service_id, upload_id): drop_values={'to'} ) return render_template( - 'views/check-sms.html', + 'views/check.html', upload_result=upload_result, template=template, + page_heading=page_headings[template.template_type], column_headers=['to'] + list( template.placeholders if upload_result['valid'] else template.placeholders_as_markup ), original_file_name=upload_data.get('original_file_name'), service_id=service_id, + service=service, form=CsvUploadForm() ) elif request.method == 'POST': @@ -193,7 +204,7 @@ def _get_rows(contents, raw_template): rows.append(row) try: validate_recipient( - row['to'], + row.get('to', ''), template_type=raw_template['template_type'] ) Template(raw_template, values=row, drop_values={'to'}).replaced diff --git a/app/templates/views/check-email.html b/app/templates/views/check-email.html deleted file mode 100644 index 3ca4830bf..000000000 --- a/app/templates/views/check-email.html +++ /dev/null @@ -1,18 +0,0 @@ -{% extends "withnav_template.html" %} - -{% block page_title %} - Send email – GOV.UK Notify -{% endblock %} - -{% block maincolumn_content %} - -

Send email

- -

This page will be where we check the email messages we're about to send

- -

- Send email messages -

- - -{% endblock %} diff --git a/app/templates/views/check-sms.html b/app/templates/views/check.html similarity index 74% rename from app/templates/views/check-sms.html rename to app/templates/views/check.html index 38c793639..48967ca7a 100644 --- a/app/templates/views/check-sms.html +++ b/app/templates/views/check.html @@ -1,4 +1,5 @@ {% extends "withnav_template.html" %} +{% from "components/email-message.html" import email_message %} {% from "components/sms-message.html" import sms_message %} {% from "components/table.html" import list_table, field %} {% from "components/placeholder.html" import placeholder %} @@ -6,7 +7,7 @@ {% from "components/page-footer.html" import page_footer %} {% block page_title %} - Check and confirm – GOV.UK Notify + {{ "Check and confirm" if upload_result.valid else page_heading }} – GOV.UK Notify {% endblock %} {% block maincolumn_content %} @@ -25,18 +26,23 @@ {% endif %}

- {{ "Check and confirm" if upload_result.valid else "Send text messages" }} + {{ "Check and confirm" if upload_result.valid else page_heading }}

-
-
- {% if template.missing_data or template.additional_data %} - {{ sms_message(template.formatted_as_markup)}} - {% else %} - {{ sms_message(template.replaced)}} - {% endif %} + {% if 'email' == template.template_type %} + {{ email_message( + template.subject, + template, + from_address='{}@notifications.service.gov.uk'.format(service.email_from), + from_name=service.name + )}} + {% elif 'sms' == template.template_type %} +
+
+ {{ sms_message(template)}} +
-
+ {% endif %} {% if upload_result.valid %}
@@ -53,7 +59,7 @@ caption=original_file_name, field_headings=column_headers ) %} - {% if item.to|valid_phone_number %} + {% if item.to or ''|valid_phone_number %} {% call field() %} {{ item.to }} {% endcall %} diff --git a/app/templates/views/choose-email-template.html b/app/templates/views/choose-email-template.html deleted file mode 100644 index 5452a5da8..000000000 --- a/app/templates/views/choose-email-template.html +++ /dev/null @@ -1,36 +0,0 @@ -{% extends "withnav_template.html" %} -{% from "components/email-message.html" import email_message %} -{% from "components/page-footer.html" import page_footer %} -{% from "components/textbox.html" import textbox %} - -{% block page_title %} - Send emails – GOV.UK Notify -{% endblock %} - -{% block maincolumn_content %} - -

Send emails

- - - - {% if templates %} -
- {% for template in templates %} -
- {{ email_message(template.subject, template.formatted_as_markup, name=template.name) }} -
-
-
- Edit template -
-
- {% endfor %} -
- {% endif %} - -

- Add a new template -

- -
-{% endblock %} diff --git a/app/templates/views/choose-sms-template.html b/app/templates/views/choose-template.html similarity index 69% rename from app/templates/views/choose-sms-template.html rename to app/templates/views/choose-template.html index 5508ca4fb..7fb87b827 100644 --- a/app/templates/views/choose-sms-template.html +++ b/app/templates/views/choose-template.html @@ -1,15 +1,16 @@ {% extends "withnav_template.html" %} +{% from "components/email-message.html" import email_message %} {% from "components/sms-message.html" import sms_message %} {% from "components/page-footer.html" import page_footer %} {% from "components/textbox.html" import textbox %} {% block page_title %} - Send text messages – GOV.UK Notify + {{ page_heading }} – GOV.UK Notify {% endblock %} {% block maincolumn_content %} -

Send text messages

+

{{ page_heading }}

@@ -26,7 +27,15 @@
{% for template in templates %}
- {{ sms_message(template.formatted_as_markup, name=template.name) }} + {% if 'email' == template_type %} + {{ email_message( + template.subject, + template.formatted_as_markup, + name=template.name + ) }} + {% elif 'sms' == template_type %} + {{ sms_message(template.formatted_as_markup, name=template.name) }} + {% endif %}
@@ -40,7 +49,7 @@ {% endif %}

- Add a new template + Add a new template

diff --git a/app/templates/views/job.html b/app/templates/views/job.html index 0c038141c..ae60fed13 100644 --- a/app/templates/views/job.html +++ b/app/templates/views/job.html @@ -14,13 +14,22 @@ {{ uploaded_file_name }} -
-
- {{ sms_message( - template.formatted_as_markup, - )}} + {% if 'sms' == template.template_type %} +
+
+ {{ sms_message( + template, + )}} +
-
+ {% elif 'email' == template.template_type %} + {{ email_message( + template.subject, + template, + from_address='{}@notifications.service.gov.uk'.format(service.email_from), + from_name=service.name + )}} + {% endif %}

Started {{ uploaded_file_time|format_datetime }} diff --git a/app/templates/views/send.html b/app/templates/views/send.html index 8aef83078..f98c819b1 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -13,18 +13,23 @@

Add recipients

+ {% if 'sms' == template.template_type %} +
+
+ {{ sms_message(template) }} +
+
+ {% elif 'email' == template.template_type %} + {{ email_message( + template.subject, + template, + from_address='{}@notifications.service.gov.uk'.format(service.email_from), + from_name=service.name + ) }} + {% endif %} +
- {% if 'sms' == template.template_type %} - {{ sms_message(template.formatted_as_markup) }} - {% elif 'email' == template.template_type %} - {{ email_message( - template.subject, - template.formatted_as_markup, - from_address='{}@notifications.service.gov.uk'.format(service.email_from), - from_name=service.name - ) }} - {% endif %} {{ banner( 'You can upload real data, but we’ll only send to your mobile number until you request to go live'.format( url_for('.service_request_to_go_live', service_id=service_id) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 98f99e514..c3147555f 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -27,6 +27,7 @@ def test_should_show_page_for_one_job(app_, mock_login, mock_get_user, mock_get_user_by_email, + mock_get_service, mock_get_service_template, job_data, mock_get_job): From 70ef2f5a6f554a392b6448b86981e9d5bef1e688 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 Feb 2016 17:16:02 +0000 Subject: [PATCH 2/4] Fix example CSV for email templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It should populate the CSV file with the user’s email address, not their phone number. --- app/main/views/send.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 67947df2b..dc52d55cf 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -105,7 +105,12 @@ def get_example_csv(service_id, template_id): output = io.StringIO() writer = csv.writer(output) writer.writerow(['to'] + placeholders) - writer.writerow([current_user.mobile_number] + ["test {}".format(header) for header in placeholders]) + writer.writerow([ + { + 'email': current_user.email_address, + 'sms': current_user.mobile_number + }[template['template_type']] + ] + ["test {}".format(header) for header in placeholders]) return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} From 2f91848a42b00c48037951cab094250ea4ca4a66 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 09:33:45 +0000 Subject: [PATCH 3/4] Prefix text messages with service name Brings in: https://github.com/alphagov/notifications-utils/pull/4 --- app/main/views/jobs.py | 3 ++- app/main/views/send.py | 11 ++++++++--- app/templates/views/check.html | 6 ++++-- app/templates/views/send.html | 4 ++-- requirements.txt | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 15c21e65c..9469fa810 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -58,7 +58,8 @@ def view_job(service_id, job_id): uploaded_file_name=job['original_file_name'], uploaded_file_time=job['created_at'], template=Template( - templates_dao.get_service_template_or_404(service_id, job['template'])['data'] + templates_dao.get_service_template_or_404(service_id, job['template'])['data'], + prefix=service['name'] ), service_id=service_id, service=service diff --git a/app/main/views/send.py b/app/main/views/send.py index dc52d55cf..cfdbd97d3 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -51,7 +51,10 @@ def choose_template(service_id, template_type): return render_template( 'views/choose-template.html', templates=[ - Template(template) for template in templates_dao.get_service_templates(service_id)['data'] + Template( + template, + prefix=service['name'] + ) for template in templates_dao.get_service_templates(service_id)['data'] if template['template_type'] == template_type ], template_type=template_type, @@ -84,7 +87,8 @@ def send_messages(service_id, template_id): service = services_dao.get_service_by_id_or_404(service_id) template = Template( - templates_dao.get_service_template_or_404(service_id, template_id)['data'] + templates_dao.get_service_template_or_404(service_id, template_id)['data'], + prefix=service['name'] ) return render_template( @@ -155,7 +159,8 @@ def check_messages(service_id, upload_id): template = Template( raw_template, values=upload_result['rows'][0] if upload_result['valid'] else {}, - drop_values={'to'} + drop_values={'to'}, + prefix=service['name'] ) return render_template( 'views/check.html', diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 48967ca7a..315bc32eb 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -32,14 +32,16 @@ {% if 'email' == template.template_type %} {{ email_message( template.subject, - template, + template.replaced if upload_result.valid else template.formatted_as_markup, from_address='{}@notifications.service.gov.uk'.format(service.email_from), from_name=service.name )}} {% elif 'sms' == template.template_type %}
- {{ sms_message(template)}} + {{ sms_message( + template.replaced if upload_result.valid else template.formatted_as_markup + )}}
{% endif %} diff --git a/app/templates/views/send.html b/app/templates/views/send.html index f98c819b1..25eda7364 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -16,13 +16,13 @@ {% if 'sms' == template.template_type %}
- {{ sms_message(template) }} + {{ sms_message(template.formatted_as_markup) }}
{% elif 'email' == template.template_type %} {{ email_message( template.subject, - template, + template.formatted_as_markup, from_address='{}@notifications.service.gov.uk'.format(service.email_from), from_name=service.name ) }} diff --git a/requirements.txt b/requirements.txt index cbb153dd6..2c746a497 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,4 +14,4 @@ Pygments==2.0.2 git+https://github.com/alphagov/notifications-python-client.git@0.2.8#egg=notifications-python-client==0.2.8 -git+https://github.com/alphagov/notifications-utils.git@0.1.1#egg=notifications-utils==0.1.1 +git+https://github.com/alphagov/notifications-utils.git@0.1.2#egg=notifications-utils==0.1.2 From 04e7aac99c8f4e18e32ff914cc7172de2c728808 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 09:35:07 +0000 Subject: [PATCH 4/4] Column headers on the check page should always be blue --- app/main/views/send.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index cfdbd97d3..07f50b5e2 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -167,9 +167,7 @@ def check_messages(service_id, upload_id): upload_result=upload_result, template=template, page_heading=page_headings[template.template_type], - column_headers=['to'] + list( - template.placeholders if upload_result['valid'] else template.placeholders_as_markup - ), + column_headers=['to'] + list(template.placeholders_as_markup), original_file_name=upload_data.get('original_file_name'), service_id=service_id, service=service,