From b4b124d681faf1c19c4eb53baf6d037a0654a64e Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 4 Nov 2021 11:21:58 +0000 Subject: [PATCH] Fix bug with reply to email addresses https://www.pivotaltracker.com/story/show/180026726 There was a bug where if you enter an invalid email address in to the edit reply to email address form and click save, the form you get shown with your error message will always contain the field to set as default the reply to and also delete. This should not have been the case. If you make an error on the form when changing a reply to that is already a default, then you should not be given the chance to change it to not default, nor should you be able to delete it. This commit fixes that bug by making sure the additional form fields are only shown if the reply to being changed is not the default. --- app/main/views/service_settings.py | 5 ++ .../service-settings/email-reply-to/edit.html | 2 +- tests/app/main/views/test_service_settings.py | 62 ++++++++++++++++++- 3 files changed, 67 insertions(+), 2 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 4a20f6888..7a9bef890 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -617,9 +617,13 @@ def get_service_verify_reply_to_address_partials(service_id, notification_id): def service_edit_email_reply_to(service_id, reply_to_email_id): form = ServiceReplyToEmailForm() reply_to_email_address = current_service.get_email_reply_to_address(reply_to_email_id) + if request.method == 'GET': form.email_address.data = reply_to_email_address['email_address'] form.is_default.data = reply_to_email_address['is_default'] + + show_choice_of_default_checkbox = not reply_to_email_address['is_default'] + if form.validate_on_submit(): if form.email_address.data == reply_to_email_address["email_address"] or current_user.platform_admin: service_api_client.update_reply_to_email_address( @@ -653,6 +657,7 @@ def service_edit_email_reply_to(service_id, reply_to_email_id): 'views/service-settings/email-reply-to/edit.html', form=form, reply_to_email_address_id=reply_to_email_id, + show_choice_of_default_checkbox=show_choice_of_default_checkbox ) 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 af59668e0..8f8e86423 100644 --- a/app/templates/views/service-settings/email-reply-to/edit.html +++ b/app/templates/views/service-settings/email-reply-to/edit.html @@ -23,7 +23,7 @@ }, error_message_with_html=True ) }} - {% if form.is_default.data %} + {% if not show_choice_of_default_checkbox %}

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

diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 2c3fd9a8c..4b61d0dd6 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2893,7 +2893,7 @@ def test_add_edit_reply_to_email_address_goes_straight_to_update_if_address_not_ None, ), ]) -def test_shows_delete_link_for_email_reply_to_address( +def test_shows_delete_link_for_get_request_for_edit_email_reply_to_address( mocker, reply_to_address, expected_link_text, @@ -2923,6 +2923,66 @@ def test_shows_delete_link_for_email_reply_to_address( assert not page.select('.page-footer a') +@pytest.mark.parametrize('reply_to_address, default_choice_and_delete_link_expected, default_checkbox_checked', [ + ( + create_reply_to_email_address(is_default=False), + True, + False + ), + ( + create_reply_to_email_address(is_default=False), + True, + True + ), + ( + create_reply_to_email_address(is_default=True), + False, + False # not expecting a checkbox to even be shown to be ticked + ), +]) +def test_shows_delete_link_for_error_on_post_request_for_edit_email_reply_to_address( + mocker, + reply_to_address, + default_choice_and_delete_link_expected, + default_checkbox_checked, + fake_uuid, + client_request, +): + mocker.patch('app.service_api_client.get_reply_to_email_address', return_value=reply_to_address) + + data = {'email_address': "not a valid email address"} + if default_checkbox_checked: + data["is_default"] = "y" + + page = client_request.post( + 'main.service_edit_email_reply_to', + service_id=SERVICE_ONE_ID, + reply_to_email_id=sample_uuid(), + _data=data, + _expected_status=200 + ) + + assert page.select_one('.govuk-back-link').text.strip() == 'Back' + assert page.select_one('.govuk-back-link')['href'] == url_for( + '.service_email_reply_to', + service_id=SERVICE_ONE_ID, + ) + assert page.select_one('.govuk-error-message').text.strip() == 'Error: Enter a valid email address' + assert page.select_one('input#email_address').get('value') == 'not a valid email address' + + if default_choice_and_delete_link_expected: + link = page.select_one('.page-footer a') + assert normalize_spaces(link.text) == "Delete" + assert link['href'] == url_for( + 'main.service_confirm_delete_email_reply_to', + service_id=SERVICE_ONE_ID, + reply_to_email_id=sample_uuid() + ) + assert page.select_one('input#is_default').has_attr('checked') == default_checkbox_checked + else: + assert not page.select('.page-footer a') + + def test_confirm_delete_reply_to_email_address( fake_uuid, client_request,