From c3e2bce98b977a2a3bc1bcb508c06525da7d9747 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 6 Dec 2017 22:04:58 +0000 Subject: [PATCH] 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(