From 8c03feb334dbd9e04e3c80154ec674a1077c4f04 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 4 May 2017 09:30:55 +0100 Subject: [PATCH 1/9] Show one field at a time on send yourself a test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The send yourself a test feature is useful for two things: - constructing an email/text message/letter without uploading a CSV file - seeing what the thing your going to send will look like (either by getting it in your inbox or downloading the PDF) - learning the concept of placeholders, ie understanding they’re thing that gets populated with _stuff_ The problem we’re seeing is that the current UI breaks when a template has a lot of placeholders. This is especially apparent with letter templates, which have a minimum of 7 placeholders by virtue of the address. The idea behind having the form fields side-by-side was to help people understand the relationship between their spreadsheet columns and the placeholders. But this means that the page was doing a lot of work, trying to teach: - replacement of placeholders - link between placeholders and spreadsheet columns The latter is better explained by the example spreadsheet shown on the upload page. So it can safely be removed from the send yourself a test page – in other words the fields don’t need to be shown side by side. Showing them one-at-a-time works well because: - it’s really obvious, even on first use, what the page is asking you to do - as your step through each placeholder, you see the message build up with the data you’ve entered – you’re learning how replacement of placeholders works by repetition This also means adding a matching endpoint for viewing each step of making the test letter as a PDF/PNG because we can’t reuse the view of the template without any placeholders filled any more. --- app/main/forms.py | 25 ++- app/main/views/send.py | 179 ++++++++++++++---- app/templates/views/send-test.html | 24 +-- app/utils.py | 9 + tests/app/main/views/test_send.py | 280 +++++++++++++++++++++++++++-- tests/app/test_utils.py | 16 ++ 6 files changed, 466 insertions(+), 67 deletions(-) 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))) From 3106ea0e15a954e0422e2474d42be9f5d490cbd5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 16 May 2017 16:56:40 +0100 Subject: [PATCH 2/9] Make send yourself a test form stay at top of page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit You might need to scroll this page quite a lot to see where a placeholder appears in your template – especially if you have a long email or letter. One of the things I’m trying to stop happening so much is a lot of scrolling back and forth. This would happen if you were scrolling down to see the placeholder, then back up to fill in its value. So this commit makes the textbox ‘sticky’, ie it always stays at the top of the viewport, even when you scroll down. This lets you see the placeholder and the textbox side by side, no matter how long the template is. The code to do this mostly comes from the GOV.UK Frontend Toolkit (documented here: https://github.com/alphagov/govuk_frontend_toolkit/blob/d9489a987086471fe30b4b925a81c12cd198c91d/docs/javascript.md#stick-at-top-when-scrolling). I had to add some extra CSS to make it look good when it overlaps the content of the page, which the GOV.UK Frontend Toolkit implementation doesn’t really anticipate. --- app/assets/javascripts/main.js | 2 ++ .../stick-at-top-when-scrolling.scss | 30 +++++++++++++++++++ app/assets/stylesheets/main.scss | 1 + app/templates/views/send-test.html | 6 ++-- gulpfile.babel.js | 2 ++ 5 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 app/assets/stylesheets/components/stick-at-top-when-scrolling.scss diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index f5e930438..59cf901c8 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -1,5 +1,7 @@ $(() => $("time.timeago").timeago()); +$(() => GOVUK.stickAtTopWhenScrolling.init()); + $(() => GOVUK.modules.start()); $(() => $('.error-message').eq(0).parent('label').next('input').trigger('focus')); diff --git a/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss new file mode 100644 index 000000000..dcb6ed677 --- /dev/null +++ b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss @@ -0,0 +1,30 @@ +// CSS adapted from +// https://github.com/alphagov/govuk_frontend_toolkit/blob/d9489a987086471fe30b4b925a81c12cd198c91d/docs/javascript.md#stick-at-top-when-scrolling + +.js-stick-at-top-when-scrolling { + + overflow: hidden; + margin-left: -$gutter-half; + margin-top: -10px; + padding: 10px 0 0 $gutter-half; + + .form-group { + margin-bottom: 20px; + } + +} + +.content-fixed { + position: fixed; + top: 0; + background: $white; + z-index: 100; + margin-top: 0; + padding-right: $gutter-half; + border-bottom: 1px solid $border-colour; + box-shadow: 0 2px 0 0 rgba($border-colour, 0.2); +} + +.shim { + display: block; +} diff --git a/app/assets/stylesheets/main.scss b/app/assets/stylesheets/main.scss index a197fe5d0..29c376245 100644 --- a/app/assets/stylesheets/main.scss +++ b/app/assets/stylesheets/main.scss @@ -57,6 +57,7 @@ $path: '/static/images/'; @import 'components/list-entry'; @import 'components/letter'; @import 'components/live-search'; +@import 'components/stick-at-top-when-scrolling'; @import 'components/vendor/breadcrumbs'; @import 'components/vendor/responsive-embed'; diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index 5653db8b2..f743a1fce 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -26,9 +26,7 @@ {% endif %} - {{ template|string }} - -
+ {{ textbox( form.placeholder_value, hint='Optional' if optional_placeholder else None @@ -36,4 +34,6 @@ {{ page_footer('Next', back_link=back_link) }}
+ {{ template|string }} + {% endblock %} diff --git a/gulpfile.babel.js b/gulpfile.babel.js index 667f55499..a4d044fba 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -54,6 +54,8 @@ gulp.task('copy:govuk_template:images', () => gulp.src(paths.template + 'assets/ gulp.task('javascripts', () => gulp .src([ paths.toolkit + 'javascripts/govuk/modules.js', + paths.toolkit + 'javascripts/govuk/stop-scrolling-at-footer.js', + paths.toolkit + 'javascripts/govuk/stick-at-top-when-scrolling.js', paths.src + 'javascripts/detailsPolyfill.js', paths.src + 'javascripts/apiKey.js', paths.src + 'javascripts/autofocus.js', From 7169a7fbf8a6990362bc5072d41a0facc12f78f2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 16 May 2017 16:12:32 +0100 Subject: [PATCH 3/9] Focus form field on send yourself a test People are going to hammer through this form _fast_, so not making them click into the form field on every page load is a nice enhancement. Reuses the code written to do this on the page where you enter your verification code. --- app/templates/views/send-test.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index f743a1fce..c7b0b0e2e 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -26,7 +26,7 @@ {% endif %} -
+ {{ textbox( form.placeholder_value, hint='Optional' if optional_placeholder else None From 16c7d17329a620aee597737729770afe1461c2fd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 16 May 2017 17:17:11 +0100 Subject: [PATCH 4/9] Unfill current placeholder if revisiting page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we don’t do this then you can’t see where in the composed message the value for your placeholder will appear. --- app/main/views/send.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/main/views/send.py b/app/main/views/send.py index 17c72821b..5f1274dab 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -240,6 +240,7 @@ def send_test_step(service_id, template_id, step_index): ) template.values = get_normalised_send_test_values_from_session() + template.values[current_placeholder] = None return render_template( 'views/send-test.html', From e7896f283a0d34d433d032987ec90ef430dd0ba9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 16 May 2017 17:52:19 +0100 Subject: [PATCH 5/9] Cache letter page count on send yourself test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calculating the number of pages in a letter is quite slow. And the send yourself a test pages need to load _fast_. Since filling in placeholders is very unlikely to change the number of pages in the resultant letter, it’s pretty safe to cache that count, and makes the subsequent pages load a lot faster. --- app/main/views/send.py | 7 +++-- tests/app/main/views/test_send.py | 48 +++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 5f1274dab..64fb70eb1 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -162,6 +162,7 @@ def get_example_csv(service_id, template_id): @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_test(service_id, template_id): session['send_test_values'] = dict() + session['send_test_letter_page_count'] = None return redirect(url_for( '.send_test_step', service_id=service_id, @@ -178,6 +179,9 @@ def send_test_step(service_id, template_id, step_index): template = service_api_client.get_service_template(service_id, template_id)['data'] + if not session.get('send_test_letter_page_count'): + session['send_test_letter_page_count'] = get_page_count_for_letter(template) + template = get_template( template, current_service, @@ -189,7 +193,7 @@ def send_test_step(service_id, template_id, step_index): template_id=template_id, filetype='png', ), - page_count=get_page_count_for_letter(template), + page_count=session['send_test_letter_page_count'] ) placeholders = fields_to_fill_in(template) @@ -271,7 +275,6 @@ def send_test_preview(service_id, template_id, filetype): template_id=template_id, filetype='png', ), - page_count=get_page_count_for_letter(template), ) template.values = get_normalised_send_test_values_from_session() diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index b36fbec15..7e7eb1fee 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -421,6 +421,30 @@ def test_send_test_sms_message_with_placeholders_shows_first_field( assert session['send_test_values']['phone number'] == '07700 900762' +def test_send_test_letter_clears_previous_page_cache( + logged_in_platform_admin_client, + mocker, + service_one, + mock_login, + mock_get_service, + mock_get_service_letter_template, + fake_uuid, +): + + with logged_in_platform_admin_client.session_transaction() as session: + session['send_test_letter_page_count'] = 'WRONG' + + response = logged_in_platform_admin_client.get(url_for( + 'main.send_test', + service_id=service_one['id'], + template_id=fake_uuid, + )) + assert response.status_code == 302 + + with logged_in_platform_admin_client.session_transaction() as session: + assert session['send_test_letter_page_count'] is None + + def test_send_test_populates_field_from_session( logged_in_client, mocker, @@ -447,6 +471,30 @@ def test_send_test_populates_field_from_session( assert page.select('input')[0]['value'] == 'Jo' +def test_send_test_caches_page_count( + 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=99) + + response = logged_in_client.get( + url_for( + 'main.send_test', + service_id=service_one['id'], + template_id=fake_uuid, + ), + follow_redirects=True, + ) + with logged_in_client.session_transaction() as session: + assert session['send_test_letter_page_count'] == 99 + + def test_send_test_indicates_optional_address_columns( logged_in_client, mocker, From 5da24288f2ef9c5d2cbaef90d917544f31fd5420 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 May 2017 10:24:05 +0100 Subject: [PATCH 6/9] Remove unused import --- app/main/forms.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/main/forms.py b/app/main/forms.py index 24702adf9..a11a5c431 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1,7 +1,6 @@ import re import pytz -from flask_login import current_user from flask_wtf import Form from datetime import datetime, timedelta from notifications_utils.recipients import ( From c8a64aeb1908fbb2c3cddf2dd8f8253e7dd08cff Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 May 2017 10:32:22 +0100 Subject: [PATCH 7/9] Clean up unused test fixtures The combination of `service_one` and `logged_in_client` takes care of most of the permissions stuff. --- tests/app/main/views/test_send.py | 127 ++++++++++-------------------- 1 file changed, 40 insertions(+), 87 deletions(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 7e7eb1fee..8e616e1bf 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -48,19 +48,16 @@ def test_upload_files_in_different_formats( filename, acceptable_file, logged_in_client, - api_user_active, + service_one, mocker, - mock_login, - mock_get_service, mock_get_service_template, mock_s3_upload, - mock_has_permissions, fake_uuid, ): with open(filename, 'rb') as uploaded: response = logged_in_client.post( - url_for('main.send_messages', service_id=fake_uuid, template_id=fake_uuid), + url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), data={'file': (BytesIO(uploaded.read()), filename)}, content_type='multipart/form-data' ) @@ -81,13 +78,10 @@ def test_upload_files_in_different_formats( def test_upload_csvfile_with_errors_shows_check_page_with_errors( logged_in_client, - api_user_active, + service_one, mocker, - 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, @@ -103,13 +97,13 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( ) initial_upload = logged_in_client.post( - url_for('main.send_messages', service_id=fake_uuid, template_id=fake_uuid), + url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')}, content_type='multipart/form-data', follow_redirects=True ) reupload = logged_in_client.post( - url_for('main.check_messages', service_id=fake_uuid, template_type='sms', upload_id='abc123'), + url_for('main.check_messages', service_id=service_one['id'], template_type='sms', upload_id='abc123'), data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')}, content_type='multipart/form-data', follow_redirects=True @@ -191,20 +185,14 @@ def test_upload_csvfile_with_missing_columns_shows_error( def test_upload_csv_invalid_extension( logged_in_client, - api_user_active, - mocker, mock_login, - mock_get_service, + service_one, mock_get_service_template, - mock_s3_upload, - mock_has_permissions, - mock_get_users_by_service, - mock_get_detailed_service_for_today, fake_uuid, ): resp = logged_in_client.post( - url_for('main.send_messages', service_id=fake_uuid, template_id=fake_uuid), + url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), data={'file': (BytesIO('contents'.encode('utf-8')), 'invalid.txt')}, content_type='multipart/form-data', follow_redirects=True @@ -312,48 +300,38 @@ def test_send_test_doesnt_show_file_contents( def test_send_test_sms_message( logged_in_client, + service_one, + fake_uuid, mocker, - api_user_active, - mock_login, - mock_get_service, mock_get_service_template, mock_s3_upload, - mock_has_permissions, mock_get_users_by_service, mock_get_detailed_service_for_today, - fake_uuid ): expected_data = {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'Test message'} mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') response = logged_in_client.get( - url_for('main.send_test', service_id=fake_uuid, template_id=fake_uuid), + url_for('main.send_test', service_id=service_one['id'], template_id=fake_uuid), follow_redirects=True ) assert response.status_code == 200 - mock_s3_upload.assert_called_with(fake_uuid, expected_data, 'eu-west-1') + mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') 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 + service_one, + fake_uuid, ): response = logged_in_client.get( - url_for('main.send_test', service_id=fake_uuid, template_id=fake_uuid, help=1) + url_for('main.send_test', service_id=service_one['id'], template_id=fake_uuid, help=1) ) assert response.status_code == 302 assert response.location == url_for( 'main.send_test_step', - service_id=fake_uuid, + service_id=service_one['id'], template_id=fake_uuid, step_index=0, help=1, @@ -364,26 +342,23 @@ def test_send_test_sms_message_redirects_with_help_argument( def test_send_test_email_message_without_placeholders( logged_in_client, mocker, - api_user_active, - mock_login, - mock_get_service, + service_one, mock_get_service_email_template_without_placeholders, mock_s3_upload, - mock_has_permissions, mock_get_users_by_service, mock_get_detailed_service_for_today, - fake_uuid + fake_uuid, ): expected_data = {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'Test message'} mocker.patch('app.main.views.send.s3download', return_value='email address\r\ntest@user.gov.uk') response = logged_in_client.get( - url_for('main.send_test', service_id=fake_uuid, template_id=fake_uuid), + url_for('main.send_test', service_id=service_one['id'], template_id=fake_uuid), follow_redirects=True ) assert response.status_code == 200 - mock_s3_upload.assert_called_with(fake_uuid, expected_data, 'eu-west-1') + mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') def test_send_test_sms_message_with_placeholders_shows_first_field( @@ -569,15 +544,12 @@ def test_send_test_allows_empty_optional_address_columns( def test_send_test_sms_message_puts_submitted_data_in_session_and_file( logged_in_client, mocker, - api_user_active, - mock_login, - mock_get_service, + service_one, 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 + fake_uuid, ): with logged_in_client.session_transaction() as session: @@ -588,7 +560,7 @@ def test_send_test_sms_message_puts_submitted_data_in_session_and_file( response = logged_in_client.post( url_for( 'main.send_test_step', - service_id=fake_uuid, + service_id=service_one['id'], template_id=fake_uuid, step_index=0, ), @@ -602,7 +574,7 @@ def test_send_test_sms_message_puts_submitted_data_in_session_and_file( assert session['send_test_values']['name'] == 'Jo' mock_s3_upload.assert_called_with( - fake_uuid, + service_one['id'], { 'data': 'name,phone number\r\nJo,07700 900762\r\n', 'file_name': 'Test message' @@ -656,15 +628,8 @@ def test_send_test_works_as_letter_preview( 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 + service_one, + fake_uuid, ): with logged_in_client.session_transaction() as session: @@ -673,7 +638,7 @@ def test_send_test_clears_session( response = logged_in_client.get( url_for( 'main.send_test', - service_id=fake_uuid, + service_id=service_one['id'], template_id=fake_uuid, ), ) @@ -705,16 +670,13 @@ def test_download_example_csv( def test_upload_csvfile_with_valid_phone_shows_all_numbers( logged_in_client, - mocker, - api_user_active, - mock_login, - mock_get_service, mock_get_service_template, - mock_s3_upload, - mock_has_permissions, mock_get_users_by_service, mock_get_detailed_service_for_today, - fake_uuid + service_one, + fake_uuid, + mock_s3_upload, + mocker, ): mocker.patch( @@ -725,7 +687,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( ) response = logged_in_client.post( - url_for('main.send_messages', service_id=fake_uuid, template_id=fake_uuid), + url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), data={'file': (BytesIO(''.encode('utf-8')), 'valid.csv')}, content_type='multipart/form-data', follow_redirects=True @@ -742,7 +704,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( assert '07700 900750' not in content assert 'Only showing the first 50 rows' in content - mock_get_detailed_service_for_today.assert_called_once_with(fake_uuid) + mock_get_detailed_service_for_today.assert_called_once_with(service_one['id']) @pytest.mark.parametrize('service_mock, should_allow_international', [ @@ -784,12 +746,9 @@ def test_upload_csvfile_with_international_validates( def test_test_message_can_only_be_sent_now( logged_in_client, mocker, - api_user_active, - mock_login, - mock_get_service, + service_one, mock_get_service_template, mock_s3_download, - mock_has_permissions, mock_get_users_by_service, mock_get_detailed_service_for_today, fake_uuid @@ -804,7 +763,7 @@ def test_test_message_can_only_be_sent_now( } response = logged_in_client.get(url_for( 'main.check_messages', - service_id=fake_uuid, + service_id=service_one['id'], upload_id=fake_uuid, template_type='sms', from_test=True @@ -817,10 +776,9 @@ def test_test_message_can_only_be_sent_now( def test_letter_can_only_be_sent_now( logged_in_client, mocker, - mock_get_service, + service_one, mock_get_service_letter_template, mock_s3_download, - mock_has_permissions, mock_get_users_by_service, mock_get_detailed_service_for_today, fake_uuid, @@ -838,7 +796,7 @@ def test_letter_can_only_be_sent_now( response = logged_in_client.get(url_for( 'main.check_messages', - service_id=fake_uuid, + service_id=service_one['id'], upload_id=fake_uuid, template_type='letter', from_test=True @@ -848,15 +806,12 @@ def test_letter_can_only_be_sent_now( assert 'name="scheduled_for"' not in content -@pytest.mark.parametrize( - 'when', [ - '', '2016-08-25T13:04:21.767198' - ] -) +@pytest.mark.parametrize('when', [ + '', '2016-08-25T13:04:21.767198' +]) def test_create_job_should_call_api( logged_in_client, service_one, - active_user_with_permissions, mock_create_job, mock_get_job, mock_get_notifications, @@ -991,13 +946,11 @@ def test_dont_show_preview_letter_templates_for_bad_filetype( def test_check_messages_should_revalidate_file_when_uploading_file( logged_in_client, service_one, - active_user_with_permissions, mock_create_job, mock_get_job, mock_get_service_template_with_placeholders, mock_s3_upload, mocker, - mock_has_permissions, mock_get_detailed_service_for_today, mock_get_users_by_service, fake_uuid @@ -1020,7 +973,7 @@ def test_check_messages_should_revalidate_file_when_uploading_file( 'notification_count': data['notification_count'], 'valid': True} response = logged_in_client.post( - url_for('main.start_job', service_id=service_id, upload_id=data['id']), + url_for('main.start_job', service_id=service_one['id'], upload_id=data['id']), data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')}, content_type='multipart/form-data', follow_redirects=True From cd7c27925ced46a7dba24d928a0468ca6191e2f7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 May 2017 11:49:15 +0100 Subject: [PATCH 8/9] =?UTF-8?q?Check=20that=20users=20can=E2=80=99t=20skip?= =?UTF-8?q?=20steps=20in=20send=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because we put the step in the URL, users could: - skip ahead to a later step - navigate to a step which doesn’t exist (ie an index greater than the number of placeholders) This commit adds some checks to do the sensible thing in the unlikely event that either of these situations occur. --- app/main/views/send.py | 12 +++- tests/app/main/views/test_send.py | 115 ++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 1 deletion(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 64fb70eb1..638f67cc0 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -177,6 +177,11 @@ def send_test(service_id, template_id): @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_test_step(service_id, template_id, step_index): + if 'send_test_values' not in session: + return redirect(url_for( + '.send_test', service_id=service_id, template_id=template_id + )) + template = service_api_client.get_service_template(service_id, template_id)['data'] if not session.get('send_test_letter_page_count'): @@ -201,7 +206,12 @@ def send_test_step(service_id, template_id, step_index): if len(placeholders) == 0: return make_and_upload_csv_file(service_id, template) - current_placeholder = placeholders[step_index] + try: + current_placeholder = placeholders[step_index] + except IndexError: + return redirect(url_for( + '.send_test', service_id=service_id, template_id=template_id + )) optional_placeholder = (current_placeholder in optional_address_columns) form = get_placeholder_form_instance( current_placeholder, diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 8e616e1bf..a90d690f1 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -21,6 +21,8 @@ from tests.conftest import ( mock_get_service_letter_template, mock_get_service, mock_get_international_service, + mock_get_service_template, + mock_get_service_email_template, ) template_types = ['email', 'sms'] @@ -320,6 +322,119 @@ def test_send_test_sms_message( mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') +def test_send_test_step_redirects_if_session_not_setup( + logged_in_client, + service_one, + fake_uuid, + mock_get_service_email_template, +): + + 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_step', service_id=service_one['id'], template_id=fake_uuid, step_index=0), + follow_redirects=True + ) + assert response.status_code == 200 + + with logged_in_client.session_transaction() as session: + assert session['send_test_values'] == {'email address': 'test@user.gov.uk'} + + +def test_send_test_redirects_to_end_if_step_out_of_bounds( + logged_in_client, + service_one, + fake_uuid, + mock_get_service_template, + mock_s3_upload, + mock_get_users_by_service, + mock_get_detailed_service_for_today, +): + + with logged_in_client.session_transaction() as session: + session['send_test_values'] = {'name': 'foo'} + + response = logged_in_client.get(url_for( + 'main.send_test_step', + service_id=service_one['id'], + template_id=fake_uuid, + step_index=999, + )) + + assert response.status_code == 302 + expected_url = url_for( + 'main.check_messages', + service_id=service_one['id'], + upload_id=fake_uuid, + template_type='sms', + _external=True, + ) + assert response.location in ( + expected_url + '?help=0&from_test=True', + expected_url + '?from_test=True&help=0', + ) + + +def test_send_test_redirects_to_start_if_you_skip_steps( + logged_in_platform_admin_client, + service_one, + fake_uuid, + mock_get_service_letter_template, + mock_s3_upload, + mock_get_users_by_service, + mock_get_detailed_service_for_today, + mocker, +): + + with logged_in_platform_admin_client.session_transaction() as session: + session['send_test_letter_page_count'] = 1 + session['send_test_values'] = {'address_line_1': 'foo'} + + response = logged_in_platform_admin_client.get(url_for( + 'main.send_test_step', + service_id=service_one['id'], + template_id=fake_uuid, + step_index=7, # letter template has 7 placeholders – we’re at the end + )) + assert response.status_code == 302 + assert response.location == url_for( + 'main.send_test', + service_id=service_one['id'], + template_id=fake_uuid, + _external=True, + ) + + +def test_send_test_redirects_to_start_if_index_out_of_bounds_and_some_placeholders_empty( + logged_in_client, + service_one, + fake_uuid, + mock_get_service_email_template, + mock_s3_download, + mock_get_users_by_service, + mock_get_detailed_service_for_today, +): + + with logged_in_client.session_transaction() as session: + session['send_test_values'] = {'name': 'foo'} + + response = logged_in_client.get(url_for( + 'main.send_test_step', + service_id=service_one['id'], + template_id=fake_uuid, + step_index=999, + )) + + assert response.status_code == 302 + assert response.location == url_for( + 'main.send_test', + service_id=service_one['id'], + template_id=fake_uuid, + _external=True, + ) + + def test_send_test_sms_message_redirects_with_help_argument( logged_in_client, service_one, From 1d76b22bc0090580e4ccbfb43e2f5d88d86f2bc7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 May 2017 12:02:09 +0100 Subject: [PATCH 9/9] Add extra tests to make sure that the form is safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous implementations of this functionality mutated the base form class, which broke a bunch of stuff. I want to make sure that getting this form for one placeholder doesn’t change other forms that have already been instantiated for other placeholders. Mutation is scary. --- tests/app/main/test_placeholder_form.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 tests/app/main/test_placeholder_form.py diff --git a/tests/app/main/test_placeholder_form.py b/tests/app/main/test_placeholder_form.py new file mode 100644 index 000000000..07fac6675 --- /dev/null +++ b/tests/app/main/test_placeholder_form.py @@ -0,0 +1,18 @@ +from app.main.forms import get_placeholder_form_instance +from wtforms import Label + + +def test_form_class_not_mutated(app_): + + with app_.test_request_context( + method='POST', + data={'placeholder_value': ''} + ) as req: + form1 = get_placeholder_form_instance('name', {}, optional_placeholder=False) + form2 = get_placeholder_form_instance('city', {}, optional_placeholder=True) + + assert not form1.validate_on_submit() + assert form2.validate_on_submit() + + assert str(form1.placeholder_value.label) == '' + assert str(form2.placeholder_value.label) == ''