diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 52812dd6..19bc95b7 100644 --- a/CHANGELOG.rst +++ b/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 ------- diff --git a/docs/source/major_changes.rst b/docs/source/major_changes.rst index f77fb620..81383d3d 100644 --- a/docs/source/major_changes.rst +++ b/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 diff --git a/example_project_app/plugins.py b/example_project_app/plugins.py index d4e3760b..e84fffb0 100644 --- a/example_project_app/plugins.py +++ b/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 diff --git a/projectroles/forms.py b/projectroles/forms.py index 3b9d36bb..fcbd4ff6 100644 --- a/projectroles/forms.py +++ b/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 diff --git a/projectroles/plugins.py b/projectroles/plugins.py index 93036cf8..49b721be 100644 --- a/projectroles/plugins.py +++ b/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 diff --git a/projectroles/tests/test_app_settings.py b/projectroles/tests/test_app_settings.py index 1c363dc3..9b2a1c03 100644 --- a/projectroles/tests/test_app_settings.py +++ b/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}) diff --git a/projectroles/tests/test_views.py b/projectroles/tests/test_views.py index d3bee259..e8606666 100644 --- a/projectroles/tests/test_views.py +++ b/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): diff --git a/userprofile/forms.py b/userprofile/forms.py index ae396c42..8d513ed4 100644 --- a/userprofile/forms.py +++ b/userprofile/forms.py @@ -4,7 +4,7 @@ # Projectroles dependency from projectroles.app_settings import AppSettingAPI -from projectroles.forms import SODARForm +from projectroles.forms import SODARForm, SETTING_CUSTOM_VALIDATE_ERROR from projectroles.models import APP_SETTING_VAL_MAXLENGTH, SODAR_CONSTANTS from projectroles.plugins import get_active_plugins @@ -26,7 +26,6 @@ class UserSettingsForm(SODARForm): def __init__(self, *args, **kwargs): self.user = kwargs.pop('current_user') super().__init__(*args, **kwargs) - # Add settings fields self.app_plugins = get_active_plugins(plugin_type='project_app') self.user_plugins = get_active_plugins(plugin_type='site_app') @@ -138,21 +137,21 @@ def __init__(self, *args, **kwargs): def clean(self): """Function for custom form validation and cleanup""" for plugin in self.app_plugins + [None]: + p_kwargs = {'user_modifiable': True} if plugin: - name = plugin.name - p_settings = app_settings.get_definitions( - APP_SETTING_SCOPE_USER, plugin=plugin, user_modifiable=True - ) + p_name = plugin.name + p_kwargs['plugin'] = plugin else: - name = 'projectroles' - p_settings = app_settings.get_definitions( - APP_SETTING_SCOPE_USER, app_name=name, user_modifiable=True - ) - plugin_app_settings = {} + p_name = 'projectroles' + p_kwargs['app_name'] = p_name + p_defs = app_settings.get_definitions( + APP_SETTING_SCOPE_USER, **p_kwargs + ) + p_settings = {} - for s_key, s_val in p_settings.items(): - s_field = 'settings.{}.{}'.format(name, s_key) - plugin_app_settings[s_key] = self.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] = self.cleaned_data.get(s_field) if s_val['type'] == 'JSON': if not self.cleaned_data.get(s_field): @@ -163,11 +162,8 @@ def clean(self): ) except json.JSONDecodeError as err: self.add_error(s_field, 'Invalid JSON\n' + str(err)) - elif s_val['type'] == 'INTEGER': - # When field is a select/dropdown, the information of the - # data type gets lost. We need to convert that here, - # otherwise subsequent checks will fail. + # Convert integers from select fields self.cleaned_data[s_field] = int(self.cleaned_data[s_field]) if not app_settings.validate( @@ -177,16 +173,22 @@ def clean(self): user=self.user, ): self.add_error(s_field, 'Invalid value') - try: - app_settings_errors = plugin.validate_form_app_settings( - plugin_app_settings, user=self.user - ) - if app_settings_errors: - for field, error in app_settings_errors.items(): - if error: - self.add_error(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, user=self.user + ) + if p_errors: + for field, error in p_errors.items(): + f_name = '.'.join(['settings', p_name, field]) + self.add_error(f_name, error) + except Exception as ex: + self.add_error( + None, + SETTING_CUSTOM_VALIDATE_ERROR.format( + plugin=p_name, exception=ex + ), + ) return self.cleaned_data diff --git a/userprofile/tests/test_views.py b/userprofile/tests/test_views.py index 53af2e8a..b8de64f9 100644 --- a/userprofile/tests/test_views.py +++ b/userprofile/tests/test_views.py @@ -2,7 +2,6 @@ from django.contrib import auth from django.contrib.messages import get_messages -from django.test import RequestFactory from django.urls import reverse from test_plus.test import TestCase @@ -11,6 +10,9 @@ from projectroles.app_settings import AppSettingAPI from projectroles.tests.test_models import EXAMPLE_APP_NAME, AppSettingMixin +# example_project_app dependency +from example_project_app.plugins import INVALID_SETTING_VALUE + from userprofile.views import SETTING_UPDATE_MSG @@ -18,11 +20,10 @@ User = auth.get_user_model() -class TestViewsBase(TestCase): +class UserViewsTestBase(TestCase): """Base class for view testing""" def setUp(self): - self.req_factory = RequestFactory() # Init superuser self.user = self.make_user('superuser') self.user.is_staff = True @@ -33,25 +34,25 @@ def setUp(self): # View tests ------------------------------------------------------------------- -class TestUserDetailView(TestViewsBase): - """Tests for the user profile detail view""" +class TestUserDetailView(UserViewsTestBase): + """Tests for UserDetailView""" - def test_render(self): - """Test to ensure the user profile detail view renders correctly""" + def test_get(self): + """Test UserDetailView GET""" with self.login(self.user): response = self.client.get(reverse('userprofile:detail')) self.assertEqual(response.status_code, 200) self.assertIsNotNone(response.context['user_settings']) -class TestUserSettingsForm(AppSettingMixin, TestViewsBase): - """Tests for the user settings form.""" +class TestUserSettingsView(AppSettingMixin, UserViewsTestBase): + """Tests for UserSettingsView""" - # NOTE: This assumes an example app is available - def setUp(self): - # Init user & role - self.user = self.make_user('owner') + def _get_setting(self, name): + return app_settings.get(EXAMPLE_APP_NAME, name, user=self.user) + def setUp(self): + super().setUp() # Init test setting self.setting_str = self.make_setting( app_name=EXAMPLE_APP_NAME, @@ -60,7 +61,6 @@ def setUp(self): value='test', user=self.user, ) - # Init integer setting self.setting_int = self.make_setting( app_name=EXAMPLE_APP_NAME, @@ -69,7 +69,6 @@ def setUp(self): value=170, user=self.user, ) - # Init test setting with options self.setting_str_options = self.make_setting( app_name=EXAMPLE_APP_NAME, @@ -78,7 +77,6 @@ def setUp(self): value='string1', user=self.user, ) - # Init integer setting with options self.setting_int_options = self.make_setting( app_name=EXAMPLE_APP_NAME, @@ -87,7 +85,6 @@ def setUp(self): value=0, user=self.user, ) - # Init boolean setting self.setting_bool = self.make_setting( app_name=EXAMPLE_APP_NAME, @@ -96,7 +93,6 @@ def setUp(self): value=True, user=self.user, ) - # Init json setting self.setting_json = self.make_setting( app_name=EXAMPLE_APP_NAME, @@ -108,7 +104,7 @@ def setUp(self): ) def test_get(self): - """Test GET request on settings update form""" + """Test UserSettingsView GET""" with self.login(self.user): response = self.client.get(reverse('userprofile:settings_update')) self.assertEqual(response.status_code, 200) @@ -145,42 +141,16 @@ def test_get(self): ) def test_post(self): - """Test POST request on settings update form""" - self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_str_setting', user=self.user - ), - 'test', - ) - self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_int_setting', user=self.user - ), - 170, - ) - self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_str_setting_options', user=self.user - ), - 'string1', - ) - self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_int_setting_options', user=self.user - ), - 0, - ) + """Test UserSettingsView POST""" + self.assertEqual(self._get_setting('user_str_setting'), 'test') + self.assertEqual(self._get_setting('user_int_setting'), 170) self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_bool_setting', user=self.user - ), - True, + self._get_setting('user_str_setting_options'), 'string1' ) + self.assertEqual(self._get_setting('user_int_setting_options'), 0) + self.assertEqual(self._get_setting('user_bool_setting'), True) self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_json_setting', user=self.user - ), - {'Test': 'More'}, + self._get_setting('user_json_setting'), {'Test': 'More'} ) values = { @@ -196,7 +166,6 @@ def test_post(self): self.user.sodar_uuid ), } - with self.login(self.user): response = self.client.post( reverse('userprofile:settings_update'), values @@ -209,55 +178,41 @@ def test_post(self): list(get_messages(response.wsgi_request))[0].message, SETTING_UPDATE_MSG, ) - # Assert settings state after update + self.assertEqual(self._get_setting('user_str_setting'), 'another-text') + self.assertEqual(self._get_setting('user_int_setting'), 123) self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_str_setting', user=self.user - ), - 'another-text', - ) - self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_int_setting', user=self.user - ), - 123, - ) - self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_str_setting_options', user=self.user - ), - 'string2', - ) - self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_int_setting_options', user=self.user - ), - 1, + self._get_setting('user_str_setting_options'), 'string2' ) + self.assertEqual(self._get_setting('user_int_setting_options'), 1) + self.assertEqual(self._get_setting('user_bool_setting'), False) self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_bool_setting', user=self.user - ), - False, - ) - self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_json_setting', user=self.user - ), - {'Test': 'Less'}, - ) - self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, 'user_callable_setting', user=self.user - ), - 'Test', + self._get_setting('user_json_setting'), {'Test': 'Less'} ) + self.assertEqual(self._get_setting('user_callable_setting'), 'Test') self.assertEqual( - app_settings.get( - EXAMPLE_APP_NAME, - 'user_callable_setting_options', - user=self.user, - ), + self._get_setting('user_callable_setting_options'), str(self.user.sodar_uuid), ) + + def test_post_custom_validation(self): + """Test POST with custom validation and invalid value""" + values = { + 'settings.example_project_app.user_str_setting': INVALID_SETTING_VALUE, + 'settings.example_project_app.user_int_setting': '170', + 'settings.example_project_app.user_str_setting_options': 'string1', + 'settings.example_project_app.user_int_setting_options': '0', + 'settings.example_project_app.user_bool_setting': True, + 'settings.example_project_app.' + 'user_json_setting': '{"Test": "More"}', + 'settings.example_project_app.user_callable_setting': 'Test', + 'settings.example_project_app.user_callable_setting_options': str( + self.user.sodar_uuid + ), + } + with self.login(self.user): + response = self.client.post( + reverse('userprofile:settings_update'), values + ) + self.assertEqual(response.status_code, 200) + self.assertEqual(self._get_setting('user_str_setting'), 'test') diff --git a/userprofile/urls.py b/userprofile/urls.py index 87ce4b90..a7bd9492 100644 --- a/userprofile/urls.py +++ b/userprofile/urls.py @@ -12,7 +12,7 @@ ), path( route='profile/settings/update', - view=views.UserSettingUpdateView.as_view(), + view=views.UserSettingsView.as_view(), name='settings_update', ), ] diff --git a/userprofile/views.py b/userprofile/views.py index bfbc42d5..66fda9e2 100644 --- a/userprofile/views.py +++ b/userprofile/views.py @@ -65,7 +65,7 @@ def get_context_data(self, **kwargs): return result -class UserSettingUpdateView( +class UserSettingsView( LoginRequiredMixin, LoggedInPermissionMixin, HTTPRefererMixin, FormView ): """User settings update view"""