fix custom app settings validation (#1305, #1306, #1307, #1308)

CHANGELOG.rst

@@ -30,8 +30,15 @@ Fixed
     - Browser-specific ``sodar-btn-submit-once`` spinner padding (#1291)
     - Hidden JSON app settings reset on non-superuser project update (#1295)
     - Request object not provided to ``perform_project_modify()`` on create (#1301)
+    - ``validate_form_app_settings()`` not called in ``ProjectForm`` (#1305)
+    - Unhandled exceptions in ``validate_form_app_settings()`` calls (#1306)
+    - ``validate_form_app_settings()`` results handling crash in ``ProjectForm`` (#1307)
+    - ``RoleAssignment`` provided to ``validate_form_app_settings()`` in ``ProjectForm`` (#1308)
 - **Timeline**
     - ``get_timestamp()`` template tag crash from missing ``ProjectEventStatus`` (#1297)
+- **Userprofile**
+    - Unhandled exceptions in ``validate_form_app_settings()`` calls (#1306)
+    - ``validate_form_app_settings()`` results handling crash in ``UserSettingForm`` (#1307)
 
 Removed
 -------

docs/source/major_changes.rst

@@ -15,6 +15,7 @@ v0.13.3 (WIP)
 
 - Add common project badge template
 - Fix hidden JSON project setting reset on non-superuser project update
+- Fix custom app setting validation calls in forms
 - General bug fixes and minor updates
 
 

example_project_app/plugins.py

@@ -18,11 +18,17 @@
 from example_project_app.urls import urlpatterns
 
 
+# SODAR constants
+PROJECT_TYPE_PROJECT = SODAR_CONSTANTS['PROJECT_TYPE_PROJECT']
+PROJECT_TYPE_CATEGORY = SODAR_CONSTANTS['PROJECT_TYPE_CATEGORY']
+
+# Local constants
 EXAMPLE_MODIFY_API_MSG = (
     'Example project app plugin API called from {project_type} {action}.'
 )
-PROJECT_TYPE_PROJECT = SODAR_CONSTANTS['PROJECT_TYPE_PROJECT']
-PROJECT_TYPE_CATEGORY = SODAR_CONSTANTS['PROJECT_TYPE_CATEGORY']
+# Invalid app setting value for testing custom validation
+INVALID_SETTING_VALUE = 'ieDeequ6Eizohxi7Eebiliu6shou5aiT'
+INVALID_SETTING_MSG = 'INVALID_SETTING_VALUE detected'
 
 
 class ProjectAppPlugin(ProjectModifyPluginMixin, ProjectAppPluginPoint):
@@ -349,16 +355,16 @@ def perform_project_modify(
             )
 
     def validate_form_app_settings(self, app_settings, project=None, user=None):
-        """Example implementation for app setting validation plugin API"""
-        errors = {}
-        for setting_name, setting_value in app_settings.items():
-            if (
-                setting_name == 'project_hidden_setting'
-                and setting_value == 'Example project hidden setting'
-            ):
-                errors[
-                    setting_name
-                ] = 'Invalid value for a custom validation method'
-        if errors == {}:
-            return None
-        return errors
+        """Example implementation for custom form app setting validation"""
+        ret = {}
+        if (
+            project
+            and app_settings.get('project_str_setting') == INVALID_SETTING_VALUE
+        ):
+            ret['project_str_setting'] = INVALID_SETTING_MSG
+        if (
+            user
+            and app_settings.get('user_str_setting') == INVALID_SETTING_VALUE
+        ):
+            ret['user_str_setting'] = INVALID_SETTING_MSG
+        return ret

projectroles/forms.py

@@ -62,6 +62,10 @@
     ),
     (PROJECT_TYPE_PROJECT, get_display_name(PROJECT_TYPE_PROJECT, title=True)),
 ]
+SETTING_CUSTOM_VALIDATE_ERROR = (
+    'Exception in custom app setting validation for plugin "{plugin}": '
+    '{exception}'
+)
 
 
 # Base Classes and Mixins ------------------------------------------------------
@@ -525,29 +529,27 @@ def _validate_app_settings(
         app_settings,
         p_kwargs,
         instance,
-        instance_owner_as,
     ):
         """Validate and clean app_settings form fields"""
         errors = []
         for plugin in app_plugins + [None]:
             if plugin:
-                name = plugin.name
-                p_settings = app_settings.get_definitions(
-                    APP_SETTING_SCOPE_PROJECT, app_name=name, **p_kwargs
-                )
+                p_name = plugin.name
+                def_kwarg = {'plugin': plugin}
             else:
-                name = 'projectroles'
-                p_settings = app_settings.get_definitions(
-                    APP_SETTING_SCOPE_PROJECT, app_name=name, **p_kwargs
-                )
+                p_name = 'projectroles'
+                def_kwarg = {'app_name': p_name}
+            p_defs = app_settings.get_definitions(
+                APP_SETTING_SCOPE_PROJECT, **{**p_kwargs, **def_kwarg}
+            )
+            p_settings = {}
 
-            plugin_app_settings = {}
-            for s_key, s_val in p_settings.items():
-                s_field = 'settings.{}.{}'.format(name, s_key)
-                plugin_app_settings[s_key] = cleaned_data.get(s_field)
+            for s_key, s_val in p_defs.items():
+                s_field = '.'.join(['settings', p_name, s_key])
+                p_settings[s_key] = cleaned_data.get(s_field)
 
                 if s_val['type'] == 'JSON':
-                    if not plugin_app_settings[s_key]:
+                    if not p_settings[s_key]:
                         cleaned_data[s_field] = '{}'
                     try:
                         cleaned_data[s_field] = json.loads(
@@ -556,9 +558,7 @@ def _validate_app_settings(
                     except json.JSONDecodeError as err:
                         errors.append((s_field, 'Invalid JSON\n' + str(err)))
                 elif s_val['type'] == 'INTEGER':
-                    # When the field is a select/dropdown the information of
-                    # the datatype gets lost. We need to convert that here,
-                    # otherwise subsequent checks will fail.
+                    # Convert integers from select fields
                     cleaned_data[s_field] = int(cleaned_data[s_field])
 
                 if not app_settings.validate(
@@ -569,20 +569,21 @@ def _validate_app_settings(
                 ):
                     errors.append((s_field, 'Invalid value'))
 
-            # Custom validation for app settings
-            try:
-                app_settings_errors = plugin.validate_app_settings(
-                    plugin_app_settings,
-                    project=instance,
-                    user=instance_owner_as,
-                )
-                if app_settings_errors:
-                    for field, error in app_settings_errors.items():
-                        if error:
-                            errors.append((field, error))
-            except AttributeError:
-                # Plugin does not have a validate_form_app_settings method
-                pass
+            # Custom plugin validation for app settings
+            if plugin and hasattr(plugin, 'validate_form_app_settings'):
+                try:
+                    p_errors = plugin.validate_form_app_settings(
+                        p_settings, project=instance
+                    )
+                    if p_errors:
+                        for field, error in p_errors.items():
+                            f_name = '.'.join(['settings', p_name, field])
+                            errors.append((f_name, error))
+                except Exception as ex:
+                    err_msg = SETTING_CUSTOM_VALIDATE_ERROR.format(
+                        plugin=p_name, exception=ex
+                    )
+                    errors.append((None, err_msg))
         return cleaned_data, errors
 
     def __init__(self, project=None, current_user=None, *args, **kwargs):
@@ -777,7 +778,6 @@ def clean(self):
             self.app_settings,
             self.p_kwargs,
             self.instance,
-            self.instance_owner_as,
         )
         for key, value in cleaned_data.items():
             self.cleaned_data[key] = value

projectroles/plugins.py

@@ -433,10 +433,11 @@ def get_project_list_value(self, column_id, project, user):
     def validate_form_app_settings(self, app_settings, project=None, user=None):
         """
         Validate app settings form data and return a dict of errors.
+
         :param app_settings: Dict of app settings
-        :param project: Project object
-        :param user: User object
-        :return: dict in format of {'setting_name': 'Error string' or None}
+        :param project: Project object or None
+        :param user: User object or None
+        :return: dict in format of {setting_name: 'Error string'}
         """
         # TODO: Implement this in your app plugin (optional)
         return None
@@ -624,9 +625,10 @@ def get_extra_data_link(self, _extra_data, _name):
     def validate_form_app_settings(self, app_settings, user=None):
         """
         Validate app settings form data and return a dict of errors.
+
         :param app_settings: Dict of app settings
-        :param user: User object
-        :return: dict in format of {'setting_name': 'Error string' or None}
+        :param user: User object or None
+        :return: dict in format of {setting_name: 'Error string'}
         """
         # TODO: Implement this in your app plugin (optional)
         return None

projectroles/tests/test_app_settings.py

@@ -15,6 +15,12 @@
     AppSettingMixin,
 )
 
+# example_project_app dependency
+from example_project_app.plugins import (
+    INVALID_SETTING_VALUE,
+    INVALID_SETTING_MSG,
+)
+
 
 app_settings = AppSettingAPI()
 
@@ -1039,22 +1045,37 @@ def test_delete_by_scope_param_user(self):
             )
 
     def test_validate_form_app_settings(self):
-        """Test validate_form_app_settings() method on valid app_setting"""
+        """Test validate_form_app_settings() with valid project setting value"""
         app_plugin = get_app_plugin(EXAMPLE_APP_NAME)
-        valid_setting = {'valid_setting': True}
+        app_settings = {'project_str_setting': 'String'}
         errors = app_plugin.validate_form_app_settings(
-            valid_setting, project=self.project, user=self.user
+            app_settings, project=self.project
         )
-        self.assertEqual(errors, None)
+        self.assertEqual(errors, {})
 
-    def test_validate_form_app_settings_user_scope_error(self):
-        """Test validate_form_app_settings() method on invalid app_setting"""
+    def test_validate_form_app_settings_invalid(self):
+        """Test validate_form_app_settings() with invalid project setting value"""
         app_plugin = get_app_plugin(EXAMPLE_APP_NAME)
-        settings = {'project_hidden_setting': 'Example project hidden setting'}
+        app_settings = {'project_str_setting': INVALID_SETTING_VALUE}
         errors = app_plugin.validate_form_app_settings(
-            settings, project=self.project
+            app_settings, project=self.project
         )
-        self.assertIsNotNone(errors)
-        self.assertIn(
-            'Invalid value for a custom validation method', errors.values()
+        self.assertEqual(errors, {'project_str_setting': INVALID_SETTING_MSG})
+
+    def test_validate_form_app_settings_user(self):
+        """Test validate_form_app_settings() with valid user setting value"""
+        app_plugin = get_app_plugin(EXAMPLE_APP_NAME)
+        app_settings = {'user_str_setting': 'String'}
+        errors = app_plugin.validate_form_app_settings(
+            app_settings, user=self.user
+        )
+        self.assertEqual(errors, {})
+
+    def test_validate_form_app_settings_user_invalid(self):
+        """Test validate_form_app_settings() with invalid user setting value"""
+        app_plugin = get_app_plugin(EXAMPLE_APP_NAME)
+        app_settings = {'user_str_setting': INVALID_SETTING_VALUE}
+        errors = app_plugin.validate_form_app_settings(
+            app_settings, user=self.user
         )
+        self.assertEqual(errors, {'user_str_setting': INVALID_SETTING_MSG})

projectroles/tests/test_views.py

@@ -15,6 +15,9 @@
 
 from test_plus.test import TestCase
 
+# example_project_app dependency
+from example_project_app.plugins import INVALID_SETTING_VALUE
+
 # Timeline dependency
 from timeline.models import ProjectEvent
 from timeline.tests.test_models import (
@@ -1161,7 +1164,10 @@ def test_update_project_regular_user(self):
 
     def test_update_project_title_delimiter(self):
         """Test Project updating with category delimiter in title (should fail)"""
+        # TODO: Add values getter as a helper
         values = model_to_dict(self.project)
+        values['parent'] = self.category.sodar_uuid
+        values['owner'] = self.user.sodar_uuid
         values['title'] = 'Project{}Title'.format(CAT_DELIMITER)
         # Add settings values
         values.update(
@@ -1179,6 +1185,33 @@ def test_update_project_title_delimiter(self):
         self.project.refresh_from_db()
         self.assertEqual(self.project.title, 'TestProject')
 
+    def test_update_project_custom_validation(self):
+        """Test updating with custom validation and invalid value (should fail)"""
+        values = model_to_dict(self.project)
+        values['parent'] = self.category.sodar_uuid
+        values['owner'] = self.user.sodar_uuid
+        values.update(
+            app_settings.get_all(project=self.project, post_safe=True)
+        )
+        values[
+            'settings.example_project_app.project_str_setting'
+        ] = INVALID_SETTING_VALUE
+        with self.login(self.user):
+            response = self.client.post(
+                reverse(
+                    'projectroles:update',
+                    kwargs={'project': self.project.sodar_uuid},
+                ),
+                values,
+            )
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(
+            app_settings.get(
+                EXAMPLE_APP_NAME, 'project_str_setting', project=self.project
+            ),
+            '',
+        )
+
     def test_render_category(self):
         """Test rendering with existing category"""
         with self.login(self.user):