diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 720c67155..fbe0ab071 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -736,7 +736,16 @@ def service_add_letter_contact(service_id): ) -@main.route("/services//service-settings/letter-contact//edit", methods=['GET', 'POST']) +@main.route( + "/services//service-settings/letter-contact//edit", + methods=['GET', 'POST'], + endpoint="service_edit_letter_contact", +) +@main.route( + "/services//service-settings/letter-contact//delete", + methods=['GET'], + endpoint="service_confirm_delete_letter_contact", +) @user_has_permissions('manage_service') def service_edit_letter_contact(service_id, letter_contact_id): letter_contact_block = current_service.get_letter_contact_block(letter_contact_id) @@ -746,19 +755,41 @@ def service_edit_letter_contact(service_id, letter_contact_id): if request.method == 'GET': form.is_default.data = letter_contact_block['is_default'] if form.validate_on_submit(): - service_api_client.update_letter_contact( - current_service.id, - letter_contact_id=letter_contact_id, + current_service.edit_letter_contact_block( + id=letter_contact_id, contact_block=form.letter_contact_block.data.replace('\r', '') or None, - is_default=True if letter_contact_block['is_default'] else form.is_default.data + is_default=letter_contact_block['is_default'] or form.is_default.data ) return redirect(url_for('.service_letter_contact_details', service_id=service_id)) + + if (request.endpoint == "main.service_confirm_delete_letter_contact"): + flash("Are you sure you want to delete this contact block?", 'delete') return render_template( 'views/service-settings/letter-contact/edit.html', form=form, letter_contact_id=letter_contact_block['id']) +@main.route("/services//service-settings/letter-contact/make-blank-default") +@user_has_permissions('manage_service') +def service_make_blank_default_letter_contact(service_id): + current_service.remove_default_letter_contact_block() + return redirect(url_for('.service_letter_contact_details', service_id=service_id)) + + +@main.route( + "/services//service-settings/letter-contact//delete", + methods=['POST'], +) +@user_has_permissions('manage_service') +def service_delete_letter_contact(service_id, letter_contact_id): + service_api_client.delete_letter_contact( + service_id=current_service.id, + letter_contact_id=letter_contact_id, + ) + return redirect(url_for('.service_letter_contact_details', service_id=current_service.id)) + + @main.route("/services//service-settings/sms-sender", methods=['GET']) @user_has_permissions('manage_service', 'manage_api_keys') def service_sms_senders(service_id): diff --git a/app/models/service.py b/app/models/service.py index 7e7662b20..f9dc2706b 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -1,5 +1,7 @@ -from flask import abort, current_app +from flask import Markup, abort, current_app from notifications_utils.field import Field +from notifications_utils.formatters import nl2br +from notifications_utils.take import Take from werkzeug.utils import cached_property from app.models import JSONModel @@ -324,11 +326,36 @@ class Service(JSONModel): def default_letter_contact_block(self): return next( ( - Field(x['contact_block'], html='escape') - for x in self.letter_contact_details if x['is_default'] + letter_contact_block + for letter_contact_block in self.letter_contact_details + if letter_contact_block['is_default'] ), None ) + @property + def default_letter_contact_block_html(self): + if self.default_letter_contact_block: + return Markup(Take(Field( + self.default_letter_contact_block['contact_block'], + html='escape', + )).then( + nl2br + )) + return '' + + def edit_letter_contact_block(self, id, contact_block, is_default): + service_api_client.update_letter_contact( + self.id, letter_contact_id=id, contact_block=contact_block, is_default=is_default, + ) + + def remove_default_letter_contact_block(self): + if self.default_letter_contact_block: + self.edit_letter_contact_block( + self.default_letter_contact_block['id'], + self.default_letter_contact_block['contact_block'], + is_default=False, + ) + def get_letter_contact_block(self, id): return service_api_client.get_letter_contact(self.id, id) diff --git a/app/navigation.py b/app/navigation.py index e2161f8ff..d643f4c6a 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -247,10 +247,12 @@ class HeaderNavigation(Navigation): 'service_accept_agreement', 'service_confirm_agreement', 'service_confirm_delete_email_reply_to', + 'service_confirm_delete_letter_contact', 'service_confirm_delete_sms_sender', 'service_dashboard', 'service_dashboard_updates', 'service_delete_email_reply_to', + 'service_delete_letter_contact', 'service_delete_sms_sender', 'service_download_agreement', 'service_edit_email_reply_to', @@ -259,6 +261,7 @@ class HeaderNavigation(Navigation): 'service_email_reply_to', 'service_letter_contact_details', 'service_letter_validation_preview', + 'service_make_blank_default_letter_contact', 'service_name_change', 'service_name_change_confirm', 'service_preview_email_branding', @@ -382,12 +385,14 @@ class MainNavigation(Navigation): 'service_accept_agreement', 'service_confirm_agreement', 'service_confirm_delete_email_reply_to', + 'service_confirm_delete_letter_contact', 'service_confirm_delete_sms_sender', 'service_edit_email_reply_to', 'service_edit_letter_contact', 'service_edit_sms_sender', 'service_email_reply_to', 'service_letter_contact_details', + 'service_make_blank_default_letter_contact', 'service_name_change', 'service_name_change_confirm', 'service_preview_email_branding', @@ -537,6 +542,7 @@ class MainNavigation(Navigation): 'send_notification', 'service_dashboard_updates', 'service_delete_email_reply_to', + 'service_delete_letter_contact', 'service_delete_sms_sender', 'service_download_agreement', 'service_letter_validation_preview', @@ -772,10 +778,12 @@ class CaseworkNavigation(Navigation): 'service_accept_agreement', 'service_confirm_agreement', 'service_confirm_delete_email_reply_to', + 'service_confirm_delete_letter_contact', 'service_confirm_delete_sms_sender', 'service_dashboard', 'service_dashboard_updates', 'service_delete_email_reply_to', + 'service_delete_letter_contact', 'service_delete_sms_sender', 'service_download_agreement', 'service_edit_email_reply_to', @@ -784,6 +792,7 @@ class CaseworkNavigation(Navigation): 'service_email_reply_to', 'service_letter_contact_details', 'service_letter_validation_preview', + 'service_make_blank_default_letter_contact', 'service_name_change', 'service_name_change_confirm', 'service_preview_email_branding', @@ -1043,10 +1052,12 @@ class OrgNavigation(Navigation): 'service_accept_agreement', 'service_confirm_agreement', 'service_confirm_delete_email_reply_to', + 'service_confirm_delete_letter_contact', 'service_confirm_delete_sms_sender', 'service_dashboard', 'service_dashboard_updates', 'service_delete_email_reply_to', + 'service_delete_letter_contact', 'service_delete_sms_sender', 'service_download_agreement', 'service_edit_email_reply_to', @@ -1055,6 +1066,7 @@ class OrgNavigation(Navigation): 'service_email_reply_to', 'service_letter_contact_details', 'service_letter_validation_preview', + 'service_make_blank_default_letter_contact', 'service_name_change', 'service_name_change_confirm', 'service_preview_email_branding', diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 8adbee048..519e268d3 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -458,6 +458,13 @@ class ServiceAPIClient(NotifyAdminAPIClient): } ) + @cache.delete('service-{service_id}') + def delete_letter_contact(self, service_id, letter_contact_id): + return self.post( + "/service/{}/letter-contact/{}/archive".format(service_id, letter_contact_id), + data=None + ) + def get_sms_senders(self, service_id): return self.get( "/service/{}/sms-sender".format(service_id) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index bc750e825..a74a91d58 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -213,8 +213,14 @@ {% call settings_row(if_has_permission='letter') %} {{ text_field('Sender addresses') }} - {% call field(status='default' if current_service.default_letter_contact_block == None else '') %} - {{ current_service.default_letter_contact_block | string | nl2br | safe if current_service.default_letter_contact_block else 'Not set'}} + {% call field(status='' if current_service.count_letter_contact_details else 'default') %} + {% if current_service.default_letter_contact_block %} + {{ current_service.default_letter_contact_block_html }} + {% elif current_service.count_letter_contact_details %} + Blank + {% else %} + Not set + {% endif %} {% if current_service.count_letter_contact_details > 1 %}
{{ '…and %d more' | format(current_service.count_letter_contact_details - 1) }} diff --git a/app/templates/views/service-settings/letter-contact-details.html b/app/templates/views/service-settings/letter-contact-details.html index f4de702a0..3a18b16be 100644 --- a/app/templates/views/service-settings/letter-contact-details.html +++ b/app/templates/views/service-settings/letter-contact-details.html @@ -17,11 +17,18 @@ ) }}
- {% if not letter_contact_details %} -
- You haven’t added any letter contact details yet -
- {% endif %} +
+ + Blank + {% if current_service.default_letter_contact_block %} + {% if current_user.has_permissions('manage_service') %} + Make default + {% endif %} + {% else %} + (default) + {% endif %} + +
{% for item in letter_contact_details %}

diff --git a/app/templates/views/service-settings/letter-contact/edit.html b/app/templates/views/service-settings/letter-contact/edit.html index 3dc8dc4d9..8c17b67b6 100644 --- a/app/templates/views/service-settings/letter-contact/edit.html +++ b/app/templates/views/service-settings/letter-contact/edit.html @@ -24,16 +24,24 @@ rows=10, highlight_tags=True ) }} + + {% if form.is_default.data %}

- This is currently your default address for {{ current_service.name }} + This is the default address for {{ current_service.name }}

{% else %}
{{ checkbox(form.is_default) }}
{% endif %} - {{ page_footer('Save') }} + + {{ page_footer( + 'Save', + delete_link=url_for('.service_confirm_delete_letter_contact', service_id=current_service.id, letter_contact_id=letter_contact_id), + delete_link_text='Delete' + ) }} + {% endcall %} {% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index c45d2777c..d983490ff 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1852,10 +1852,10 @@ def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender( assert get_row(page, 11) == "Sender addresses 1 Example Street …and 2 more Manage" -@pytest.mark.parametrize('sender_list_page, expected_output', [ - ('main.service_email_reply_to', 'test@example.com (default) Change'), - ('main.service_letter_contact_details', '1 Example Street (default) Change'), - ('main.service_sms_senders', 'GOVUK (default) Change') +@pytest.mark.parametrize('sender_list_page, index, expected_output', [ + ('main.service_email_reply_to', 0, 'test@example.com (default) Change'), + ('main.service_letter_contact_details', 1, '1 Example Street (default) Change'), + ('main.service_sms_senders', 0, 'GOVUK (default) Change') ]) def test_api_ids_dont_show_on_option_pages_with_a_single_sender( client_request, @@ -1864,7 +1864,8 @@ def test_api_ids_dont_show_on_option_pages_with_a_single_sender( mock_get_service_organisation, single_sms_sender, sender_list_page, - expected_output + index, + expected_output, ): rows = client_request.get( sender_list_page, @@ -1873,34 +1874,41 @@ def test_api_ids_dont_show_on_option_pages_with_a_single_sender( '.user-list-item' ) - assert normalize_spaces(rows[0].text) == expected_output - assert len(rows) == 1 + assert normalize_spaces(rows[index].text) == expected_output + assert len(rows) == index + 1 @pytest.mark.parametrize( - 'sender_list_page, \ - sample_data, \ - expected_default_sender_output, \ - expected_second_sender_output, \ - expected_third_sender_output', + ( + 'sender_list_page,' + 'sample_data,' + 'expected_items,' + ), [( 'main.service_email_reply_to', multiple_reply_to_email_addresses, - 'test@example.com (default) Change 1234', - 'test2@example.com Change 5678', - 'test3@example.com Change 9457' + [ + 'test@example.com (default) Change 1234', + 'test2@example.com Change 5678', + 'test3@example.com Change 9457', + ], ), ( 'main.service_letter_contact_details', multiple_letter_contact_blocks, - '1 Example Street (default) Change 1234', - '2 Example Street Change 5678', - '3 Example Street Change 9457' + [ + 'Blank Make default', + '1 Example Street (default) Change 1234', + '2 Example Street Change 5678', + '3 Example Street Change 9457', + ], ), ( 'main.service_sms_senders', multiple_sms_senders, - 'Example (default and receives replies) Change 1234', - 'Example 2 Change 5678', - 'Example 3 Change 9457' + [ + 'Example (default and receives replies) Change 1234', + 'Example 2 Change 5678', + 'Example 3 Change 9457', + ], ), ] ) @@ -1909,9 +1917,7 @@ def test_default_option_shows_for_default_sender( mocker, sender_list_page, sample_data, - expected_default_sender_output, - expected_second_sender_output, - expected_third_sender_output + expected_items, ): sample_data(mocker) @@ -1922,10 +1928,40 @@ def test_default_option_shows_for_default_sender( '.user-list-item' ) - assert normalize_spaces(rows[0].text) == expected_default_sender_output - assert normalize_spaces(rows[1].text) == expected_second_sender_output - assert normalize_spaces(rows[2].text) == expected_third_sender_output - assert len(rows) == 3 + assert [normalize_spaces(row.text) for row in rows] == expected_items + + +def test_remove_default_from_default_letter_contact_block( + client_request, + mocker, + multiple_letter_contact_blocks, + mock_update_letter_contact, +): + letter_contact_details_page = url_for( + 'main.service_letter_contact_details', + service_id=SERVICE_ONE_ID, + _external=True, + ) + + link = client_request.get_url(letter_contact_details_page).select_one('.user-list-item a') + assert link.text == 'Make default' + assert link['href'] == url_for( + '.service_make_blank_default_letter_contact', + service_id=SERVICE_ONE_ID, + ) + + client_request.get_url( + link['href'], + _expected_status=302, + _expected_redirect=letter_contact_details_page, + ) + + mock_update_letter_contact.assert_called_once_with( + SERVICE_ONE_ID, + letter_contact_id='1234', + contact_block='1 Example Street', + is_default=False, + ) @pytest.mark.parametrize('sender_list_page, sample_data, expected_output', [ @@ -1937,7 +1973,7 @@ def test_default_option_shows_for_default_sender( ( 'main.service_letter_contact_details', no_letter_contact_blocks, - 'You haven’t added any letter contact details yet' + 'Blank (default)' ), ( 'main.service_sms_senders', @@ -2464,6 +2500,51 @@ def test_edit_letter_contact_block( ) +def test_confirm_delete_letter_contact_block( + fake_uuid, + client_request, + get_default_letter_contact_block, +): + + page = client_request.get( + 'main.service_confirm_delete_letter_contact', + service_id=SERVICE_ONE_ID, + letter_contact_id=fake_uuid, + _test_page_title=False, + ) + + assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( + 'Are you sure you want to delete this contact block? ' + 'Yes, delete' + ) + assert 'action' not in page.select_one('.banner-dangerous form') + assert page.select_one('.banner-dangerous form')['method'] == 'post' + + +def test_delete_letter_contact_block( + client_request, + service_one, + fake_uuid, + get_default_letter_contact_block, + mocker, +): + mock_delete = mocker.patch('app.service_api_client.delete_letter_contact') + client_request.post( + '.service_delete_letter_contact', + service_id=SERVICE_ONE_ID, + letter_contact_id=fake_uuid, + _expected_redirect=url_for( + 'main.service_letter_contact_details', + service_id=SERVICE_ONE_ID, + _external=True, + ) + ) + mock_delete.assert_called_once_with( + service_id=SERVICE_ONE_ID, + letter_contact_id=fake_uuid, + ) + + @pytest.mark.parametrize('fixture, data, api_default_args', [ (get_default_sms_sender, {"is_default": "y", "sms_sender": "test"}, True), (get_default_sms_sender, {"sms_sender": "test"}, True), @@ -2513,14 +2594,14 @@ def test_edit_sms_sender( ( 'main.service_edit_letter_contact', get_default_letter_contact_block, - 'This is currently your default address for service one', + 'This is the default address for service one', 'letter_contact_id', False ), ( 'main.service_edit_letter_contact', get_non_default_letter_contact_block, - 'This is the default contact details for service one letters', + 'THIS TEXT WONT BE TESTED', 'letter_contact_id', True ), diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 24d350146..d12e16b01 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -355,6 +355,7 @@ def test_returns_value_from_cache( (service_api_client, 'delete_reply_to_email_address', [SERVICE_ONE_ID, ''], {}), (service_api_client, 'add_letter_contact', [SERVICE_ONE_ID, ''], {}), (service_api_client, 'update_letter_contact', [SERVICE_ONE_ID] + [''] * 2, {}), + (service_api_client, 'delete_letter_contact', [SERVICE_ONE_ID, ''], {}), (service_api_client, 'add_sms_sender', [SERVICE_ONE_ID, ''], {}), (service_api_client, 'update_sms_sender', [SERVICE_ONE_ID] + [''] * 2, {}), (service_api_client, 'delete_sms_sender', [SERVICE_ONE_ID, ''], {}),