Merge pull request #2572 from alphagov/flash-errors

Show move errors on the template folder management flow

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')

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',

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