From 5bac0214935ad6560e43cda90307764ed780c00e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 6 Dec 2017 21:59:34 +0000 Subject: [PATCH 1/7] Remove unused variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This variable isn’t used by our Jinja templates. Presumably it was from the time before instances of `Template` were responsible for showing who the email/text message was being sent to. --- app/main/views/send.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 6e8b81f44..d53053ed9 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -21,7 +21,6 @@ from flask import ( from flask_login import login_required, current_user from notifications_python_client.errors import HTTPError -from notifications_utils.columns import Columns from notifications_utils.recipients import ( RecipientCSV, first_column_headings, @@ -523,18 +522,12 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): choose_time_form = ChooseTimeForm() with suppress(StopIteration): - first_recipient = None template.values = next(recipients.rows) - 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 return dict( recipients=recipients, - first_recipient=first_recipient, template=template, errors=recipients.has_errors, row_errors=get_errors_for_csv(recipients, template.template_type), From 71368301f758c9335f950c2a27ce0e31fddf3a88 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 7 Dec 2017 14:32:05 +0000 Subject: [PATCH 2/7] Make test use new fixture The `client_request` fixture means the tests take fewer lines of code and also checks some extra things (eg page titles matching

s). Changing it in this commit, independently of functional changes to this test. --- tests/app/main/views/test_send.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index f3618f384..c148d03cb 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -441,7 +441,7 @@ def test_upload_csv_invalid_extension( def test_upload_valid_csv_shows_file_contents( - logged_in_client, + client_request, mocker, mock_get_service_template_with_placeholders, mock_s3_upload, @@ -455,14 +455,13 @@ def test_upload_valid_csv_shows_file_contents( 07700900986, Jo, foo, foo, foo """) - 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')), 'valid.csv')}, - follow_redirects=True, + page = client_request.post( + 'main.send_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid, + _data={'file': (BytesIO(''.encode('utf-8')), 'valid.csv')}, + _follow_redirects=True, + _expected_status=200, ) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.text.strip() == 'Preview of Two week reminder' for index, cell in enumerate([ ' 2 ', From 3e5deeffdb97f9baf92dc4e35dcd65e0c878e4a6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 7 Dec 2017 15:42:58 +0000 Subject: [PATCH 3/7] Split successful upload test into two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Easier to test the individual aspects of what’s going on here if they’re two separate tests, one hitting the initial `POST`, and one hitting the page that they’re subsequently redirected to. --- tests/app/main/views/test_send.py | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index c148d03cb..6cba9b778 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -440,7 +440,21 @@ def test_upload_csv_invalid_extension( assert "invalid.txt isn’t a spreadsheet that Notify can read" in resp.get_data(as_text=True) -def test_upload_valid_csv_shows_file_contents( +def test_upload_valid_csv_redirects_to_check_page( + client_request, + mock_get_service_template_with_placeholders, + mock_s3_upload, + fake_uuid, +): + client_request.post( + 'main.send_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid, + _data={'file': (BytesIO(''.encode('utf-8')), 'valid.csv')}, + _expected_status=302, + expected_redirect='foo' + ) + + +def test_upload_valid_csv_shows_preview_and_table( client_request, mocker, mock_get_service_template_with_placeholders, @@ -450,16 +464,19 @@ def test_upload_valid_csv_shows_file_contents( fake_uuid, ): + with client_request.session_transaction() as session: + session['upload_data'] = {'template_id': fake_uuid} + mocker.patch('app.main.views.send.s3download', return_value=""" phone number,name,thing,thing,thing 07700900986, Jo, foo, foo, foo """) - page = client_request.post( - 'main.send_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid, - _data={'file': (BytesIO(''.encode('utf-8')), 'valid.csv')}, - _follow_redirects=True, - _expected_status=200, + page = client_request.get( + 'main.check_messages', + service_id=SERVICE_ONE_ID, + template_type='sms', + upload_id=fake_uuid, ) assert page.h1.text.strip() == 'Preview of Two week reminder' From 587e18d2efa7f01a1a54ccca171e7a9add9d5d43 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 6 Dec 2017 22:00:45 +0000 Subject: [PATCH 4/7] Lookup index, rather than iterating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since instances of `Recipients` are now iterable we can just look up the first row by index, rather than having to hit the rows property as a generator. There is a chance that there won’t be a first row, if the file has 1 or fewer columns. So we still need to handle a possible exception (albeit it a different one to before). --- app/main/views/send.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index d53053ed9..305f5a107 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -521,8 +521,8 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): back_link = url_for('.send_messages', service_id=service_id, template_id=template.id) choose_time_form = ChooseTimeForm() - with suppress(StopIteration): - template.values = next(recipients.rows) + with suppress(IndexError): + template.values = recipients[0] session['upload_data']['notification_count'] = len(list(recipients.rows)) session['upload_data']['valid'] = not recipients.has_errors From c3e2bce98b977a2a3bc1bcb508c06525da7d9747 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 6 Dec 2017 22:04:58 +0000 Subject: [PATCH 5/7] Add URLs for previewing all rows of a spreadsheet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve heard from some users, especially those sending letters, that they’d like to check that a spreadsheet they’ve uploaded has populated the template correctly. My reckon is that seeing just one row of the spreadsheet populate the template isn’t enough to give people confidence that everything’s working properly. So this commit is the first step towards being able to preview an arbitrary row of a template, by extending the URL structure to optionally accept a row number for pages or files (ie PNG) that preview successfully uploaded spreadsheets. What this commit doesn’t do is link to these pages; that will come as part of a subsequent commit. --- app/main/views/send.py | 24 +++++--- tests/app/main/views/test_send.py | 94 +++++++++++++++++++++++++++++-- 2 files changed, 103 insertions(+), 15 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 305f5a107..d7b920d98 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -2,7 +2,6 @@ import itertools from string import ascii_uppercase from orderedset import OrderedSet -from contextlib import suppress from zipfile import BadZipFile from xlrd.biffh import XLRDError from werkzeug.routing import RequestRedirect @@ -460,7 +459,7 @@ def send_test_preview(service_id, template_id, filetype): return TemplatePreview.from_utils_template(template, filetype, page=request.args.get('page')) -def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): +def _check_messages(service_id, template_type, upload_id, preview_row, letters_as_pdf=False): if not session.get('upload_data'): # if we just return a `redirect` (302) object here, we'll get errors when we try and unpack in the @@ -496,6 +495,7 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): template_type=template_type, upload_id=upload_id, filetype='png', + row_index=preview_row, ) if not letters_as_pdf else None, email_reply_to=email_reply_to, sms_sender=sms_sender @@ -521,8 +521,11 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): back_link = url_for('.send_messages', service_id=service_id, template_id=template.id) choose_time_form = ChooseTimeForm() - with suppress(IndexError): - template.values = recipients[0] + try: + template.values = recipients[preview_row] + except IndexError: + if preview_row > 0: + abort(404) session['upload_data']['notification_count'] = len(list(recipients.rows)) session['upload_data']['valid'] = not recipients.has_errors @@ -554,11 +557,12 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): @main.route("/services///check/", methods=['GET']) +@main.route("/services///check//row-", methods=['GET']) @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') -def check_messages(service_id, template_type, upload_id): +def check_messages(service_id, template_type, upload_id, row_index=0): - data = _check_messages(service_id, template_type, upload_id) + data = _check_messages(service_id, template_type, upload_id, row_index) if ( data['recipients'].too_many_rows or @@ -581,22 +585,24 @@ def check_messages(service_id, template_type, upload_id): @main.route("/services///check/.", methods=['GET']) +@main.route("/services///check//row-.", methods=['GET']) @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') -def check_messages_preview(service_id, template_type, upload_id, filetype): +def check_messages_preview(service_id, template_type, upload_id, filetype, row_index=0): if filetype not in ('pdf', 'png'): abort(404) template = _check_messages( - service_id, template_type, upload_id, letters_as_pdf=True + service_id, template_type, upload_id, row_index, letters_as_pdf=True )['template'] return TemplatePreview.from_utils_template(template, filetype) @main.route("/services///check/", methods=['POST']) +@main.route("/services///check//row-", methods=['POST']) @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') -def recheck_messages(service_id, template_type, upload_id): +def recheck_messages(service_id, template_type, upload_id, row_index=0): if not session.get('upload_data'): return redirect(url_for('main.choose_template', service_id=service_id)) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 6cba9b778..d8b5b40ee 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -454,14 +454,34 @@ def test_upload_valid_csv_redirects_to_check_page( ) +@pytest.mark.parametrize('extra_args, expected_recipient, expected_message', [ + ( + {}, + 'To: 07700900001', + 'Test Service: A, Template content with & entity', + ), + ( + {'row_index': 0}, + 'To: 07700900001', + 'Test Service: A, Template content with & entity', + ), + ( + {'row_index': 2}, + 'To: 07700900003', + 'Test Service: C, Template content with & entity', + ), +]) def test_upload_valid_csv_shows_preview_and_table( client_request, mocker, + mock_get_live_service, mock_get_service_template_with_placeholders, - mock_s3_upload, mock_get_users_by_service, mock_get_detailed_service_for_today, fake_uuid, + extra_args, + expected_recipient, + expected_message, ): with client_request.session_transaction() as session: @@ -469,7 +489,9 @@ def test_upload_valid_csv_shows_preview_and_table( mocker.patch('app.main.views.send.s3download', return_value=""" phone number,name,thing,thing,thing - 07700900986, Jo, foo, foo, foo + 07700900001, A, foo, foo, foo + 07700900002, B, foo, foo, foo + 07700900003, C, foo, foo, foo """) page = client_request.get( @@ -477,13 +499,17 @@ def test_upload_valid_csv_shows_preview_and_table( service_id=SERVICE_ONE_ID, template_type='sms', upload_id=fake_uuid, + **extra_args ) assert page.h1.text.strip() == 'Preview of Two week reminder' + assert page.select_one('.sms-message-recipient').text.strip() == expected_recipient + assert page.select_one('.sms-message-wrapper').text.strip() == expected_message + for index, cell in enumerate([ ' 2 ', - '
07700900986
', - '
Jo
', + '
07700900001
', + '
A
', ( ' ' '
' @@ -497,6 +523,43 @@ def test_upload_valid_csv_shows_preview_and_table( assert normalize_spaces(str(page.select('table tbody td')[index])) == cell +@pytest.mark.parametrize('row_index, expected_status', [ + (0, 200), + (2, 200), + (3, 404), +]) +def test_404_for_previewing_a_row_out_of_range( + client_request, + mocker, + mock_get_live_service, + mock_get_service_template_with_placeholders, + mock_get_users_by_service, + mock_get_detailed_service_for_today, + fake_uuid, + row_index, + expected_status, +): + + with client_request.session_transaction() as session: + session['upload_data'] = {'template_id': fake_uuid} + + mocker.patch('app.main.views.send.s3download', return_value=""" + phone number,name,thing,thing,thing + 07700900001, A, foo, foo, foo + 07700900002, B, foo, foo, foo + 07700900003, C, foo, foo, foo + """) + + client_request.get( + 'main.check_messages', + service_id=SERVICE_ONE_ID, + template_type='sms', + upload_id=fake_uuid, + row_index=row_index, + _expected_status=expected_status, + ) + + def test_send_test_doesnt_show_file_contents( logged_in_client, mocker, @@ -1389,6 +1452,20 @@ def test_can_start_letters_job( @pytest.mark.parametrize('filetype', ['pdf', 'png']) +@pytest.mark.parametrize('extra_args, expected_values', [ + ( + {}, + {'postcode': 'abc123', 'addressline1': '123 street'}, + ), + ( + {'row_index': 0}, + {'postcode': 'abc123', 'addressline1': '123 street'}, + ), + ( + {'row_index': 1}, + {'postcode': 'cba321', 'addressline1': '321 avenue'}, + ), +]) def test_should_show_preview_letter_message( filetype, logged_in_platform_admin_client, @@ -1398,6 +1475,8 @@ def test_should_show_preview_letter_message( service_one, fake_uuid, mocker, + extra_args, + expected_values, ): service_one['permissions'] = ['letter'] mocker.patch('app.service_api_client.get_service', return_value={"data": service_one}) @@ -1407,7 +1486,8 @@ def test_should_show_preview_letter_message( 'app.main.views.send.s3download', return_value='\n'.join( ['address line 1, postcode'] + - ['123 street, abc123'] + ['123 street, abc123'] + + ['321 avenue, cba321'] ) ) mocked_preview = mocker.patch( @@ -1430,7 +1510,8 @@ def test_should_show_preview_letter_message( service_id=service_id, template_type='letter', upload_id=fake_uuid, - filetype=filetype + filetype=filetype, + **extra_args ) ) @@ -1441,6 +1522,7 @@ def test_should_show_preview_letter_message( assert mocked_preview.call_args[0][0].id == template_id assert type(mocked_preview.call_args[0][0]) == LetterPreviewTemplate assert mocked_preview.call_args[0][1] == filetype + assert mocked_preview.call_args[0][0].values == expected_values def test_dont_show_preview_letter_templates_for_bad_filetype( From 5f5dd3ac410dbc52ad8ee7a6cbc7b2bc9822c4c5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Dec 2017 11:48:30 +0000 Subject: [PATCH 6/7] Rewrite check for row existence as conditional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because exceptions can be expensive performance wise (see: https://docs.python.org/3/faq/design.html#how-fast-are-exceptions). Since we’re counting the number of rows anyway this doesn’t introduce any performance overhead there. And I think it’s equally readable/same number of lines of code. --- app/main/views/send.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index d7b920d98..f375c1645 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -521,20 +521,21 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a back_link = url_for('.send_messages', service_id=service_id, template_id=template.id) choose_time_form = ChooseTimeForm() - try: - template.values = recipients[preview_row] - except IndexError: - if preview_row > 0: - abort(404) + count_of_recipients = len(list(recipients.rows)) - session['upload_data']['notification_count'] = len(list(recipients.rows)) + if preview_row < count_of_recipients: + template.values = recipients[preview_row] + elif preview_row > 0: + abort(404) + + session['upload_data']['notification_count'] = count_of_recipients session['upload_data']['valid'] = not recipients.has_errors return dict( recipients=recipients, template=template, errors=recipients.has_errors, row_errors=get_errors_for_csv(recipients, template.template_type), - count_of_recipients=session['upload_data']['notification_count'], + count_of_recipients=count_of_recipients, count_of_displayed_recipients=( len(list(recipients.initial_annotated_rows_with_errors)) if any(recipients.rows_with_errors) and not recipients.missing_column_headers else From eb395db2d6fe8392089be40488b9d98ed0bf697b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Dec 2017 11:59:51 +0000 Subject: [PATCH 7/7] Fix error when file has header rows but no data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously the error was saying ‘It needs at least one row of data, and columns called None’. --- app/templates/views/check/column-errors.html | 18 ++++++++++++------ tests/app/main/views/test_send.py | 10 ++++++++++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index 553449600..6f3b626d1 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -38,12 +38,18 @@

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' - ) }}. -

+ {% if recipients.missing_column_headers %} +

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

+ {% else %} +

+ It needs at least one row of data. +

+ {% endif %} {% elif not recipients.has_recipient_columns %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index d8b5b40ee..bc7423dea 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -352,6 +352,16 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( 'Skip to file contents' ) ), + ( + """ + phone number, name + """, + ( + 'Your file is missing some rows ' + 'It needs at least one row of data. ' + 'Skip to file contents' + ) + ), ( "+447700900986", (