From 8ce1497fc0dab0f5a8331007b59343d1239f502f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sat, 11 Apr 2020 11:06:58 +0100 Subject: [PATCH 1/4] Make test more specific Tests that just assert some content `in` the whole page are tricky to debug, and make it harder to be sure that said content is showing up in the right place, with the right markup and styling. --- tests/app/main/views/test_send.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 57e7e4122..72812158d 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -475,9 +475,19 @@ def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( assert 'file_uploads' not in session assert response.status_code == 200 - content = response.get_data(as_text=True) - assert 'There’s a problem with invalid.csv' in content - assert 'check you have content for the empty message in 1 row' in content + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert normalize_spaces( + page.select_one('.banner-dangerous').text + ) == ( + 'There’s a problem with invalid.csv ' + 'You need to check you have content for the empty message in 1 row. ' + 'Skip to file contents' + ) + assert [ + normalize_spaces(row.text) for row in page.select('tbody tr') + ] == [ + '3 +447700900986 no', + ] @pytest.mark.parametrize('file_contents, expected_error,', [ From f37137cdb380df289e9fcbe5068a05f0e3f667ac Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sat, 11 Apr 2020 11:07:20 +0100 Subject: [PATCH 2/4] Convert list table to mapping table A list table takes a list of input, and assumes one row of output. In order to show errors that span multiple rows we need to output two rows for one item of input. The list table inherits from the mapping table; using the mapping table directly gives us the flexibility we need. --- app/templates/views/check/row-errors.html | 51 ++++++++++++----------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index 69b6bc373..b29aa88d9 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -1,7 +1,7 @@ {% 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/table.html" import mapping_table, row, field, text_field, index_field, hidden_field_heading %} {% from "components/file-upload.html" import file_upload %} {% from "components/back-link/macro.njk" import govukBackLink %} {% from "components/message-count-label.html" import message_count_label %} @@ -59,38 +59,41 @@
- {% call(item, row_number) list_table( - recipients.displayed_rows, + {% call(item, row_number) mapping_table( caption=original_file_name, caption_visible=False, field_headings=[ 'Row in file'|safe ] + recipients.column_headers ) %} - {% call index_field() %} - - {{ item.index + 2 }} - - {% endcall %} - {% for column in recipients.column_headers %} - {% if item[column].error and not recipients.missing_column_headers %} - {% call field() %} - - {{ item[column].error }} - {{ item[column].data if item[column].data != None }} + {% for item in recipients.displayed_rows %} + {% call row(item.id) %} + {% call index_field() %} + + {{ item.index + 2 }} {% endcall %} - {% elif item[column].ignore %} - {{ text_field(item[column].data or '', status='default') }} - {% else %} - {{ text_field(item[column].data or '') }} - {% endif %} + {% for column in recipients.column_headers %} + {% if item[column].error and not recipients.missing_column_headers %} + {% call field() %} + + {{ item[column].error }} + {{ item[column].data if item[column].data != None }} + + {% endcall %} + {% elif item[column].ignore %} + {{ text_field(item[column].data or '', status='default') }} + {% else %} + {{ text_field(item[column].data or '') }} + {% endif %} + {% endfor %} + {% if item[None].data %} + {% for column in item[None].data %} + {{ text_field(column, status='default') }} + {% endfor %} + {% endif %} + {% endcall %} {% endfor %} - {% if item[None].data %} - {% for column in item[None].data %} - {{ text_field(column, status='default') }} - {% endfor %} - {% endif %} {% endcall %}
{% if count_of_displayed_recipients < count_of_recipients %} From e2d1401b54828138ec2fbe5dcadcca2d91d60a83 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sat, 11 Apr 2020 11:42:13 +0100 Subject: [PATCH 3/4] Show error across the whole row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Errors with messages being too long or empty aren’t specific to a single cell of the uploaded spreadsheet, they’re the results of combining all the cells with the template. Previously we could only show errors against a specific cell. This commit makes it possible to add a super-row which spans all the cells, into which we can put errors. The index (header) column then spans both these rows, to show that they are both associated with the same row of input. Depends on: - [x] https://github.com/alphagov/notifications-utils/pull/719 --- app/templates/components/table.html | 8 +++--- app/templates/views/check/row-errors.html | 30 +++++++++++++++++++---- tests/app/main/views/test_send.py | 10 +++++++- 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/app/templates/components/table.html b/app/templates/components/table.html index 4eba1c54e..f8b03e248 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -65,12 +65,12 @@ {% endif %} {%- endmacro %} -{% macro field(align='left', status='', border=True) -%} +{% macro field(align='left', status='', border=True, colspan=None) -%} {% set field_alignment = 'table-field-right-aligned' if align == 'right' else 'table-field-left-aligned' %} {% set border = '' if border else 'table-field-noborder' %} - +
{{ caller() }}
{%- endmacro %} @@ -81,8 +81,8 @@ {%- endmacro %} -{% macro index_field(text=None) -%} - +{% macro index_field(text=None, rowspan=None) -%} + {{ text if text != None else caller() }} {%- endmacro %} diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index b29aa88d9..3ddc43b29 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -67,12 +67,32 @@ ] + recipients.column_headers ) %} {% for item in recipients.displayed_rows %} - {% call row(item.id) %} - {% call index_field() %} - - {{ item.index + 2 }} - + {% if item.has_error_spanning_multiple_cells %} + {% call row() %} + {% call index_field(rowspan=2) %} + + {{ item.index + 2 }} + + {% endcall %} + {% call field(colspan=recipients.column_headers|length) %} + + {% if item.message_empty %} + No content for this message + {% elif item.message_too_long %} + Message is too long + {% endif %} + + {% endcall %} {% endcall %} + {% endif %} + {% call row(item.id) %} + {% if not item.has_error_spanning_multiple_cells %} + {% call index_field() %} + + {{ item.index + 2 }} + + {% endcall %} + {% endif %} {% for column in recipients.column_headers %} {% if item[column].error and not recipients.missing_column_headers %} {% call field() %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 72812158d..09c7ce4b7 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -486,8 +486,16 @@ def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( assert [ normalize_spaces(row.text) for row in page.select('tbody tr') ] == [ - '3 +447700900986 no', + '3 No content for this message', + '+447700900986 no', ] + assert normalize_spaces(page.select_one('.table-field-index').text) == '3' + assert page.select_one('.table-field-index')['rowspan'] == '2' + assert normalize_spaces(page.select('tbody tr td')[0].text) == '3' + assert normalize_spaces(page.select('tbody tr td')[1].text) == ( + 'No content for this message' + ) + assert page.select('tbody tr td')[1]['colspan'] == '2' @pytest.mark.parametrize('file_contents, expected_error,', [ From c36e75b3db73471f2d192c7474a25c62be60f444 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Apr 2020 12:35:59 +0100 Subject: [PATCH 4/4] Add test for message too long error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This didn’t seem to be tested anywhere before. We want to make sure it works in the same sort of way as the message is empty error, which is tested. --- tests/app/main/views/test_send.py | 58 +++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 09c7ce4b7..89e851f11 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -498,6 +498,64 @@ def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( assert page.select('tbody tr td')[1]['colspan'] == '2' +def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors( + logged_in_client, + service_one, + mocker, + mock_get_service_template_with_placeholders, + mock_s3_upload, + mock_get_users_by_service, + mock_get_service_statistics, + mock_get_job_doesnt_exist, + mock_get_jobs, + fake_uuid, +): + big_placeholder = ' '.join(['not ok'] * 102) + mocker.patch( + 'app.main.views.send.s3download', + return_value=f""" + phone number, name + +447700900986, {big_placeholder} + +447700900987, {big_placeholder} + """ + ) + + response = logged_in_client.post( + url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), + data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')}, + content_type='multipart/form-data', + follow_redirects=True + ) + + with logged_in_client.session_transaction() as session: + assert 'file_uploads' not in session + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert normalize_spaces( + page.select_one('.banner-dangerous').text + ) == ( + 'There’s a problem with invalid.csv ' + 'You need to shorten the messages in 2 rows. ' + 'Skip to file contents' + ) + assert [ + normalize_spaces(row.text) for row in page.select('tbody tr') + ] == [ + '2 Message is too long', + f'+447700900986 {big_placeholder}', + '3 Message is too long', + f'+447700900987 {big_placeholder}', + ] + assert normalize_spaces(page.select_one('.table-field-index').text) == '2' + assert page.select_one('.table-field-index')['rowspan'] == '2' + assert normalize_spaces(page.select('tbody tr td')[0].text) == '2' + assert normalize_spaces(page.select('tbody tr td')[1].text) == ( + 'Message is too long' + ) + assert page.select('tbody tr td')[1]['colspan'] == '2' + + @pytest.mark.parametrize('file_contents, expected_error,', [ ( """