diff --git a/app/main/forms.py b/app/main/forms.py index a4455a45b..1796e0d2b 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -24,7 +24,9 @@ from wtforms import ( HiddenField, IntegerField, PasswordField, - RadioField, +) +from wtforms import RadioField as WTFormsRadioField +from wtforms import ( SelectMultipleField, StringField, TextAreaField, @@ -115,6 +117,24 @@ class MultiCheckboxField(SelectMultipleField): option_widget = CheckboxInput() +class RadioField(WTFormsRadioField): + + def __init__( + self, + *args, + thing='an option', + **kwargs + ): + super().__init__(*args, **kwargs) + self.thing = thing + self.validate_choice = False + + def pre_validate(self, form): + super().pre_validate(form) + if self.data not in dict(self.choices).keys(): + raise ValidationError(f'Select {self.thing}') + + def email_address(label='Email address', gov_user=True, required=True): validators = [ @@ -266,7 +286,8 @@ class OrganisationTypeField(RadioField): (value, label) for value, label in Organisation.TYPES if not include_only or value in include_only ], - validators=[DataRequired()] + (validators or []), + thing='the type of organisation', + validators=validators or [], **kwargs ) @@ -376,10 +397,17 @@ class PostalAddressField(TextAreaField): class OnOffField(RadioField): def __init__(self, label, choices=None, *args, **kwargs): - super().__init__(label, choices=choices or [ + choices = choices or [ (True, 'On'), (False, 'Off'), - ], *args, **kwargs) + ] + super().__init__( + label, + choices=choices, + thing=f'{choices[0][1].lower()} or {choices[1][1].lower()}', + *args, + **kwargs, + ) def process_formdata(self, valuelist): if valuelist: @@ -482,6 +510,7 @@ class PermissionsForm(PermissionsAbstract): ('sms_auth', 'Text message code'), ('email_auth', 'Email link'), ], + thing='how this team member should sign in', validators=[DataRequired()] ) @@ -616,6 +645,7 @@ class AddNHSLocalOrganisationForm(StripWhitespaceForm): organisations = RadioField( 'Which NHS Trust or Clinical Commissioning Group do you work for?', + thing='an NHS Trust or Clinical Commissioning Group' ) @@ -633,9 +663,7 @@ class OrganisationCrownStatusForm(StripWhitespaceForm): ('non-crown', 'No'), ('unknown', 'Not sure'), ], - validators=[ - DataRequired(message='Cannot be empty') - ], + thing='whether this organisation is a crown body', ) @@ -649,9 +677,7 @@ class OrganisationAgreementSignedForm(StripWhitespaceForm): ('no', 'No'), ('unknown', 'No (but we have some service-specific agreements in place)'), ], - validators=[ - DataRequired(message='Cannot be empty') - ], + thing='whether this organisation has signed the agreement', ) @@ -742,6 +768,7 @@ class BaseTemplateForm(StripWhitespaceForm): ('priority', 'Yes'), ('normal', 'No'), ], + thing='yes or no', validators=[DataRequired()], default='normal' ) @@ -806,6 +833,7 @@ class LetterTemplatePostageForm(StripWhitespaceForm): ('first', 'First class'), ('second', 'Second class'), ], + thing='first class or second class', validators=[DataRequired()] ) @@ -890,9 +918,6 @@ class ChooseTimeForm(StripWhitespaceForm): scheduled_for = RadioField( 'When should Notify send these messages?', default='', - validators=[ - DataRequired() - ] ) @@ -906,9 +931,7 @@ class CreateKeyForm(StripWhitespaceForm): key_type = RadioField( 'Type of key', - validators=[ - DataRequired() - ] + thing='the type of key', ) key_name = StringField(u'Description of key', validators=[ @@ -927,7 +950,6 @@ class SupportType(StripWhitespaceForm): (PROBLEM_TICKET_TYPE, 'Report a problem'), (QUESTION_TICKET_TYPE, 'Ask a question or give feedback'), ], - validators=[DataRequired()] ) @@ -938,7 +960,6 @@ class SupportRedirect(StripWhitespaceForm): ('public-sector', 'I work in the public sector and need to send emails, text messages or letters'), ('public', 'I’m a member of the public with a question for the government'), ], - validators=[DataRequired()] ) @@ -955,7 +976,7 @@ class Triage(StripWhitespaceForm): ('yes', 'Yes'), ('no', 'No'), ], - validators=[DataRequired()] + thing='yes or no', ) @@ -982,7 +1003,7 @@ class EstimateUsageForm(StripWhitespaceForm): ('yes', 'Yes'), ('no', 'No'), ], - validators=[DataRequired()] + thing='yes or no', ) at_least_one_volume_filled = True @@ -1024,7 +1045,6 @@ class ServiceContactDetailsForm(StripWhitespaceForm): ('email_address', 'Email address'), ('phone_number', 'Phone number'), ], - validators=[DataRequired()] ) url = StringField("URL") @@ -1120,9 +1140,7 @@ class SetEmailBranding(StripWhitespaceForm): branding_style = RadioFieldWithNoneOption( 'Branding style', - validators=[ - DataRequired() - ] + thing='a branding style', ) DEFAULT = (FieldWithNoneOption.NONE_OPTION_VALUE, 'GOV.UK') @@ -1314,9 +1332,7 @@ class ServiceInboundNumberForm(StripWhitespaceForm): inbound_number = RadioField( "Select your inbound number", - validators=[ - DataRequired("Option must be selected") - ] + thing='an inbound number', ) @@ -1679,7 +1695,6 @@ class TemplateAndFoldersSelectionForm(Form): class ClearCacheForm(StripWhitespaceForm): model_type = RadioField( 'What do you want to clear today', - validators=[DataRequired()] ) @@ -1725,7 +1740,6 @@ class AcceptAgreementForm(StripWhitespaceForm): 'Someone else', ), ), - validators=[DataRequired()], ) on_behalf_of_name = StringField( diff --git a/app/main/validators.py b/app/main/validators.py index cc3f55afc..d403f9003 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -8,7 +8,6 @@ from notifications_utils.recipients import ( ) from notifications_utils.sanitise_text import SanitiseSMS from wtforms import ValidationError -from wtforms.validators import Email from app.main._blacklisted_passwords import blacklisted_passwords from app.utils import Spreadsheet, is_gov_user @@ -51,10 +50,9 @@ class ValidGovEmail: raise ValidationError(message) -class ValidEmail(Email): +class ValidEmail: - def __init__(self): - super().__init__('Enter a valid email address') + message = 'Enter a valid email address' def __call__(self, form, field): @@ -66,8 +64,6 @@ class ValidEmail(Email): except InvalidEmailError: raise ValidationError(self.message) - return super().__call__(form, field) - class NoCommasInPlaceHolders: diff --git a/requirements-app.txt b/requirements-app.txt index 42fc82b9d..3cf074cdf 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -18,7 +18,6 @@ pytz==2019.3 gunicorn==20.0.4 eventlet==0.25.1 notifications-python-client==5.5.1 -WTForms==2.2.1 # Pinned because of breaking change in 2.3.0 # PaaS awscli-cwlogs>=1.4,<1.5 diff --git a/requirements.txt b/requirements.txt index 1b74170b0..927033ec1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,7 +20,6 @@ pytz==2019.3 gunicorn==20.0.4 eventlet==0.25.1 notifications-python-client==5.5.1 -WTForms==2.2.1 # Pinned because of breaking change in 2.3.0 # PaaS awscli-cwlogs>=1.4,<1.5 @@ -30,10 +29,10 @@ git+https://github.com/alphagov/notifications-utils.git@37.2.0#egg=notifications git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha ## The following requirements were added by pip freeze: -awscli==1.18.43 +awscli==1.18.44 bleach==3.1.4 boto3==1.10.38 -botocore==1.15.43 +botocore==1.15.44 certifi==2020.4.5.1 chardet==3.0.4 click==7.1.1 @@ -75,5 +74,6 @@ texttable==1.6.2 urllib3==1.25.9 webencodings==0.5.1 Werkzeug==1.0.1 +WTForms==2.3.1 xlrd==1.2.0 xlwt==1.3.0 diff --git a/tests/app/main/test_create_api_key_form.py b/tests/app/main/test_create_api_key_form.py index 539083780..bd9100ef0 100644 --- a/tests/app/main/test_create_api_key_form.py +++ b/tests/app/main/test_create_api_key_form.py @@ -36,8 +36,8 @@ def test_return_validation_error_when_key_name_exists( @pytest.mark.parametrize( 'key_type, expected_error', [ - ('', 'This field is required.'), - ('invalid', 'Not a valid choice') + ('', 'Select the type of key'), + ('invalid', 'Select the type of key') ] ) def test_return_validation_error_when_key_type_not_chosen(client, key_type, expected_error): diff --git a/tests/app/main/test_service_contact_details_form.py b/tests/app/main/test_service_contact_details_form.py index 26a3e2367..10f665710 100644 --- a/tests/app/main/test_service_contact_details_form.py +++ b/tests/app/main/test_service_contact_details_form.py @@ -9,7 +9,7 @@ def test_form_fails_validation_with_no_radio_buttons_selected(app_): assert not form.validate_on_submit() assert len(form.errors) == 1 - assert form.errors['contact_details_type'] == ['Not a valid choice'] + assert form.errors['contact_details_type'] == ['Select an option'] @pytest.mark.parametrize('selected_radio_button, selected_text_box, text_box_data', [ diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index dae5de6f2..ade47c582 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -158,8 +158,8 @@ def test_create_new_organisation_validates( for error in page.select('.error-message') ] == [ ('name', 'Cannot be empty'), - ('organisation_type', 'Not a valid choice'), - ('crown_status', 'Not a valid choice'), + ('organisation_type', 'Select the type of organisation'), + ('crown_status', 'Select whether this organisation is a crown body'), ] assert mock_create_organisation.called is False @@ -334,7 +334,7 @@ def test_gps_can_name_their_organisation( { 'name': 'Dr. Example', }, - 'Not a valid choice', + 'Select yes or no', ), ( { diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 63192c38d..582460468 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -178,7 +178,7 @@ def test_add_service_has_to_choose_org_type( _expected_status=200, ) assert normalize_spaces(page.select_one('.error-message').text) == ( - 'Not a valid choice' + 'Select the type of organisation' ) assert mock_create_service.called is False assert mock_create_service_template.called is False diff --git a/tests/app/main/views/test_agreement.py b/tests/app/main/views/test_agreement.py index f84707cd8..907980a51 100644 --- a/tests/app/main/views/test_agreement.py +++ b/tests/app/main/views/test_agreement.py @@ -278,7 +278,7 @@ def test_accept_agreement_page_populates( 'on_behalf_of_email': '', }, [ - 'Not a valid choice', + 'Select an option', 'Must be a number', ], ), diff --git a/tests/app/main/views/test_feedback.py b/tests/app/main/views/test_feedback.py index 736406859..b05921ce8 100644 --- a/tests/app/main/views/test_feedback.py +++ b/tests/app/main/views/test_feedback.py @@ -396,7 +396,7 @@ def test_doesnt_lose_message_if_post_across_closing( ) with client_request.session_transaction() as session: - assert page.find('textarea', {'name': 'feedback'}).text == 'foo' + assert page.find('textarea', {'name': 'feedback'}).text == '\r\nfoo' assert 'feedback_message' not in session diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index ea6168963..ab9fcf657 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -792,7 +792,7 @@ def test_clear_cache_requires_option(client_request, platform_admin_user, mocker page = client_request.post('main.clear_cache', _data={}, _expected_status=200) - assert normalize_spaces(page.find('span', class_='error-message').text) == 'Not a valid choice' + assert normalize_spaces(page.find('span', class_='error-message').text) == 'Select an option' assert not redis.delete_cache_keys_by_pattern.called diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 89e851f11..86b5215b7 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -2119,7 +2119,7 @@ def test_send_one_off_back_link_populates_address_textarea( textarea = form.select_one('textarea') assert textarea.attrs['name'] == 'address' - assert textarea.text == 'foo\nbar' + assert textarea.text == '\r\nfoo\nbar' @pytest.mark.parametrize('placeholder', ( diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 383d9942b..00126e9e8 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1377,7 +1377,7 @@ def test_should_show_persist_estimated_volumes( 'consent_to_research': '', }, '[data-error-label="consent_to_research"]', - 'This field is required.' + 'Select yes or no' ), )) def test_should_error_if_bad_estimations_given( @@ -4109,7 +4109,7 @@ def test_send_files_by_email_contact_details_displays_error_message_when_no_radi }, _follow_redirects=True ) - assert normalize_spaces(page.find('span', class_='error-message').text) == 'Not a valid choice' + assert normalize_spaces(page.find('span', class_='error-message').text) == 'Select an option' assert normalize_spaces(page.h1.text) == "Send files by email" diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index d1162eae8..02fd8365d 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -1182,7 +1182,7 @@ def test_load_edit_template_with_copy_of_template( expected_name ) assert page.select_one('textarea').text == ( - 'Your ((thing)) is due soon' + '\r\nYour ((thing)) is due soon' ) mock_get_service_email_template.assert_called_once_with( SERVICE_TWO_ID,