Merge branch 'master' into arslan/fix-validation-api

Author: arslanashraf7

lms/djangoapps/course_api/blocks/api.py

@@ -1,7 +1,7 @@
 """
 API function for retrieving course blocks data
 """
-
+from edx_django_utils.cache import RequestCache
 
 import lms.djangoapps.course_blocks.api as course_blocks_api
 from lms.djangoapps.course_blocks.transformers.access_denied_filter import AccessDeniedMessageFilterTransformer
@@ -14,6 +14,7 @@
 from .toggles import HIDE_ACCESS_DENIALS_FLAG
 from .transformers.blocks_api import BlocksAPITransformer
 from .transformers.milestones import MilestonesAndSpecialExamsTransformer
+from .utils import COURSE_API_REQUEST_CACHE_NAMESPACE, REUSABLE_BLOCKS_CACHE_KEY
 
 
 def get_blocks(
@@ -29,6 +30,7 @@ def get_blocks(
         block_types_filter=None,
         hide_access_denials=False,
         allow_start_dates_in_future=False,
+        cache_with_future_dates=False,
 ):
     """
     Return a serialized representation of the course blocks.
@@ -61,6 +63,7 @@ def get_blocks(
         allow_start_dates_in_future (bool): When True, will allow blocks to be
             returned that can bypass the StartDateTransformer's filter to show
             blocks with start dates in the future.
+        cache_with_future_dates (bool): When True, will use the block caching logic using RequestCache
     """
 
     if HIDE_ACCESS_DENIALS_FLAG.is_enabled():
@@ -118,6 +121,10 @@ def get_blocks(
         ),
     ]
 
+    if cache_with_future_dates:
+        # Include future dates such that get_course_assignments can reuse the block structure from RequestCache
+        allow_start_dates_in_future = True
+
     # transform
     blocks = course_blocks_api.get_course_blocks(
         user,
@@ -128,6 +135,19 @@ def get_blocks(
         include_has_scheduled_content=include_has_scheduled_content
     )
 
+    if cache_with_future_dates:
+        # Store a copy of the transformed, but still unfiltered, course blocks in RequestCache to be reused
+        # wherever possible for optimization. Copying is required to make sure the cached structure is not mutated
+        # by the filtering below.
+        request_cache = RequestCache(COURSE_API_REQUEST_CACHE_NAMESPACE)
+        request_cache.set(REUSABLE_BLOCKS_CACHE_KEY, blocks.copy())
+
+        # Since we included blocks with future start dates in our block structure,
+        # we need to include the 'start' field to filter out such blocks before returning the response.
+        # If 'start' field is not requested, it will be removed from the response.
+        requested_fields = set(requested_fields)
+        requested_fields.add('start')
+
     # filter blocks by types
     if block_types_filter:
         block_keys_to_remove = []
@@ -142,7 +162,7 @@ def get_blocks(
     serializer_context = {
         'request': request,
         'block_structure': blocks,
-        'requested_fields': requested_fields or [],
+        'requested_fields': requested_fields,
     }
 
     if return_type == 'dict':

lms/djangoapps/course_api/blocks/utils.py

@@ -1,6 +1,7 @@
 """
    Utils for Blocks
 """
+from edx_django_utils.cache import RequestCache
 from rest_framework.utils.serializer_helpers import ReturnList
 
 from openedx.core.djangoapps.discussions.models import (
@@ -9,6 +10,10 @@
 )
 
 
+COURSE_API_REQUEST_CACHE_NAMESPACE = "course_api"
+REUSABLE_BLOCKS_CACHE_KEY = "reusable_transformed_blocks"
+
+
 def filter_discussion_xblocks_from_response(response, course_key):
     """
     Removes discussion xblocks if discussion provider is openedx.
@@ -63,3 +68,18 @@ def filter_discussion_xblocks_from_response(response, course_key):
         response.data['blocks'] = filtered_blocks
 
     return response
+
+
+def get_cached_transformed_blocks():
+    """
+    Helper function to get an unfiltered course structure from RequestCache,
+    including blocks with start dates in the future.
+
+    Caution: For performance reasons, the function returns the structure object itself, not its copy.
+    This means the retrieved structure is supposed to be read-only and should not be mutated by consumers.
+    """
+    request_cache = RequestCache(COURSE_API_REQUEST_CACHE_NAMESPACE)
+    cached_response = request_cache.get_cached_response(REUSABLE_BLOCKS_CACHE_KEY)
+    reusable_transformed_blocks = cached_response.value if cached_response.is_found else None
+
+    return reusable_transformed_blocks

lms/djangoapps/course_api/blocks/views.py

@@ -2,6 +2,7 @@
 CourseBlocks API views
 """
 
+from datetime import datetime, timezone
 
 from django.core.exceptions import ValidationError
 from django.db import transaction
@@ -237,6 +238,7 @@ def list(self, request, usage_key_string, hide_access_denials=False):  # pylint:
                     params.cleaned_data['return_type'],
                     params.cleaned_data.get('block_types_filter', None),
                     hide_access_denials=hide_access_denials,
+                    cache_with_future_dates=True
                 )
             )
             # If the username is an empty string, and not None, then we are requesting
@@ -339,9 +341,50 @@ def list(self, request, hide_access_denials=False):  # pylint: disable=arguments
         if not root:
             raise ValidationError(f"Unable to find course block in '{course_key_string}'")
 
+        # Earlier we included blocks with future start dates in the collected/cached block structure.
+        # Now we need to emulate allow_start_dates_in_future=False by removing any such blocks.
+        include_start = "start" in request.query_params['requested_fields']
+        self.remove_future_blocks(course_blocks, include_start)
+
         recurse_mark_complete(root, course_blocks)
         return response
 
+    @staticmethod
+    def remove_future_blocks(course_blocks, include_start: bool):
+        """
+        Mutates course_blocks in place:
+        - removes blocks whose 'start' is in the future
+        - also removes references to them from parents' 'children' lists
+        - removes 'start' key from all blocks if it wasn't requested
+        """
+        if not course_blocks:
+            return course_blocks
+
+        now = datetime.now(timezone.utc)
+
+        # 1. Collect IDs of blocks to remove
+        to_remove = set()
+        for block_id, block in course_blocks.items():
+            get_field = block.get if include_start else block.pop
+            start = get_field("start")
+            if start and start > now:
+                to_remove.add(block_id)
+
+        if not to_remove:
+            return course_blocks
+
+        # 2. Remove the blocks themselves
+        for block_id in to_remove:
+            course_blocks.pop(block_id, None)
+
+        # 3. Clean up children lists
+        for block in course_blocks.values():
+            children = block.get("children")
+            if children:
+                block["children"] = [cid for cid in children if cid not in to_remove]
+
+        return course_blocks
+
 
 @method_decorator(transaction.non_atomic_requests, name='dispatch')
 @view_auth_classes(is_authenticated=False)

lms/djangoapps/courseware/courses.py

@@ -26,6 +26,7 @@
 from common.djangoapps.static_replace import replace_static_urls
 from common.djangoapps.util.date_utils import strftime_localized
 from lms.djangoapps import branding
+from lms.djangoapps.course_api.blocks.utils import get_cached_transformed_blocks
 from lms.djangoapps.course_blocks.api import get_course_blocks
 from lms.djangoapps.courseware.access import has_access
 from lms.djangoapps.courseware.access_response import (
@@ -636,7 +637,10 @@ def get_course_assignments(course_key, user, include_access=False, include_witho
 
     store = modulestore()
     course_usage_key = store.make_course_usage_key(course_key)
-    block_data = get_course_blocks(user, course_usage_key, allow_start_dates_in_future=True, include_completion=True)
+
+    block_data = get_cached_transformed_blocks() or get_course_blocks(
+        user, course_usage_key, allow_start_dates_in_future=True, include_completion=True
+    )
 
     now = datetime.now(pytz.UTC)
     assignments = []

lms/djangoapps/courseware/tests/test_video_mongo.py

@@ -88,7 +88,6 @@ def test_video_constructor(self):
 
         expected_context = {
             'autoadvance_enabled': False,
-            'branding_info': None,
             'license': None,
             'bumper_metadata': 'null',
             'block_id': str(self.block.location),
@@ -177,7 +176,6 @@ def test_video_constructor(self):
 
         expected_context = {
             'autoadvance_enabled': False,
-            'branding_info': None,
             'license': None,
             'bumper_metadata': 'null',
             'block_id': str(self.block.location),
@@ -454,7 +452,6 @@ def test_get_html_track(self):
 
         expected_context = {
             'autoadvance_enabled': False,
-            'branding_info': None,
             'license': None,
             'bumper_metadata': 'null',
             'block_id': str(self.block.location),
@@ -587,7 +584,6 @@ def test_get_html_source(self):
 
         initial_context = {
             'autoadvance_enabled': False,
-            'branding_info': None,
             'license': None,
             'bumper_metadata': 'null',
             'block_id': str(self.block.location),
@@ -726,7 +722,6 @@ def test_get_html_with_mocked_edx_video_id(self):
 
         initial_context = {
             'autoadvance_enabled': False,
-            'branding_info': None,
             'license': None,
             'bumper_metadata': 'null',
             'block_id': str(self.block.location),
@@ -914,7 +909,6 @@ def helper_get_html_with_edx_video_id(self, data):
 
         initial_context = {
             'autoadvance_enabled': False,
-            'branding_info': None,
             'license': None,
             'bumper_metadata': 'null',
             'block_id': str(self.block.location),
@@ -971,21 +965,12 @@ def helper_get_html_with_edx_video_id(self, data):
         return context, expected_context
 
     # pylint: disable=invalid-name
-    @patch('xmodule.video_block.video_block.BrandingInfoConfig')
     @patch('xmodule.video_block.video_block.rewrite_video_url')
-    def test_get_html_cdn_source(self, mocked_get_video, mock_BrandingInfoConfig):
+    def test_get_html_cdn_source(self, mocked_get_video):
         """
         Test if sources got from CDN
         """
 
-        mock_BrandingInfoConfig.get_config.return_value = {
-            "CN": {
-                'url': 'http://www.xuetangx.com',
-                'logo_src': 'http://www.xuetangx.com/static/images/logo.png',
-                'logo_tag': 'Video hosted by XuetangX.com'
-            }
-        }
-
         def side_effect(*args, **kwargs):  # lint-amnesty, pylint: disable=unused-argument
             cdn = {
                 'http://example.com/example.mp4': 'http://cdn-example.com/example.mp4',
@@ -1031,11 +1016,6 @@ def side_effect(*args, **kwargs):  # lint-amnesty, pylint: disable=unused-argume
 
         initial_context = {
             'autoadvance_enabled': False,
-            'branding_info': {
-                'logo_src': 'http://www.xuetangx.com/static/images/logo.png',
-                'logo_tag': 'Video hosted by XuetangX.com',
-                'url': 'http://www.xuetangx.com'
-            },
             'license': None,
             'bumper_metadata': 'null',
             'block_id': str(self.block.location),
@@ -1138,7 +1118,6 @@ def test_get_html_cdn_source_external_video(self):
 
         initial_context = {
             'autoadvance_enabled': False,
-            'branding_info': None,
             'license': None,
             'bumper_metadata': 'null',
             'cdn_eval': False,
@@ -2380,7 +2359,6 @@ def test_bumper_metadata(self, get_url_for_profiles, get_bumper_settings, is_bum
 
         expected_context = {
             'autoadvance_enabled': False,
-            'branding_info': None,
             'license': None,
             'bumper_metadata': json.dumps(OrderedDict({
                 'saveStateUrl': self.block.ajax_url + '/save_user_state',
@@ -2480,7 +2458,6 @@ def prepare_expected_context(self, autoadvanceenabled_flag, autoadvance_flag):
 
         context = {
             'autoadvance_enabled': autoadvanceenabled_flag,
-            'branding_info': None,
             'block_id': str(self.block.location),
             'course_id': str(self.block.location.course_key),
             'license': None,

lms/djangoapps/grades/course_data.py

@@ -2,12 +2,12 @@
 Code used to get and cache the requested course-data
 """
 
-
 from lms.djangoapps.course_blocks.api import get_course_blocks
 from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager
 from xmodule.modulestore.django import modulestore  # lint-amnesty, pylint: disable=wrong-import-order
 
 from .transformer import GradesTransformer
+from ..course_api.blocks.utils import get_cached_transformed_blocks
 
 
 class CourseData:
@@ -56,7 +56,11 @@ def location(self):  # lint-amnesty, pylint: disable=missing-function-docstring
     @property
     def structure(self):  # lint-amnesty, pylint: disable=missing-function-docstring
         if self._structure is None:
-            self._structure = get_course_blocks(
+            # The get_course_blocks function proved to be a major time sink during a request at "blocks/".
+            # This caching logic helps improve the response time by getting a copy of the already transformed, but still
+            # unfiltered, course blocks from RequestCache and thus reducing the number of times that
+            # the get_course_blocks function is called.
+            self._structure = get_cached_transformed_blocks() or get_course_blocks(
                 self.user,
                 self.location,
                 collected_block_structure=self._collected_block_structure,