add get_all_by_scope(), deprecate get_all() (#1534)

CHANGELOG.rst

@@ -17,6 +17,7 @@ Added
     - ``PluginAppSettingDef`` class for app setting definitions (#1456)
     - Django check for unique app setting names within each plugin (#1456)
     - App setting ``user_modifiable`` validation (#1536)
+    - ``AppSettingAPI.get_all_by_scope()`` helper (#1534)
 
 Changed
 -------
@@ -28,6 +29,7 @@ Changed
     - Deprecate ``get_user_display_name()``, use ``SODARUser.get_display_name()`` (#1487)
     - Deprecate declaring app setting definitions as dict (#1456)
     - Allow ``scope=None`` in ``AppSettingAPI.get_definitions()`` (#1535)
+    - Deprecate ``AppSettingAPI.get_all()`` (#1534)
 
 Removed
 -------

docs/source/major_changes.rst

@@ -44,9 +44,10 @@ App Setting Definitions as Dict
     Instead, you should provide a list of ``PluginAppSettingDef`` objects. See
     the :ref:`app settings documentation <dev_resource_app_settings>` for
     details.
+``AppSettingAPI.get_all()``
+    Replaced by ``AppSettingAPI.get_all_by_scope()``.
 ``projectroles.utils.get_user_display_name()``
-    This utility method has been deprecated. Please use
-    ``SODARUser.get_display_name()`` instead.
+    Replaced by ``SODARUser.get_display_name()``.
 
 Previously Deprecated Features Removed
 --------------------------------------

projectroles/app_settings.py

@@ -59,6 +59,10 @@
     'Provide definitions as a list of PluginAppSettingDef '
     'objects (plugin={plugin_name})'
 )
+GET_ALL_DEPRECATE_MSG = (
+    'The get_all() method has been deprecated in v1.1 and will be removed in '
+    'v1.2. Use get_all_by_scope() instead'
+)
 
 
 # Define App Settings for projectroles app
@@ -384,41 +388,53 @@ def get(
             return json.dumps(val)
         return val
 
-    # TODO: Remove, deprecate or refactor (see #1534)
     @classmethod
-    def get_all(cls, project=None, user=None, post_safe=False):
+    def get_all_by_scope(cls, scope, project=None, user=None, post_safe=False):
         """
-        Return all setting values. If the value is not found, return
-        the default.
+        Return all setting values by scope. If a value is not set, return the
+        default.
 
+        :param scope: String
         :param project: Project object (optional)
         :param user: User object (optional)
         :param post_safe: Whether POST safe values should be returned (bool)
         :return: Dict
-        :raise: ValueError if neither project nor user are set
+        :raise: ValueError if scope or project and user args are invalid
         """
-        if not project and not user:
-            raise ValueError('Project and user are both unset')
-
+        PluginAppSettingDef.validate_scope(scope)
+        cls._validate_project_and_user(scope, project, user)
         ret = {}
-        app_plugins = get_active_plugins()
-        for plugin in app_plugins:
-            p_defs = cls.get_definitions(
-                APP_SETTING_SCOPE_PROJECT, plugin=plugin
-            )
-            for s_key in p_defs:
-                ret['settings.{}.{}'.format(plugin.name, s_key)] = cls.get(
-                    plugin.name, s_key, project, user, post_safe
+        all_defs = cls.get_all_defs()
+        for plugin_name, s_defs in all_defs.items():
+            for s_def in [d for d in s_defs.values() if d.scope == scope]:
+                ret[f'settings.{plugin_name}.{s_def.name}'] = cls.get(
+                    plugin_name, s_def.name, project, user, post_safe
                 )
+        return ret
 
-        p_defs = cls.get_definitions(
-            APP_SETTING_SCOPE_PROJECT, plugin_name='projectroles'
+    # TODO: Remove in v1.2 (see #1538)
+    @classmethod
+    def get_all(cls, project=None, user=None, post_safe=False):
+        """
+        Return all setting values with the PROJECT scope. If a value is not
+        found, return the default.
+
+        This method is DEPRECATED and will be removed in v1.2. Please use
+        get_all_by_scope() instead.
+
+        :param project: Project object (optional)
+        :param user: User object (NOTE: Not actually used)
+        :param post_safe: Whether POST safe values should be returned (bool)
+        :return: Dict
+        :raise: ValueError if neither project nor user are set
+        """
+        logger.warning(GET_ALL_DEPRECATE_MSG)
+        return cls.get_all_by_scope(
+            scope=APP_SETTING_SCOPE_PROJECT,
+            project=project,
+            user=None,
+            post_safe=post_safe,
         )
-        for s_key in p_defs:
-            ret['settings.{}.{}'.format('projectroles', s_key)] = cls.get(
-                'projectroles', s_key, post_safe
-            )
-        return ret
 
     @classmethod
     def get_defaults(cls, scope, project=None, user=None, post_safe=False):

projectroles/tests/test_app_settings.py

@@ -45,6 +45,7 @@
 EXAMPLE_APP_NAME = 'example_project_app'
 INVALID_SETTING_VALUE = 'INVALID VALUE'
 INVALID_SETTING_MSG = 'INVALID_SETTING_VALUE detected'
+S_PREFIX = 'settings.{}.'
 
 
 class AppSettingInitMixin:
@@ -306,7 +307,7 @@ def test_get(self):
             val = app_settings.get(**data)
             self.assertEqual(val, setting['value'])
 
-    def test_get_with_default(self):
+    def test_get_default(self):
         """Test get() with default value for existing setting"""
         s_defs = app_settings.get_definitions(
             scope=APP_SETTING_SCOPE_PROJECT, plugin_name=EXAMPLE_APP_NAME
@@ -319,7 +320,7 @@ def test_get_with_default(self):
         )
         self.assertEqual(val, default_val)
 
-    def test_get_with_nonexisting(self):
+    def test_get_nonexisting(self):
         """Test get() with non-existing setting"""
         with self.assertRaises(KeyError):
             app_settings.get(
@@ -328,7 +329,7 @@ def test_get_with_nonexisting(self):
                 project=self.project,
             )
 
-    def test_get_with_post_safe(self):
+    def test_get_post_safe(self):
         """Test get() with JSON setting and post_safe=True"""
         val = app_settings.get(
             plugin_name=self.project_json_setting['plugin_name'],
@@ -338,6 +339,101 @@ def test_get_with_post_safe(self):
         )
         self.assertEqual(type(val), str)
 
+    def test_get_all_by_scope_project(self):
+        """Test get_all_by_scope() with PROJECT scope"""
+        all_defs = app_settings.get_all_defs()
+        vals = app_settings.get_all_by_scope(
+            APP_SETTING_SCOPE_PROJECT, project=self.project
+        )
+        for k, v in all_defs.items():
+            p_vals = [v for v in vals if v.startswith(S_PREFIX.format(k))]
+            s_defs = [
+                d for d in v.values() if d.scope == APP_SETTING_SCOPE_PROJECT
+            ]
+            self.assertEqual(len(p_vals), len(s_defs))
+            for s_def in s_defs:
+                self.assertEqual(
+                    vals[f'settings.{k}.{s_def.name}'],
+                    app_settings.get(
+                        plugin_name=k,
+                        setting_name=s_def.name,
+                        project=self.project,
+                    ),
+                )
+
+    def test_get_all_by_scope_user(self):
+        """Test get_all_by_scope() with USER scope"""
+        all_defs = app_settings.get_all_defs()
+        vals = app_settings.get_all_by_scope(
+            APP_SETTING_SCOPE_USER, user=self.user
+        )
+        for k, v in all_defs.items():
+            p_vals = [v for v in vals if v.startswith(S_PREFIX.format(k))]
+            s_defs = [
+                d for d in v.values() if d.scope == APP_SETTING_SCOPE_USER
+            ]
+            self.assertEqual(len(p_vals), len(s_defs))
+            for s_def in s_defs:
+                self.assertEqual(
+                    vals[f'settings.{k}.{s_def.name}'],
+                    app_settings.get(
+                        plugin_name=k,
+                        setting_name=s_def.name,
+                        user=self.user,
+                    ),
+                )
+
+    def test_get_all_by_scope_project_user(self):
+        """Test get_all_by_scope() with PROJECT_USER scope"""
+        all_defs = app_settings.get_all_defs()
+        vals = app_settings.get_all_by_scope(
+            APP_SETTING_SCOPE_PROJECT_USER, project=self.project, user=self.user
+        )
+        for k, v in all_defs.items():
+            p_vals = [v for v in vals if v.startswith(S_PREFIX.format(k))]
+            s_defs = [
+                d
+                for d in v.values()
+                if d.scope == APP_SETTING_SCOPE_PROJECT_USER
+            ]
+            self.assertEqual(len(p_vals), len(s_defs))
+            for s_def in s_defs:
+                self.assertEqual(
+                    vals[f'settings.{k}.{s_def.name}'],
+                    app_settings.get(
+                        plugin_name=k,
+                        setting_name=s_def.name,
+                        project=self.project,
+                        user=self.user,
+                    ),
+                )
+
+    def test_get_all_by_scope_site(self):
+        """Test get_all_by_scope() with SITE scope"""
+        all_defs = app_settings.get_all_defs()
+        vals = app_settings.get_all_by_scope(APP_SETTING_SCOPE_SITE)
+        for k, v in all_defs.items():
+            p_vals = [v for v in vals if v.startswith(S_PREFIX.format(k))]
+            s_defs = [
+                d for d in v.values() if d.scope == APP_SETTING_SCOPE_SITE
+            ]
+            self.assertEqual(len(p_vals), len(s_defs))
+            for s_def in s_defs:
+                self.assertEqual(
+                    vals[f'settings.{k}.{s_def.name}'],
+                    app_settings.get(
+                        plugin_name=k,
+                        setting_name=s_def.name,
+                    ),
+                )
+
+    def test_get_all_by_scope_invalid_args(self):
+        """Test get_all_by_scope() with invalid args"""
+        with self.assertRaises(ValueError):
+            app_settings.get_all_by_scope(
+                APP_SETTING_SCOPE_PROJECT_USER, project=self.project  # No user
+            )
+
     def test_is_set_no_object(self):
         """Test is_set() with no setting object set"""
         n = 'project_star'

projectroles/tests/test_views.py

@@ -1078,7 +1078,9 @@ def _get_post_app_settings(cls, project, user):
         """Get postable app settings for project of type PROJECT"""
         if project.type != PROJECT_TYPE_PROJECT:
             raise ValueError('Can only be called for a project')
-        ps = app_settings.get_all(project=project, post_safe=True)
+        ps = app_settings.get_all_by_scope(
+            APP_SETTING_SCOPE_PROJECT, project=project, post_safe=True
+        )
         # Omit hidden settings for regular user
         if user and not user.is_superuser:
             ps = {k: ps[k] for k in ps if k not in HIDDEN_PROJECT_SETTINGS}
@@ -1475,7 +1477,11 @@ def test_post_project_title_delimiter(self):
         data['parent'] = self.category.sodar_uuid
         data['owner'] = self.user.sodar_uuid
         data['title'] = 'Project{}Title'.format(CAT_DELIMITER)
-        data.update(app_settings.get_all(project=self.project, post_safe=True))
+        data.update(
+            app_settings.get_all_by_scope(
+                APP_SETTING_SCOPE_PROJECT, project=self.project, post_safe=True
+            )
+        )
         with self.login(self.user):
             response = self.client.post(self.url, data)
         self.assertEqual(response.status_code, 200)
@@ -1487,7 +1493,11 @@ def test_post_project_custom_validation(self):
         data = model_to_dict(self.project)
         data['parent'] = self.category.sodar_uuid
         data['owner'] = self.user.sodar_uuid
-        data.update(app_settings.get_all(project=self.project, post_safe=True))
+        data.update(
+            app_settings.get_all_by_scope(
+                APP_SETTING_SCOPE_PROJECT, project=self.project, post_safe=True
+            )
+        )
         data['settings.example_project_app.project_str_setting'] = (
             INVALID_SETTING_VALUE
         )
@@ -1514,7 +1524,11 @@ def test_post_project_public_access(self):
         data['public_guest_access'] = True
         data['parent'] = self.category.sodar_uuid  # NOTE: Must add parent
         data['owner'] = self.user.sodar_uuid  # NOTE: Must add owner
-        data.update(app_settings.get_all(project=self.project, post_safe=True))
+        data.update(
+            app_settings.get_all_by_scope(
+                APP_SETTING_SCOPE_PROJECT, project=self.project, post_safe=True
+            )
+        )
         with self.login(self.user):
             response = self.client.post(self.url, data)
 
@@ -1533,7 +1547,11 @@ def test_post_category(self):
         data['description'] = 'updated description'
         data['owner'] = self.user.sodar_uuid  # NOTE: Must add owner
         data['parent'] = ''
-        data.update(app_settings.get_all(project=self.category, post_safe=True))
+        data.update(
+            app_settings.get_all_by_scope(
+                APP_SETTING_SCOPE_PROJECT, project=self.category, post_safe=True
+            )
+        )
         with self.login(self.user):
             response = self.client.post(self.url_cat, data)
         self.assertEqual(response.status_code, 302)
@@ -1596,7 +1614,11 @@ def test_post_category_parent(self):
         data['description'] = self.category.description
         data['owner'] = self.user.sodar_uuid  # NOTE: Must add owner
         data['parent'] = category_new.sodar_uuid  # Updated category
-        data.update(app_settings.get_all(project=self.category, post_safe=True))
+        data.update(
+            app_settings.get_all_by_scope(
+                APP_SETTING_SCOPE_PROJECT, project=self.category, post_safe=True
+            )
+        )
         with self.login(self.user):
             response = self.client.post(self.url_cat, data)
 
@@ -1634,7 +1656,11 @@ def test_post_parent_different_owner(self):
         data = model_to_dict(self.project)
         data['owner'] = self.user.sodar_uuid  # NOTE: Must add owner
         data['parent'] = category_new.sodar_uuid  # Updated category
-        data.update(app_settings.get_all(project=self.project, post_safe=True))
+        data.update(
+            app_settings.get_all_by_scope(
+                APP_SETTING_SCOPE_PROJECT, project=self.project, post_safe=True
+            )
+        )
         with self.login(self.user):
             response = self.client.post(self.url, data)
 
@@ -1666,7 +1692,11 @@ def test_post_parent_different_owner_disable_email(self):
         data = model_to_dict(self.project)
         data['owner'] = self.user.sodar_uuid  # NOTE: Must add owner
         data['parent'] = category_new.sodar_uuid  # Updated category
-        data.update(app_settings.get_all(project=self.project, post_safe=True))
+        data.update(
+            app_settings.get_all_by_scope(
+                APP_SETTING_SCOPE_PROJECT, project=self.project, post_safe=True
+            )
+        )
         with self.login(self.user):
             response = self.client.post(self.url, data)