diff --git a/app/main/forms.py b/app/main/forms.py index ccb11dae5..0ae859fea 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -736,6 +736,18 @@ class HiddenFieldWithNoneOption(FieldWithNoneOption, HiddenField): pass +class RadioFieldWithRequiredMessage(RadioField): + def __init__(self, *args, required_message='Not a valid choice', **kwargs): + self.required_message = required_message + super().__init__(*args, **kwargs) + + def pre_validate(self, form): + try: + return super().pre_validate(form) + except ValueError: + raise ValueError(self.required_message) + + class ServiceSetBranding(StripWhitespaceForm): branding_style = RadioFieldWithNoneOption( @@ -1151,17 +1163,17 @@ def required_for_ops(*operations): operations = set(operations) def validate(form, field): - if form.op not in operations and field.data: + if form.op not in operations and any(field.raw_data): # super weird - raise ValidationError('Must be empty') - if form.op in operations and not field.data: - raise ValidationError('Can’t be empty') + raise validators.StopValidation('Must be empty') + if form.op in operations and not any(field.raw_data): + raise validators.StopValidation('Can’t be empty') return validate class TemplateAndFoldersSelectionForm(Form): """ - This form also expects the form data to include an operation, based on which submit button is clicked. + This form expects the form data to include an operation, based on which submit button is clicked. If enter is pressed, unknown will be sent by a hidden submit button at the top of the form. The value of this operation affects which fields are required, expected to be empty, or optional. @@ -1247,7 +1259,7 @@ class TemplateAndFoldersSelectionForm(Form): add_new_folder_name = StringField('Folder name', validators=[required_for_ops('add-new-folder')]) move_to_new_folder_name = StringField('Folder name', validators=[required_for_ops('move-to-new-folder')]) - add_template_by_template_type = RadioField('Add new', validators=[ + add_template_by_template_type = RadioFieldWithRequiredMessage('Add new', validators=[ + required_for_ops('add-new-template'), Optional(), - required_for_ops('add-new-template') - ]) + ], required_message='Select the type of template you want to add') diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 151f301de..ec3c39a56 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -125,7 +125,13 @@ def choose_template(service_id, template_type='all', template_folder_id=None): if request.method == 'POST' and templates_and_folders_form.validate_on_submit(): if not can_manage_folders(): abort(403) - return process_folder_management_form(templates_and_folders_form, template_folder_id) + try: + return process_folder_management_form(templates_and_folders_form, template_folder_id) + except HTTPError as e: + flash(e.message) + + if 'templates_and_folders' in templates_and_folders_form.errors: + flash('Select at least one template or folder') return render_template( 'views/templates/choose.html', diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 977d77994..993987771 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -2,6 +2,7 @@ import uuid import pytest from flask import url_for +from notifications_python_client.errors import HTTPError from tests.conftest import ( SERVICE_ONE_ID, @@ -959,6 +960,10 @@ def test_should_be_able_to_move_a_sub_item( 'move_to': '__NONE__', 'add_template_by_template_type': 'email', }, + # add a new template, but don't select anything + { + 'operation': 'add-new-template', + }, ]) def test_no_action_if_user_fills_in_ambiguous_fields( client_request, @@ -1088,3 +1093,72 @@ def test_should_be_able_to_move_to_new_folder( folder_ids={FOLDER_TWO_ID}, template_ids={TEMPLATE_ONE_ID}, ) + + +def test_radio_button_with_no_value_shows_custom_error_message( + client_request, + service_one, + mock_get_service_templates, + mock_get_template_folders, + mock_move_to_template_folder, + mock_create_template_folder, +): + service_one['permissions'] += ['edit_folders'] + + page = client_request.post( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + _data={'operation': 'add-new-template'}, + _expected_status=200, + _expected_redirect=None, + ) + + assert mock_move_to_template_folder.called is False + assert mock_create_template_folder.called is False + + assert page.select_one('span.error-message').text.strip() == 'Select the type of template you want to add' + + +@pytest.mark.parametrize('data, error_msg', [ + # nothing selected when moving + ( + {'operation': 'move-to-new-folder', 'templates_and_folders': [], 'move_to_new_folder_name': 'foo'}, + 'Select at least one template or folder' + ), + ( + {'operation': 'move-to-existing-folder', 'templates_and_folders': [], 'move_to': PARENT_FOLDER_ID}, + 'Select at least one template or folder' + ), + # api error (eg moving folder to itself) + ( + {'operation': 'move-to-existing-folder', 'templates_and_folders': [FOLDER_TWO_ID], 'move_to': FOLDER_TWO_ID}, + 'Some api error msg' + ) + +]) +def test_show_custom_error_message( + client_request, + service_one, + mock_get_service_templates, + mock_get_template_folders, + mock_move_to_template_folder, + mock_create_template_folder, + data, + error_msg, +): + service_one['permissions'] += ['edit_folders'] + mock_get_template_folders.return_value = [ + {'id': PARENT_FOLDER_ID, 'name': 'folder_one', 'parent_id': None}, + {'id': FOLDER_TWO_ID, 'name': 'folder_two', 'parent_id': None}, + ] + mock_move_to_template_folder.side_effect = HTTPError(message='Some api error msg') + + page = client_request.post( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + _data=data, + _expected_status=200, + _expected_redirect=None, + ) + + assert page.select_one('div.banner-dangerous').text.strip() == error_msg