refactor: set setting map dynamically instead of hardcoded values

Author: marslanabdulrauf

cms/djangoapps/contentstore/rest_api/v0/tests/test_advanced_settings.py

@@ -11,6 +11,7 @@
 from rest_framework.exceptions import ValidationError
 
 from cms.djangoapps.contentstore.tests.utils import CourseTestCase
+from openedx.core.djangoapps.course_apps.plugins import CourseAppsPluginManager
 
 
 @ddt.ddt
@@ -216,3 +217,49 @@ def test_patch_advanced_setting_with_none_value(self, mock_set_course_app_status
         # Verify set_course_app_status was not called when value is None
         mock_set_course_app_status.assert_not_called()
         assert response.status_code == 200
+
+    def test_dynamic_course_app_settings_mapping(self):
+        """
+        Test that the course app settings mapping is dynamically discovered from plugins.
+        """
+        # Get the dynamic mapping
+        mapping = CourseAppsPluginManager.get_course_app_settings_mapping()
+
+        # Verify that calculator and edxnotes are in the mapping
+        assert "show_calculator" in mapping
+        assert mapping["show_calculator"] == "calculator"
+        assert "edxnotes" in mapping
+        assert mapping["edxnotes"] == "edxnotes"
+
+    @patch(
+        'cms.djangoapps.contentstore.rest_api.v0.views.advanced_settings.'
+        'CourseAppsPluginManager.get_course_app_settings_mapping'
+    )
+    @patch('cms.djangoapps.contentstore.rest_api.v0.views.advanced_settings.set_course_app_status')
+    def test_patch_uses_dynamic_mapping(self, mock_set_course_app_status, mock_get_mapping):
+        """
+        Test that PATCH uses the dynamic mapping instead of hardcoded values.
+        """
+        # Mock the dynamic mapping to return custom mapping
+        mock_get_mapping.return_value = {
+            "show_calculator": "calculator",
+            "custom_app_setting": "custom_app"
+        }
+        mock_set_course_app_status.return_value = True
+
+        data = {
+            "custom_app_setting": {
+                "value": True
+            }
+        }
+        response = self.client.patch(self.url, json.dumps(data), content_type="application/json")
+
+        # Verify the dynamic mapping was called
+        mock_get_mapping.assert_called_once()
+
+        # Verify set_course_app_status was called with the custom app
+        mock_set_course_app_status.assert_called_once()
+        call_args = mock_set_course_app_status.call_args.kwargs
+        assert call_args['app_id'] == 'custom_app'
+        assert call_args['enabled'] is True
+        assert response.status_code == 200

cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py

@@ -13,6 +13,7 @@
 from cms.djangoapps.contentstore.api.views.utils import get_bool_param
 from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access
 from openedx.core.djangoapps.course_apps.api import set_course_app_status
+from openedx.core.djangoapps.course_apps.plugins import CourseAppsPluginManager
 from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, verify_course_exists, view_auth_classes
 from ..serializers import CourseAdvancedSettingsSerializer
 from ....views.course import update_course_advanced_settings
@@ -144,6 +145,7 @@ def get(self, request: Request, course_id: str):
     def patch(self, request: Request, course_id: str):
         """
         Update a course's advanced settings.
+        Also update the status of the course apps that have corresponding advanced settings fields.
 
         **Example Request**
 
@@ -188,12 +190,7 @@ def patch(self, request: Request, course_id: str):
         if not has_studio_write_access(request.user, course_key):
             self.permission_denied(request)
 
-        # These settings correspond to course apps
-        # The keys are the advanced setting names, and the values are the corresponding app IDs
-        course_app_settings_map = {
-            "show_calculator": "calculator",
-            "edxnotes": "edxnotes",
-        }
+        course_app_settings_map = CourseAppsPluginManager.get_course_app_settings_mapping()
         for setting in course_app_settings_map:
             if setting_to_update := request.data.get(setting):
                 course_app_enabled = setting_to_update.get("value", None)
@@ -209,7 +206,8 @@ def patch(self, request: Request, course_id: str):
                         # the advanced settings, so we remove it from the request data
                         request.data.pop(setting)
                     except ValidationError:
-                        # Ignore errors and let the normal flow handle updates
+                        # Failed to update the course app status,
+                        # we ignore and let the normal flow handle updates
                         pass
 
         course_block = modulestore().get_course(course_key)

lms/djangoapps/courseware/plugins.py

@@ -127,6 +127,7 @@ class CalculatorCourseApp(CourseApp):
     documentation_links = {
         "learn_more_configuration": settings.CALCULATOR_HELP_URL,
     }
+    advanced_setting_field = "show_calculator"
 
     @classmethod
     def is_available(cls, course_key: CourseKey) -> bool:
@@ -140,15 +141,15 @@ def is_enabled(cls, course_key: CourseKey) -> bool:
         """
         Get calculator enabled status from course overview model.
         """
-        return CourseOverview.get_from_id(course_key).show_calculator
+        return getattr(CourseOverview.get_from_id(course_key), cls.advanced_setting_field, False)
 
     @classmethod
     def set_enabled(cls, course_key: CourseKey, enabled: bool, user: 'User') -> bool:
         """
         Update calculator enabled status in modulestore.
         """
         course = get_course_by_id(course_key)
-        course.show_calculator = enabled
+        setattr(course, cls.advanced_setting_field, enabled)
         modulestore().update_item(course, user.id)
         return enabled
 

lms/djangoapps/edxnotes/plugins.py

@@ -59,6 +59,7 @@ class EdxNotesCourseApp(CourseApp):
     documentation_links = {
         "learn_more_configuration": settings.EDXNOTES_HELP_URL,
     }
+    advanced_setting_field = "edxnotes"
 
     @classmethod
     def is_available(cls, course_key: CourseKey) -> bool:  # pylint: disable=unused-argument
@@ -72,15 +73,15 @@ def is_enabled(cls, course_key: CourseKey) -> bool:  # pylint: disable=unused-ar
         """
         Get enabled/disabled status from modulestore.
         """
-        return CourseOverview.get_from_id(course_key).edxnotes
+        return getattr(CourseOverview.get_from_id(course_key), cls.advanced_setting_field, False)
 
     @classmethod
     def set_enabled(cls, course_key: CourseKey, enabled: bool, user: 'User') -> bool:
         """
         Enable/disable edxnotes in the modulestore.
         """
         course = get_course_by_id(course_key)
-        course.edxnotes = enabled
+        setattr(course, cls.advanced_setting_field, enabled)
         if enabled:
             notes_tab = CourseTabList.get_tab_by_id(course.tabs, 'edxnotes')
             if notes_tab is None:

openedx/core/djangoapps/course_apps/plugins.py

@@ -28,6 +28,9 @@ class CourseApp(ABC):
         # eg:
         # "learn_more_configuration": "https://..."
     }
+    # Advanced setting field name that corresponds to this app (optional)
+    # Should be a string representing the field name in course advanced settings
+    advanced_setting_field = None
 
     @classmethod
     @abstractmethod
@@ -115,3 +118,22 @@ def get_apps_available_for_course(cls, course_key: CourseKey) -> Iterator[Course
         for plugin in super().get_available_plugins().values():
             if plugin.is_available(course_key):
                 yield plugin
+
+    @classmethod
+    def get_course_app_settings_mapping(cls) -> Dict[str, str]:
+        """
+        Returns a mapping of advanced setting field names to course app IDs.
+
+        This method dynamically discovers all course app plugins and builds a mapping
+        based on their advanced_setting_field attributes.
+
+        Returns:
+            Dict[str, str]: A dictionary mapping advanced setting field names to app IDs.
+        """
+        settings_mapping = {}
+
+        for plugin in super().get_available_plugins().values():
+            if hasattr(plugin, 'advanced_setting_field') and plugin.advanced_setting_field:
+                settings_mapping[plugin.advanced_setting_field] = plugin.app_id
+
+        return settings_mapping

openedx/core/djangoapps/course_apps/tests/test_dynamic_settings_mapping.py

@@ -0,0 +1,98 @@
+"""
+Tests for dynamic course app settings mapping functionality.
+"""
+from unittest.mock import patch
+
+from django.test import TestCase
+
+from openedx.core.djangoapps.course_apps.plugins import CourseAppsPluginManager, CourseApp
+
+
+class MockCourseAppWithSetting(CourseApp):
+    """Mock course app with an advanced setting field."""
+
+    app_id = "settings_app"
+    name = "Settings App"
+    description = "An app with advanced settings"
+    advanced_setting_field = "settings_app_field"
+
+    @classmethod
+    def is_available(cls, course_key):
+        return True
+
+    @classmethod
+    def is_enabled(cls, course_key):
+        return True
+
+    @classmethod
+    def set_enabled(cls, course_key, enabled, user):
+        return enabled
+
+    @classmethod
+    def get_allowed_operations(cls, course_key, user=None):
+        return {"enable": True, "configure": True}
+
+
+class MockCourseAppNoSettings(CourseApp):
+    """Mock course app with no advanced settings mapping."""
+
+    app_id = "no_settings_app"
+    name = "No Settings App"
+    description = "An app without advanced settings"
+
+    @classmethod
+    def is_available(cls, course_key):
+        return True
+
+    @classmethod
+    def is_enabled(cls, course_key):
+        return True
+
+    @classmethod
+    def set_enabled(cls, course_key, enabled, user):
+        return enabled
+
+    @classmethod
+    def get_allowed_operations(cls, course_key, user=None):
+        return {"enable": True, "configure": True}
+
+
+class DynamicSettingsMappingTest(TestCase):
+    """Test dynamic course app settings mapping functionality."""
+
+    def test_app_with_advanced_setting_mapping(self):
+        """Test that a course app with an advanced setting field is mapped correctly."""
+        mock_plugins = {
+            "settings_app": MockCourseAppWithSetting(),
+        }
+
+        with patch('edx_django_utils.plugins.PluginManager.get_available_plugins', return_value=mock_plugins):
+            mapping = CourseAppsPluginManager.get_course_app_settings_mapping()
+
+            assert "settings_app_field" in mapping
+            assert mapping["settings_app_field"] == "settings_app"
+
+    def test_no_advanced_setting_fields(self):
+        """Test that a course app without advanced_setting_fields is not included in mapping."""
+        mock_plugins = {
+            "no_settings_app": MockCourseAppNoSettings(),
+        }
+
+        with patch('edx_django_utils.plugins.PluginManager.get_available_plugins', return_value=mock_plugins):
+            mapping = CourseAppsPluginManager.get_course_app_settings_mapping()
+
+            assert len(mapping) == 0
+
+    def test_mixed_apps_mapping(self):
+        """Test mapping with a mix of apps with and without advanced settings."""
+        mock_plugins = {
+            "settings_app": MockCourseAppWithSetting(),
+            "no_settings_app": MockCourseAppNoSettings(),
+        }
+
+        with patch('edx_django_utils.plugins.PluginManager.get_available_plugins', return_value=mock_plugins):
+            mapping = CourseAppsPluginManager.get_course_app_settings_mapping()
+
+            # Should only include apps with advanced_setting_field
+            assert len(mapping) == 1
+            assert mapping["settings_app_field"] == "settings_app"