From 849b3737beb2cd38fd364143cc852f089f11581a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 28 Jul 2017 09:07:19 +0100 Subject: [PATCH 1/4] Track errors in analytics on multiple row errors Accidentally missed out on adding the tracking code to this particular error message. --- app/templates/views/check/row-errors.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index ce470bcd8..0a3dc6dd9 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -29,7 +29,7 @@ You need to {{ row_errors[0] }}

{% else %} -

+

There are some problems with your data

From 25e701553704065a52bdd64d842c4597cbe079a2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 28 Jul 2017 09:09:22 +0100 Subject: [PATCH 2/4] Remove stuff about test jobs on row errors page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The send yourself a test flow doesn’t reference any kind of file/rows/table in the front end any more, and doesn’t create a CSV in the background (except for letters). And it should validate any mistakes before getting to the check page. So any user doing a send yourself a test flow should never get to this template, which means we can remove some redundant code. --- app/templates/views/check/row-errors.html | 76 ++++++++++------------- 1 file changed, 34 insertions(+), 42 deletions(-) diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index 0a3dc6dd9..91af69965 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -46,52 +46,44 @@

- {% if request.args.from_test %} - Back - {% else %} - {{file_upload(form.file, button_text='Re-upload your file')}} - {% endif %} + {{ file_upload(form.file, button_text='Re-upload your file') }}
- {% if not request.args.from_test %} +

{{ original_file_name }}

-

{{ 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=[ - ''|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 %} + {% 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=[ + ''|safe + ] + recipients.column_headers + ) %} + {% call index_field() %} + + {{ item.index + 2 }} + {% endcall %} - - {% endif %} + {% 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 %} {% if count_of_displayed_recipients < count_of_recipients %}

From 5c872e79360da708d34935e3a6e976e8af09bac2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 28 Jul 2017 09:24:09 +0100 Subject: [PATCH 3/4] Put file name in error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ‘Your data’ sounds ambiguous and computery. Using the name of the file is better because it’s a thing you know about. --- app/templates/views/check/row-errors.html | 6 ++---- tests/app/main/views/test_send.py | 16 ++++++++++------ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index 91af69965..4d4c2f584 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -23,14 +23,14 @@ {% call banner_wrapper(type='dangerous') %} {% if row_errors|length == 1 %}

- There is a problem with your data + There is a problem with {{ original_file_name }}

You need to {{ row_errors[0] }}

{% else %}

- There are some problems with your data + There are some problems with {{ original_file_name }}

You need to: @@ -49,8 +49,6 @@ {{ file_upload(form.file, button_text='Re-upload your file') }} -

{{ 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, diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index b323237a2..4b56a17f1 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -140,7 +140,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( for response in [initial_upload, reupload]: assert response.status_code == 200 content = response.get_data(as_text=True) - assert 'There is a problem with your data' in content + assert 'There is a problem with invalid.csv' in content assert '+447700900986' in content assert 'Missing' in content assert 'Re-upload your file' in content @@ -193,7 +193,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( +447700900986, example """, ( - 'There is a problem with your data ' + 'There is a problem with invalid.csv ' 'You need to enter missing data in 1 row ' 'Skip to file contents' ) @@ -206,7 +206,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( +447700900986, example """, ( - 'There is a problem with your data ' + 'There is a problem with invalid.csv ' 'You need to enter missing data in 1 row ' 'Skip to file contents' ) @@ -1283,7 +1283,7 @@ def test_check_messages_should_revalidate_file_when_uploading_file( follow_redirects=True ) assert response.status_code == 200 - assert 'There is a problem with your data' in response.get_data(as_text=True) + assert 'There is a problem with invalid.csv' in response.get_data(as_text=True) @pytest.mark.parametrize('route, response_code', [ @@ -1637,7 +1637,11 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error( ) with logged_in_client.session_transaction() as session: - session['upload_data'] = {'template_id': fake_uuid} + session['upload_data'] = { + 'template_id': fake_uuid, + 'original_file_name': 'unicode.csv', + } + response = logged_in_client.get(url_for( 'main.check_messages', service_id=fake_uuid, @@ -1650,7 +1654,7 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error( assert ' '.join( page.find('div', class_='banner-dangerous').text.split() ) == ( - 'There is a problem with your data ' + 'There is a problem with unicode.csv ' 'You need to fix 1 address ' 'Skip to file contents' ) From 5b4a4ea1b1efb6989e5d6330a091933f027ccf2e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 28 Jul 2017 09:27:44 +0100 Subject: [PATCH 4/4] Remove message preview from row error page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reasons to remove this: 1. It’s potentially confusing because you’ll see a preview of the first message, but the first row might not appear in the table shown on screen if it doesn’t have any errors. 2. If there are row-level errors in your file then they’re not related to the template – it’s a distraction. --- app/templates/views/check/row-errors.html | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index 4d4c2f584..537710516 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -97,7 +97,4 @@

{% endif %} -

Preview of {{ template.name }}

- {{ template|string }} - {% endblock %}