From c4d839d4f55d0db6c8c510c8e92839f354460c4a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 10 Mar 2020 15:12:19 +0000 Subject: [PATCH] input letter address data in a single block rather than in multiple placeholders - this is the first step towards making postcodes non-required, which is the first step towards international letters. they still populate address_line_# and postcode fields under the hood - to keep validation working the same, the last line always goes into `postcode`. the form normalises whitespace, removes extra new lines, and enforces that you have between three and seven lines. if the letter repeats address placeholders further down (eg "Dear ((address_line_1))"), then it'll fill those in as well. It'll still prompt you to fill them in, but they'll be pre-filled. --- app/main/forms.py | 62 ++++- app/main/views/send.py | 72 ++++++ app/navigation.py | 4 + .../views/send-one-off-letter-address.html | 45 ++++ app/templates/views/templates/_template.html | 10 +- tests/app/main/views/test_send.py | 218 +++++++++++++++++- tests/conftest.py | 13 ++ 7 files changed, 411 insertions(+), 13 deletions(-) create mode 100644 app/templates/views/send-one-off-letter-address.html diff --git a/app/main/forms.py b/app/main/forms.py index 85639dfa7..a665db984 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1,3 +1,4 @@ +import re import weakref from datetime import datetime, timedelta from itertools import chain @@ -9,12 +10,17 @@ from flask_wtf import FlaskForm as Form from flask_wtf.file import FileAllowed from flask_wtf.file import FileField as FileField_wtf from notifications_utils.columns import Columns -from notifications_utils.formatters import strip_whitespace +from notifications_utils.formatters import ( + normalise_whitespace_and_newlines, + remove_whitespace_before_punctuation, + strip_whitespace, +) from notifications_utils.recipients import ( InvalidPhoneError, normalise_phone_number, validate_phone_number, ) +from notifications_utils.take import Take from wtforms import ( BooleanField, DateField, @@ -362,6 +368,23 @@ class StripWhitespaceStringField(StringField): super(StringField, self).__init__(label, **kwargs) +class StripWhitespaceTextAreaField(TextAreaField): + def process_formdata(self, valuelist): + if valuelist: + self.data = Take( + valuelist[0] + ).then( + remove_whitespace_before_punctuation + ).then( + normalise_whitespace_and_newlines + ).then( + # similar to normalise_multiple_newlines but taking everything down to one `\n` instead of two + lambda value: re.compile(r'\n{2,}').sub('\n', value) + ).then( + str.strip + ) + + class OnOffField(RadioField): def __init__(self, label, choices=None, *args, **kwargs): @@ -741,6 +764,43 @@ class SMSTemplateForm(BaseTemplateForm): OnlySMSCharacters()(None, field) +class LetterAddressForm(StripWhitespaceForm): + MIN_ADDRESS_LINES = 3 + MAX_ADDRESS_LINES = 7 + + address = StripWhitespaceTextAreaField( + 'Address', + validators=[DataRequired(message="Cannot be empty")] + ) + + def validate_address(self, field): + lines = field.data.splitlines() + if len(lines) < self.MIN_ADDRESS_LINES: + raise ValidationError('Address must be at least 3 lines long') + if len(lines) > self.MAX_ADDRESS_LINES: + raise ValidationError('Address must be no more than 7 lines long') + + @property + def as_address_lines_1_to_7_with_postcode(self): + lines = self.address.data.splitlines() + placeholders = {} + + # set all placeholders to empty strings, or all_placeholders_in_session will always return false. + # note that it must be `address line #` with spaces, not underscores or dashes + for i in range(1, 7): + placeholders[f'address line {i}'] = '' + + # unroll the address into lines, and place into the session in the underlying placeholder names + # postcode is required so make sure we put the last value in that + # TODO: When postcode is no longer a required field, remove this special case and just use `address line #` + address_lines, last_address_line = lines[:-1], lines[-1] + for i, line in enumerate(address_lines, start=1): + placeholders[f'address line {i}'] = line + placeholders['postcode'] = last_address_line + + return placeholders + + class EmailTemplateForm(BaseTemplateForm): subject = TextAreaField( u'Subject', diff --git a/app/main/views/send.py b/app/main/views/send.py index 7784dd902..8db4d931e 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -39,6 +39,7 @@ from app.main import main, no_cookie from app.main.forms import ( ChooseTimeForm, CsvUploadForm, + LetterAddressForm, SetSenderForm, get_placeholder_form_instance, ) @@ -307,6 +308,11 @@ def send_test(service_id, template_id): return_to='view_template', template_id=template_id)) + if db_template['template_type'] == 'letter': + return redirect( + url_for('.send_one_off_letter_address', service_id=service_id, template_id=template_id) + ) + return redirect(url_for( { 'main.send_test': '.send_test_step', @@ -329,6 +335,72 @@ def get_notification_check_endpoint(service_id, template): )) +@main.route( + "/services//send//one-off/address", + methods=['GET', 'POST'] +) +@user_has_permissions('send_messages', restrict_admin_usage=True) +def send_one_off_letter_address(service_id, template_id): + if {'recipient', 'placeholders'} - set(session.keys()): + # if someone has come here via a bookmark or back button they might have some stuff still in their session + return redirect(url_for('.send_one_off', service_id=service_id, template_id=template_id)) + + db_template = current_service.get_template_with_user_permission_or_403(template_id, current_user) + + session['send_test_letter_page_count'] = get_page_count_for_letter(db_template) + + template = get_template( + db_template, + current_service, + show_recipient=True, + letter_preview_url=url_for( + 'no_cookie.send_test_preview', + service_id=service_id, + template_id=template_id, + filetype='png', + ), + page_count=session['send_test_letter_page_count'], + email_reply_to=None, + sms_sender=None + ) + + form = LetterAddressForm() + + if form.validate_on_submit(): + session['placeholders'].update(form.as_address_lines_1_to_7_with_postcode) + + placeholders = fields_to_fill_in( + template, + prefill_current_user=(request.endpoint == 'main.send_test_step'), + ) + if all_placeholders_in_session(placeholders): + return get_notification_check_endpoint(service_id, template) + + first_non_address_placeholder_index = len(first_column_headings['letter']) + return redirect(url_for( + 'main.send_one_off_step', + service_id=service_id, + template_id=template_id, + step_index=first_non_address_placeholder_index, + )) + + return render_template( + 'views/send-one-off-letter-address.html', + page_title=get_send_test_page_title( + template_type='letter', + help_argument=None, + entering_recipient=True, + name=template.name, + ), + template=template, + form=form, + optional_placeholder=False, + back_link=get_back_link(service_id, template, 0), + help=False, + link_to_upload=True, + ) + + @main.route( "/services//send//test/step-", methods=['GET', 'POST'], diff --git a/app/navigation.py b/app/navigation.py index 0f11b3df1..7343559d6 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -259,6 +259,7 @@ class HeaderNavigation(Navigation): 'send_messages', 'send_notification', 'send_one_off', + 'send_one_off_letter_address', 'send_one_off_step', 'send_test', 'no_cookie.send_test_preview', @@ -386,6 +387,7 @@ class MainNavigation(Navigation): 'manage_template_folder', 'send_messages', 'send_one_off', + 'send_one_off_letter_address', 'send_one_off_step', 'send_test', 'no_cookie.send_test_preview', @@ -673,6 +675,7 @@ class CaseworkNavigation(Navigation): 'choose_from_contact_list', 'choose_template', 'send_one_off', + 'send_one_off_letter_address', 'send_one_off_step', 'send_test', 'send_test_step', @@ -1156,6 +1159,7 @@ class OrgNavigation(Navigation): 'send_messages', 'send_notification', 'send_one_off', + 'send_one_off_letter_address', 'send_one_off_step', 'send_test', 'no_cookie.send_test_preview', diff --git a/app/templates/views/send-one-off-letter-address.html b/app/templates/views/send-one-off-letter-address.html new file mode 100644 index 000000000..2bed9c15a --- /dev/null +++ b/app/templates/views/send-one-off-letter-address.html @@ -0,0 +1,45 @@ +{% extends "withnav_template.html" %} +{% from "components/page-header.html" import page_header %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/message-count-label.html" import recipient_count_label %} +{% from "components/textbox.html" import textbox %} +{% from "components/form.html" import form_wrapper %} + +{% block service_page_title %} + {{ page_title }} +{% endblock %} + +{% block maincolumn_content %} + + {{ page_header( + page_title, + back_link=back_link + ) }} + + {% call form_wrapper( + class='send-one-off-form', + module="autofocus", + data_kwargs={'force-focus': True} + ) %} +
+
+ {{ textbox( + form.address, + rows=4, + width='1-1', + autofocus=True, + autosize=True, + ) }} +
+
+

+ + Upload a list of {{ recipient_count_label(999, template.template_type) }} + +

+ {{ page_footer('Continue') }} + {% endcall %} + + {{ template|string }} + +{% endblock %} diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index 97de4182f..0b1c39e77 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -16,11 +16,11 @@
{% if template.template_type == 'letter' %} - {% if letter_too_long %} - {% call banner_wrapper(type='dangerous') %} - {% include "partials/check/letter-too-long.html" %} - {% endcall %} - {% endif %} + {% if letter_too_long %} + {% call banner_wrapper(type='dangerous') %} + {% include "partials/check/letter-too-long.html" %} + {% endcall %} + {% endif %} {% if current_user.has_permissions('send_messages', restrict_admin_usage=True) and not letter_too_long %}
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 9d0665a5c..9a3352184 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -779,6 +779,7 @@ def test_upload_valid_csv_only_sets_meta_if_filename_known( mock_get_job_doesnt_exist, mock_get_jobs, mock_s3_set_metadata, + mock_template_preview, fake_uuid, ): @@ -790,10 +791,6 @@ def test_upload_valid_csv_only_sets_meta_if_filename_known( 'app.main.views.send.get_page_count_for_letter', return_value=5, ) - mocker.patch( - 'app.main.views.send.TemplatePreview.from_utils_template', - return_value='foo' - ) client_request.get( 'no_cookie.check_messages_preview', @@ -1331,9 +1328,9 @@ def test_send_one_off_has_skip_link( @pytest.mark.parametrize('template_type, expected_sticky', [ ('sms', False), ('email', True), - ('letter', True), + ('letter', False), ]) -def test_send_one_off_has_sticky_header_for_email_and_letter( +def test_send_one_off_has_sticky_header_for_email( mocker, client_request, fake_uuid, @@ -1342,7 +1339,7 @@ def test_send_one_off_has_sticky_header_for_email_and_letter( template_type, expected_sticky, ): - template_data = create_template(template_type=template_type) + template_data = create_template(template_type=template_type, content='((body))') mocker.patch('app.service_api_client.get_service_template', return_value={'data': template_data}) mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) @@ -1357,6 +1354,39 @@ def test_send_one_off_has_sticky_header_for_email_and_letter( assert bool(page.select('.js-stick-at-top-when-scrolling')) == expected_sticky +def test_send_one_off_has_sticky_header_for_letter_on_non_address_placeholders( + mocker, + client_request, + fake_uuid, + mock_get_live_service, +): + template_data = create_template(template_type='letter', content='((body))') + mocker.patch('app.service_api_client.get_service_template', return_value={'data': template_data}) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) + + with client_request.session_transaction() as session: + session['send_test_letter_page_count'] = 1 + session['recipient'] = '' + session['placeholders'] = { + 'address line 1': 'foo', + 'address line 2': 'bar', + 'address line 3': '', + 'address line 4': '', + 'address line 5': '', + 'address line 6': '', + 'postcode': 'SW1 1AA', + } + + page = client_request.get( + 'main.send_one_off_step', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + step_index=7, # letter template has 7 placeholders – we’re at the end + _follow_redirects=True, + ) + assert page.select('.js-stick-at-top-when-scrolling') + + @pytest.mark.parametrize('user', ( create_active_user_with_permissions(), create_active_caseworking_user(), @@ -2146,6 +2176,180 @@ def test_send_test_clears_session( assert session['placeholders'] == {} +def test_send_one_off_redirects_to_letter_address(client_request, fake_uuid, mock_get_service_letter_template): + with client_request.session_transaction() as session: + session['placeholders'] = {'foo': 'some old data that we dont care about'} + + client_request.get( + 'main.send_one_off', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _expected_redirect=url_for( + 'main.send_one_off_letter_address', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _external=True, + ) + ) + # make sure it cleared session first + with client_request.session_transaction() as session: + assert session['recipient'] is None + assert session['placeholders'] == {} + + +def test_send_one_off_letter_address_shows_form( + client_request, + fake_uuid, + mock_get_service_letter_template, + mock_template_preview, +): + with client_request.session_transaction() as session: + session['recipient'] = None + session['placeholders'] = {} + + page = client_request.get( + 'main.send_one_off_letter_address', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid + ) + + assert page.select_one('h1').text.strip() == 'Send ‘Two week reminder’' + + form = page.select_one('form') + + assert form.select_one('label').text.strip() == 'Address' + assert form.select_one('textarea').attrs['name'] == 'address' + + upload_link = form.select_one('a') + + assert upload_link.text.strip() == 'Upload a list of addresses' + assert upload_link['href'] == url_for( + 'main.send_messages', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + ) + + assert ( + page.find_all('a', {'class': 'govuk-back-link'})[0]['href'] + ) == url_for('main.view_template', service_id=SERVICE_ONE_ID, template_id=fake_uuid) + + +@pytest.mark.parametrize(['form_data', 'expected_placeholders'], [ + # minimal + ('\n'.join(['a', 'b', 'c']), { + 'address line 1': 'a', + 'address line 2': 'b', + 'address line 3': '', + 'address line 4': '', + 'address line 5': '', + 'address line 6': '', + 'postcode': 'c', + }), + # maximal + ('\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g']), { + 'address line 1': 'a', + 'address line 2': 'b', + 'address line 3': 'c', + 'address line 4': 'd', + 'address line 5': 'e', + 'address line 6': 'f', + 'postcode': 'g', + }), + # it ignores empty lines and strips whitespace from each line. + # It also strips extra whitespace from the middle of lines. + ('\n a\ta \n\n\n \n\n\n\nb b \r\nc', { + 'address line 1': 'a\ta', + 'address line 2': 'b b', + 'address line 3': '', + 'address line 4': '', + 'address line 5': '', + 'address line 6': '', + 'postcode': 'c', + }), +]) +def test_send_one_off_letter_address_populates_address_fields_in_session( + client_request, + fake_uuid, + mock_get_service_letter_template, + mock_template_preview, + form_data, + expected_placeholders +): + with client_request.session_transaction() as session: + session['recipient'] = None + session['placeholders'] = {} + + client_request.post( + 'main.send_one_off_letter_address', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _data={'address': form_data}, + # there are no additional placeholders so go straight to the check page + _expected_redirect=url_for( + 'main.check_notification', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _external=True, + ), + ) + with client_request.session_transaction() as session: + assert session['placeholders'] == expected_placeholders + + +@pytest.mark.parametrize(['form_data', 'expected_error_message'], [ + ('', 'Cannot be empty'), + ('a\n\n\n\nb', 'Address must be at least 3 lines long'), + ('\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h']), 'Address must be no more than 7 lines long'), +]) +def test_send_one_off_letter_address_rejects_bad_addresses( + client_request, + fake_uuid, + mock_get_service_letter_template, + mock_template_preview, + form_data, + expected_error_message +): + with client_request.session_transaction() as session: + session['recipient'] = None + session['placeholders'] = {} + + page = client_request.post( + 'main.send_one_off_letter_address', + _data={'address': form_data}, + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _expected_status=200 + ) + + error = page.select('form .error-message') + assert normalize_spaces(error[0].text) == expected_error_message + + +def test_send_one_off_letter_address_goes_to_next_placeholder(client_request, mock_template_preview, mocker): + with client_request.session_transaction() as session: + session['recipient'] = None + session['placeholders'] = {} + + template_data = create_template(template_type='letter', content='((foo))') + + mocker.patch('app.service_api_client.get_service_template', return_value={'data': template_data}) + + client_request.post( + 'main.send_one_off_letter_address', + service_id=SERVICE_ONE_ID, + template_id=template_data['id'], + _data={'address': 'a\nb\nc'}, + # step 0-6 represent address line 1-6 and postcode. step 7 is the first non address placeholder + _expected_redirect=url_for( + 'main.send_one_off_step', + service_id=SERVICE_ONE_ID, + template_id=template_data['id'], + step_index=7, + _external=True, + ) + ) + + def test_download_example_csv( logged_in_client, mocker, diff --git a/tests/conftest.py b/tests/conftest.py index 78d83225f..1510ccf1f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3483,6 +3483,19 @@ def mock_get_returned_letter_summary_with_no_returned_letters(mocker): ) +@pytest.fixture +def mock_template_preview(mocker): + content = b'{"count":1}' + status_code = 200 + headers = {} + example_response = (content, status_code, headers) + mocker.patch('app.template_previews.TemplatePreview.from_database_object', return_value=example_response) + mocker.patch('app.template_previews.TemplatePreview.from_valid_pdf_file', return_value=example_response) + mocker.patch('app.template_previews.TemplatePreview.from_invalid_pdf_file', return_value=example_response) + mocker.patch('app.template_previews.TemplatePreview.from_example_template', return_value=example_response) + mocker.patch('app.template_previews.TemplatePreview.from_utils_template', return_value=example_response) + + def create_api_user_active(with_unique_id=False): return { 'id': str(uuid4()) if with_unique_id else sample_uuid(),