From c3b580b645e3b5860885d29d2de05d89ff1592ee Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 8 Nov 2016 14:04:18 +0000 Subject: [PATCH 01/10] Update utils --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 37f233393..55958c75d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -18,4 +18,4 @@ pytz==2016.4 git+https://github.com/alphagov/notifications-python-client.git@1.3.0#egg=notifications-python-client==1.3.0 -git+https://github.com/alphagov/notifications-utils.git@9.1.1#egg=notifications-utils==9.1.1 +git+https://github.com/alphagov/notifications-utils.git@9.3.0#egg=notifications-utils==9.3.0 From fb410496dca672ac429a99375d668c42f183bd00 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 8 Nov 2016 13:08:53 +0000 Subject: [PATCH 02/10] Add letter component Similar to the components we have for previewing email and text messages. Style stolen from PDF thumbnail at https://www.gov.uk/government/publications/honey-bees --- app/assets/stylesheets/components/letter.scss | 6 ++++++ app/templates/components/letter.html | 7 +++++++ 2 files changed, 13 insertions(+) create mode 100644 app/assets/stylesheets/components/letter.scss create mode 100644 app/templates/components/letter.html diff --git a/app/assets/stylesheets/components/letter.scss b/app/assets/stylesheets/components/letter.scss new file mode 100644 index 000000000..825968d46 --- /dev/null +++ b/app/assets/stylesheets/components/letter.scss @@ -0,0 +1,6 @@ +.letter { + font-family: Helvetica, Arial, sans-serif; + box-shadow: 0 2px 2px rgba($text-colour, 0.4); + outline: 5px solid rgba($text-colour, 0.1); + padding: 20px; +} diff --git a/app/templates/components/letter.html b/app/templates/components/letter.html new file mode 100644 index 000000000..8440ba31b --- /dev/null +++ b/app/templates/components/letter.html @@ -0,0 +1,7 @@ +{% macro letter( + body +) %} +
+ {{ body }} +
+{% endmacro %} From 4a0c860a04594b35eb3d7a3214b96343c4e9a4ec Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 10 Nov 2016 14:31:25 +0000 Subject: [PATCH 03/10] Make border around letter neater The drop shadow stolen from GOV.UK was a bit crude; this commit makes it a bit more refined. Also makes things line up a bit better. --- app/assets/stylesheets/components/letter.scss | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/components/letter.scss b/app/assets/stylesheets/components/letter.scss index 825968d46..009c53286 100644 --- a/app/assets/stylesheets/components/letter.scss +++ b/app/assets/stylesheets/components/letter.scss @@ -1,6 +1,13 @@ +$outline-width: 5px; + .letter { font-family: Helvetica, Arial, sans-serif; - box-shadow: 0 2px 2px rgba($text-colour, 0.4); - outline: 5px solid rgba($text-colour, 0.1); + box-shadow: + 1px 1px 0 0 $panel-colour, + 2px 2px 0 0 rgba($panel-colour, 0.5), + -1px 1px 0 0 $panel-colour, + -2px 2px 0 0 rgba($panel-colour, 0.5); + outline: $outline-width solid rgba($text-colour, 0.1); padding: 20px; + margin: $outline-width $outline-width $gutter; } From 88631a680c88df49672ee61ddb3054e4ba40a465 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 8 Nov 2016 13:12:07 +0000 Subject: [PATCH 04/10] Sprinkle `letter` throughout the app Let users create/edit/delete letter templates. Let them upload a CSV file or send a test against a letter template. Big assumption at the moment is that addresses only have one line, and therefore one column in the CSV file. --- app/assets/stylesheets/main.scss | 1 + app/main/forms.py | 4 +++ app/main/views/send.py | 12 ++++--- app/main/views/templates.py | 9 +++-- .../components/message-count-label.html | 6 ++++ app/templates/views/check.html | 5 ++- app/templates/views/edit-letter-template.html | 36 +++++++++++++++++++ app/templates/views/send-test.html | 3 ++ app/templates/views/send.html | 3 ++ app/templates/views/templates/_template.html | 3 ++ app/templates/views/templates/template.html | 4 ++- tests/app/main/views/test_letters.py | 23 ++++++++++-- 12 files changed, 97 insertions(+), 12 deletions(-) create mode 100644 app/templates/views/edit-letter-template.html diff --git a/app/assets/stylesheets/main.scss b/app/assets/stylesheets/main.scss index 830a68391..f5a0e4d4f 100644 --- a/app/assets/stylesheets/main.scss +++ b/app/assets/stylesheets/main.scss @@ -58,6 +58,7 @@ $path: '/static/images/'; @import 'components/research-mode'; @import 'components/tick-cross'; @import 'components/list-entry'; +@import 'components/letter'; @import 'views/job'; @import 'views/edit-template'; diff --git a/app/main/forms.py b/app/main/forms.py index 95afc47c6..57599ee78 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -276,6 +276,10 @@ class EmailTemplateForm(SMSTemplateForm): validators=[DataRequired(message="Can’t be empty")]) +class LetterTemplateForm(EmailTemplateForm): + pass + + class ForgotPasswordForm(Form): email_address = email_address(gov_user=False) diff --git a/app/main/views/send.py b/app/main/views/send.py index 90f423d13..d79fbc194 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -33,7 +33,8 @@ from app.utils import user_has_permissions, get_errors_for_csv, Spreadsheet, get def get_page_headings(template_type): return { 'email': 'Email templates', - 'sms': 'Text message templates' + 'sms': 'Text message templates', + 'letter': 'Letter templates' }[template_type] @@ -52,7 +53,8 @@ def get_example_csv_rows(template, use_example_as_example=True, submitted_fields 'email': 'test@example.com' if use_example_as_example else current_user.email_address, 'sms': '07700 900321' if use_example_as_example else validate_and_format_phone_number( current_user.mobile_number, human_readable=True - ) + ), + 'letter': current_user.name }[template.template_type] ] + get_example_csv_fields(template.placeholders, use_example_as_example, submitted_fields) @@ -66,8 +68,10 @@ def get_example_csv_rows(template, use_example_as_example=True, submitted_fields 'manage_api_keys', admin_override=True, any_=True) def choose_template(service_id, template_type): - if template_type not in ['email', 'sms']: + if template_type not in ['email', 'sms', 'letter']: abort(404) + if not current_service['can_send_letters'] and template_type == 'letter': + abort(403) return render_template( 'views/templates/choose.html', templates=[ @@ -233,7 +237,7 @@ def check_messages(service_id, template_type, upload_id): max_initial_rows_shown=50, max_errors_shown=50, whitelist=itertools.chain.from_iterable( - [user.mobile_number, user.email_address] for user in users + [user.name, user.mobile_number, user.email_address] for user in users ) if current_service['restricted'] else None, remaining_messages=remaining_messages ) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index d835fd5af..65886896a 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -11,14 +11,15 @@ 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.forms import SMSTemplateForm, EmailTemplateForm, LetterTemplateForm from app.main.views.send import get_example_csv_rows from app import service_api_client, current_service, template_statistics_client form_objects = { 'email': EmailTemplateForm, - 'sms': SMSTemplateForm + 'sms': SMSTemplateForm, + 'letter': LetterTemplateForm } page_headings = { @@ -74,8 +75,10 @@ def view_template_version(service_id, template_id, version): @login_required @user_has_permissions('manage_templates', admin_override=True) def add_service_template(service_id, template_type): - if template_type not in ['sms', 'email']: + if template_type not in ['sms', 'email', 'letter']: abort(404) + if not current_service['can_send_letters'] and template_type == 'letter': + abort(403) form = form_objects[template_type]() if form.validate_on_submit(): diff --git a/app/templates/components/message-count-label.html b/app/templates/components/message-count-label.html index a4d59afed..23a0a3ee6 100644 --- a/app/templates/components/message-count-label.html +++ b/app/templates/components/message-count-label.html @@ -11,5 +11,11 @@ {%- else -%} emails {%- endif -%} + {%- elif template_type == 'letter' -%} + {%- if count == 1 -%} + letter + {%- else -%} + letters + {%- endif -%} {%- endif %} {{ suffix }} {%- endmacro %} diff --git a/app/templates/views/check.html b/app/templates/views/check.html index a942579e9..3146d32e0 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -3,6 +3,7 @@ {% from "components/email-message.html" import email_message %} {% from "components/radios.html" import radio_select %} {% from "components/sms-message.html" import sms_message %} +{% from "components/letter.html" import letter %} {% from "components/table.html" import list_table, field, text_field, index_field, hidden_field_heading %} {% from "components/file-upload.html" import file_upload %} {% from "components/page-footer.html" import page_footer %} @@ -148,7 +149,9 @@ )}} - {% endif %} + {% elif 'letter' == template.template_type %} + {{ letter(template.formatted_as_markup if errors else template.replaced|safe) }} + {% endif %} {% if errors %} {% if request.args.from_test %} diff --git a/app/templates/views/edit-letter-template.html b/app/templates/views/edit-letter-template.html new file mode 100644 index 000000000..f53a75a94 --- /dev/null +++ b/app/templates/views/edit-letter-template.html @@ -0,0 +1,36 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} + +{% block page_title %} + {{ heading_action }} letter template – GOV.UK Notify +{% endblock %} + +{% block maincolumn_content %} + +

+ {{ heading_action }} letter template +

+ +
+
+
+ {{ textbox(form.name, width='1-1', hint='Your recipients won’t see this', rows=10) }} + {{ textbox(form.subject, width='1-1', highlight_tags=True, rows=2) }} +
+
+ {{ textbox(form.template_content, highlight_tags=True, width='1-1', rows=8) }} + {{ page_footer( + 'Save', + delete_link=url_for('.delete_service_template', service_id=current_service.id, template_id=template_id) if template_id or None, + delete_link_text='Delete this template' + ) }} +
+ +
+
+ +{% endblock %} diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index 498eaf8da..5a6c67643 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -1,6 +1,7 @@ {% extends "withnav_template.html" %} {% from "components/sms-message.html" import sms_message %} {% from "components/email-message.html" import email_message %} +{% from "components/letter.html" import letter %} {% from "components/page-footer.html" import page_footer %} {% from "components/file-upload.html" import file_upload %} {% from "components/table.html" import list_table, field, text_field, index_field, index_field_heading %} @@ -38,6 +39,8 @@ recipient='', show_placeholder_for_recipient=True ) }} + {% elif 'letter' == template.template_type %} + {{ letter(template.formatted_as_markup) }} {% endif %}
diff --git a/app/templates/views/send.html b/app/templates/views/send.html index 87309f156..398ea7eb6 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -1,6 +1,7 @@ {% extends "withnav_template.html" %} {% from "components/sms-message.html" import sms_message %} {% from "components/email-message.html" import email_message %} +{% from "components/letter.html" import letter %} {% from "components/page-footer.html" import page_footer %} {% from "components/file-upload.html" import file_upload %} {% from "components/table.html" import list_table, text_field, index_field, index_field_heading %} @@ -30,6 +31,8 @@ recipient='', show_placeholder_for_recipient=True ) }} + {% elif 'letter' == template.template_type %} + {{ letter(template.formatted_as_markup) }} {% endif %}
diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index 50c01ef71..0eeafe408 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -1,5 +1,6 @@ {% from "components/email-message.html" import email_message %} {% from "components/sms-message.html" import sms_message %} +{% from "components/letter.html" import letter %}
{% if 'email' == template.template_type %} @@ -14,6 +15,8 @@ template.formatted_as_markup, id=template.id ) }} + {% elif 'letter' == template.template_type %} + {{ letter(template.formatted_as_markup) }} {% endif %}
diff --git a/app/templates/views/templates/template.html b/app/templates/views/templates/template.html index 3f73a33ae..f75443f6e 100644 --- a/app/templates/views/templates/template.html +++ b/app/templates/views/templates/template.html @@ -21,7 +21,9 @@ {{ page_footer( secondary_link=url_for('.choose_template', service_id=current_service.id, template_type=template.template_type), - secondary_link_text='All {} templates'.format('email' if 'email' == template.template_type else 'text message') + secondary_link_text='All {} templates'.format( + 'email' if 'email' == template.template_type else 'text message' if 'sms' == template.template_type else 'letter' + ) ) }} {% endblock %} diff --git a/tests/app/main/views/test_letters.py b/tests/app/main/views/test_letters.py index 0051d3df3..cf253dd52 100644 --- a/tests/app/main/views/test_letters.py +++ b/tests/app/main/views/test_letters.py @@ -1,34 +1,51 @@ import pytest from flask import url_for +from functools import partial from tests import service_json +letters_urls = [ + partial(url_for, 'main.choose_template', template_type='letter'), + partial(url_for, 'main.add_service_template', template_type='letter'), +] + +@pytest.mark.parametrize('url', letters_urls) @pytest.mark.parametrize('can_send_letters, response_code', [ (True, 200), (False, 403) ]) -def test_letters_access_restricted(logged_in_client, mocker, can_send_letters, response_code): +def test_letters_access_restricted( + logged_in_client, + mocker, + can_send_letters, + response_code, + mock_get_service_templates, + url +): service = service_json(can_send_letters=can_send_letters) mocker.patch('app.service_api_client.get_service', return_value={"data": service}) - response = logged_in_client.get(url_for('main.letters', service_id=service['id'])) + response = logged_in_client.get(url(service_id=service['id'])) assert response.status_code == response_code +@pytest.mark.parametrize('url', letters_urls) def test_letters_lets_in_without_permission( client, mocker, mock_login, mock_has_permissions, api_user_active, + mock_get_service_templates, + url ): service = service_json(can_send_letters=True) mocker.patch('app.service_api_client.get_service', return_value={"data": service}) client.login(api_user_active) - response = client.get(url_for('main.letters', service_id=service['id'])) + response = client.get(url(service_id=service['id'])) assert api_user_active.permissions == {} assert response.status_code == 200 From 0c5a224fef736c6c1bab793fb0d0c25f1df582c6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 8 Nov 2016 13:56:02 +0000 Subject: [PATCH 05/10] Add letter-specific formatting advice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Letters can’t have callouts. --- .../guidance-formatting-letters.html | 19 +++++++++++++++++++ app/templates/views/edit-letter-template.html | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 app/templates/partials/templates/guidance-formatting-letters.html diff --git a/app/templates/partials/templates/guidance-formatting-letters.html b/app/templates/partials/templates/guidance-formatting-letters.html new file mode 100644 index 000000000..77d8d6be4 --- /dev/null +++ b/app/templates/partials/templates/guidance-formatting-letters.html @@ -0,0 +1,19 @@ +

Formatting

+

+ To put a title in your template, use a hash: +

+
+

+ # This is a title +

+
+

+ To make bullet points, use asterisks: +

+
+

+ * point 1
+ * point 2
+ * point 3
+

+
diff --git a/app/templates/views/edit-letter-template.html b/app/templates/views/edit-letter-template.html index f53a75a94..1dc29f6fb 100644 --- a/app/templates/views/edit-letter-template.html +++ b/app/templates/views/edit-letter-template.html @@ -27,7 +27,7 @@ ) }}
From 70eec8fe7316cd32702b2e3d0216e10285ca7165 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 8 Nov 2016 13:10:38 +0000 Subject: [PATCH 06/10] =?UTF-8?q?Don=E2=80=99t=20let=20people=20actually?= =?UTF-8?q?=20start=20a=20letter=20job?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Who knows what would happen if a job with a letter template actually got into the database. `403`ing the page is a quick and dirty hack to stop this from happening. --- app/main/views/send.py | 8 ++++++++ tests/app/main/views/test_send.py | 27 +++++++++++++++++++++++++++ tests/conftest.py | 15 +++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/app/main/views/send.py b/app/main/views/send.py index d79fbc194..b240512ed 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -310,6 +310,14 @@ def start_job(service_id, upload_id): session.pop('upload_data') + template = service_api_client.get_service_template( + service_id, + upload_data.get('template_id') + )['data'] + + if template['template_type'] == 'letter': + abort(403) + job_api_client.create_job( upload_id, service_id, diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 5c7736972..f3a339cae 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -405,6 +405,33 @@ def test_create_job_should_call_api( ) +def test_cant_start_letters_job( + app_, + client, + service_one, + mock_get_service, + active_user_with_permissions, + mock_create_job, + mock_get_service_letter_template, + mocker, + fake_uuid +): + client.login(active_user_with_permissions, mocker, service_one) + with client.session_transaction() as session: + session['upload_data'] = { + 'original_file_name': 'example.csv', + 'template_id': fake_uuid, + 'notification_count': 123, + 'valid': True + } + response = client.post( + url_for('main.start_job', service_id=fake_uuid, upload_id=fake_uuid), + data={} + ) + assert response.status_code == 403 + mock_create_job.assert_not_called() + + def test_check_messages_should_revalidate_file_when_uploading_file( app_, service_one, diff --git a/tests/conftest.py b/tests/conftest.py index d526b06e6..e9f7cc8f9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -335,6 +335,21 @@ def mock_get_service_email_template(mocker): 'app.service_api_client.get_service_template', side_effect=_create) +@pytest.fixture(scope='function') +def mock_get_service_letter_template(mocker): + def _create(service_id, template_id): + template = template_json( + service_id, + template_id, + "Two week reminder", + "letter", + "Your vehicle tax is about to expire", "Subject") + return {'data': template} + + return mocker.patch( + 'app.service_api_client.get_service_template', side_effect=_create) + + @pytest.fixture(scope='function') def mock_create_service_template(mocker, fake_uuid): def _create(name, type_, content, service, subject=None): From 5772fd34fee00366755e0fdc71001c6620dccaa3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 8 Nov 2016 13:09:41 +0000 Subject: [PATCH 07/10] Remove placeholder letters page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 👋 emoji --- app/main/__init__.py | 3 +-- app/main/views/letters.py | 14 -------------- app/templates/main_nav.html | 2 +- app/templates/views/letters.html | 19 ------------------- 4 files changed, 2 insertions(+), 36 deletions(-) delete mode 100644 app/main/views/letters.py delete mode 100644 app/templates/views/letters.html diff --git a/app/main/__init__.py b/app/main/__init__.py index de243703f..b70ccb942 100644 --- a/app/main/__init__.py +++ b/app/main/__init__.py @@ -26,6 +26,5 @@ from app.main.views import ( invites, feedback, providers, - platform_admin, - letters + platform_admin ) diff --git a/app/main/views/letters.py b/app/main/views/letters.py deleted file mode 100644 index bf268a12e..000000000 --- a/app/main/views/letters.py +++ /dev/null @@ -1,14 +0,0 @@ -from flask import render_template, abort -from flask_login import login_required - -from app import current_service -from app.main import main -from app.utils import user_has_permissions - - -@main.route("/services//letters") -@login_required -def letters(service_id): - if not current_service['can_send_letters']: - abort(403) - return render_template('views/letters.html') diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 01711286a..0ef4ddc35 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -51,7 +51,7 @@
  • Email templates
  • Text message templates
    • {% if current_service.can_send_letters %} -
  • Letter templates
    • +
  • Letter templates
    • {% endif %} {% endif %} {% if current_user.has_permissions(['manage_users', 'manage_settings'], admin_override=True) %} diff --git a/app/templates/views/letters.html b/app/templates/views/letters.html deleted file mode 100644 index 3a5fdf16f..000000000 --- a/app/templates/views/letters.html +++ /dev/null @@ -1,19 +0,0 @@ -{% extends "withnav_template.html" %} -{% from "components/sms-message.html" import sms_message %} -{% from "components/email-message.html" import email_message %} -{% from "components/page-footer.html" import page_footer %} -{% from "components/file-upload.html" import file_upload %} -{% from "components/table.html" import list_table, text_field, index_field, index_field_heading %} - -{% block page_title %} - Upload recipients – GOV.UK Notify -{% endblock %} - -{% block maincolumn_content %} - -

    Letters 📩📨💌📮📜📑👌👌👌

    -

    - [insert content here] -

    - -{% endblock %} From 69ad5fe156293528a8929d70c05a1b6ea283a67d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 10 Nov 2016 13:39:05 +0000 Subject: [PATCH 08/10] Handle multi-column recipients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements https://github.com/alphagov/notifications-utils/pull/81 Handles addresses as multiple columns: - in ‘Send yourself a test’ - in example CSV files - in validating that a CSV file has recipients (eg at least an ‘address line 1’ and ‘postcode’ column) - when showing the contents of a CSV file As few UI changes as possible, once we have the thing working end-to-end we can think about how the UI might need to work differently. --- app/main/views/send.py | 42 ++++++++++--------- app/main/views/templates.py | 4 +- .../partials/check/too-many-messages.html | 22 ++++++++-- app/templates/views/check.html | 6 ++- app/templates/views/send-test.html | 4 +- requirements.txt | 2 +- 6 files changed, 50 insertions(+), 30 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index b240512ed..f263126d1 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -17,8 +17,9 @@ from flask import ( ) from flask_login import login_required, current_user +from notifications_utils.columns import Columns from notifications_utils.template import Template -from notifications_utils.recipients import RecipientCSV, first_column_heading, validate_and_format_phone_number +from notifications_utils.recipients import RecipientCSV, first_column_headings, validate_and_format_phone_number from app.main import main from app.main.forms import CsvUploadForm, ChooseTimeForm, get_next_days_until, get_furthest_possible_scheduled_time @@ -48,15 +49,16 @@ def get_example_csv_fields(column_headers, use_example_as_example, submitted_fie def get_example_csv_rows(template, use_example_as_example=True, submitted_fields=False): - return [ - { - 'email': 'test@example.com' if use_example_as_example else current_user.email_address, - 'sms': '07700 900321' if use_example_as_example else validate_and_format_phone_number( - current_user.mobile_number, human_readable=True - ), - 'letter': current_user.name - }[template.template_type] - ] + get_example_csv_fields(template.placeholders, use_example_as_example, submitted_fields) + return { + 'email': ['test@example.com'] if use_example_as_example else [current_user.email_address], + 'sms': ['07700 900321'] if use_example_as_example else [validate_and_format_phone_number( + current_user.mobile_number, human_readable=True + )], + 'letter': [current_user.name] + [ + (submitted_fields or {}).get(key, 'example' if use_example_as_example else key) + for key in first_column_headings['letter'][1:] + ] + }[template.template_type] + get_example_csv_fields(template.placeholders, use_example_as_example, submitted_fields) @main.route("/services//send/", methods=['GET']) @@ -118,14 +120,13 @@ def send_messages(service_id, template_id): form.file.data.filename )) + column_headings = first_column_headings[template.template_type] + list(template.placeholders) + return render_template( 'views/send.html', template=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) - ], + column_headings=list(ascii_uppercase[:len(column_headings)]), + example=[column_headings, get_example_csv_rows(template)], form=form ) @@ -136,7 +137,7 @@ def send_messages(service_id, template_id): def get_example_csv(service_id, template_id): template = Template(service_api_client.get_service_template(service_id, template_id)['data']) return Spreadsheet.from_rows([ - [first_column_heading[template.template_type]] + list(template.placeholders), + first_column_headings[template.template_type] + list(template.placeholders), get_example_csv_rows(template) ]).as_csv_data, 200, { 'Content-Type': 'text/csv; charset=utf-8', @@ -163,7 +164,7 @@ def send_test(service_id, template_id): { 'file_name': file_name, 'data': Spreadsheet.from_rows([ - [first_column_heading[template.template_type]] + list(template.placeholders), + first_column_headings[template.template_type] + list(template.placeholders), get_example_csv_rows(template, use_example_as_example=False, submitted_fields=request.form) ]).as_csv_data }, @@ -185,7 +186,7 @@ def send_test(service_id, template_id): return render_template( 'views/send-test.html', template=template, - recipient_column=first_column_heading[template.template_type], + recipient_columns=first_column_headings[template.template_type], example=[get_example_csv_rows(template, use_example_as_example=False)], help=get_help_argument() ) @@ -259,7 +260,10 @@ def check_messages(service_id, template_type, upload_id): with suppress(StopIteration): template.values = next(recipients.rows) - first_recipient = template.values.get(recipients.recipient_column_header, '') + first_recipient = template.values.get( + Columns.make_key(recipients.recipient_column_headers[0]), + '' + ) session['upload_data']['notification_count'] = len(list(recipients.rows)) session['upload_data']['valid'] = not recipients.has_errors diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 65886896a..a970a4b21 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -6,7 +6,7 @@ from flask_login import login_required from dateutil.parser import parse from notifications_utils.template import Template -from notifications_utils.recipients import first_column_heading +from notifications_utils.recipients import first_column_headings from notifications_python_client.errors import HTTPError from app.main import main @@ -136,7 +136,7 @@ def edit_service_template(service_id, template_id): 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), + first_column_headings[new_template.template_type] + list(new_template.placeholders), get_example_csv_rows(new_template), get_example_csv_rows(new_template) ], diff --git a/app/templates/partials/check/too-many-messages.html b/app/templates/partials/check/too-many-messages.html index b063640ee..761ca0801 100644 --- a/app/templates/partials/check/too-many-messages.html +++ b/app/templates/partials/check/too-many-messages.html @@ -15,10 +15,24 @@ You can still send {{ remaining_messages }} messages today, but {% endif %} ‘{{ original_file_name }}’ contains - {{ count_of_recipients }} {{ recipients.recipient_column_header }} - {%- if count_of_recipients != 1 -%} - {{ 'es' if 'email address' == recipients.recipient_column_header else 's' }} - {%- endif %}. + {{ count_of_recipients }} + {% if count_of_recipients == 1 -%} + {%- if template.template_type == 'email' -%} + email address + {%- elif template.template_type == 'sms' -%} + phone number + {%- elif template.template_type == 'letter' -%} + address + {%- endif -%} + {%- else -%} + {%- if template.template_type == 'email' -%} + email addresses + {%- elif template.template_type == 'sms' -%} + phone numbers + {%- elif template.template_type == 'letter' -%} + addresses + {%- endif -%} + {%- endif -%}.

    {% endcall %} diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 3146d32e0..5c3410a8f 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -31,7 +31,7 @@ {% endcall %} - {% elif not recipients.has_recipient_column %} + {% elif not recipients.has_recipient_columns %}
    {% call banner_wrapper(type='dangerous') %} @@ -193,7 +193,9 @@ {{ item['columns'][column].data if item['columns'][column].data != None }} {% endcall %} {% else %} - {{ text_field(item['columns'][column].data) }} + {% call field() %} + {{ item['columns'][column].data if item['columns'][column].data != None }} + {% endcall %} {% endif %} {% endfor %} {% if item['columns'].get(None) %} diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index 5a6c67643..0ce9d9a0c 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -46,8 +46,8 @@ {% call(item, row_number) list_table( example, - caption="Fill in the {}".format('field' if template.placeholders|length == 1 else 'fields'), - field_headings=[recipient_column] + template.placeholders|list + caption="Fill in the {}".format('field' if (recipient_columns + template.placeholders|list)|length == 2 else 'fields'), + field_headings=recipient_columns + template.placeholders|list ) %} {% for column in item %} {% call field() %} diff --git a/requirements.txt b/requirements.txt index 55958c75d..b85ce5dd4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -18,4 +18,4 @@ pytz==2016.4 git+https://github.com/alphagov/notifications-python-client.git@1.3.0#egg=notifications-python-client==1.3.0 -git+https://github.com/alphagov/notifications-utils.git@9.3.0#egg=notifications-utils==9.3.0 +git+https://github.com/alphagov/notifications-utils.git@10.0.0#egg=notifications-utils==10.0.0 From 34e01824fa01dbb8ac7c92cac6dd9b1642bdae66 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 10 Nov 2016 14:10:39 +0000 Subject: [PATCH 09/10] Add errors for missing address(es) --- app/utils.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/utils.py b/app/utils.py index 3da4547d3..3108fc40f 100644 --- a/app/utils.py +++ b/app/utils.py @@ -86,6 +86,11 @@ def get_errors_for_csv(recipients, template_type): errors.append("fix 1 email address") else: errors.append("fix {} email addresses".format(number_of_bad_recipients)) + elif 'letter' == template_type: + if 1 == number_of_bad_recipients: + errors.append("fix 1 address") + else: + errors.append("fix {} addresses".format(number_of_bad_recipients)) if recipients.rows_with_missing_data: number_of_rows_with_missing_data = len(list(recipients.rows_with_missing_data)) From a108165e4775b1e5abd51740752573d5d22c8c80 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 11 Nov 2016 09:51:20 +0000 Subject: [PATCH 10/10] Give more realistic example address data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Posit that examples of where you can put different parts of the address is more helpful than ‘example, example, example’. Also shows that you don’t have to fill all of the address columns. Spot the Easter egg 🎅 --- app/main/views/send.py | 17 ++++++++++++++--- app/templates/views/send-test.html | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index f263126d1..5e6f5b60c 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -54,13 +54,24 @@ def get_example_csv_rows(template, use_example_as_example=True, submitted_fields 'sms': ['07700 900321'] if use_example_as_example else [validate_and_format_phone_number( current_user.mobile_number, human_readable=True )], - 'letter': [current_user.name] + [ - (submitted_fields or {}).get(key, 'example' if use_example_as_example else key) - for key in first_column_headings['letter'][1:] + 'letter': [ + (submitted_fields or {}).get( + key, get_example_letter_address(key) if use_example_as_example else key + ) + for key in first_column_headings['letter'] ] }[template.template_type] + get_example_csv_fields(template.placeholders, use_example_as_example, submitted_fields) +def get_example_letter_address(key): + return { + 'address line 1': 'A. Name', + 'address line 2': '123 Example Street', + 'address line 3': 'Example town', + 'postcode': 'XM4 5HQ' + }.get(key, '') + + @main.route("/services//send/", methods=['GET']) @login_required @user_has_permissions('view_activity', diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index 0ce9d9a0c..4f8fa4980 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -51,7 +51,7 @@ ) %} {% for column in item %} {% call field() %} - {% if loop.index > 1 %} + {% if loop.index > 1 or template.template_type == 'letter' %} {% else %}