From 180654046aaf62d035a2400db554dfaaf9e1b093 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 5 Dec 2018 11:30:47 +0000 Subject: [PATCH 1/4] flash error message when moving folder to itself --- app/main/views/templates.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 151f301de..fc15241b4 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -125,7 +125,10 @@ 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 Exception as e: + flash(e.message) return render_template( 'views/templates/choose.html', From 6f8dc7e6d3192d5631418b78248e6a87140acd73 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 7 Dec 2018 16:38:48 +0000 Subject: [PATCH 2/4] fix being able to add template without selecting a radio button --- app/main/forms.py | 12 ++++++------ app/main/views/templates.py | 3 +++ tests/app/main/views/test_template_folders.py | 4 ++++ 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index ccb11dae5..472ead468 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1151,17 +1151,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. @@ -1248,6 +1248,6 @@ class TemplateAndFoldersSelectionForm(Form): 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=[ + required_for_ops('add-new-template'), Optional(), - required_for_ops('add-new-template') ]) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index fc15241b4..986b949d5 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -130,6 +130,9 @@ def choose_template(service_id, template_type='all', template_folder_id=None): except Exception 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', current_template_folder_id=template_folder_id, diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 977d77994..4b41617fd 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -959,6 +959,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, From 5c3c4ec78c3cf4fd0648b21c601baac3bcea7287 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 7 Dec 2018 17:09:15 +0000 Subject: [PATCH 3/4] change error message for not selecting a template needed to subclass RadioField for this --- app/main/forms.py | 16 +++++++++++-- tests/app/main/views/test_template_folders.py | 24 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 472ead468..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( @@ -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_message='Select the type of template you want to add') diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 4b41617fd..7681f67f1 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -1092,3 +1092,27 @@ 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' From 94da05ebc4089c5a4dbb91538b0383970671d729 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 17 Dec 2018 15:24:24 +0000 Subject: [PATCH 4/4] only catch HTTPError also add tests --- app/main/views/templates.py | 2 +- tests/app/main/views/test_template_folders.py | 46 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 986b949d5..ec3c39a56 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -127,7 +127,7 @@ def choose_template(service_id, template_type='all', template_folder_id=None): abort(403) try: return process_folder_management_form(templates_and_folders_form, template_folder_id) - except Exception as e: + except HTTPError as e: flash(e.message) if 'templates_and_folders' in templates_and_folders_form.errors: diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 7681f67f1..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, @@ -1116,3 +1117,48 @@ def test_radio_button_with_no_value_shows_custom_error_message( 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