From e42853205ccd11a455691dcc10b65f50be53ed6d Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 10 Dec 2021 14:59:18 +0000 Subject: [PATCH 1/3] Update govuk_text_input_field_widget to only show one error If there were multiple errors, this widget was joining the messages together and displaying all error messages. If a text input field does have more than one validation error, we only want to show one. --- app/main/forms.py | 2 +- tests/app/main/views/test_service_settings.py | 21 +++++++++++++++++++ tests/app/main/views/test_user_profile.py | 20 ++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/app/main/forms.py b/app/main/forms.py index 344bee3b6..f8ed35e1c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -231,7 +231,7 @@ def govuk_text_input_field_widget(self, field, type=None, param_extensions=None, "data-error-type": field.errors[0], "data-error-label": field.name }, - error_message_format: " ".join(field.errors).strip() + error_message_format: field.errors[0] } # convert to parameters that govuk understands diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index e46b380d5..6a595ab6a 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2361,6 +2361,27 @@ def test_incorrect_sms_sender_input( assert count_of_api_calls == 0 +def test_incorrect_sms_sender_input_with_multiple_errors_only_shows_the_first( + client_request, + no_sms_senders, + mock_add_sms_sender, +): + # There are two errors with the SMS sender - the length and characters used. Only one + # should be displayed on the page. + page = client_request.post( + 'main.service_add_sms_sender', + service_id=SERVICE_ONE_ID, + _data={'sms_sender': '{}'}, + _expected_status=200 + ) + + error_message = page.select_one('.govuk-error-message') + count_of_api_calls = len(mock_add_sms_sender.call_args_list) + + assert normalize_spaces(error_message.text) == 'Error: Enter 3 characters or more' + assert count_of_api_calls == 0 + + @pytest.mark.parametrize('reply_to_addresses, data, api_default_args', [ ([], {}, True), (create_multiple_email_reply_to_addresses(), {}, False), diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index 498c17153..c9dfcef7b 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -110,6 +110,26 @@ def test_should_redirect_after_email_change( ) +@pytest.mark.parametrize('email_address,error_message', [ + ('me@example.com', 'Enter a public sector email address or find out who can use Notify'), + ('not_valid', 'Enter a valid email address') # 2 errors with email address, only first error shown +]) +def test_should_show_errors_if_new_email_address_does_not_validate( + client_request, + mock_email_is_not_already_in_use, + mock_get_organisations, + email_address, + error_message, +): + page = client_request.post( + 'main.user_profile_email', + _data={'email_address': email_address}, + _expected_status=200, + ) + + assert normalize_spaces(page.find('span', class_='govuk-error-message').text) == f'Error: {error_message}' + + def test_should_show_authenticate_after_email_change( client_request, ): From 1da285cf520a2627810868f13dcc8f95c8b12a10 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 10 Dec 2021 15:08:40 +0000 Subject: [PATCH 2/3] Only show one error for radio field and check boxes We don't currently have any radio fields or check boxes where it's possible to get more than one validation error. However, since we never want to show more than one error at a time for a field, this changes the error messages for the relevant widgets to only show the first error if there ever were multiple. --- app/main/forms.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index f8ed35e1c..3b05c7b9e 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -657,7 +657,7 @@ def govuk_checkbox_field_widget(self, field, param_extensions=None, **kwargs): "data-error-type": field.errors[0], "data-error-label": field.name }, - "text": " ".join(field.errors).strip() + "text": field.errors[0] } params = { @@ -710,7 +710,7 @@ def govuk_checkboxes_field_widget(self, field, wrap_in_collapsible=False, param_ "data-error-type": field.errors[0], "data-error-label": field.name }, - "text": " ".join(field.errors).strip() + "text": field.errors[0] } # returns either a list or a hierarchy of lists @@ -765,7 +765,7 @@ def govuk_radios_field_widget(self, field, param_extensions=None, **kwargs): "data-error-type": field.errors[0], "data-error-label": field.name }, - "text": " ".join(field.errors).strip() + "text": field.errors[0] } # returns either a list or a hierarchy of lists From d8ebcdce22e10a7c0fcc31a5ea08bae7d4e3da24 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 10 Dec 2021 16:56:08 +0000 Subject: [PATCH 3/3] Stop errors when changing an email address to an invalid one We use the `ChangeEmailForm` if you want to change your own email address or someone else's email address. This has various validators which get run. We check if the email address is valid (by using a function from utils) and if the email address is already in use (by calling API). If the email address is not valid, we should not call API to see if it's already in use because this will cause an exception in API leading to a `500` in admin. We now only call API if there were no other errors with the email address. (The `test_should_redirect_after_name_change` test didn't need the `mock_email_is_not_already_in_use` fixture, so this has been removed.) --- app/main/forms.py | 6 ++++++ tests/app/main/views/test_user_profile.py | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/main/forms.py b/app/main/forms.py index 3b05c7b9e..4b1deece3 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1535,6 +1535,12 @@ class ChangeEmailForm(StripWhitespaceForm): email_address = email_address() def validate_email_address(self, field): + # The validate_email_func can be used to call API to check if the email address is already in + # use. We don't want to run that check for invalid email addresses, since that will cause an error. + # If there are any other validation errors on the email_address, we should skip this check. + if self.email_address.errors: + return + is_valid = self.validate_email_func(field.data) if is_valid: raise ValidationError("The email address is already in use") diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index c9dfcef7b..0368f8fca 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -74,7 +74,6 @@ def test_should_show_name_page( def test_should_redirect_after_name_change( client_request, mock_update_user_attribute, - mock_email_is_not_already_in_use ): client_request.post( 'main.user_profile_name', @@ -109,6 +108,8 @@ def test_should_redirect_after_email_change( ) ) + assert mock_email_is_not_already_in_use.called + @pytest.mark.parametrize('email_address,error_message', [ ('me@example.com', 'Enter a public sector email address or find out who can use Notify'), @@ -128,6 +129,8 @@ def test_should_show_errors_if_new_email_address_does_not_validate( ) assert normalize_spaces(page.find('span', class_='govuk-error-message').text) == f'Error: {error_message}' + # We only call API to check if the email address is already in use if there are no other errors + assert not mock_email_is_not_already_in_use.called def test_should_show_authenticate_after_email_change(