diff --git a/app/assets/stylesheets/_grids.scss b/app/assets/stylesheets/_grids.scss index c7becdee2..4fbd20f8f 100644 --- a/app/assets/stylesheets/_grids.scss +++ b/app/assets/stylesheets/_grids.scss @@ -26,6 +26,19 @@ @include grid-column(7/8); } +%top-gutter, +.top-gutter { + @extend %contain-floats; + display: block; + margin-top: $gutter; + clear: both; +} + +.top-gutter-4-3 { + @extend %top-gutter; + margin-top: $gutter * 4 / 3; +} + %bottom-gutter, .bottom-gutter { @extend %contain-floats; diff --git a/app/config.py b/app/config.py index 9d3a34671..f9b737929 100644 --- a/app/config.py +++ b/app/config.py @@ -57,7 +57,7 @@ class Config(object): CSV_UPLOAD_BUCKET_NAME = 'local-notifications-csv-upload' DESKPRO_PERSON_EMAIL = 'donotreply@notifications.service.gov.uk' ACTIVITY_STATS_LIMIT_DAYS = 7 - TEST_MESSAGE_FILENAME = 'Test message' + TEST_MESSAGE_FILENAME = 'Report' STATSD_ENABLED = False STATSD_HOST = "statsd.hostedgraphite.com" diff --git a/app/main/forms.py b/app/main/forms.py index eb60cc8f4..bf95901b3 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -7,6 +7,7 @@ from notifications_utils.recipients import ( validate_phone_number, InvalidPhoneError ) +from notifications_utils.columns import Columns from wtforms import ( validators, StringField, @@ -102,11 +103,26 @@ class UKMobileNumber(TelField): raise ValidationError(str(e)) -def mobile_number(): - return UKMobileNumber('Mobile number', +class InternationalPhoneNumber(TelField): + def pre_validate(self, form): + try: + validate_phone_number(self.data, international=True) + except InvalidPhoneError as e: + raise ValidationError(str(e)) + + +def mobile_number(label='Mobile number'): + return UKMobileNumber(label, validators=[DataRequired(message='Can’t be empty')]) +def international_phone_number(label='Mobile number'): + return InternationalPhoneNumber( + label, + validators=[DataRequired(message='Can’t be empty')] + ) + + def password(label='Password'): return PasswordField(label, validators=[DataRequired(message='Can’t be empty'), @@ -633,15 +649,25 @@ class PlaceholderForm(Form): def get_placeholder_form_instance( placeholder_name, dict_to_populate_from, - optional_placeholder=False + optional_placeholder=False, + allow_international_phone_numbers=False, ): - PlaceholderForm.placeholder_value = StringField( - placeholder_name, - validators=[ + if Columns.make_key(placeholder_name) == 'emailaddress': + field = email_address(label=placeholder_name, gov_user=False) + elif Columns.make_key(placeholder_name) == 'phonenumber': + if allow_international_phone_numbers: + field = international_phone_number(label=placeholder_name) + else: + field = mobile_number(label=placeholder_name) + elif optional_placeholder: + field = StringField(placeholder_name) + else: + field = StringField(placeholder_name, validators=[ DataRequired(message='Can’t be empty') - ] if not optional_placeholder else [] - ) + ]) + + PlaceholderForm.placeholder_value = field 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 638f67cc0..de6be4ff1 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -157,14 +157,18 @@ def get_example_csv(service_id, template_id): } -@main.route("/services//send//test") +@main.route("/services//send//test", endpoint='send_test') +@main.route("/services//send//one-off", endpoint='send_one_off') @login_required @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', + { + 'main.send_test': '.send_test_step', + 'main.send_one_off': '.send_one_off_step', + }[request.endpoint], service_id=service_id, template_id=template_id, step_index=0, @@ -172,14 +176,28 @@ def send_test(service_id, template_id): )) -@main.route("/services//send//test/step-", methods=['GET', 'POST']) +@main.route( + "/services//send//test/step-", + methods=['GET', 'POST'], + endpoint='send_test_step', +) +@main.route( + "/services//send//one-off/step-", + methods=['GET', 'POST'], + endpoint='send_one_off_step', +) @login_required @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 + { + 'main.send_test_step': '.send_test', + 'main.send_one_off_step': '.send_one_off', + }[request.endpoint], + service_id=service_id, + template_id=template_id, )) template = service_api_client.get_service_template(service_id, template_id)['data'] @@ -201,7 +219,10 @@ def send_test_step(service_id, template_id, step_index): page_count=session['send_test_letter_page_count'] ) - placeholders = fields_to_fill_in(template) + placeholders = fields_to_fill_in( + template, + prefill_current_user=(request.endpoint == 'main.send_test_step'), + ) if len(placeholders) == 0: return make_and_upload_csv_file(service_id, template) @@ -209,28 +230,33 @@ def send_test_step(service_id, template_id, step_index): try: current_placeholder = placeholders[step_index] except IndexError: + if all_placeholders_in_session(placeholders): + return make_and_upload_csv_file(service_id, template) return redirect(url_for( - '.send_test', service_id=service_id, template_id=template_id + { + 'main.send_test_step': '.send_test', + 'main.send_one_off_step': '.send_one_off', + }[request.endpoint], + service_id=service_id, + template_id=template_id, )) 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, + allow_international_phone_numbers=current_service['can_send_international_sms'], ) 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 - ): + if all_placeholders_in_session(placeholders): return make_and_upload_csv_file(service_id, template) return redirect(url_for( - '.send_test_step', + request.endpoint, service_id=service_id, template_id=template_id, step_index=step_index + 1, @@ -247,7 +273,7 @@ def send_test_step(service_id, template_id, step_index): ) else: back_link = url_for( - '.send_test_step', + request.endpoint, service_id=service_id, template_id=template_id, step_index=step_index - 1, @@ -256,13 +282,27 @@ def send_test_step(service_id, template_id, step_index): template.values = get_normalised_send_test_values_from_session() template.values[current_placeholder] = None + if ( + request.endpoint == 'main.send_one_off_step' and + step_index == 0 and + template.template_type != 'letter' + ): + skip_link = ( + 'Use my {}'.format(first_column_headings[template.template_type][0]), + url_for('.send_test', service_id=service_id, template_id=template.id), + ) + else: + skip_link = None + return render_template( 'views/send-test.html', + page_title=get_send_test_page_title(template.template_type, get_help_argument()), template=template, form=form, + skip_link=skip_link, optional_placeholder=optional_placeholder, - help=get_help_argument(), back_link=back_link, + help=get_help_argument(), ) @@ -470,11 +510,11 @@ def get_check_messages_back_url(service_id, template_type): return url_for('main.choose_template', service_id=service_id) -def fields_to_fill_in(template): +def fields_to_fill_in(template, prefill_current_user=False): recipient_columns = first_column_headings[template.template_type] - if 'letter' == template.template_type: + if 'letter' == template.template_type or not prefill_current_user: return recipient_columns + list(template.placeholders) session['send_test_values'][recipient_columns[0]] = { @@ -513,3 +553,18 @@ def make_and_upload_csv_file(service_id, template): from_test=True, help=2 if get_help_argument() else 0 )) + + +def all_placeholders_in_session(placeholders): + return all( + get_normalised_send_test_values_from_session().get(placeholder, False) not in (False, None) + for placeholder in placeholders + ) + + +def get_send_test_page_title(template_type, help_argument): + if help_argument: + return 'Example text message' + if template_type == 'letter': + return 'Print a test letter' + return 'Send to one recipient' diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index c7b0b0e2e..bae3939cc 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -5,32 +5,30 @@ {% from "components/table.html" import list_table, field, text_field, index_field, index_field_heading %} {% block service_page_title %} - {% if request.args['help'] %} - Example text message - {% else %} - Send yourself a test - {% endif %} + {{ page_title }} {% endblock %} {% block maincolumn_content %}

- {% if request.args['help'] %} - Example text message - {% else %} - {% if template.template_type == 'letter' %} - Print a test letter - {% else %} - Send yourself a test - {% endif %} - {% endif %} + {{ page_title }}

- {{ textbox( - form.placeholder_value, - hint='Optional' if optional_placeholder else None - ) }} +
+
+ {{ textbox( + form.placeholder_value, + hint='Optional' if optional_placeholder else None, + width='1-1', + ) }} +
+ {% if skip_link %} +
+ {{ skip_link[0] }} +
+ {% endif %} +
{{ page_footer('Next', back_link=back_link) }}
diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index e74e8e0a5..6a2add4df 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -13,8 +13,8 @@
- - {{ 'Print a test letter' if template.template_type == 'letter' else 'Send yourself a test' }} + + {{ 'Print a test letter' if template.template_type == 'letter' else 'Send to one recipient' }}
{% endif %} diff --git a/tests/app/main/test_placeholder_form.py b/tests/app/main/test_placeholder_form.py index 07fac6675..4c6093944 100644 --- a/tests/app/main/test_placeholder_form.py +++ b/tests/app/main/test_placeholder_form.py @@ -1,3 +1,4 @@ +import pytest from app.main.forms import get_placeholder_form_instance from wtforms import Label @@ -16,3 +17,46 @@ def test_form_class_not_mutated(app_): assert str(form1.placeholder_value.label) == '' assert str(form2.placeholder_value.label) == '' + + +@pytest.mark.parametrize('service_can_send_international_sms, placeholder_name, value, expected_error', [ + + (False, 'email address', '', 'Can’t be empty'), + (False, 'email address', '12345', 'Enter a valid email address'), + (False, 'email address', 'test@example.com', None), + (False, 'email address', 'test@example.gov.uk', None), + + (False, 'phone number', '', 'Can’t be empty'), + (False, 'phone number', '+1-2345-678890', 'Not a UK mobile number'), + (False, 'phone number', '07900900123', None), + (False, 'phone number', '+44(0)7900 900-123', None), + + (True, 'phone number', '+123', 'Not enough digits'), + (True, 'phone number', '+44(0)7900 900-123', None), + (True, 'phone number', '+1-2345-678890', None), + + (False, 'anything else', '', 'Can’t be empty'), + +]) +def test_validates_recipients( + app_, + placeholder_name, + value, + service_can_send_international_sms, + expected_error, +): + with app_.test_request_context( + method='POST', + data={'placeholder_value': value} + ): + form = get_placeholder_form_instance( + placeholder_name, + {}, + allow_international_phone_numbers=service_can_send_international_sms, + ) + + if expected_error: + assert not form.validate_on_submit() + assert form.placeholder_value.errors[0] == expected_error + else: + assert form.validate_on_submit() diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e22550798..69ebddf74 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -286,7 +286,6 @@ def test_send_test_sms_message( mock_get_detailed_service_for_today, ): - 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( @@ -294,29 +293,174 @@ def test_send_test_sms_message( follow_redirects=True ) assert response.status_code == 200 - mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') + mock_s3_upload.assert_called_with( + service_one['id'], + {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'Report'}, + 'eu-west-1' + ) +@pytest.mark.parametrize('endpoint, template_mock, expected_session_contents', [ + ('main.send_test_step', mock_get_service_template_with_placeholders, {'phone number': '07700 900762'}), + ('main.send_test_step', mock_get_service_email_template, {'email address': 'test@user.gov.uk'}), + ('main.send_test_step', mock_get_service_letter_template, {}), + ('main.send_one_off_step', mock_get_service_template, {}), + ('main.send_one_off_step', mock_get_service_email_template, {}), + ('main.send_one_off_step', mock_get_service_letter_template, {}), +]) def test_send_test_step_redirects_if_session_not_setup( + mocker, logged_in_client, - service_one, + mock_get_detailed_service_for_today, + mock_get_users_by_service, fake_uuid, - mock_get_service_email_template, + endpoint, + template_mock, + expected_session_contents, ): + template_mock(mocker) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=99) + 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), + url_for(endpoint, 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'} + assert session['send_test_values'] == expected_session_contents +@pytest.mark.parametrize('template_mock, partial_url, expected_h1, tour_shown', [ + ( + mock_get_service_template_with_placeholders, + partial(url_for, 'main.send_test'), + 'Send to one recipient', + False, + ), + ( + mock_get_service_template_with_placeholders, + partial(url_for, 'main.send_one_off'), + 'Send to one recipient', + False, + ), + ( + mock_get_service_template_with_placeholders, + partial(url_for, 'main.send_test', help=1), + 'Example text message', + True, + ), + ( + mock_get_service_email_template, + partial(url_for, 'main.send_test', help=1), + 'Example text message', + True, + ), + ( + mock_get_service_email_template, + partial(url_for, 'main.send_test'), + 'Send to one recipient', + False, + ), + ( + mock_get_service_email_template, + partial(url_for, 'main.send_one_off'), + 'Send to one recipient', + False, + ), + ( + mock_get_service_letter_template, + partial(url_for, 'main.send_test'), + 'Print a test letter', + False, + ), + ( + mock_get_service_letter_template, + partial(url_for, 'main.send_one_off'), + 'Print a test letter', + False, + ), +]) +def test_send_one_off_or_test_has_correct_page_titles( + logged_in_client, + service_one, + fake_uuid, + mocker, + template_mock, + partial_url, + expected_h1, + tour_shown, +): + + template_mock(mocker) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=99) + + response = logged_in_client.get( + partial_url(service_id=service_one['id'], template_id=fake_uuid, step_index=0), + follow_redirects=True, + ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert response.status_code == 200 + assert page.h1.text.strip() == expected_h1 + + assert (len(page.select('.banner-tour')) == 1) == tour_shown + + +@pytest.mark.parametrize('template_mock, expected_link_text, expected_link_url', [ + (mock_get_service_template, 'Use my phone number', partial(url_for, 'main.send_test')), + (mock_get_service_email_template, 'Use my email address', partial(url_for, 'main.send_test')), + (mock_get_service_letter_template, None, None), +]) +def test_send_one_off_has_skip_link( + logged_in_client, + service_one, + fake_uuid, + mock_get_service_email_template, + mocker, + template_mock, + expected_link_text, + expected_link_url, +): + + template_mock(mocker) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=99) + + response = logged_in_client.get( + url_for('main.send_one_off_step', service_id=service_one['id'], template_id=fake_uuid, step_index=0), + follow_redirects=True + ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + skip_links = page.select('a.top-gutter-4-3') + + assert response.status_code == 200 + + if expected_link_text and expected_link_url: + assert skip_links[0].text.strip() == expected_link_text + assert skip_links[0]['href'] == expected_link_url( + service_id=service_one['id'], + template_id=fake_uuid, + ) + else: + assert not skip_links + + +@pytest.mark.parametrize('endpoint, expected_redirect, send_test_values', [ + ( + 'main.send_test_step', + 'main.send_test', + {'name': 'foo'}, + ), + ( + 'main.send_one_off_step', + 'main.send_one_off', + {'name': 'foo', 'phone number': '07900900123'}, + ), +]) def test_send_test_redirects_to_end_if_step_out_of_bounds( logged_in_client, service_one, @@ -325,13 +469,16 @@ def test_send_test_redirects_to_end_if_step_out_of_bounds( mock_s3_upload, mock_get_users_by_service, mock_get_detailed_service_for_today, + endpoint, + send_test_values, + expected_redirect, ): with logged_in_client.session_transaction() as session: - session['send_test_values'] = {'name': 'foo'} + session['send_test_values'] = send_test_values response = logged_in_client.get(url_for( - 'main.send_test_step', + endpoint, service_id=service_one['id'], template_id=fake_uuid, step_index=999, @@ -351,6 +498,10 @@ def test_send_test_redirects_to_end_if_step_out_of_bounds( ) +@pytest.mark.parametrize('endpoint, expected_redirect', [ + ('main.send_test_step', 'main.send_test'), + ('main.send_one_off_step', 'main.send_one_off'), +]) def test_send_test_redirects_to_start_if_you_skip_steps( logged_in_platform_admin_client, service_one, @@ -360,6 +511,8 @@ def test_send_test_redirects_to_start_if_you_skip_steps( mock_get_users_by_service, mock_get_detailed_service_for_today, mocker, + endpoint, + expected_redirect, ): with logged_in_platform_admin_client.session_transaction() as session: @@ -367,20 +520,24 @@ def test_send_test_redirects_to_start_if_you_skip_steps( session['send_test_values'] = {'address_line_1': 'foo'} response = logged_in_platform_admin_client.get(url_for( - 'main.send_test_step', + endpoint, 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', + expected_redirect, service_id=service_one['id'], template_id=fake_uuid, _external=True, ) +@pytest.mark.parametrize('endpoint, expected_redirect', [ + ('main.send_test_step', 'main.send_test'), + ('main.send_one_off_step', 'main.send_one_off'), +]) def test_send_test_redirects_to_start_if_index_out_of_bounds_and_some_placeholders_empty( logged_in_client, service_one, @@ -389,13 +546,15 @@ def test_send_test_redirects_to_start_if_index_out_of_bounds_and_some_placeholde mock_s3_download, mock_get_users_by_service, mock_get_detailed_service_for_today, + endpoint, + expected_redirect, ): 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', + endpoint, service_id=service_one['id'], template_id=fake_uuid, step_index=999, @@ -403,24 +562,30 @@ def test_send_test_redirects_to_start_if_index_out_of_bounds_and_some_placeholde assert response.status_code == 302 assert response.location == url_for( - 'main.send_test', + expected_redirect, service_id=service_one['id'], template_id=fake_uuid, _external=True, ) +@pytest.mark.parametrize('endpoint, expected_redirect', [ + ('main.send_test', 'main.send_test_step'), + ('main.send_one_off', 'main.send_one_off_step'), +]) def test_send_test_sms_message_redirects_with_help_argument( logged_in_client, service_one, fake_uuid, + endpoint, + expected_redirect, ): response = logged_in_client.get( - url_for('main.send_test', service_id=service_one['id'], template_id=fake_uuid, help=1) + url_for(endpoint, 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', + expected_redirect, service_id=service_one['id'], template_id=fake_uuid, step_index=0, @@ -440,7 +605,6 @@ def test_send_test_email_message_without_placeholders( 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( @@ -448,7 +612,11 @@ def test_send_test_email_message_without_placeholders( follow_redirects=True ) assert response.status_code == 200 - mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') + mock_s3_upload.assert_called_with( + service_one['id'], + {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'Report'}, + 'eu-west-1' + ) def test_send_test_sms_message_with_placeholders_shows_first_field( @@ -667,7 +835,7 @@ def test_send_test_sms_message_puts_submitted_data_in_session_and_file( service_one['id'], { 'data': 'name,phone number\r\nJo,07700 900762\r\n', - 'file_name': 'Test message' + 'file_name': 'Report' }, 'eu-west-1' ) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index cf0bc3fdd..acafae751 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -44,11 +44,11 @@ def test_should_show_page_for_one_template( ), ( ['send_texts', 'send_emails', 'send_letters'], - ['.send_messages', '.send_test'] + ['.send_messages', '.send_one_off'] ), ( ['send_texts', 'send_emails', 'send_letters', 'manage_templates'], - ['.send_messages', '.send_test', '.edit_service_template'] + ['.send_messages', '.send_one_off', '.edit_service_template'] ), ]) def test_should_be_able_to_view_a_template_with_links(