Merge pull request #1696 from alphagov/preview-any-row

Add URLs for previewing all rows of a spreadsheet
2026-07-26 02:49:16 -04:00 · 2017-12-20 12:07:19 +00:00
parent acdaad966c eb395db2d6
commit abdbaf6cbb
3 changed files with 152 additions and 38 deletions
--- 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/<service_id>/<template_type>/check/<upload_id>", methods=['GET'])
+@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>", 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/<service_id>/<template_type>/check/<upload_id>.<filetype>", methods=['GET'])
+@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>.<filetype>", 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/<service_id>/<template_type>/check/<upload_id>", methods=['POST'])
+@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>", 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))
--- a/app/templates/views/check/column-errors.html
+++ b/app/templates/views/check/column-errors.html
@@ -38,12 +38,18 @@
        <h1 class='banner-title' data-module="track-error" data-error-type="No rows" data-error-label="{{ upload_id }}">
          Your file is missing some rows
        </h1>
-        <p>
-          It needs at least one row of data, and {{ recipients.missing_column_headers | sort() | formatted_list(
-            prefix='a column called',
-            prefix_plural='columns called'
-          ) }}.
-        </p>
+        {% if recipients.missing_column_headers %}
+          <p>
+            It needs at least one row of data, and {{ recipients.missing_column_headers | sort() | formatted_list(
+              prefix='a column called',
+              prefix_plural='columns called'
+            ) }}.
+          </p>
+        {% else %}
+          <p>
+            It needs at least one row of data.
+          </p>
+        {% endif %}

      {% elif not recipients.has_recipient_columns %}

--- 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 <em>content</em> with & entity',
+    ),
+    (
+        {'row_index': 0},
+        'To: 07700900001',
+        'Test Service: A, Template <em>content</em> with & entity',
+    ),
+    (
+        {'row_index': 2},
+        'To: 07700900003',
+        'Test Service: C, Template <em>content</em> 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([
        '<td class="table-field-index"> <span class=""> 2 </span> </td>',
-        '<td class="table-field-center-aligned "> <div class=""> 07700900986 </div> </td>',
-        '<td class="table-field-center-aligned "> <div class=""> Jo </div> </td>',
+        '<td class="table-field-center-aligned "> <div class=""> 07700900001 </div> </td>',
+        '<td class="table-field-center-aligned "> <div class=""> A </div> </td>',
        (
            '<td class="table-field-center-aligned "> '
            '<div class="table-field-status-default"> '
@@ -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(