From eabd9fcbf1324dd62b80aa536b31699907e4390d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 08:55:05 +0100 Subject: [PATCH 1/7] Add a third route through populating a template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds a route which is identical to send yourself a test, but with its own endpoint. This will let us add a slightly different ‘send a one-off message’ flow. This commit just adds the route though, and makes sure that the tests pass for both routes. --- app/main/views/send.py | 37 +++++++++++++++++++++----- tests/app/main/views/test_send.py | 44 +++++++++++++++++++++++++------ 2 files changed, 66 insertions(+), 15 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 638f67cc0..d650c90dd 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'] @@ -210,7 +228,12 @@ def send_test_step(service_id, template_id, step_index): current_placeholder = placeholders[step_index] except IndexError: 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( @@ -230,7 +253,7 @@ def send_test_step(service_id, template_id, step_index): 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 +270,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, diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e22550798..e5152763a 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -297,18 +297,23 @@ def test_send_test_sms_message( mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') +@pytest.mark.parametrize('endpoint', [ + 'main.send_test_step', + 'main.send_one_off_step', +]) def test_send_test_step_redirects_if_session_not_setup( logged_in_client, service_one, fake_uuid, mock_get_service_email_template, + endpoint, ): 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 @@ -317,6 +322,10 @@ def test_send_test_step_redirects_if_session_not_setup( assert session['send_test_values'] == {'email address': 'test@user.gov.uk'} +@pytest.mark.parametrize('endpoint', [ + 'main.send_test_step', + 'main.send_one_off_step', +]) def test_send_test_redirects_to_end_if_step_out_of_bounds( logged_in_client, service_one, @@ -325,13 +334,14 @@ 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, ): 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, @@ -351,6 +361,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 +374,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 +383,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 +409,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 +425,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, From 7310e8b435bc725dfc4dc0aa5a7a7d6357a468c0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 09:18:26 +0100 Subject: [PATCH 2/7] Make send one-off route ask for phone number/email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds an extra, initial, step to the ‘send one-off message’ flow to ask for a phone number or email address. This is the first pass at making a feature which caseworkers or similar could use Notify to send individual messages while they’re working a case. --- app/main/views/send.py | 23 +++++++++++------ tests/app/main/views/test_send.py | 42 +++++++++++++++++++++++-------- 2 files changed, 47 insertions(+), 18 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index d650c90dd..b874bc07c 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -219,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) @@ -227,6 +230,8 @@ 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( { 'main.send_test_step': '.send_test', @@ -246,10 +251,7 @@ def send_test_step(service_id, template_id, step_index): 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( @@ -493,11 +495,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]] = { @@ -536,3 +538,10 @@ 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 + ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e5152763a..3f07e4108 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -297,34 +297,52 @@ def test_send_test_sms_message( mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') -@pytest.mark.parametrize('endpoint', [ - 'main.send_test_step', - 'main.send_one_off_step', +@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(endpoint, 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('endpoint', [ - 'main.send_test_step', - 'main.send_one_off_step', +@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, @@ -335,10 +353,12 @@ def test_send_test_redirects_to_end_if_step_out_of_bounds( 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( endpoint, From 27c14632130d27b199488ffb91199c2565fca3dd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 09:40:37 +0100 Subject: [PATCH 3/7] Validate recipients in send a one-off message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It would be annoying to get all the way to the end of the flow and get told that the phone number or email address you entered isn’t valid. So this commit reuses the existing WTForms objects that we have to do some extra validation on the first step in the send one-off message flow. It also accounts for international phone numbers, if the service is allowed to send them. It doesn’t reject other people’s phone numbers if your service is restricted, because I think it’s better to let users play with the feature – it’s good for learning. --- app/main/forms.py | 42 ++++++++++++++++++----- app/main/views/send.py | 1 + tests/app/main/test_placeholder_form.py | 44 +++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 8 deletions(-) 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 b874bc07c..7051094b2 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -245,6 +245,7 @@ def send_test_step(service_id, template_id, step_index): 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(): 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() From 82549c817a1656aa3c045225f28271ab5b442863 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 13:31:23 +0100 Subject: [PATCH 4/7] Ensure correct page titles on send test flows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have some fairly complicated nested if statements in our Jinja that decide what the page titles should be. It’s only going to get more complicated with the send individual message routes. So this commit: - moves the logic from Jinja to Python - adds tests to check things are working as expected - sets the page titles to the right thing for each flow --- app/main/views/send.py | 12 ++++++ app/templates/views/send-test.html | 16 +------- tests/app/main/views/test_send.py | 65 ++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 14 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 7051094b2..cbf7acf0e 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -284,6 +284,7 @@ def send_test_step(service_id, template_id, step_index): return render_template( 'views/send-test.html', + page_title=get_send_test_page_title(template.template_type, request.endpoint), template=template, form=form, optional_placeholder=optional_placeholder, @@ -546,3 +547,14 @@ def all_placeholders_in_session(placeholders): 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, endpoint): + if get_help_argument(): + return 'Example text message' + if template_type == 'letter': + return 'Print a test letter' + return { + 'main.send_test_step': 'Send yourself a test', + 'main.send_one_off_step': 'Send one-off message', + }[endpoint] diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index c7b0b0e2e..05eae2aeb 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -5,25 +5,13 @@ {% 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 }}

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 3f07e4108..ded6677a5 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -332,6 +332,71 @@ def test_send_test_step_redirects_if_session_not_setup( assert session['send_test_values'] == expected_session_contents +@pytest.mark.parametrize('template_mock, partial_url, expected_h1', [ + ( + mock_get_service_template_with_placeholders, + partial(url_for, 'main.send_test'), + 'Send yourself a test', + ), + ( + mock_get_service_template_with_placeholders, + partial(url_for, 'main.send_one_off'), + 'Send one-off message', + ), + ( + mock_get_service_template_with_placeholders, + partial(url_for, 'main.send_test', help=1), + 'Example text message', + ), + ( + mock_get_service_email_template, + partial(url_for, 'main.send_test', help=1), + 'Example text message', + ), + ( + mock_get_service_email_template, + partial(url_for, 'main.send_test'), + 'Send yourself a test', + ), + ( + mock_get_service_email_template, + partial(url_for, 'main.send_one_off'), + 'Send one-off message', + ), + ( + mock_get_service_letter_template, + partial(url_for, 'main.send_test'), + 'Print a test letter', + ), + ( + mock_get_service_letter_template, + partial(url_for, 'main.send_one_off'), + 'Print a test letter', + ), +]) +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, +): + + 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 + + @pytest.mark.parametrize('endpoint, expected_redirect, send_test_values', [ ( 'main.send_test_step', From 43f6d21e1dd22e2778431a86f3886c45bf9f528b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 13:31:51 +0100 Subject: [PATCH 5/7] Replace test with one-off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s a confusing proposition to have two features which are almost identical. Even differentiating between them in the template menu would be tricky. I think the better thing to do is rename the whole feature to ‘send one-off message’. Then if someone wants to use there own phone number or email address, give them a quick shortcut to doing that, once they’re in the flow. In the background this reuses the ‘send yourself a test’ code, but the user is never aware that they’re going through a different route to send an individual message. So the proposition stays nice and clean. --- app/assets/stylesheets/_grids.scss | 13 +++++ app/config.py | 2 +- app/main/views/send.py | 25 ++++++--- app/templates/views/send-test.html | 18 ++++-- app/templates/views/templates/_template.html | 4 +- tests/app/main/views/test_send.py | 58 +++++++++++++++++--- tests/app/main/views/test_templates.py | 4 +- 7 files changed, 100 insertions(+), 24 deletions(-) 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..d48821d8b 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 = 'One-off message' STATSD_ENABLED = False STATSD_HOST = "statsd.hostedgraphite.com" diff --git a/app/main/views/send.py b/app/main/views/send.py index cbf7acf0e..2ff3cef75 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -282,13 +282,25 @@ 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, request.endpoint), + 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, ) @@ -549,12 +561,9 @@ def all_placeholders_in_session(placeholders): ) -def get_send_test_page_title(template_type, endpoint): - if get_help_argument(): +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 { - 'main.send_test_step': 'Send yourself a test', - 'main.send_one_off_step': 'Send one-off message', - }[endpoint] + return 'Send one-off message' diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index 05eae2aeb..bae3939cc 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -15,10 +15,20 @@ - {{ 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..31ce14918 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 one-off message' }}
{% endif %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index ded6677a5..eeb7d6ecf 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,7 +293,11 @@ 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': 'One-off message'}, + 'eu-west-1' + ) @pytest.mark.parametrize('endpoint, template_mock, expected_session_contents', [ @@ -336,7 +339,7 @@ def test_send_test_step_redirects_if_session_not_setup( ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_test'), - 'Send yourself a test', + 'Send one-off message', ), ( mock_get_service_template_with_placeholders, @@ -356,7 +359,7 @@ def test_send_test_step_redirects_if_session_not_setup( ( mock_get_service_email_template, partial(url_for, 'main.send_test'), - 'Send yourself a test', + 'Send one-off message', ), ( mock_get_service_email_template, @@ -397,6 +400,44 @@ def test_send_one_off_or_test_has_correct_page_titles( assert page.h1.text.strip() == expected_h1 +@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', @@ -553,7 +594,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( @@ -561,7 +601,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': 'One-off message'}, + 'eu-west-1' + ) def test_send_test_sms_message_with_placeholders_shows_first_field( @@ -780,7 +824,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': 'One-off message' }, '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( From 6425dcbc9461ad2f69870acb65ba03d1a6fabe32 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 1 Jun 2017 13:03:22 +0100 Subject: [PATCH 6/7] =?UTF-8?q?Rename=20the=20feature=20to=20=E2=80=98send?= =?UTF-8?q?=20to=20one=20recipient=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ‘One-off’ is a bit wooly. Feels like our name for the thing. ‘Send to one recipient’ matches ‘Upload recipients’. This also means making the `

` on job page ‘Report’ for one-off messages. It doesn’t make sense to call the feature ‘send to one recipient’ when we’re not using the language of one-off any more. --- app/config.py | 2 +- app/main/views/send.py | 2 +- app/templates/views/templates/_template.html | 2 +- tests/app/main/views/test_send.py | 14 +++++++------- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/config.py b/app/config.py index d48821d8b..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 = 'One-off message' + TEST_MESSAGE_FILENAME = 'Report' STATSD_ENABLED = False STATSD_HOST = "statsd.hostedgraphite.com" diff --git a/app/main/views/send.py b/app/main/views/send.py index 2ff3cef75..bfa47c33a 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -566,4 +566,4 @@ def get_send_test_page_title(template_type, help_argument): return 'Example text message' if template_type == 'letter': return 'Print a test letter' - return 'Send one-off message' + return 'Send to one recipient' diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index 31ce14918..6a2add4df 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -14,7 +14,7 @@
- {{ 'Print a test letter' if template.template_type == 'letter' else 'Send one-off message' }} + {{ 'Print a test letter' if template.template_type == 'letter' else 'Send to one recipient' }}
{% endif %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index eeb7d6ecf..f5559bba8 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -295,7 +295,7 @@ def test_send_test_sms_message( assert response.status_code == 200 mock_s3_upload.assert_called_with( service_one['id'], - {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'One-off message'}, + {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'Report'}, 'eu-west-1' ) @@ -339,12 +339,12 @@ def test_send_test_step_redirects_if_session_not_setup( ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_test'), - 'Send one-off message', + 'Send to one recipient', ), ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_one_off'), - 'Send one-off message', + 'Send to one recipient', ), ( mock_get_service_template_with_placeholders, @@ -359,12 +359,12 @@ def test_send_test_step_redirects_if_session_not_setup( ( mock_get_service_email_template, partial(url_for, 'main.send_test'), - 'Send one-off message', + 'Send to one recipient', ), ( mock_get_service_email_template, partial(url_for, 'main.send_one_off'), - 'Send one-off message', + 'Send to one recipient', ), ( mock_get_service_letter_template, @@ -603,7 +603,7 @@ def test_send_test_email_message_without_placeholders( assert response.status_code == 200 mock_s3_upload.assert_called_with( service_one['id'], - {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'One-off message'}, + {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'Report'}, 'eu-west-1' ) @@ -824,7 +824,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': 'One-off message' + 'file_name': 'Report' }, 'eu-west-1' ) From f12e0fde394b3d67b5617bea681d13311cc878b6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 1 Jun 2017 13:29:30 +0100 Subject: [PATCH 7/7] Ensure the tour sidebar gets shown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I accidentally broke it by removing a parameter. This commit reinstates that parameter and adds some tests to make sure it doesn’t happen again. --- app/main/views/send.py | 1 + tests/app/main/views/test_send.py | 13 ++++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index bfa47c33a..de6be4ff1 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -302,6 +302,7 @@ def send_test_step(service_id, template_id, step_index): skip_link=skip_link, optional_placeholder=optional_placeholder, back_link=back_link, + help=get_help_argument(), ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index f5559bba8..69ebddf74 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -335,46 +335,54 @@ def test_send_test_step_redirects_if_session_not_setup( assert session['send_test_values'] == expected_session_contents -@pytest.mark.parametrize('template_mock, partial_url, expected_h1', [ +@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( @@ -385,6 +393,7 @@ def test_send_one_off_or_test_has_correct_page_titles( template_mock, partial_url, expected_h1, + tour_shown, ): template_mock(mocker) @@ -399,6 +408,8 @@ def test_send_one_off_or_test_has_correct_page_titles( 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')),