refactor: logging added with some code refactoring

Author: marslanabdulrauf

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

@@ -223,7 +223,7 @@ 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()
+        mapping = CourseAppsPluginManager.get_course_app_settings_mapping(self.course.id)
 
         # Verify that calculator and edxnotes are in the mapping
         assert "show_calculator" in mapping

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

@@ -1,5 +1,6 @@
 """ API Views for course advanced settings """
 
+import logging
 from django import forms
 import edx_api_doc_tools as apidocs
 from opaque_keys.edx.keys import CourseKey
@@ -18,6 +19,8 @@
 from ..serializers import CourseAdvancedSettingsSerializer
 from ....views.course import update_course_advanced_settings
 
+log = logging.getLogger(__name__)
+
 
 @view_auth_classes(is_authenticated=True)
 class AdvancedCourseSettingsView(DeveloperErrorViewMixin, APIView):
@@ -190,26 +193,114 @@ def patch(self, request: Request, course_id: str):
         if not has_studio_write_access(request.user, course_key):
             self.permission_denied(request)
 
-        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)
-                if course_app_enabled is not None:
-                    try:
-                        set_course_app_status(
-                            course_key=course_key,
-                            app_id=course_app_settings_map[setting],
-                            enabled=course_app_enabled,
-                            request=request,
-                        )
-                        # enabling/disabling the course app setting also updates
-                        # the advanced settings, so we remove it from the request data
-                        request.data.pop(setting)
-                    except ValidationError:
-                        # Failed to update the course app status,
-                        # we ignore and let the normal flow handle updates
-                        pass
+        # Process course app settings that have corresponding advanced settings
+        self._process_course_app_settings(request, course_key)
 
         course_block = modulestore().get_course(course_key)
         updated_data = update_course_advanced_settings(course_block, request.data, request.user)
         return Response(updated_data)
+
+    def _process_course_app_settings(self, request: Request, course_key: CourseKey) -> None:
+        """
+        Process course app settings that have corresponding advanced settings.
+
+        Updates course app status for settings that are managed by course apps,
+        and removes them from the request data to avoid duplicate processing.
+
+        Args:
+            request: The HTTP request containing settings to update
+            course_key: The course key for the course being updated
+        """
+        course_app_settings_map = CourseAppsPluginManager.get_course_app_settings_mapping(course_key)
+
+        if not course_app_settings_map:
+            log.debug("No course app settings mapping found for course: %s", course_key)
+            return
+
+        log.debug(
+            "Processing course app settings: course_key=%s, user=%s, "
+            "available_app_settings=%s, request_settings=%s",
+            course_key, request.user.username,
+            list(course_app_settings_map.keys()), list(request.data.keys())
+        )
+
+        settings_processed = 0
+        settings_failed = 0
+
+        for setting_name in course_app_settings_map:
+            setting_data = request.data.get(setting_name)
+            if not setting_data:
+                continue
+
+            new_value = setting_data.get("value")
+            if new_value is None:
+                log.debug("Skipping setting %s - no value provided", setting_name)
+                continue
+
+            app_id = course_app_settings_map[setting_name]
+
+            if self._update_course_app_setting(
+                request=request,
+                course_key=course_key,
+                setting_name=setting_name,
+                app_id=app_id,
+                enabled=new_value
+            ):
+                settings_processed += 1
+                # Remove from request data since it's been handled by course app
+                request.data.pop(setting_name)
+            else:
+                settings_failed += 1
+
+        log.info(
+            "Course app settings processing complete: course_key=%s, "
+            "processed=%d, failed=%d (falling back to advanced settings)",
+            course_key, settings_processed, settings_failed
+        )
+
+    def _update_course_app_setting(
+        self, request: Request, course_key: CourseKey,
+        setting_name: str, app_id: str, enabled: bool
+    ) -> bool:
+        """
+        Update a single course app setting.
+
+        Args:
+            request: The HTTP request
+            course_key: The course key
+            setting_name: Name of the advanced setting
+            app_id: ID of the course app
+            enabled: Whether to enable or disable the app
+
+        Returns:
+            bool: True if update was successful, False if it failed
+        """
+        try:
+            log.debug(
+                "Attempting course app update: course_key=%s, app_id=%s, "
+                "setting=%s, enabled=%s, user=%s",
+                course_key, app_id, setting_name, enabled, request.user.username
+            )
+
+            set_course_app_status(
+                course_key=course_key,
+                app_id=app_id,
+                enabled=enabled,
+                user=request.user,
+            )
+
+            log.info(
+                "Successfully updated course app via advanced settings: "
+                "course_key=%s, app_id=%s, setting=%s, enabled=%s, user=%s",
+                course_key, app_id, setting_name, enabled, request.user.username
+            )
+            return True
+
+        except ValidationError as e:
+            log.warning(
+                "Course app update failed with validation error, "
+                "will fallback to advanced settings flow: "
+                "course_key=%s, app_id=%s, setting=%s, enabled=%s, user=%s, error=%s",
+                course_key, app_id, setting_name, enabled, request.user.username, str(e)
+            )
+            return False

openedx/core/djangoapps/course_apps/api.py

@@ -66,15 +66,15 @@ def set_course_app_enabled(course_key: CourseKey, app_id: str, enabled: bool, us
     return enabled
 
 
-def set_course_app_status(course_key, app_id, enabled, request):
+def set_course_app_status(course_key: CourseKey, app_id: str, enabled: bool, user: User) -> bool:
     """
     Enable or disable a course app for a given course.
 
     Args:
         course_key (CourseKey): The course key for the course to update.
         app_id (str): The app ID of the course app to enable/disable.
         enabled (bool): The desired enabled status.
-        request (HttpRequest): The HTTP request object
+        user (User): The user performing the operation.
 
     Returns:
         bool: The final enabled/disabled status of the app.
@@ -86,4 +86,4 @@ def set_course_app_status(course_key, app_id, enabled, request):
     if not course_app or not course_app.is_available(course_key):
         raise ValidationError({"id": "Invalid app ID"})
 
-    return set_course_app_enabled(course_key=course_key, app_id=app_id, enabled=enabled, user=request.user)
+    return set_course_app_enabled(course_key=course_key, app_id=app_id, enabled=enabled, user=user)

openedx/core/djangoapps/course_apps/plugins.py

@@ -30,7 +30,7 @@ class CourseApp(ABC):
     }
     # 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
+    advanced_setting_field: Optional[str] = None
 
     @classmethod
     @abstractmethod
@@ -120,20 +120,23 @@ def get_apps_available_for_course(cls, course_key: CourseKey) -> Iterator[Course
                 yield plugin
 
     @classmethod
-    def get_course_app_settings_mapping(cls) -> Dict[str, str]:
+    def get_course_app_settings_mapping(cls, course_key: CourseKey) -> 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.
 
+        Args:
+            course_key (CourseKey): The course key for which the mapping is to be generated.
+
         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:
+        for plugin in cls.get_apps_available_for_course(course_key):
+            if getattr(plugin, 'advanced_setting_field', None):
                 settings_mapping[plugin.advanced_setting_field] = plugin.app_id
 
         return settings_mapping

openedx/core/djangoapps/course_apps/rest_api/v1/views.py

@@ -191,8 +191,7 @@ def patch(self, request: Request, course_id: str):
             raise ValidationError({"id": "App id is missing"})
         if enabled is None:
             raise ValidationError({"enabled": "Must provide value for `enabled` field."})
-
-        is_enabled = set_course_app_status(course_key=course_key, app_id=app_id, enabled=enabled, request=request)
+        is_enabled = set_course_app_status(course_key, app_id, enabled, request.user)
         course_app = CourseAppsPluginManager.get_plugin(app_id)
         serializer = CourseAppSerializer(
             course_app,

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

@@ -3,8 +3,7 @@
 """
 from unittest.mock import patch
 
-from django.test import TestCase
-
+from cms.djangoapps.contentstore.tests.utils import CourseTestCase
 from openedx.core.djangoapps.course_apps.plugins import CourseAppsPluginManager, CourseApp
 
 
@@ -57,7 +56,7 @@ def get_allowed_operations(cls, course_key, user=None):
         return {"enable": True, "configure": True}
 
 
-class DynamicSettingsMappingTest(TestCase):
+class DynamicSettingsMappingTest(CourseTestCase):
     """Test dynamic course app settings mapping functionality."""
 
     def test_app_with_advanced_setting_mapping(self):
@@ -67,7 +66,7 @@ def test_app_with_advanced_setting_mapping(self):
         }
 
         with patch('edx_django_utils.plugins.PluginManager.get_available_plugins', return_value=mock_plugins):
-            mapping = CourseAppsPluginManager.get_course_app_settings_mapping()
+            mapping = CourseAppsPluginManager.get_course_app_settings_mapping(self.course.id)
 
             assert "settings_app_field" in mapping
             assert mapping["settings_app_field"] == "settings_app"
@@ -79,7 +78,7 @@ def test_no_advanced_setting_fields(self):
         }
 
         with patch('edx_django_utils.plugins.PluginManager.get_available_plugins', return_value=mock_plugins):
-            mapping = CourseAppsPluginManager.get_course_app_settings_mapping()
+            mapping = CourseAppsPluginManager.get_course_app_settings_mapping(self.course.id)
 
             assert len(mapping) == 0
 
@@ -91,7 +90,7 @@ def test_mixed_apps_mapping(self):
         }
 
         with patch('edx_django_utils.plugins.PluginManager.get_available_plugins', return_value=mock_plugins):
-            mapping = CourseAppsPluginManager.get_course_app_settings_mapping()
+            mapping = CourseAppsPluginManager.get_course_app_settings_mapping(self.course.id)
 
             # Should only include apps with advanced_setting_field
             assert len(mapping) == 1