From ddeed88210052644f77c3f138ca1574dd7349e19 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jul 2017 08:59:49 +0100 Subject: [PATCH 1/7] Refactor check page into 3 separate templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check page is a very complex Jinja template. It needs breaking up to make it easier to understand what’s going on. I think there are three di --- app/main/views/send.py | 14 +- .../{check.html => check/column-errors.html} | 0 app/templates/views/check/ok.html | 235 ++++++++++++++++++ app/templates/views/check/row-errors.html | 235 ++++++++++++++++++ 4 files changed, 480 insertions(+), 4 deletions(-) rename app/templates/views/{check.html => check/column-errors.html} (100%) create mode 100644 app/templates/views/check/ok.html create mode 100644 app/templates/views/check/row-errors.html diff --git a/app/main/views/send.py b/app/main/views/send.py index 8b72c3b9a..9cdd56419 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -447,10 +447,16 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') def check_messages(service_id, template_type, upload_id): - return render_template( - 'views/check.html', - **_check_messages(service_id, template_type, upload_id) - ) + + data = _check_messages(service_id, template_type, upload_id) + + if data['row_errors']: + return render_template('views/check/row-errors.html', **data) + + if data['errors']: + return render_template('views/check/column-errors.html', **data) + + return render_template('views/check/ok.html', **data) @main.route("/services///check/.", methods=['GET']) diff --git a/app/templates/views/check.html b/app/templates/views/check/column-errors.html similarity index 100% rename from app/templates/views/check.html rename to app/templates/views/check/column-errors.html diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html new file mode 100644 index 000000000..ebb207edd --- /dev/null +++ b/app/templates/views/check/ok.html @@ -0,0 +1,235 @@ +{% extends "withnav_template.html" %} +{% from "components/banner.html" import banner_wrapper %} +{% from "components/radios.html" import radio_select %} +{% 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 %} +{% from "components/message-count-label.html" import message_count_label %} + +{% set file_contents_header_id = 'file-preview' %} +{% macro skip_to_file_contents() %} + +{% endmacro %} + +{% block service_page_title %} + {{ "Error" if errors else "Preview of {}".format(template.name) }} +{% endblock %} + +{% block maincolumn_content %} + + {% if recipients.too_many_rows %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ Your file has too many rows +

+

+ Notify can process up to + {{ "{:,}".format(recipients.max_rows) }} rows at once. Your + file has {{ "{:,}".format(recipients|length) }} rows. +

+ {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif not count_of_recipients %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ Your file is missing some rows +

+

+ It needs at least one row of data, and {{ recipients.missing_column_headers | sort() | formatted_list( + prefix='a column called', + prefix_plural='columns called' + ) }}. +

+ {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif not recipients.has_recipient_columns %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ Your file needs {{ recipients.recipient_column_headers | formatted_list( + prefix='a column called', + prefix_plural='columns called' + ) }} +

+

+ Right now it has {{ recipients.column_headers | formatted_list( + prefix='one column, called ', + prefix_plural='columns called ' + ) }}. +

+ {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif recipients.missing_column_headers %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ The columns in your file need to match the double brackets in + your template +

+

+ Your file is missing {{ recipients.missing_column_headers | formatted_list( + conjunction='and', + prefix='a column called ', + prefix_plural='columns called ' + ) }}. +

+ {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif row_errors %} + +
+ {% call banner_wrapper(type='dangerous') %} + {% if row_errors|length == 1 %} +

+ There is a problem with your data +

+

+ You need to {{ row_errors[0] }} +

+ {% else %} +

+ There are some problems with your data +

+

+ You need to: +

+
    + {% for error in row_errors %} +
  • {{ error }}
    • + {% endfor %} +
+ {% endif %} + {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif not recipients.allowed_to_send_to %} + {% with + count_of_recipients=count_of_recipients, + template_type_label=recipients.recipient_column_headers[0] + %} + {% include "partials/check/not-allowed-to-send-to.html" %} + {% endwith %} + {% elif recipients.more_rows_than_can_send %} + {% include "partials/check/too-many-messages.html" %} + {% else %} + +

+ Preview of {{ template.name }} +

+ {{ skip_to_file_contents() }} + + {% endif %} + + {% if not errors %} + {{ template|string }} + {% endif %} + +
+ {% if errors %} + {% if request.args.from_test %} + Back + {% else %} + {{file_upload(form.file, button_text='Re-upload your file')}} + {% endif %} + {% else %} +
+ + + {% if choose_time_form and template.template_type != 'letter' %} + {{ radio_select( + choose_time_form.scheduled_for, + wrapping_class='bottom-gutter-2-3' + ) }} + {% endif %} + {% if template.template_type != 'letter' or not request.args.from_test %} + + {% else %} + Download as a printable PDF + {% endif %} + Back +
+ {% endif %} +
+ + {% if not request.args.from_test %} + +

{{ original_file_name }}

+ + {% call(item, row_number) list_table( + recipients.initial_annotated_rows_with_errors if row_errors and not recipients.missing_column_headers else recipients.initial_annotated_rows, + caption=original_file_name, + caption_visible=False, + field_headings=[ + ''.format("table-field-invisible-error" if errors else "")|safe + ] + recipients.column_headers + ) %} + {% call index_field() %} + + {{ item.index + 2 }} + + {% endcall %} + {% for column in recipients.column_headers %} + {% if item['columns'][column].error and not recipients.missing_column_headers %} + {% call field() %} + + {{ item['columns'][column].error }} + {{ item['columns'][column].data if item['columns'][column].data != None }} + + {% endcall %} + {% elif item['columns'][column].ignore %} + {{ text_field(item['columns'][column].data or '', status='default') }} + {% else %} + {{ text_field(item['columns'][column].data or '') }} + {% endif %} + {% endfor %} + {% if item['columns'].get(None) %} + {% for column in item['columns'][None].data %} + {{ text_field(column, status='default') }} + {% endfor %} + {% endif %} + {% endcall %} + + {% endif %} + + + {% if recipients.too_many_rows %} +

+ Can’t show the contents of this file +

+ {% elif count_of_displayed_recipients < count_of_recipients %} +

+ {% if row_errors and not recipients.missing_column_headers %} + Only showing the first {{ count_of_displayed_recipients }} rows with errors + {% else %} + Only showing the first {{ count_of_displayed_recipients }} rows + {% endif %} +

+ {% elif row_errors and not recipients.missing_column_headers %} +

+ Only showing rows with errors +

+ {% endif %} + + {% if errors %} +

Preview of {{ template.name }}

+ {{ template|string }} + {% endif %} + +{% endblock %} diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html new file mode 100644 index 000000000..ebb207edd --- /dev/null +++ b/app/templates/views/check/row-errors.html @@ -0,0 +1,235 @@ +{% extends "withnav_template.html" %} +{% from "components/banner.html" import banner_wrapper %} +{% from "components/radios.html" import radio_select %} +{% 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 %} +{% from "components/message-count-label.html" import message_count_label %} + +{% set file_contents_header_id = 'file-preview' %} +{% macro skip_to_file_contents() %} + +{% endmacro %} + +{% block service_page_title %} + {{ "Error" if errors else "Preview of {}".format(template.name) }} +{% endblock %} + +{% block maincolumn_content %} + + {% if recipients.too_many_rows %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ Your file has too many rows +

+

+ Notify can process up to + {{ "{:,}".format(recipients.max_rows) }} rows at once. Your + file has {{ "{:,}".format(recipients|length) }} rows. +

+ {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif not count_of_recipients %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ Your file is missing some rows +

+

+ It needs at least one row of data, and {{ recipients.missing_column_headers | sort() | formatted_list( + prefix='a column called', + prefix_plural='columns called' + ) }}. +

+ {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif not recipients.has_recipient_columns %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ Your file needs {{ recipients.recipient_column_headers | formatted_list( + prefix='a column called', + prefix_plural='columns called' + ) }} +

+

+ Right now it has {{ recipients.column_headers | formatted_list( + prefix='one column, called ', + prefix_plural='columns called ' + ) }}. +

+ {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif recipients.missing_column_headers %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ The columns in your file need to match the double brackets in + your template +

+

+ Your file is missing {{ recipients.missing_column_headers | formatted_list( + conjunction='and', + prefix='a column called ', + prefix_plural='columns called ' + ) }}. +

+ {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif row_errors %} + +
+ {% call banner_wrapper(type='dangerous') %} + {% if row_errors|length == 1 %} +

+ There is a problem with your data +

+

+ You need to {{ row_errors[0] }} +

+ {% else %} +

+ There are some problems with your data +

+

+ You need to: +

+
    + {% for error in row_errors %} +
  • {{ error }}
    • + {% endfor %} +
+ {% endif %} + {{ skip_to_file_contents() }} + {% endcall %} +
+ + {% elif not recipients.allowed_to_send_to %} + {% with + count_of_recipients=count_of_recipients, + template_type_label=recipients.recipient_column_headers[0] + %} + {% include "partials/check/not-allowed-to-send-to.html" %} + {% endwith %} + {% elif recipients.more_rows_than_can_send %} + {% include "partials/check/too-many-messages.html" %} + {% else %} + +

+ Preview of {{ template.name }} +

+ {{ skip_to_file_contents() }} + + {% endif %} + + {% if not errors %} + {{ template|string }} + {% endif %} + +
+ {% if errors %} + {% if request.args.from_test %} + Back + {% else %} + {{file_upload(form.file, button_text='Re-upload your file')}} + {% endif %} + {% else %} +
+ + + {% if choose_time_form and template.template_type != 'letter' %} + {{ radio_select( + choose_time_form.scheduled_for, + wrapping_class='bottom-gutter-2-3' + ) }} + {% endif %} + {% if template.template_type != 'letter' or not request.args.from_test %} + + {% else %} + Download as a printable PDF + {% endif %} + Back +
+ {% endif %} +
+ + {% if not request.args.from_test %} + +

{{ original_file_name }}

+ + {% call(item, row_number) list_table( + recipients.initial_annotated_rows_with_errors if row_errors and not recipients.missing_column_headers else recipients.initial_annotated_rows, + caption=original_file_name, + caption_visible=False, + field_headings=[ + ''.format("table-field-invisible-error" if errors else "")|safe + ] + recipients.column_headers + ) %} + {% call index_field() %} + + {{ item.index + 2 }} + + {% endcall %} + {% for column in recipients.column_headers %} + {% if item['columns'][column].error and not recipients.missing_column_headers %} + {% call field() %} + + {{ item['columns'][column].error }} + {{ item['columns'][column].data if item['columns'][column].data != None }} + + {% endcall %} + {% elif item['columns'][column].ignore %} + {{ text_field(item['columns'][column].data or '', status='default') }} + {% else %} + {{ text_field(item['columns'][column].data or '') }} + {% endif %} + {% endfor %} + {% if item['columns'].get(None) %} + {% for column in item['columns'][None].data %} + {{ text_field(column, status='default') }} + {% endfor %} + {% endif %} + {% endcall %} + + {% endif %} + + + {% if recipients.too_many_rows %} +

+ Can’t show the contents of this file +

+ {% elif count_of_displayed_recipients < count_of_recipients %} +

+ {% if row_errors and not recipients.missing_column_headers %} + Only showing the first {{ count_of_displayed_recipients }} rows with errors + {% else %} + Only showing the first {{ count_of_displayed_recipients }} rows + {% endif %} +

+ {% elif row_errors and not recipients.missing_column_headers %} +

+ Only showing rows with errors +

+ {% endif %} + + {% if errors %} +

Preview of {{ template.name }}

+ {{ template|string }} + {% endif %} + +{% endblock %} From 5a9f817e52c8ec24a358003713b198203d479283 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jul 2017 09:02:08 +0100 Subject: [PATCH 2/7] Remove anything error-related from the OK view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you’ve uploaded a file and everything is OK, the page that you see doesn’t need to look at any errors (because there won’t be any). --- app/templates/views/check/ok.html | 160 ++---------------------------- 1 file changed, 8 insertions(+), 152 deletions(-) diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html index ebb207edd..c09b73756 100644 --- a/app/templates/views/check/ok.html +++ b/app/templates/views/check/ok.html @@ -14,141 +14,19 @@ {% endmacro %} {% block service_page_title %} - {{ "Error" if errors else "Preview of {}".format(template.name) }} + {{ "Preview of {}".format(template.name) }} {% endblock %} {% block maincolumn_content %} - {% if recipients.too_many_rows %} +

+ Preview of {{ template.name }} +

+ {{ skip_to_file_contents() }} -
- {% call banner_wrapper(type='dangerous') %} -

- Your file has too many rows -

-

- Notify can process up to - {{ "{:,}".format(recipients.max_rows) }} rows at once. Your - file has {{ "{:,}".format(recipients|length) }} rows. -

- {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif not count_of_recipients %} - -
- {% call banner_wrapper(type='dangerous') %} -

- Your file is missing some rows -

-

- It needs at least one row of data, and {{ recipients.missing_column_headers | sort() | formatted_list( - prefix='a column called', - prefix_plural='columns called' - ) }}. -

- {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif not recipients.has_recipient_columns %} - -
- {% call banner_wrapper(type='dangerous') %} -

- Your file needs {{ recipients.recipient_column_headers | formatted_list( - prefix='a column called', - prefix_plural='columns called' - ) }} -

-

- Right now it has {{ recipients.column_headers | formatted_list( - prefix='one column, called ', - prefix_plural='columns called ' - ) }}. -

- {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif recipients.missing_column_headers %} - -
- {% call banner_wrapper(type='dangerous') %} -

- The columns in your file need to match the double brackets in - your template -

-

- Your file is missing {{ recipients.missing_column_headers | formatted_list( - conjunction='and', - prefix='a column called ', - prefix_plural='columns called ' - ) }}. -

- {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif row_errors %} - -
- {% call banner_wrapper(type='dangerous') %} - {% if row_errors|length == 1 %} -

- There is a problem with your data -

-

- You need to {{ row_errors[0] }} -

- {% else %} -

- There are some problems with your data -

-

- You need to: -

-
    - {% for error in row_errors %} -
  • {{ error }}
    • - {% endfor %} -
- {% endif %} - {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif not recipients.allowed_to_send_to %} - {% with - count_of_recipients=count_of_recipients, - template_type_label=recipients.recipient_column_headers[0] - %} - {% include "partials/check/not-allowed-to-send-to.html" %} - {% endwith %} - {% elif recipients.more_rows_than_can_send %} - {% include "partials/check/too-many-messages.html" %} - {% else %} - -

- Preview of {{ template.name }} -

- {{ skip_to_file_contents() }} - - {% endif %} - - {% if not errors %} - {{ template|string }} - {% endif %} + {{ template|string }}
- {% if errors %} - {% if request.args.from_test %} - Back - {% else %} - {{file_upload(form.file, button_text='Re-upload your file')}} - {% endif %} - {% else %}
@@ -165,7 +43,6 @@ {% endif %} Back
- {% endif %}
{% if not request.args.from_test %} @@ -186,14 +63,7 @@ {% endcall %} {% for column in recipients.column_headers %} - {% if item['columns'][column].error and not recipients.missing_column_headers %} - {% call field() %} - - {{ item['columns'][column].error }} - {{ item['columns'][column].data if item['columns'][column].data != None }} - - {% endcall %} - {% elif item['columns'][column].ignore %} + {% if item['columns'][column].ignore %} {{ text_field(item['columns'][column].data or '', status='default') }} {% else %} {{ text_field(item['columns'][column].data or '') }} @@ -208,12 +78,7 @@ {% endif %} - - {% if recipients.too_many_rows %} -

- Can’t show the contents of this file -

- {% elif count_of_displayed_recipients < count_of_recipients %} + {% if count_of_displayed_recipients < count_of_recipients %}

{% if row_errors and not recipients.missing_column_headers %} Only showing the first {{ count_of_displayed_recipients }} rows with errors @@ -221,15 +86,6 @@ Only showing the first {{ count_of_displayed_recipients }} rows {% endif %}

- {% elif row_errors and not recipients.missing_column_headers %} -

- Only showing rows with errors -

- {% endif %} - - {% if errors %} -

Preview of {{ template.name }}

- {{ template|string }} {% endif %} {% endblock %} From 94bc8191aec3fc5ab1a71fd815fd83cc236ae91c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jul 2017 09:10:23 +0100 Subject: [PATCH 3/7] Remove irrelevant stuff from error check pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you have errors in your file then there’s stuff you’re not going to see on the page. So this doesn’t need to be in the Jinja templates that are only used when there are errors. Basically the conditional stuff is moving up to the level above these templates. --- app/main/views/send.py | 8 + app/templates/views/check/column-errors.html | 66 +------- app/templates/views/check/row-errors.html | 164 +++---------------- 3 files changed, 32 insertions(+), 206 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 9cdd56419..ba92533d4 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -450,6 +450,14 @@ def check_messages(service_id, template_type, upload_id): data = _check_messages(service_id, template_type, upload_id) + if ( + data['recipients'].too_many_rows or + not data['count_of_recipients'] or + not data['recipients'].has_recipient_columns or + data['recipients'].missing_column_headers + ): + return render_template('views/check/column-errors.html', **data) + if data['row_errors']: return render_template('views/check/row-errors.html', **data) diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index ebb207edd..d331eb1a2 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -14,7 +14,7 @@ {% endmacro %} {% block service_page_title %} - {{ "Error" if errors else "Preview of {}".format(template.name) }} + Error {% endblock %} {% block maincolumn_content %} @@ -91,34 +91,6 @@ {% endcall %} - {% elif row_errors %} - -
- {% call banner_wrapper(type='dangerous') %} - {% if row_errors|length == 1 %} -

- There is a problem with your data -

-

- You need to {{ row_errors[0] }} -

- {% else %} -

- There are some problems with your data -

-

- You need to: -

-
    - {% for error in row_errors %} -
  • {{ error }}
    • - {% endfor %} -
- {% endif %} - {{ skip_to_file_contents() }} - {% endcall %} -
- {% elif not recipients.allowed_to_send_to %} {% with count_of_recipients=count_of_recipients, @@ -128,44 +100,14 @@ {% endwith %} {% elif recipients.more_rows_than_can_send %} {% include "partials/check/too-many-messages.html" %} - {% else %} - -

- Preview of {{ template.name }} -

- {{ skip_to_file_contents() }} - - {% endif %} - - {% if not errors %} - {{ template|string }} {% endif %}
- {% if errors %} {% if request.args.from_test %} Back {% else %} {{file_upload(form.file, button_text='Re-upload your file')}} {% endif %} - {% else %} -
- - - {% if choose_time_form and template.template_type != 'letter' %} - {{ radio_select( - choose_time_form.scheduled_for, - wrapping_class='bottom-gutter-2-3' - ) }} - {% endif %} - {% if template.template_type != 'letter' or not request.args.from_test %} - - {% else %} - Download as a printable PDF - {% endif %} - Back -
- {% endif %}
{% if not request.args.from_test %} @@ -227,9 +169,7 @@

{% endif %} - {% if errors %} -

Preview of {{ template.name }}

- {{ template|string }} - {% endif %} +

Preview of {{ template.name }}

+ {{ template|string }} {% endblock %} diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index ebb207edd..8071e7804 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -14,158 +14,43 @@ {% endmacro %} {% block service_page_title %} - {{ "Error" if errors else "Preview of {}".format(template.name) }} + Error {% endblock %} {% block maincolumn_content %} - {% if recipients.too_many_rows %} - -
- {% call banner_wrapper(type='dangerous') %} +
+ {% call banner_wrapper(type='dangerous') %} + {% if row_errors|length == 1 %}

- Your file has too many rows + There is a problem with your data

- Notify can process up to - {{ "{:,}".format(recipients.max_rows) }} rows at once. Your - file has {{ "{:,}".format(recipients|length) }} rows. + You need to {{ row_errors[0] }}

- {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif not count_of_recipients %} - -
- {% call banner_wrapper(type='dangerous') %} + {% else %}

- Your file is missing some rows + There are some problems with your data

- It needs at least one row of data, and {{ recipients.missing_column_headers | sort() | formatted_list( - prefix='a column called', - prefix_plural='columns called' - ) }}. + You need to:

- {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif not recipients.has_recipient_columns %} - -
- {% call banner_wrapper(type='dangerous') %} -

- Your file needs {{ recipients.recipient_column_headers | formatted_list( - prefix='a column called', - prefix_plural='columns called' - ) }} -

-

- Right now it has {{ recipients.column_headers | formatted_list( - prefix='one column, called ', - prefix_plural='columns called ' - ) }}. -

- {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif recipients.missing_column_headers %} - -
- {% call banner_wrapper(type='dangerous') %} -

- The columns in your file need to match the double brackets in - your template -

-

- Your file is missing {{ recipients.missing_column_headers | formatted_list( - conjunction='and', - prefix='a column called ', - prefix_plural='columns called ' - ) }}. -

- {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif row_errors %} - -
- {% call banner_wrapper(type='dangerous') %} - {% if row_errors|length == 1 %} -

- There is a problem with your data -

-

- You need to {{ row_errors[0] }} -

- {% else %} -

- There are some problems with your data -

-

- You need to: -

-
    - {% for error in row_errors %} -
  • {{ error }}
    • - {% endfor %} -
- {% endif %} - {{ skip_to_file_contents() }} - {% endcall %} -
- - {% elif not recipients.allowed_to_send_to %} - {% with - count_of_recipients=count_of_recipients, - template_type_label=recipients.recipient_column_headers[0] - %} - {% include "partials/check/not-allowed-to-send-to.html" %} - {% endwith %} - {% elif recipients.more_rows_than_can_send %} - {% include "partials/check/too-many-messages.html" %} - {% else %} - -

- Preview of {{ template.name }} -

- {{ skip_to_file_contents() }} - - {% endif %} - - {% if not errors %} - {{ template|string }} - {% endif %} +
    + {% for error in row_errors %} +
  • {{ error }}
    • + {% endfor %} +
+ {% endif %} + {{ skip_to_file_contents() }} + {% endcall %} +
- {% if errors %} {% if request.args.from_test %} Back {% else %} {{file_upload(form.file, button_text='Re-upload your file')}} {% endif %} - {% else %} -
- - - {% if choose_time_form and template.template_type != 'letter' %} - {{ radio_select( - choose_time_form.scheduled_for, - wrapping_class='bottom-gutter-2-3' - ) }} - {% endif %} - {% if template.template_type != 'letter' or not request.args.from_test %} - - {% else %} - Download as a printable PDF - {% endif %} - Back -
- {% endif %}
{% if not request.args.from_test %} @@ -208,12 +93,7 @@ {% endif %} - - {% if recipients.too_many_rows %} -

- Can’t show the contents of this file -

- {% elif count_of_displayed_recipients < count_of_recipients %} + {% if count_of_displayed_recipients < count_of_recipients %}

{% if row_errors and not recipients.missing_column_headers %} Only showing the first {{ count_of_displayed_recipients }} rows with errors @@ -227,9 +107,7 @@

{% endif %} - {% if errors %} -

Preview of {{ template.name }}

- {{ template|string }} - {% endif %} +

Preview of {{ template.name }}

+ {{ template|string }} {% endblock %} From 8e947f315d9b864f79ffd8771b36cd0061eacc3d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jul 2017 09:20:01 +0100 Subject: [PATCH 4/7] Refactor check error templates to repeat less MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was a lot of repetetive wrapping code being repeated for every conditional block in these files. Let’s wrap it around the whole conditional bit once instead. --- .../check/not-allowed-to-send-to.html | 37 +++----- .../partials/check/too-many-messages.html | 84 +++++++++---------- app/templates/views/check/column-errors.html | 56 +++++-------- 3 files changed, 73 insertions(+), 104 deletions(-) diff --git a/app/templates/partials/check/not-allowed-to-send-to.html b/app/templates/partials/check/not-allowed-to-send-to.html index 5d2f001fb..5b1537ac2 100644 --- a/app/templates/partials/check/not-allowed-to-send-to.html +++ b/app/templates/partials/check/not-allowed-to-send-to.html @@ -1,25 +1,12 @@ -{% from "components/banner.html" import banner_wrapper %} - -{% macro skip_to_file_contents() %} - -{% endmacro %} - -
- {% call banner_wrapper(type='dangerous') %} -

- You can’t send to - {{ 'this' if count_of_recipients == 1 else 'these' }} - {{ template_type_label }} - {%- if count_of_recipients != 1 -%} - {{ 'es' if 'email address' == template_type_label else 's' }} - {%- endif %} -

-

- In trial mode you can only - send to yourself and members of your team -

- {{ skip_to_file_contents() }} - {% endcall %} -
+

+ You can’t send to + {{ 'this' if count_of_recipients == 1 else 'these' }} + {{ template_type_label }} + {%- if count_of_recipients != 1 -%} + {{ 'es' if 'email address' == template_type_label else 's' }} + {%- endif %} +

+

+ In trial mode you can only + send to yourself and members of your team +

diff --git a/app/templates/partials/check/too-many-messages.html b/app/templates/partials/check/too-many-messages.html index 217ebad33..1f42c87f1 100644 --- a/app/templates/partials/check/too-many-messages.html +++ b/app/templates/partials/check/too-many-messages.html @@ -1,46 +1,40 @@ -{% from "components/banner.html" import banner_wrapper %} - -
- {% call banner_wrapper(type='dangerous') %} -

- {% if original_file_name %} - Too many recipients - {% else %} - Daily limit reached - {% endif %} -

-

- You can only send {{ current_service.message_limit }} messages per day - {%- if current_service.restricted %} - in trial mode - {%- endif -%} - . -

- {% if original_file_name %} -

- {% if current_service.message_limit != remaining_messages %} - You can still send {{ remaining_messages }} messages today, but - {% endif %} - ‘{{ original_file_name }}’ contains - {{ 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 -%}. -

+

+ {% if original_file_name %} + Too many recipients + {% else %} + Daily limit reached + {% endif %} +

+

+ You can only send {{ current_service.message_limit }} messages per day + {%- if current_service.restricted %} + in trial mode + {%- endif -%} + . +

+{% if original_file_name %} +

+ {% if current_service.message_limit != remaining_messages %} + You can still send {{ remaining_messages }} messages today, but {% endif %} - {% endcall %} -

+ ‘{{ original_file_name }}’ contains + {{ 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 -%}. +

+{% endif %} diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index d331eb1a2..032b4dd8c 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -19,10 +19,11 @@ {% block maincolumn_content %} - {% if recipients.too_many_rows %} +
+ {% call banner_wrapper(type='dangerous') %} + + {% if recipients.too_many_rows %} -
- {% call banner_wrapper(type='dangerous') %}

Your file has too many rows

@@ -31,14 +32,9 @@ {{ "{:,}".format(recipients.max_rows) }} rows at once. Your file has {{ "{:,}".format(recipients|length) }} rows.

- {{ skip_to_file_contents() }} - {% endcall %} -
- {% elif not count_of_recipients %} + {% elif not count_of_recipients %} -
- {% call banner_wrapper(type='dangerous') %}

Your file is missing some rows

@@ -48,14 +44,9 @@ prefix_plural='columns called' ) }}.

- {{ skip_to_file_contents() }} - {% endcall %} -
- {% elif not recipients.has_recipient_columns %} + {% elif not recipients.has_recipient_columns %} -
- {% call banner_wrapper(type='dangerous') %}

Your file needs {{ recipients.recipient_column_headers | formatted_list( prefix='a column called', @@ -68,14 +59,9 @@ prefix_plural='columns called ' ) }}.

- {{ skip_to_file_contents() }} - {% endcall %} -

- {% elif recipients.missing_column_headers %} + {% elif recipients.missing_column_headers %} -
- {% call banner_wrapper(type='dangerous') %}

The columns in your file need to match the double brackets in your template @@ -87,20 +73,22 @@ prefix_plural='columns called ' ) }}.

- {{ skip_to_file_contents() }} - {% endcall %} -

- {% elif not recipients.allowed_to_send_to %} - {% with - count_of_recipients=count_of_recipients, - template_type_label=recipients.recipient_column_headers[0] - %} - {% include "partials/check/not-allowed-to-send-to.html" %} - {% endwith %} - {% elif recipients.more_rows_than_can_send %} - {% include "partials/check/too-many-messages.html" %} - {% endif %} + {% elif not recipients.allowed_to_send_to %} + {% with + count_of_recipients=count_of_recipients, + template_type_label=recipients.recipient_column_headers[0] + %} + {% include "partials/check/not-allowed-to-send-to.html" %} + {% endwith %} + {% elif recipients.more_rows_than_can_send %} + {% include "partials/check/too-many-messages.html" %} + {% endif %} + + {{ skip_to_file_contents() }} + + {% endcall %} +
{% if request.args.from_test %} From 56945565ba8aa4fc32f8e3b1f121ef2cc83eec25 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jul 2017 09:31:04 +0100 Subject: [PATCH 5/7] =?UTF-8?q?Don=E2=80=99t=20indent=20column=20headings?= =?UTF-8?q?=20if=20no=20row=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reason to indent the first column heading is so that the number 1 lines up with the numbers of subsequent rows. This only happens when the subsequent rows are indented because of the red bars. This is only when there are row errors, not when there are more general errors. --- app/templates/views/check/column-errors.html | 2 +- app/templates/views/check/ok.html | 2 +- app/templates/views/check/row-errors.html | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index 032b4dd8c..0dc1dffa4 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -107,7 +107,7 @@ caption=original_file_name, caption_visible=False, field_headings=[ - ''.format("table-field-invisible-error" if errors else "")|safe + ''|safe ] + recipients.column_headers ) %} {% call index_field() %} diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html index c09b73756..affa5a4ff 100644 --- a/app/templates/views/check/ok.html +++ b/app/templates/views/check/ok.html @@ -54,7 +54,7 @@ caption=original_file_name, caption_visible=False, field_headings=[ - ''.format("table-field-invisible-error" if errors else "")|safe + ''|safe ] + recipients.column_headers ) %} {% call index_field() %} diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index 8071e7804..3fdf0171d 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -62,7 +62,7 @@ caption=original_file_name, caption_visible=False, field_headings=[ - ''.format("table-field-invisible-error" if errors else "")|safe + ''|safe ] + recipients.column_headers ) %} {% call index_field() %} From eb264f34b766d92ce79cb116b6093698de593caf Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jul 2017 11:22:09 +0100 Subject: [PATCH 6/7] Add a little JS module to track events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Google analytics lets you send: - pageviews - events Page views are used by default. But sometimes you wanna count something which isn’t the user navigating to a new page, or you wanna track something which isn’t just what page they were looking it. This is where events are useful. This commit adds a small JS module that lets us fire off an event when the presence of an element with the right data attributes are present on the page. --- app/assets/javascripts/errorTracking.js | 22 ++++++++++++++++++++++ gulpfile.babel.js | 1 + 2 files changed, 23 insertions(+) create mode 100644 app/assets/javascripts/errorTracking.js diff --git a/app/assets/javascripts/errorTracking.js b/app/assets/javascripts/errorTracking.js new file mode 100644 index 000000000..815643b73 --- /dev/null +++ b/app/assets/javascripts/errorTracking.js @@ -0,0 +1,22 @@ +(function(Modules) { + "use strict"; + + Modules.TrackEvent = function() { + + this.start = function(component) { + + if (!ga) return; + + ga( + 'send', + 'event', + 'Error', + $(component).data('error-type'), + $(component).data('error-label') + ); + + }; + + }; + +})(window.GOVUK.Modules); diff --git a/gulpfile.babel.js b/gulpfile.babel.js index a4d044fba..26dd6c96b 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -66,6 +66,7 @@ gulp.task('javascripts', () => gulp paths.src + 'javascripts/updateContent.js', paths.src + 'javascripts/listEntry.js', paths.src + 'javascripts/liveSearch.js', + paths.src + 'javascripts/errorTracking.js', paths.src + 'javascripts/main.js' ]) .pipe(plugins.prettyerror()) From 82233340b63b524785cb2d2c19af10fd4c815be0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jul 2017 11:28:26 +0100 Subject: [PATCH 7/7] Track errors when uploading spreadsheets Uses the new javascript event tracking stuff so that we can see what errors people are getting when they upload spreadsheets. --- app/assets/javascripts/errorTracking.js | 2 +- app/templates/partials/check/not-allowed-to-send-to.html | 2 +- app/templates/partials/check/too-many-messages.html | 2 +- app/templates/views/check/column-errors.html | 8 ++++---- app/templates/views/check/row-errors.html | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/errorTracking.js b/app/assets/javascripts/errorTracking.js index 815643b73..3b7053841 100644 --- a/app/assets/javascripts/errorTracking.js +++ b/app/assets/javascripts/errorTracking.js @@ -1,7 +1,7 @@ (function(Modules) { "use strict"; - Modules.TrackEvent = function() { + Modules.TrackError = function() { this.start = function(component) { diff --git a/app/templates/partials/check/not-allowed-to-send-to.html b/app/templates/partials/check/not-allowed-to-send-to.html index 5b1537ac2..4d445e9c6 100644 --- a/app/templates/partials/check/not-allowed-to-send-to.html +++ b/app/templates/partials/check/not-allowed-to-send-to.html @@ -1,4 +1,4 @@ -

+

You can’t send to {{ 'this' if count_of_recipients == 1 else 'these' }} {{ template_type_label }} diff --git a/app/templates/partials/check/too-many-messages.html b/app/templates/partials/check/too-many-messages.html index 1f42c87f1..fd16b0534 100644 --- a/app/templates/partials/check/too-many-messages.html +++ b/app/templates/partials/check/too-many-messages.html @@ -1,4 +1,4 @@ -

+

{% if original_file_name %} Too many recipients {% else %} diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index 0dc1dffa4..f8407792f 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -24,7 +24,7 @@ {% if recipients.too_many_rows %} -

+

Your file has too many rows

@@ -35,7 +35,7 @@ {% elif not count_of_recipients %} -

+

Your file is missing some rows

@@ -47,7 +47,7 @@ {% elif not recipients.has_recipient_columns %} -

+

Your file needs {{ recipients.recipient_column_headers | formatted_list( prefix='a column called', prefix_plural='columns called' @@ -62,7 +62,7 @@ {% elif recipients.missing_column_headers %} -

+

The columns in your file need to match the double brackets in your template

diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index 3fdf0171d..ce470bcd8 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -22,7 +22,7 @@
{% call banner_wrapper(type='dangerous') %} {% if row_errors|length == 1 %} -

+

There is a problem with your data