diff --git a/app/main/views/send.py b/app/main/views/send.py index 6e8b81f44..f375c1645 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 @@ -21,7 +20,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, @@ -461,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 @@ -497,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 @@ -522,23 +521,21 @@ 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): - first_recipient = None - template.values = next(recipients.rows) - first_recipient = template.values.get( - Columns.make_key(recipients.recipient_column_headers[0]), - '' - ) + 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, - first_recipient=first_recipient, 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 @@ -561,11 +558,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 @@ -588,22 +586,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/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 f3618f384..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", ( @@ -440,34 +450,76 @@ 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( - logged_in_client, - mocker, +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' + ) + + +@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_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: + 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 + 07700900001, A, foo, foo, foo + 07700900002, B, foo, foo, foo + 07700900003, C, 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.get( + 'main.check_messages', + service_id=SERVICE_ONE_ID, + template_type='sms', + upload_id=fake_uuid, + **extra_args ) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') 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
', ( ' ' '
' @@ -481,6 +533,43 @@ def test_upload_valid_csv_shows_file_contents( 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, @@ -1373,6 +1462,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, @@ -1382,6 +1485,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}) @@ -1391,7 +1496,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( @@ -1414,7 +1520,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 ) ) @@ -1425,6 +1532,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(