From 65bb72ef2fe750546e69f1d298569eb4c63f6226 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 22 Apr 2020 17:49:03 +0100 Subject: [PATCH] Bump WTForms to 2.3.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This involves three changes which broke our code. To validate email addresses, the optional dependency `email-validator` must be installed1. But since we don’t use WTForms’ email validation, we shouldn’t need to subclass it – it can just be its own self contained thing. Then we don’t need to add the extra dependency. When rendering textareas, and extra `\r\n` is inserted at the beginning 2. Browsers will strip this when displaying the textbox and submitting the form, but some of our tests need updating to account for this. The error message for when you don’t choose an option from some radio buttons has now changed. Rather than just accepting WTForms’ new message, this commit makes the error messages like the examples from the Design System3. By default it will say ‘Select an option’, but by passing in an extra parameter (`thing`) it can be customised to be more specific, for example ‘Select a type of organisation’. *** 1. https://github.com/wtforms/wtforms/pull/429 2. https://github.com/wtforms/wtforms/issues/238 3. https://design-system.service.gov.uk/components/radios/#error-messages --- app/main/forms.py | 72 +++++++++++-------- app/main/validators.py | 8 +-- requirements-app.txt | 1 - requirements.txt | 6 +- tests/app/main/test_create_api_key_form.py | 4 +- .../main/test_service_contact_details_form.py | 2 +- .../views/organisations/test_organisation.py | 6 +- tests/app/main/views/test_add_service.py | 2 +- tests/app/main/views/test_agreement.py | 2 +- tests/app/main/views/test_feedback.py | 2 +- tests/app/main/views/test_platform_admin.py | 2 +- tests/app/main/views/test_send.py | 2 +- tests/app/main/views/test_service_settings.py | 4 +- tests/app/main/views/test_templates.py | 2 +- 14 files changed, 62 insertions(+), 53 deletions(-) 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,