diff --git a/app/main/forms.py b/app/main/forms.py index a688c8bdb..24702adf9 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1,6 +1,6 @@ import re - import pytz + from flask_login import current_user from flask_wtf import Form from datetime import datetime, timedelta @@ -619,3 +619,26 @@ class ChooseTemplateType(Form): class SearchTemplatesForm(Form): search = SearchField('Search by name') + + +class PlaceholderForm(Form): + + pass + + +def get_placeholder_form_instance( + placeholder_name, + dict_to_populate_from, + optional_placeholder=False +): + + PlaceholderForm.placeholder_value = StringField( + placeholder_name, + validators=[ + DataRequired(message='Can’t be empty') + ] if not optional_placeholder else [] + ) + + return PlaceholderForm( + placeholder_value=dict_to_populate_from.get(placeholder_name, '') + ) diff --git a/app/main/views/send.py b/app/main/views/send.py index 78791a15e..17c72821b 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -21,10 +21,20 @@ from flask import ( from flask_login import login_required, current_user from notifications_utils.columns import Columns -from notifications_utils.recipients import RecipientCSV, first_column_headings, validate_and_format_phone_number +from notifications_utils.recipients import ( + RecipientCSV, + first_column_headings, + validate_and_format_phone_number, + optional_address_columns, +) from app.main import main -from app.main.forms import CsvUploadForm, ChooseTimeForm, get_furthest_possible_scheduled_time +from app.main.forms import ( + CsvUploadForm, + ChooseTimeForm, + get_furthest_possible_scheduled_time, + get_placeholder_form_instance +) from app.main.uploader import ( s3upload, s3download @@ -147,12 +157,24 @@ def get_example_csv(service_id, template_id): } -@main.route("/services//send//test", methods=['GET', 'POST']) +@main.route("/services//send//test") @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_test(service_id, template_id): + session['send_test_values'] = dict() + return redirect(url_for( + '.send_test_step', + service_id=service_id, + template_id=template_id, + step_index=0, + help=get_help_argument(), + )) - file_name = current_app.config['TEST_MESSAGE_FILENAME'] + +@main.route("/services//send//test/step-", methods=['GET', 'POST']) +@login_required +@user_has_permissions('send_texts', 'send_emails', 'send_letters') +def send_test_step(service_id, template_id, step_index): template = service_api_client.get_service_template(service_id, template_id)['data'] @@ -162,48 +184,100 @@ def send_test(service_id, template_id): show_recipient=True, expand_emails=True, letter_preview_url=url_for( - '.view_letter_template_preview', + '.send_test_preview', service_id=service_id, template_id=template_id, filetype='png', - page_count=get_page_count_for_letter(template), ), + page_count=get_page_count_for_letter(template), ) - if len(template.placeholders) == 0 or request.method == 'POST': - upload_id = s3upload( - service_id, - { - 'file_name': file_name, - 'data': Spreadsheet.from_rows([ - first_column_headings[template.template_type] + list(template.placeholders), - get_example_csv_rows(template, use_example_as_example=False, submitted_fields=request.form) - ]).as_csv_data - }, - current_app.config['AWS_REGION'] - ) - session['upload_data'] = { - "template_id": template_id, - "original_file_name": file_name - } + placeholders = fields_to_fill_in(template) + + if len(placeholders) == 0: + return make_and_upload_csv_file(service_id, template) + + current_placeholder = placeholders[step_index] + optional_placeholder = (current_placeholder in optional_address_columns) + form = get_placeholder_form_instance( + current_placeholder, + dict_to_populate_from=get_normalised_send_test_values_from_session(), + optional_placeholder=optional_placeholder, + ) + + if form.validate_on_submit(): + + session['send_test_values'][current_placeholder] = form.placeholder_value.data + + if all( + get_normalised_send_test_values_from_session().get(placeholder, False) not in (False, None) + for placeholder in placeholders + ): + return make_and_upload_csv_file(service_id, template) + return redirect(url_for( - '.check_messages', - upload_id=upload_id, + '.send_test_step', service_id=service_id, - template_type=template.template_type, - from_test=True, - help=2 if request.args.get('help') else 0 + template_id=template_id, + step_index=step_index + 1, + help=get_help_argument(), )) + if get_help_argument(): + back_link = None + elif step_index == 0: + back_link = url_for( + '.view_template', + service_id=service_id, + template_id=template_id, + ) + else: + back_link = url_for( + '.send_test_step', + service_id=service_id, + template_id=template_id, + step_index=step_index - 1, + ) + + template.values = get_normalised_send_test_values_from_session() + return render_template( 'views/send-test.html', template=template, - recipient_columns=first_column_headings[template.template_type], - example=[get_example_csv_rows(template, use_example_as_example=False)], - help=get_help_argument() + form=form, + optional_placeholder=optional_placeholder, + help=get_help_argument(), + back_link=back_link, ) +@main.route("/services//send//test.", methods=['GET']) +@login_required +@user_has_permissions('send_texts', 'send_emails', 'send_letters') +def send_test_preview(service_id, template_id, filetype): + + if filetype not in ('pdf', 'png'): + abort(404) + + template = service_api_client.get_service_template(service_id, template_id)['data'] + + template = get_template( + template, + current_service, + letter_preview_url=url_for( + '.send_test_preview', + service_id=service_id, + template_id=template_id, + filetype='png', + ), + page_count=get_page_count_for_letter(template), + ) + + template.values = get_normalised_send_test_values_from_session() + + 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): if not session.get('upload_data'): @@ -380,3 +454,48 @@ def get_check_messages_back_url(service_id, template_type): return url_for('.send_test', service_id=service_id, template_id=templates[0]['id'], help=1) return url_for('main.choose_template', service_id=service_id) + + +def fields_to_fill_in(template): + + recipient_columns = first_column_headings[template.template_type] + + if 'letter' == template.template_type: + return recipient_columns + list(template.placeholders) + + session['send_test_values'][recipient_columns[0]] = { + 'email': current_user.email_address, + 'sms': current_user.mobile_number, + }.get(template.template_type) + + return list(template.placeholders) + + +def get_normalised_send_test_values_from_session(): + return { + key: ''.join(value or []) + for key, value in session.get('send_test_values', {}).items() + } + + +def make_and_upload_csv_file(service_id, template): + upload_id = s3upload( + service_id, + Spreadsheet.from_dict( + session['send_test_values'], + filename=current_app.config['TEST_MESSAGE_FILENAME'] + ).as_dict, + current_app.config['AWS_REGION'], + ) + session['upload_data'] = { + "template_id": template.id, + "original_file_name": current_app.config['TEST_MESSAGE_FILENAME'] + } + return redirect(url_for( + '.check_messages', + upload_id=upload_id, + service_id=service_id, + template_type=template.template_type, + from_test=True, + help=2 if get_help_argument() else 0 + )) diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index 51c827330..5653db8b2 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -1,6 +1,7 @@ {% extends "withnav_template.html" %} {% from "components/page-footer.html" import page_footer %} {% from "components/file-upload.html" import file_upload %} +{% from "components/textbox.html" import textbox %} {% from "components/table.html" import list_table, field, text_field, index_field, index_field_heading %} {% block service_page_title %} @@ -28,26 +29,11 @@ {{ template|string }}
- {% call(item, row_number) list_table( - example, - caption="Fill in the {}".format('field' if (recipient_columns + template.placeholders|list)|length == 2 else 'fields'), - field_headings=recipient_columns + template.placeholders|list - ) %} - {% for column in item %} - {% call field() %} - {% if loop.index > 1 or template.template_type == 'letter' %} - - - {% else %} - {{ column }} - {% endif %} - {% endcall %} - {% endfor %} - {% endcall %} - - {{ page_footer("Preview", back_link=( - url_for('.send_messages', service_id=current_service.id, template_id=template.id)) if not help else None + {{ textbox( + form.placeholder_value, + hint='Optional' if optional_placeholder else None ) }} + {{ page_footer('Next', back_link=back_link) }}
{% endblock %} diff --git a/app/utils.py b/app/utils.py index c345643f0..aa1487af4 100644 --- a/app/utils.py +++ b/app/utils.py @@ -223,6 +223,15 @@ class Spreadsheet(): output.writerow(row) return cls(converted.getvalue(), filename) + @classmethod + def from_dict(cls, dictionary, filename=''): + return cls.from_rows( + zip( + *sorted(dictionary.items(), key=lambda pair: pair[0]) + ), + filename + ) + @classmethod def from_file(cls, file_content, filename=''): extension = cls.get_extension(filename) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 52d63ff57..b36fbec15 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -8,7 +8,7 @@ from functools import partial import pytest from bs4 import BeautifulSoup from flask import url_for -from notifications_utils.template import LetterPreviewTemplate +from notifications_utils.template import LetterPreviewTemplate, LetterImageTemplate from notifications_utils.recipients import RecipientCSV from app.main.views.send import get_check_messages_back_url @@ -284,7 +284,7 @@ def test_upload_valid_csv_shows_file_contents( def test_send_test_doesnt_show_file_contents( logged_in_client, mocker, - mock_get_service_template_with_placeholders, + mock_get_service_template, mock_s3_upload, mock_get_users_by_service, mock_get_detailed_service_for_today, @@ -293,13 +293,12 @@ def test_send_test_doesnt_show_file_contents( ): mocker.patch('app.main.views.send.s3download', return_value=""" - phone number,name,thing,thing,thing - 07700900986, Jo, foo, foo, foo + phone number + 07700 900 986 """) - response = logged_in_client.post( + response = logged_in_client.get( url_for('main.send_test', service_id=service_one['id'], template_id=fake_uuid), - data={}, follow_redirects=True, ) @@ -336,7 +335,33 @@ def test_send_test_sms_message( mock_s3_upload.assert_called_with(fake_uuid, expected_data, 'eu-west-1') -def test_send_test_email_message( +def test_send_test_sms_message_redirects_with_help_argument( + logged_in_client, + mocker, + api_user_active, + mock_login, + mock_get_service, + mock_get_service_template, + mock_has_permissions, + mock_get_users_by_service, + mock_get_detailed_service_for_today, + fake_uuid +): + response = logged_in_client.get( + url_for('main.send_test', service_id=fake_uuid, template_id=fake_uuid, help=1) + ) + assert response.status_code == 302 + assert response.location == url_for( + 'main.send_test_step', + service_id=fake_uuid, + template_id=fake_uuid, + step_index=0, + help=1, + _external=True, + ) + + +def test_send_test_email_message_without_placeholders( logged_in_client, mocker, api_user_active, @@ -361,7 +386,139 @@ def test_send_test_email_message( mock_s3_upload.assert_called_with(fake_uuid, expected_data, 'eu-west-1') -def test_send_test_sms_message_with_placeholders( +def test_send_test_sms_message_with_placeholders_shows_first_field( + logged_in_client, + mocker, + service_one, + mock_login, + mock_get_service, + mock_get_service_template_with_placeholders, + fake_uuid, +): + + with logged_in_client.session_transaction() as session: + assert 'send_test_values' not in session + + response = logged_in_client.get( + url_for( + 'main.send_test', + service_id=service_one['id'], + template_id=fake_uuid, + ), + follow_redirects=True, + ) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert page.select('label')[0].text.strip() == 'name' + assert page.select('input')[0]['name'] == 'placeholder_value' + assert page.select('.page-footer-back-link')[0]['href'] == url_for( + 'main.view_template', + service_id=service_one['id'], + template_id=fake_uuid, + ) + with logged_in_client.session_transaction() as session: + assert session['send_test_values']['phone number'] == '07700 900762' + + +def test_send_test_populates_field_from_session( + logged_in_client, + mocker, + service_one, + mock_login, + mock_get_service, + mock_get_service_template_with_placeholders, + fake_uuid, +): + + with logged_in_client.session_transaction() as session: + session['send_test_values'] = {} + session['send_test_values']['name'] = 'Jo' + + response = logged_in_client.get(url_for( + 'main.send_test_step', + service_id=service_one['id'], + template_id=fake_uuid, + step_index=0, + )) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert page.select('input')[0]['value'] == 'Jo' + + +def test_send_test_indicates_optional_address_columns( + logged_in_client, + mocker, + service_one, + mock_login, + mock_get_service, + mock_get_service_letter_template, + fake_uuid, +): + + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1) + + with logged_in_client.session_transaction() as session: + session['send_test_values'] = {} + + response = logged_in_client.get(url_for( + 'main.send_test_step', + service_id=service_one['id'], + template_id=fake_uuid, + step_index=3, + )) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert normalize_spaces(page.select('label')[0].text) == ( + 'address line 4 ' + 'Optional' + ) + assert page.select('.page-footer-back-link')[0]['href'] == url_for( + 'main.send_test_step', + service_id=service_one['id'], + template_id=fake_uuid, + step_index=2, + ) + + +def test_send_test_allows_empty_optional_address_columns( + logged_in_client, + mocker, + service_one, + mock_login, + mock_get_service, + mock_get_service_letter_template, + fake_uuid, +): + + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1) + + with logged_in_client.session_transaction() as session: + session['send_test_values'] = {} + + response = logged_in_client.post( + url_for( + 'main.send_test_step', + service_id=service_one['id'], + template_id=fake_uuid, + step_index=3, + ), + # no data here + ) + + assert response.status_code == 302 + assert response.location == url_for( + 'main.send_test_step', + service_id=service_one['id'], + template_id=fake_uuid, + step_index=4, + _external=True, + ) + + +def test_send_test_sms_message_puts_submitted_data_in_session_and_file( logged_in_client, mocker, api_user_active, @@ -375,23 +532,107 @@ def test_send_test_sms_message_with_placeholders( fake_uuid ): - expected_data = { - 'data': 'phone number,name\r\n07700 900762,Jo\r\n', - 'file_name': 'Test message' - } + with logged_in_client.session_transaction() as session: + session['send_test_values'] = {} + mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') response = logged_in_client.post( url_for( - 'main.send_test', + 'main.send_test_step', service_id=fake_uuid, - template_id=fake_uuid + template_id=fake_uuid, + step_index=0, ), - data={'name': 'Jo'}, - follow_redirects=True + data={'placeholder_value': 'Jo'}, + follow_redirects=True, ) assert response.status_code == 200 - mock_s3_upload.assert_called_with(fake_uuid, expected_data, 'eu-west-1') + + with logged_in_client.session_transaction() as session: + assert session['send_test_values']['phone number'] == '07700 900762' + assert session['send_test_values']['name'] == 'Jo' + + mock_s3_upload.assert_called_with( + fake_uuid, + { + 'data': 'name,phone number\r\nJo,07700 900762\r\n', + 'file_name': 'Test message' + }, + 'eu-west-1' + ) + + +@pytest.mark.parametrize('filetype', ['pdf', 'png']) +def test_send_test_works_as_letter_preview( + filetype, + logged_in_platform_admin_client, + mock_get_service_letter_template, + mock_get_users_by_service, + mock_get_detailed_service_for_today, + service_one, + fake_uuid, + mocker, +): + service_one['can_send_letters'] = True + mocker.patch('app.service_api_client.get_service', return_value={"data": service_one}) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1) + mocked_preview = mocker.patch( + 'app.main.views.send.TemplatePreview.from_utils_template', + return_value='foo' + ) + + service_id = service_one['id'] + template_id = fake_uuid + with logged_in_platform_admin_client.session_transaction() as session: + session['send_test_values'] = {'address_line_1': 'Jo Lastname'} + response = logged_in_platform_admin_client.get( + url_for( + 'main.send_test_preview', + service_id=service_id, + template_id=template_id, + filetype=filetype + ) + ) + + mock_get_service_letter_template.assert_called_with(service_id, template_id) + + assert response.status_code == 200 + assert response.get_data(as_text=True) == 'foo' + assert mocked_preview.call_args[0][0].id == template_id + assert type(mocked_preview.call_args[0][0]) == LetterImageTemplate + assert mocked_preview.call_args[0][0].values == {'address_line_1': 'Jo Lastname'} + assert mocked_preview.call_args[0][1] == filetype + + +def test_send_test_clears_session( + logged_in_client, + mocker, + api_user_active, + mock_login, + mock_get_service, + mock_get_service_template_with_placeholders, + mock_s3_upload, + mock_has_permissions, + mock_get_users_by_service, + mock_get_detailed_service_for_today, + fake_uuid +): + + with logged_in_client.session_transaction() as session: + session['send_test_values'] = {'foo': 'bar'} + + response = logged_in_client.get( + url_for( + 'main.send_test', + service_id=fake_uuid, + template_id=fake_uuid, + ), + ) + assert response.status_code == 302 + + with logged_in_client.session_transaction() as session: + assert session['send_test_values'] == {} def test_download_example_csv( @@ -527,6 +768,7 @@ def test_test_message_can_only_be_sent_now( def test_letter_can_only_be_sent_now( logged_in_client, + mocker, mock_get_service, mock_get_service_letter_template, mock_s3_download, @@ -536,6 +778,8 @@ def test_letter_can_only_be_sent_now( fake_uuid, ): + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1) + with logged_in_client.session_transaction() as session: session['upload_data'] = { 'original_file_name': 'Test message', @@ -543,6 +787,7 @@ def test_letter_can_only_be_sent_now( 'notification_count': 1, 'valid': True } + response = logged_in_client.get(url_for( 'main.check_messages', service_id=fake_uuid, @@ -634,6 +879,7 @@ def test_should_show_preview_letter_message( ): service_one['can_send_letters'] = True mocker.patch('app.service_api_client.get_service', return_value={"data": service_one}) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1) mocker.patch( 'app.main.views.send.s3download', diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 34892581f..0c0da896e 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -4,6 +4,8 @@ from csv import DictReader import pytest +from collections import OrderedDict + from app.utils import ( email_safe, generate_notifications_csv, @@ -105,6 +107,20 @@ def test_can_create_spreadsheet_from_large_excel_file(): assert ret.as_csv_data +def test_can_create_spreadsheet_from_dict(): + assert Spreadsheet.from_dict(OrderedDict( + foo='bar', + name='Jane', + )).as_csv_data == ( + "foo,name\r\n" + "bar,Jane\r\n" + ) + + +def test_can_create_spreadsheet_from_dict_with_filename(): + assert Spreadsheet.from_dict({}, filename='empty.csv').as_dict['file_name'] == "empty.csv" + + def test_generate_notifications_csv_returns_correct_csv_file(_get_notifications_csv_mock): csv_content = generate_notifications_csv(service_id='1234') csv_file = DictReader(StringIO('\n'.join(csv_content)))