diff --git a/lms/djangoapps/course_api/blocks/api.py b/lms/djangoapps/course_api/blocks/api.py index 80c2bb7fc5ff..a79e9759e191 100644 --- a/lms/djangoapps/course_api/blocks/api.py +++ b/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,7 +14,6 @@ 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( @@ -30,7 +29,6 @@ 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. @@ -63,7 +61,6 @@ 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(): @@ -121,10 +118,6 @@ 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, @@ -135,19 +128,6 @@ 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 = [] @@ -162,7 +142,7 @@ def get_blocks( serializer_context = { 'request': request, 'block_structure': blocks, - 'requested_fields': requested_fields, + 'requested_fields': requested_fields or [], } if return_type == 'dict': diff --git a/lms/djangoapps/course_api/blocks/utils.py b/lms/djangoapps/course_api/blocks/utils.py index 0686abc2fac1..6f371624b7df 100644 --- a/lms/djangoapps/course_api/blocks/utils.py +++ b/lms/djangoapps/course_api/blocks/utils.py @@ -1,7 +1,6 @@ """ 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 ( @@ -10,10 +9,6 @@ ) -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. @@ -68,18 +63,3 @@ 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 diff --git a/lms/djangoapps/course_api/blocks/views.py b/lms/djangoapps/course_api/blocks/views.py index 7f81861b9b7b..96679a562957 100644 --- a/lms/djangoapps/course_api/blocks/views.py +++ b/lms/djangoapps/course_api/blocks/views.py @@ -2,7 +2,6 @@ CourseBlocks API views """ -from datetime import datetime, timezone from django.core.exceptions import ValidationError from django.db import transaction @@ -238,7 +237,6 @@ 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 @@ -341,50 +339,9 @@ 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) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 3b527cda4dfa..bbf9d5394705 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -26,7 +26,6 @@ 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 ( @@ -637,10 +636,7 @@ 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_cached_transformed_blocks() or get_course_blocks( - user, course_usage_key, allow_start_dates_in_future=True, include_completion=True - ) + block_data = get_course_blocks(user, course_usage_key, allow_start_dates_in_future=True, include_completion=True) now = datetime.now(pytz.UTC) assignments = [] diff --git a/lms/djangoapps/grades/course_data.py b/lms/djangoapps/grades/course_data.py index 523d6e6df38d..5464c4f88105 100644 --- a/lms/djangoapps/grades/course_data.py +++ b/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,11 +56,7 @@ 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: - # 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._structure = get_course_blocks( self.user, self.location, collected_block_structure=self._collected_block_structure, diff --git a/lms/djangoapps/mobile_api/tests/test_course_info_views.py b/lms/djangoapps/mobile_api/tests/test_course_info_views.py index b7cdc6b03961..efb3f7d9fdbb 100644 --- a/lms/djangoapps/mobile_api/tests/test_course_info_views.py +++ b/lms/djangoapps/mobile_api/tests/test_course_info_views.py @@ -432,78 +432,6 @@ def test_extend_sequential_info_with_assignment_progress_for_other_types(self, b for block_info in response.data['blocks'].values(): self.assertNotEqual('assignment_progress', block_info) - def test_response_keys(self): - response = self.verify_response(url=self.url) - data = response.data - - expected_top_level_keys = { - 'blocks', - 'certificate', - 'course_about', - 'course_access_details', - 'course_handouts', - 'course_modes', - 'course_progress', - 'course_sharing_utm_parameters', - 'course_updates', - 'deprecate_youtube', - 'discussion_url', - 'end', - 'enrollment_details', - 'id', - 'is_self_paced', - 'media', - 'name', - 'number', - 'org', - 'org_logo', - 'root', - 'start', - 'start_display', - 'start_type' - } - expected_course_access_keys = { - "has_unmet_prerequisites", - "is_too_early", - "is_staff", - "audit_access_expires", - "courseware_access" - } - expected_courseware_access_keys = { - "has_access", - "error_code", - "developer_message", - "user_message", - "additional_context_user_message", - "user_fragment" - } - expected_enrollment_details_keys = {"created", "mode", "is_active", "upgrade_deadline"} - expected_media_keys = {"image"} - expected_image_keys = {"raw", "small", "large"} - expected_course_sharing_keys = {"facebook", "twitter"} - expected_course_modes_keys = {"slug", "sku", "android_sku", "ios_sku", "min_price"} - expected_course_progress_keys = {"total_assignments_count", "assignments_completed"} - - self.assertSetEqual(set(data), expected_top_level_keys) - self.assertSetEqual(set(data["course_access_details"]), expected_course_access_keys) - self.assertSetEqual(set(data["course_access_details"]["courseware_access"]), expected_courseware_access_keys) - self.assertSetEqual(set(data["enrollment_details"]), expected_enrollment_details_keys) - self.assertSetEqual(set(data["media"]), expected_media_keys) - self.assertSetEqual(set(data["media"]["image"]), expected_image_keys) - self.assertSetEqual(set(data["course_sharing_utm_parameters"]), expected_course_sharing_keys) - self.assertSetEqual(set(data["course_modes"][0]), expected_course_modes_keys) - self.assertSetEqual(set(data["course_progress"]), expected_course_progress_keys) - - def test_block_count_depends_on_depth_in_request_params(self): - response_depth_zero = self.verify_response(url=self.url, params={'depth': 0}) - response_depth_one = self.verify_response(url=self.url, params={'depth': 1}) - blocks_depth_zero = [block for block in self.store.get_items(self.course_key) if block.category == "course"] - blocks_depth_one = [ - block for block in self.store.get_items(self.course_key) if block.category in ("course", "chapter") - ] - self.assertEqual(len(response_depth_zero.data["blocks"]), len(blocks_depth_zero)) - self.assertEqual(len(response_depth_one.data["blocks"]), len(blocks_depth_one)) - class TestCourseEnrollmentDetailsView(MobileAPITestCase, MilestonesTestCaseMixin): # lint-amnesty, pylint: disable=test-inherits-tests """