From 4105b6ff55e142b5224ce73dfc11b7f60433ec91 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Nov 2017 16:26:09 +0000 Subject: [PATCH 1/4] Fix misleading `

` in one-off flow Changing the `

` in https://github.com/alphagov/notifications-admin/pull/1638 turned out to be quite confusing. The combination of the word "recipient" and a selection of email addresses on the page was confusing. This commit changes the page title to be much more explicit about what is expected from the page, rather than what is consistent with the text of the link that the user clicked. --- app/main/views/send.py | 4 ++-- tests/app/main/views/test_send.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 2eab37327..e0db1e8d1 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -217,7 +217,7 @@ def set_sender(service_id, template_id): def get_sender_context(sender_details, template_type): context = { 'email': { - 'title': 'Send to one recipient', + 'title': 'Where should replies go?', 'description': 'Where should replies go?', 'field_name': 'email_address' }, @@ -227,7 +227,7 @@ def get_sender_context(sender_details, template_type): 'field_name': 'contact_block' }, 'sms': { - 'title': 'Send to one recipient', + 'title': 'Who should the message come from?', 'description': 'Who should the message come from?', 'field_name': 'sms_sender' } diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 7b2dcc370..5de52ebee 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -44,14 +44,14 @@ test_non_spreadsheet_files = glob(path.join('tests', 'non_spreadsheet_files', '* ( mock_get_service_email_template, multiple_reply_to_email_addresses, - 'Send to one recipient', + 'Where should replies go?', 'Where should replies go?', ), ( mock_get_service_template, multiple_sms_senders, - 'Send to one recipient', - 'Who should the message come from?' + 'Who should the message come from?', + 'Who should the message come from?', ) ]) def test_show_correct_title_and_description_for_sender_type( From ecd43cee5e64d34a3f026e02abfb05e33f6d2b2b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Nov 2017 16:47:03 +0000 Subject: [PATCH 2/4] Fix misleading `

` in one-off flow Changing the `

` in https://github.com/alphagov/notifications-admin/pull/1638 turned out to be quite confusing. The combination of the word "recipient" and a selection of email addresses on the page was confusing. This commit changes the page title to be much more explicit about what is expected from the page, rather than what is consistent with the text of the link that the user clicked. --- app/main/views/send.py | 4 ++-- tests/app/main/views/test_send.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index e0db1e8d1..31fec2cd7 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -217,8 +217,8 @@ def set_sender(service_id, template_id): def get_sender_context(sender_details, template_type): context = { 'email': { - 'title': 'Where should replies go?', - 'description': 'Where should replies go?', + 'title': 'Where should replies come back to?', + 'description': 'Where should replies come back to?', 'field_name': 'email_address' }, 'letter': { diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 5de52ebee..3ae9666b0 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -44,8 +44,8 @@ test_non_spreadsheet_files = glob(path.join('tests', 'non_spreadsheet_files', '* ( mock_get_service_email_template, multiple_reply_to_email_addresses, - 'Where should replies go?', - 'Where should replies go?', + 'Where should replies come back to?', + 'Where should replies come back to?', ), ( mock_get_service_template, From 4a1dad9a66c6b783e619c25304c781e1ee550db4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Nov 2017 16:30:51 +0000 Subject: [PATCH 3/4] Hide form legend on choose reply page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `

` on this page says all the user needs to know. In research we saw that users didn’t even read the legend, even when prompted to! --- app/templates/components/radios.html | 7 +++++-- app/templates/views/templates/set-sender.html | 8 ++++---- tests/app/main/views/test_send.py | 4 +++- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/app/templates/components/radios.html b/app/templates/components/radios.html index d5d3682ea..b6149fa6b 100644 --- a/app/templates/components/radios.html +++ b/app/templates/components/radios.html @@ -2,12 +2,15 @@ field, hint=None, disable=[], - option_hints={} + option_hints={}, + hide_legend=False ) %}
- {{ field.label.text|safe }} + {% if hide_legend %}{% endif %} {% if field.errors %} {{ field.errors[0] }} diff --git a/app/templates/views/templates/set-sender.html b/app/templates/views/templates/set-sender.html index 363499dac..5397cdf7e 100644 --- a/app/templates/views/templates/set-sender.html +++ b/app/templates/views/templates/set-sender.html @@ -13,10 +13,10 @@
{{ radios( - form.sender, - option_hints=option_hints - ) - }} + form.sender, + option_hints=option_hints, + hide_legend=True + ) }} {{ page_footer( 'Continue', back_link=url_for('.view_template', service_id=current_service.id, template_id=template_id), diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 3ae9666b0..38fe48f3e 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -74,7 +74,9 @@ def test_show_correct_title_and_description_for_sender_type( ) assert page.select_one('h1').text == expected_title - assert normalize_spaces(page.select_one('legend').text) == expected_description + + for element in ('legend', 'legend .visually-hidden'): + assert normalize_spaces(page.select_one(element).text) == expected_description @pytest.mark.parametrize('template_mock, sender_data', [ From 5f40eabdf3e296714d2b0696e1e262b8067ebeb4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Nov 2017 16:45:59 +0000 Subject: [PATCH 4/4] Improve page titles later on in the one off flow Now that the page title for picking a sender/reply to has been improved, I think these pages are also less clear than they could be. This commit changes the page titles to (I hope) be clearer about what is needed from the user on these pages. --- app/main/views/send.py | 12 +++++++++--- tests/app/main/views/test_send.py | 8 ++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 31fec2cd7..6e8b81f44 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -421,7 +421,11 @@ def send_test_step(service_id, template_id, step_index): skip_link = None return render_template( 'views/send-test.html', - page_title=get_send_test_page_title(template.template_type, get_help_argument()), + page_title=get_send_test_page_title( + template.template_type, + get_help_argument(), + entering_recipient=not session['recipient'] + ), template=template, form=form, skip_link=skip_link, @@ -711,12 +715,14 @@ def all_placeholders_in_session(placeholders): ) -def get_send_test_page_title(template_type, help_argument): +def get_send_test_page_title(template_type, help_argument, entering_recipient): if help_argument: return 'Example text message' if template_type == 'letter': return 'Print a test letter' - return 'Send to one recipient' + if entering_recipient: + return 'Who should this message be sent to?' + return 'Personalise this message' def get_back_link(service_id, template_id, step_index): diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 38fe48f3e..f3618f384 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -575,13 +575,13 @@ def test_send_one_off_does_not_send_without_the_correct_permissions( ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_test'), - 'Send to one recipient', + 'Personalise this message', False, ), ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_one_off'), - 'Send to one recipient', + 'Who should this message be sent to?', False, ), ( @@ -599,13 +599,13 @@ def test_send_one_off_does_not_send_without_the_correct_permissions( ( mock_get_service_email_template, partial(url_for, 'main.send_test'), - 'Send to one recipient', + 'Personalise this message', False, ), ( mock_get_service_email_template, partial(url_for, 'main.send_one_off'), - 'Send to one recipient', + 'Who should this message be sent to?', False, ), (