From 09899eb99d22d216e1d95eb91f1467e6063edcd0 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 1 Mar 2022 13:51:04 +0000 Subject: [PATCH 1/4] Replay falsey values in input fields Currently an integer of 0 doesn't get shown because it fails the truthiness check in the govuk-frontend template [^1]. Note that we can't just do str(value) as for None this would be "None". [^1]: https://github.com/alphagov/govuk-frontend/blob/fd4952f1c02c79056e09fc9c9ebae5c078140e49/src/govuk/components/input/template.njk#L51 --- app/main/forms.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/main/forms.py b/app/main/forms.py index f6f7412a2..4fa538c3c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2,6 +2,7 @@ import math import weakref from datetime import datetime, timedelta from itertools import chain +from numbers import Number import pytz from flask import Markup, render_template, request @@ -221,6 +222,7 @@ def password(label='Password'): def govuk_text_input_field_widget(self, field, type=None, param_extensions=None, **kwargs): value = kwargs["value"] if kwargs.get("value") else field.data + value = str(value) if isinstance(value, Number) else value # error messages error_message = None From 21452649fdace2ba16778e3213ff59b09e8e4527 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Mar 2022 12:40:18 +0000 Subject: [PATCH 2/4] Fix ignoring submitted data that was falsey This was causing some tests for the "estimate_volume" endpoint to fail due to the surprising way that form handles "''": - The form is the exclusive user of the ForgivingIntegerField [^1]. - The field secretly/silently converts "''" to the integer 0 [^2]. If the validations fail, we don't want to surprise the user with a "0" when they didn't enter one. The field already handles this by massaging the values in the __call__ method that generates the HTML for the form [^3]. However, there are two scenarios: - User submits field with '' - converted to integer 0. - User submits field with '0' - remains as a string. In the case where "value" is "''", the parent class will use the converted value from form.data instead [^4]. This seems to be an oversight and so we get either the integer 0 (from form.data) or the string '0' (from the value kwarg). Complicado! Previously it was a fluke that we avoided replaying the integer 0 to the user; the previous commit removes the fluke. This fixes the conditional to always use the data in the "value" kwarg if it has been provided, as it's meant to override "form.data". [^1]: https://github.com/alphagov/notifications-admin/commit/9f634493841d3dad54ca709f4a688dabb207fd15 [^2]: https://github.com/alphagov/notifications-admin/blob/a22b8cf684b5581aad54d6e36d98e498085925a5/app/main/forms.py#L364 [^3]: https://github.com/alphagov/notifications-admin/blob/a22b8cf684b5581aad54d6e36d98e498085925a5/app/main/forms.py#L393 [^4]: https://github.com/alphagov/notifications-admin/commit/a22b8cf684b5581aad54d6e36d98e498085925a5#diff-a1c8d24b22d4478fe71f75fd43b71b18dd82aae97bc63de84473a6da1902909bR215 --- app/main/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/forms.py b/app/main/forms.py index 4fa538c3c..3b83b01cc 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -221,7 +221,7 @@ def password(label='Password'): def govuk_text_input_field_widget(self, field, type=None, param_extensions=None, **kwargs): - value = kwargs["value"] if kwargs.get("value") else field.data + value = kwargs["value"] if "value" in kwargs else field.data value = str(value) if isinstance(value, Number) else value # error messages From e0b62bed70d66dc0f8a4cae0f0c5104d6dc12638 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 11 Mar 2022 11:12:26 +0000 Subject: [PATCH 3/4] Group form tests in a directory This makes it clearer we have tests for the code in forms.py, which I missed initially. In future we could also split up forms.py in a similar way, as it's currently _very long_. As part of grouping tests for code in forms.py, I've extracted some from test_validators.py, so that what remains is focussed on testing the code in validators.py. --- .../main/{ => forms}/test_choose_time_form.py | 0 .../test_create_key_form.py} | 0 .../main/{ => forms}/test_placeholder_form.py | 0 .../app/main/forms/test_register_user_form.py | 61 ++++++++++++ .../test_service_contact_details_form.py | 0 .../forms/test_service_sms_senders_form.py | 37 ++++++++ .../test_strip_whitespace.py} | 0 .../main/{ => forms}/test_two_factor_form.py | 0 tests/app/main/test_validators.py | 93 ------------------- 9 files changed, 98 insertions(+), 93 deletions(-) rename tests/app/main/{ => forms}/test_choose_time_form.py (100%) rename tests/app/main/{test_create_api_key_form.py => forms/test_create_key_form.py} (100%) rename tests/app/main/{ => forms}/test_placeholder_form.py (100%) create mode 100644 tests/app/main/forms/test_register_user_form.py rename tests/app/main/{ => forms}/test_service_contact_details_form.py (100%) create mode 100644 tests/app/main/forms/test_service_sms_senders_form.py rename tests/app/main/{test_strip_whitespace_form.py => forms/test_strip_whitespace.py} (100%) rename tests/app/main/{ => forms}/test_two_factor_form.py (100%) diff --git a/tests/app/main/test_choose_time_form.py b/tests/app/main/forms/test_choose_time_form.py similarity index 100% rename from tests/app/main/test_choose_time_form.py rename to tests/app/main/forms/test_choose_time_form.py diff --git a/tests/app/main/test_create_api_key_form.py b/tests/app/main/forms/test_create_key_form.py similarity index 100% rename from tests/app/main/test_create_api_key_form.py rename to tests/app/main/forms/test_create_key_form.py diff --git a/tests/app/main/test_placeholder_form.py b/tests/app/main/forms/test_placeholder_form.py similarity index 100% rename from tests/app/main/test_placeholder_form.py rename to tests/app/main/forms/test_placeholder_form.py diff --git a/tests/app/main/forms/test_register_user_form.py b/tests/app/main/forms/test_register_user_form.py new file mode 100644 index 000000000..2f9a65364 --- /dev/null +++ b/tests/app/main/forms/test_register_user_form.py @@ -0,0 +1,61 @@ +import pytest + +from app.main.forms import RegisterUserForm + + +@pytest.mark.parametrize('password', [ + 'govuknotify', '11111111', 'kittykat', 'blackbox' +]) +def test_should_raise_validation_error_for_password( + client_request, + mock_get_user_by_email, + password, +): + form = RegisterUserForm() + form.name.data = 'test' + form.email_address.data = 'teset@example.gov.uk' + form.mobile_number.data = '441231231231' + form.password.data = password + + form.validate() + assert 'Choose a password that’s harder to guess' in form.errors['password'] + + +def test_valid_email_not_in_valid_domains( + client_request, + mock_get_organisations, +): + form = RegisterUserForm(email_address="test@test.com", mobile_number='441231231231') + assert not form.validate() + assert "Enter a public sector email address" in form.errors['email_address'][0] + + +def test_valid_email_in_valid_domains( + client_request, +): + form = RegisterUserForm( + name="test", + email_address="test@my.gov.uk", + mobile_number='4407888999111', + password='an uncommon password') + form.validate() + assert form.errors == {} + + +def test_invalid_email_address_error_message( + client_request, + mock_get_organisations, +): + form = RegisterUserForm( + name="test", + email_address="test.com", + mobile_number='4407888999111', + password='1234567890') + assert not form.validate() + + form = RegisterUserForm( + name="test", + email_address="test.com", + mobile_number='4407888999111', + password='1234567890') + assert not form.validate() diff --git a/tests/app/main/test_service_contact_details_form.py b/tests/app/main/forms/test_service_contact_details_form.py similarity index 100% rename from tests/app/main/test_service_contact_details_form.py rename to tests/app/main/forms/test_service_contact_details_form.py diff --git a/tests/app/main/forms/test_service_sms_senders_form.py b/tests/app/main/forms/test_service_sms_senders_form.py new file mode 100644 index 000000000..04389228e --- /dev/null +++ b/tests/app/main/forms/test_service_sms_senders_form.py @@ -0,0 +1,37 @@ +import pytest + +from app.main.forms import ServiceSmsSenderForm + + +@pytest.mark.parametrize( + "sms_sender,error_expected,error_message", + [ + ('', True, 'Cannot be empty'), + ('22', True, 'Enter 3 characters or more'), + ('333', False, None), + ('elevenchars', False, None), # 11 chars + ('twelvecharas', True, 'Enter 11 characters or fewer'), # 12 chars + ('###', True, 'Use letters and numbers only'), + ('00111222333', True, 'Cannot start with 00'), + ('UK_GOV', False, None), # Underscores are allowed + ('UK.GOV', False, None), # Full stops are allowed + ("'UC'", False, None), # Straight single quotes are allowed + ] +) +def test_sms_sender_form_validation( + client_request, + mock_get_user_by_email, + sms_sender, + error_expected, + error_message +): + form = ServiceSmsSenderForm() + form.sms_sender.data = sms_sender + + form.validate() + + if error_expected: + assert form.errors + assert error_message == form.errors['sms_sender'][0] + else: + assert not form.errors diff --git a/tests/app/main/test_strip_whitespace_form.py b/tests/app/main/forms/test_strip_whitespace.py similarity index 100% rename from tests/app/main/test_strip_whitespace_form.py rename to tests/app/main/forms/test_strip_whitespace.py diff --git a/tests/app/main/test_two_factor_form.py b/tests/app/main/forms/test_two_factor_form.py similarity index 100% rename from tests/app/main/test_two_factor_form.py rename to tests/app/main/forms/test_two_factor_form.py diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index f2dea8615..fca4f7eec 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -3,7 +3,6 @@ from unittest.mock import Mock import pytest from wtforms import ValidationError -from app.main.forms import RegisterUserForm, ServiceSmsSenderForm from app.main.validators import ( MustContainAlphanumericCharacters, NoCommasInPlaceHolders, @@ -12,64 +11,6 @@ from app.main.validators import ( ) -@pytest.mark.parametrize('password', [ - 'govuknotify', '11111111', 'kittykat', 'blackbox' -]) -def test_should_raise_validation_error_for_password( - client_request, - mock_get_user_by_email, - password, -): - form = RegisterUserForm() - form.name.data = 'test' - form.email_address.data = 'teset@example.gov.uk' - form.mobile_number.data = '441231231231' - form.password.data = password - - form.validate() - assert 'Choose a password that’s harder to guess' in form.errors['password'] - - -def test_valid_email_not_in_valid_domains( - client_request, - mock_get_organisations, -): - form = RegisterUserForm(email_address="test@test.com", mobile_number='441231231231') - assert not form.validate() - assert "Enter a public sector email address" in form.errors['email_address'][0] - - -def test_valid_email_in_valid_domains( - client_request, -): - form = RegisterUserForm( - name="test", - email_address="test@my.gov.uk", - mobile_number='4407888999111', - password='an uncommon password') - form.validate() - assert form.errors == {} - - -def test_invalid_email_address_error_message( - client_request, - mock_get_organisations, -): - form = RegisterUserForm( - name="test", - email_address="test.com", - mobile_number='4407888999111', - password='1234567890') - assert not form.validate() - - form = RegisterUserForm( - name="test", - email_address="test.com", - mobile_number='4407888999111', - password='1234567890') - assert not form.validate() - - def _gen_mock_field(x): return Mock(data=x) @@ -185,37 +126,3 @@ def test_if_string_does_not_contain_alphanumeric_characters_raises(string): @pytest.mark.parametrize("string", [".A8", "AB.", ".42...."]) def test_if_string_contains_alphanumeric_characters_does_not_raise(string): MustContainAlphanumericCharacters()(None, _gen_mock_field(string)) - - -@pytest.mark.parametrize( - "sms_sender,error_expected,error_message", - [ - ('', True, 'Cannot be empty'), - ('22', True, 'Enter 3 characters or more'), - ('333', False, None), - ('elevenchars', False, None), # 11 chars - ('twelvecharas', True, 'Enter 11 characters or fewer'), # 12 chars - ('###', True, 'Use letters and numbers only'), - ('00111222333', True, 'Cannot start with 00'), - ('UK_GOV', False, None), # Underscores are allowed - ('UK.GOV', False, None), # Full stops are allowed - ("'UC'", False, None), # Straight single quotes are allowed - ] -) -def test_sms_sender_form_validation( - client_request, - mock_get_user_by_email, - sms_sender, - error_expected, - error_message -): - form = ServiceSmsSenderForm() - form.sms_sender.data = sms_sender - - form.validate() - - if error_expected: - assert form.errors - assert error_message == form.errors['sms_sender'][0] - else: - assert not form.errors From 6ba84140e5d7ddca2ee88e8ca6562a32a66a1d6b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Mar 2022 17:35:32 +0000 Subject: [PATCH 4/4] Add test for rendering integer 0 in form input --- tests/app/main/forms/test_govuk_text_input_field.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 tests/app/main/forms/test_govuk_text_input_field.py diff --git a/tests/app/main/forms/test_govuk_text_input_field.py b/tests/app/main/forms/test_govuk_text_input_field.py new file mode 100644 index 000000000..92555738f --- /dev/null +++ b/tests/app/main/forms/test_govuk_text_input_field.py @@ -0,0 +1,12 @@ +from flask_wtf import FlaskForm as Form + +from app.main.forms import GovukTextInputField + + +def test_GovukTextInputField_renders_zero(client_request): + class FakeForm(Form): + field = GovukTextInputField() + + form = FakeForm(field=0) + html = form.field() + assert 'value="0"' in html