From 8fb55ffa78600804d1dc180b58b060a96ee98a4e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 26 Sep 2017 15:08:21 +0100 Subject: [PATCH 1/7] Grey out None when email reply to not set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is consistent with how we differentiate other ‘unset’ values on the settings page. --- app/templates/views/service-settings.html | 3 ++- .../service-settings/email_reply_to.html | 26 ++++++++++++++++--- tests/app/main/views/test_service_settings.py | 2 +- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index ebce22710..0ff3f50e3 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -45,7 +45,8 @@ {% call row() %} {{ text_field('Email reply to address') }} - {% call field() %} + {% call field(status='default' if default_reply_to_email_address == "None" else '') %} + {{ default_reply_to_email_address }} {% if reply_to_email_address_count > 1 %}
diff --git a/app/templates/views/service-settings/email_reply_to.html b/app/templates/views/service-settings/email_reply_to.html index baa3f035a..99b49b82a 100644 --- a/app/templates/views/service-settings/email_reply_to.html +++ b/app/templates/views/service-settings/email_reply_to.html @@ -22,7 +22,7 @@
{% call(item, row_number) list_table( reply_to_email_addresses, - empty_message="You haven’t added any email addresses yet.", + empty_message="You haven’t added any email reply to addresses yet", caption="Reply to email addresses", caption_visible=false, field_headings=[ @@ -39,7 +39,27 @@ {% endif %} {% endcall %} - {{ edit_field('Change', url_for('.service_edit_email_reply_to', service_id =current_service.id, reply_to_email_id = item.id)) }} + {{ edit_field('Change', url_for('.service_edit_email_reply_to', service_id =current_service.id, reply_to_email_id = item.id)) }} {% endcall %}
-{% endblock %} \ No newline at end of file +
+
+

+ Your emails will be sent from + {{ current_service.email_from }}@notifications.service.gov.uk +

+

+ This is so they have the best chance of being delivered. + This email address can’t receive replies. +

+

+ Set up separate email addresses to receive replies + from your users. + {% if current_service.restricted and not reply_to_email_addresses %} + Your service can’t go live until you’ve added at least one + reply to address. + {% endif %} +

+
+
+{% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index e720f01c6..7e97eced7 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -713,7 +713,7 @@ def test_no_reply_to_email_addresses_message_shows( service_id=SERVICE_ONE_ID ) - assert normalize_spaces(page.select('tbody tr')[0].text) == "You haven’t added any email addresses yet." + assert normalize_spaces(page.select('tbody tr')[0].text) == "You haven’t added any email reply to addresses yet" assert len(page.select('tbody tr')) == 1 From e65e98a9f16510beb75f1b11ee749d939ad498b7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 26 Sep 2017 15:26:38 +0100 Subject: [PATCH 2/7] Fix indentation --- .../views/service-settings/email-reply-to/add.html | 4 ++-- .../views/service-settings/email-reply-to/edit.html | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/templates/views/service-settings/email-reply-to/add.html b/app/templates/views/service-settings/email-reply-to/add.html index d4fe351ce..98bd4966d 100644 --- a/app/templates/views/service-settings/email-reply-to/add.html +++ b/app/templates/views/service-settings/email-reply-to/add.html @@ -27,7 +27,7 @@ 'Add', back_link=url_for('.service_email_reply_to', service_id=current_service.id), back_link_text='Back' - ) }} + ) }} -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/app/templates/views/service-settings/email-reply-to/edit.html b/app/templates/views/service-settings/email-reply-to/edit.html index 607171789..4d2c39275 100644 --- a/app/templates/views/service-settings/email-reply-to/edit.html +++ b/app/templates/views/service-settings/email-reply-to/edit.html @@ -19,7 +19,9 @@ safe_error_message=True ) }} {% if form.is_default.data %} -

This email address is the default

+

+ This email address is the default +

{% else %}
{{ checkbox(form.is_default) }} @@ -32,4 +34,4 @@ ) }} -{% endblock %} \ No newline at end of file +{% endblock %} From 8d4418cf614d6920a10803dd3eda8f0cb585b24b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 26 Sep 2017 15:30:43 +0100 Subject: [PATCH 3/7] Equalise spacing Make the amount of space above and below is equal, and consistent with the spacing when we show the checkbox. --- app/templates/views/service-settings/email-reply-to/edit.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/service-settings/email-reply-to/edit.html b/app/templates/views/service-settings/email-reply-to/edit.html index 4d2c39275..a15902847 100644 --- a/app/templates/views/service-settings/email-reply-to/edit.html +++ b/app/templates/views/service-settings/email-reply-to/edit.html @@ -19,7 +19,7 @@ safe_error_message=True ) }} {% if form.is_default.data %} -

+

This email address is the default

{% else %} From 47ebb6190a5d3fb905ca58d60829c1144cdd6ab2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 26 Sep 2017 15:34:19 +0100 Subject: [PATCH 4/7] Make language consistent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `is_default` text and text of the checkbox should use the same terminology (‘email address’ not ‘address’). --- app/main/forms.py | 2 +- app/templates/views/service-settings/email-reply-to/edit.html | 2 +- tests/app/main/views/test_service_settings.py | 4 +++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index b16f46a42..5981af266 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -469,7 +469,7 @@ class ProviderForm(Form): class ServiceReplyToEmailForm(Form): email_address = email_address(label='Email reply to address') - is_default = BooleanField("Make this address the default") + is_default = BooleanField("Make this email address the default") class ServiceSmsSender(Form): diff --git a/app/templates/views/service-settings/email-reply-to/edit.html b/app/templates/views/service-settings/email-reply-to/edit.html index a15902847..fb0fd6473 100644 --- a/app/templates/views/service-settings/email-reply-to/edit.html +++ b/app/templates/views/service-settings/email-reply-to/edit.html @@ -20,7 +20,7 @@ ) }} {% if form.is_default.data %}

- This email address is the default + This is the default reply to address for {{ current_service.name }} emails

{% else %}
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 7e97eced7..906ee0bf2 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -838,7 +838,9 @@ def test_default_box_shows_on_non_default_email_addresses_while_editing( if checkbox_present: assert page.select_one('[name=is_default]') else: - assert normalize_spaces(page.select_one('form p').text) == "This email address is the default" + assert normalize_spaces(page.select_one('form p').text) == ( + 'This is the default reply to address for service one emails' + ) def test_switch_service_to_research_mode( From b68784207be6de6345a99ce2812a351f3426261d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 26 Sep 2017 15:51:36 +0100 Subject: [PATCH 5/7] Add email reply address to ID to the page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first users of multiple email reply to addresses will be using the API. This means that the need to be able to specify the ID of the reply to address they want. We chose to implement it like this instead of by passing the address in directly because that means deploying code. For some teams deploying code can take weeks, and we’d like to let teams have the flexibility to make changes faster than this. Same as for templates, you shouldn’t have to go to the _edit_ page in order to get the ID. This means listing them on the page where you see all the reply to addresses. Listing the IDs like this means that it’s not really a table any more, because the information isn’t organised in columns. So I think it makes sense to reuse the pattern from the manage team page, which has a similar relationship between the information. --- app/templates/components/api-key.html | 10 ++-- .../service-settings/email_reply_to.html | 47 ++++++++++--------- tests/app/main/views/test_service_settings.py | 20 ++++---- 3 files changed, 42 insertions(+), 35 deletions(-) diff --git a/app/templates/components/api-key.html b/app/templates/components/api-key.html index 81e909d66..dc5e6c2ee 100644 --- a/app/templates/components/api-key.html +++ b/app/templates/components/api-key.html @@ -1,7 +1,9 @@ -{% macro api_key(key, name, thing="API key") %} -

- {{ name }} -

+{% macro api_key(key, name=None, thing="API key") %} + {% if name %} +

+ {{ name }} +

+ {% endif %}
{{ key }}
diff --git a/app/templates/views/service-settings/email_reply_to.html b/app/templates/views/service-settings/email_reply_to.html index 99b49b82a..b5efa04a7 100644 --- a/app/templates/views/service-settings/email_reply_to.html +++ b/app/templates/views/service-settings/email_reply_to.html @@ -1,5 +1,5 @@ {% extends "withnav_template.html" %} -{% from "components/textbox.html" import textbox %} +{% from "components/api-key.html" import api_key %} {% from "components/page-footer.html" import page_footer %} {% from "components/table.html" import row_group, row, text_field, edit_field, field, boolean_field, list_table %} @@ -19,28 +19,29 @@ Add email address
-
- {% call(item, row_number) list_table( - reply_to_email_addresses, - empty_message="You haven’t added any email reply to addresses yet", - caption="Reply to email addresses", - caption_visible=false, - field_headings=[ - 'Email addresses', - 'Action' - ], - field_headings_visible=False - ) %} - {% call field() %} - {{ item.email_address }} - {% if item.is_default %} - - {{ "(default)" }} - - {% endif %} - {% endcall %} - {{ edit_field('Change', url_for('.service_edit_email_reply_to', service_id =current_service.id, reply_to_email_id = item.id)) }} - {% endcall %} +
+ {% if not reply_to_email_addresses %} +
+ You haven’t added any email reply to addresses yet +
+ {% endif %} + {% for item in reply_to_email_addresses %} +
+

+ {{ item.email_address }}  + {%- if item.is_default -%} + (default) + {% endif %} + +

+ + {{ api_key(item.id, thing="ID") }} +
+ {% endfor %}
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 906ee0bf2..439feb14a 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -693,28 +693,32 @@ def test_default_email_reply_to_address_has_default_hint( client_request, multiple_reply_to_email_addresses ): - page = client_request.get( + rows = client_request.get( 'main.service_email_reply_to', service_id=SERVICE_ONE_ID + ).select( + '.user-list-item' ) - assert normalize_spaces(page.select('tbody tr')[0].text) == "test@example.com (default) Change" - assert normalize_spaces(page.select('tbody tr')[1].text) == "test2@example.com Change" - assert normalize_spaces(page.select('tbody tr')[2].text) == "test3@example.com Change" - assert len(page.select('tbody tr')) == 3 + assert normalize_spaces(rows[0].text) == "test@example.com (default) Change 1234" + assert normalize_spaces(rows[1].text) == "test2@example.com Change 5678" + assert normalize_spaces(rows[2].text) == "test3@example.com Change 9457" + assert len(rows) == 3 def test_no_reply_to_email_addresses_message_shows( client_request, no_reply_to_email_addresses ): - page = client_request.get( + rows = client_request.get( 'main.service_email_reply_to', service_id=SERVICE_ONE_ID + ).select( + '.user-list-item' ) - assert normalize_spaces(page.select('tbody tr')[0].text) == "You haven’t added any email reply to addresses yet" - assert len(page.select('tbody tr')) == 1 + assert normalize_spaces(rows[0].text) == "You haven’t added any email reply to addresses yet" + assert len(rows) == 1 @pytest.mark.parametrize('reply_to_input, expected_error', [ From c78fac911af1db99228faa81276aef22606eaf3d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 26 Sep 2017 16:00:13 +0100 Subject: [PATCH 6/7] =?UTF-8?q?Say=20manage=20if=20there=E2=80=99s=20more?= =?UTF-8?q?=20than=20none?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ‘Change’ as a label for the link is misleading, because this is also the page you go to in order to get the ID of a given reply to address. ‘Manage’ feels a bit more general. --- app/templates/views/service-settings.html | 7 +++++-- tests/app/main/views/test_service_settings.py | 10 +++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 0ff3f50e3..cf3623752 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -44,7 +44,7 @@ {% if 'email' in current_service.permissions %} {% call row() %} - {{ text_field('Email reply to address') }} + {{ text_field('Email reply to addresses') }} {% call field(status='default' if default_reply_to_email_address == "None" else '') %} {{ default_reply_to_email_address }} @@ -54,7 +54,10 @@
{% endif %} {% endcall %} - {{ edit_field('Change', url_for('.service_email_reply_to', service_id=current_service.id)) }} + {{ edit_field( + 'Manage' if reply_to_email_address_count else 'Change', + url_for('.service_email_reply_to', service_id=current_service.id)) + }} {% endcall %} {% endif %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 439feb14a..068a6fbc1 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -31,7 +31,7 @@ from tests.conftest import ( 'Label Value Action', 'Send emails On Change', - 'Email reply to address None Change', + 'Email reply to addresses None Change', 'Label Value Action', 'Send text messages On Change', @@ -50,7 +50,7 @@ from tests.conftest import ( 'Label Value Action', 'Send emails On Change', - 'Email reply to address None Change', + 'Email reply to addresses None Change', 'Label Value Action', 'Send text messages On Change', @@ -102,7 +102,7 @@ def test_should_show_overview( 'Label Value Action', 'Send emails On Change', - 'Email reply to address test@example.com Change', + 'Email reply to addresses test@example.com Manage', 'Label Value Action', 'Send text messages On Change', @@ -121,7 +121,7 @@ def test_should_show_overview( 'Label Value Action', 'Send emails On Change', - 'Email reply to address test@example.com Change', + 'Email reply to addresses test@example.com Manage', 'Label Value Action', 'Send text messages On Change', @@ -686,7 +686,7 @@ def test_reply_to_hint_appears_when_service_has_multiple_reply_to_addresses( assert normalize_spaces( page.select('tbody tr')[2].text - ) == "Email reply to address test@example.com …and 2 more Change" + ) == "Email reply to addresses test@example.com …and 2 more Manage" def test_default_email_reply_to_address_has_default_hint( From c35088796aa0a0858a2170d96a94130afd23f89b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 28 Sep 2017 11:35:52 +0100 Subject: [PATCH 7/7] Hide ID when only one reply to address is shown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most user will only have one reply to address. Which means they should never have to worry about IDs. And if you only have one then you never need its ID, because the last remaining address will always be the default. So IDs should only be shown when a service has created more than one reply to address. This required a bit of visual tweaking of the _user list_ pattern, because its spacing wasn’t defined in a way that worked when only the name of the thing, and not its details were shown on the page. --- app/assets/stylesheets/components/tick-cross.scss | 4 ++-- .../views/service-settings/email_reply_to.html | 4 +++- tests/app/main/views/test_service_settings.py | 15 +++++++++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/components/tick-cross.scss b/app/assets/stylesheets/components/tick-cross.scss index 3601b0ad4..4f80e08b3 100644 --- a/app/assets/stylesheets/components/tick-cross.scss +++ b/app/assets/stylesheets/components/tick-cross.scss @@ -42,12 +42,12 @@ &-list { @extend %grid-row; - margin-top: 5px; position: relative; &-permissions { @include grid-column(3/4); + margin-top: 5px; li { display: block; @@ -59,7 +59,7 @@ &-edit-link { text-align: right; position: absolute; - top: -1.6em; + top: -25px; right: -135px; } diff --git a/app/templates/views/service-settings/email_reply_to.html b/app/templates/views/service-settings/email_reply_to.html index b5efa04a7..71ce4c627 100644 --- a/app/templates/views/service-settings/email_reply_to.html +++ b/app/templates/views/service-settings/email_reply_to.html @@ -39,7 +39,9 @@ Change - {{ api_key(item.id, thing="ID") }} + {% if reply_to_email_addresses|length > 1 %} + {{ api_key(item.id, thing="ID") }} + {% endif %}
{% endfor %}
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 068a6fbc1..1ccc2264a 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -689,6 +689,21 @@ def test_reply_to_hint_appears_when_service_has_multiple_reply_to_addresses( ) == "Email reply to addresses test@example.com …and 2 more Manage" +def test_single_reply_to_address_shows_default_but_without_id( + client_request, + single_reply_to_email_addresses +): + rows = client_request.get( + 'main.service_email_reply_to', + service_id=SERVICE_ONE_ID + ).select( + '.user-list-item' + ) + + assert normalize_spaces(rows[0].text) == "test@example.com (default) Change" + assert len(rows) == 1 + + def test_default_email_reply_to_address_has_default_hint( client_request, multiple_reply_to_email_addresses