From 5365d8812dd13aa0d6f0d54746fe8a47cdd7996b Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Wed, 21 Oct 2026 15:16:30 +0500 Subject: [PATCH 1/7] feat: call filters for learner block binds and course API responses Run new openedx-filters hooks so plugins can adjust how course content is presented to a learner on each request: - LearnerXBlockBound at the end of get_block_for_descriptor, after the block is bound and its access checked (renders, children, handlers, sequence metadata, course pages). - CourseBlocksTransformCompleted on the transformed block structure in get_course_blocks (a per-request copy). - CourseOutlineAPIRenderStarted, CourseHomeMetadataAPIRenderStarted and CoursewareMetadataAPIRenderStarted on the serialized API responses. The navigation sidebar cache key now includes the active request language, since block structure filter steps can vary block data by it. Co-Authored-By: Claude Opus 5.5 (1M context) --- lms/djangoapps/course_blocks/api.py | 6 +++++- .../course_home_api/course_metadata/views.py | 6 +++++- .../course_home_api/outline/views.py | 6 +++++- lms/djangoapps/courseware/block_render.py | 21 +++++++++++++++++-- .../courseware/tests/test_block_render.py | 20 ++++++++++++++++++ .../content/learning_sequences/views.py | 4 +++- .../core/djangoapps/courseware_api/views.py | 11 ++++++++++ 7 files changed, 68 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/course_blocks/api.py b/lms/djangoapps/course_blocks/api.py index de4e0443fb08..65b61b909a9f 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -6,6 +6,7 @@ from django.conf import settings from edx_when import field_data +from openedx_filters.learning.filters import CourseBlocksTransformCompleted from lms.djangoapps.course_api.blocks.transformers.block_completion import BlockCompletionTransformer from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager @@ -104,9 +105,12 @@ def get_course_blocks( include_has_scheduled_content ) - return get_block_structure_manager(starting_block_usage_key.course_key).get_transformed( + block_structure = get_block_structure_manager(starting_block_usage_key.course_key).get_transformed( transformers, starting_block_usage_key, collected_block_structure, user, ) + return CourseBlocksTransformCompleted.run_filter( + block_structure=block_structure, user=user, starting_block_usage_key=starting_block_usage_key + ) diff --git a/lms/djangoapps/course_home_api/course_metadata/views.py b/lms/djangoapps/course_home_api/course_metadata/views.py index f6f15203260e..2cdc67f69501 100644 --- a/lms/djangoapps/course_home_api/course_metadata/views.py +++ b/lms/djangoapps/course_home_api/course_metadata/views.py @@ -7,6 +7,7 @@ from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from opaque_keys.edx.keys import CourseKey +from openedx_filters.learning.filters import CourseHomeMetadataAPIRenderStarted from rest_framework.exceptions import PermissionDenied from rest_framework.generics import RetrieveAPIView from rest_framework.response import Response @@ -161,4 +162,7 @@ def get(self, request, *args, **kwargs): context['course_overview'] = course context['enrollment'] = enrollment serializer = self.get_serializer_class()(data, context=context) - return Response(serializer.data) + data = CourseHomeMetadataAPIRenderStarted.run_filter( + course_key=course_key, user=request.user, data=serializer.data + ) + return Response(data) diff --git a/lms/djangoapps/course_home_api/outline/views.py b/lms/djangoapps/course_home_api/outline/views.py index c50b2eed5e4e..51279195f3fe 100644 --- a/lms/djangoapps/course_home_api/outline/views.py +++ b/lms/djangoapps/course_home_api/outline/views.py @@ -11,6 +11,7 @@ from django.core.cache import cache from django.shortcuts import get_object_or_404 # pylint: disable=wrong-import-order from django.urls import reverse # pylint: disable=wrong-import-order +from django.utils.translation import get_language # pylint: disable=wrong-import-order from django.utils.translation import gettext as _ # pylint: disable=wrong-import-order from edx_django_utils import monitoring as monitoring_utils # pylint: disable=wrong-import-order from edx_rest_framework_extensions.auth.jwt.authentication import ( @@ -436,7 +437,7 @@ class CourseNavigationBlocksView(RetrieveAPIView): serializer_class = CourseBlockSerializer COURSE_BLOCKS_CACHE_KEY_TEMPLATE = ( 'course_sidebar_blocks_{course_key_string}_{block_structure_version}_{user_id}_{user_cohort_id}' - '_{enrollment_mode}_{allow_public}_{allow_public_outline}_{is_masquerading}' + '_{enrollment_mode}_{allow_public}_{allow_public_outline}_{is_masquerading}_{language}' ) COURSE_BLOCKS_CACHE_TIMEOUT = 60 * 60 # 1 hour @@ -477,6 +478,9 @@ def get(self, request, *args, **kwargs): allow_public=allow_public, allow_public_outline=allow_public_outline, is_masquerading=user_is_masquerading, + # CourseBlocksTransformCompleted steps can vary block data by the request's + # active language, so the cached response must not be shared across languages. + language=get_language(), ) if navigation_sidebar_caching_is_disabled := courseware_disable_navigation_sidebar_blocks_caching(): course_blocks = None diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 8ef637b82bf0..d6e55d0b9449 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -33,6 +33,7 @@ from eventtracking import tracker from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx_filters.learning.filters import LearnerXBlockBound from rest_framework.decorators import api_view from rest_framework.exceptions import APIException from web_fragments.fragment import Fragment @@ -461,6 +462,9 @@ def get_block_for_descriptor( block.runtime.set('position', position) + # bind_for_student is a no-op for a block already bound to this user, which + # has then already been through LearnerXBlockBound. + already_bound = block.scope_ids.user_id is not None and block.scope_ids.user_id == user.id block.bind_for_student( user.id, [ @@ -487,9 +491,22 @@ def get_block_for_descriptor( ) if access or caller_will_handle_access_error: block.has_access_error = bool(caller_will_handle_access_error) - return block + return _run_learner_xblock_bound(block, user, course_key, already_bound) return None - return block + return _run_learner_xblock_bound(block, user, course_key, already_bound) + + +def _run_learner_xblock_bound(block, user, course_key, already_bound): + """ + Run the LearnerXBlockBound filter once per binding, for known users only. + + Unknown users are block structure collection (whose result is cached and + shared by every learner) and no-auth callbacks, so their blocks are left as + authored. + """ + if already_bound or not getattr(user, 'known', True): + return block + return LearnerXBlockBound.run_filter(block=block, user=user, course_key=course_key) def prepare_runtime_for_user( diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index c58283a3d857..ec81b548a2a2 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -476,6 +476,26 @@ def test_rebinding_same_user(self, block_type): course=self.toy_course ) + @ddt.data(True, False) + @patch( + 'lms.djangoapps.courseware.block_render.LearnerXBlockBound.run_filter', + side_effect=lambda **kwargs: kwargs['block'], + ) + def test_learner_xblock_bound_filter(self, known, run_filter): + """The bound filter runs once per binding, and only for known users.""" + request = self.request_factory.get('') + request.user = self.mock_user + self.mock_user.known = known + block = BlockFactory(category='html', parent=CourseFactory()) + field_data_cache = FieldDataCache([self.toy_course, block], self.toy_course.id, self.mock_user) + + for _ in range(2): + render.get_block_for_descriptor( + self.mock_user, request, block, field_data_cache, self.toy_course.id, course=self.toy_course + ) + + assert run_filter.call_count == (1 if known else 0) + @override_settings(FIELD_OVERRIDE_PROVIDERS=( 'lms.djangoapps.courseware.student_field_overrides.IndividualStudentOverrideProvider', )) diff --git a/openedx/core/djangoapps/content/learning_sequences/views.py b/openedx/core/djangoapps/content/learning_sequences/views.py index e1cee434df89..b86df22daa3b 100644 --- a/openedx/core/djangoapps/content/learning_sequences/views.py +++ b/openedx/core/djangoapps/content/learning_sequences/views.py @@ -11,6 +11,7 @@ from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from opaque_keys.edx.keys import CourseKey +from openedx_filters.learning.filters import CourseOutlineAPIRenderStarted from rest_framework import serializers from rest_framework.exceptions import NotAuthenticated, NotFound, PermissionDenied from rest_framework.response import Response @@ -178,7 +179,8 @@ def get(self, request, course_key_str, format=None): # pylint: disable=redefine raise NotFound() from does_not_exist_err serializer = self.UserCourseOutlineDataSerializer(user_course_outline_details) - return Response(serializer.data) + data = CourseOutlineAPIRenderStarted.run_filter(course_key=course_key, user=request.user, data=serializer.data) + return Response(data) def _determine_user(self, request, course_key: CourseKey) -> types.User: """ diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index 1b0bd0c50f5f..737926e3ce41 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -12,6 +12,7 @@ from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx_filters.learning.filters import CoursewareMetadataAPIRenderStarted from rest_framework import status from rest_framework.exceptions import NotFound from rest_framework.generics import RetrieveAPIView @@ -697,6 +698,16 @@ def set_last_seen_courseware_timezone(self, user): defaults={'last_seen_courseware_timezone': browser_timezone}, ) + def retrieve(self, request, *args, **kwargs): + """ + Return the serialized course, after the CoursewareMetadataAPIRenderStarted filter. + """ + response = super().retrieve(request, *args, **kwargs) + response.data = CoursewareMetadataAPIRenderStarted.run_filter( + course_key=CourseKey.from_string(self.kwargs['course_key_string']), user=request.user, data=response.data + ) + return response + def get_object(self): """ Return the requested course object, if the user has appropriate From b724e8efb4522fcca9b2c73cc614c7289e7b5d48 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Wed, 21 Oct 2026 16:08:58 +0500 Subject: [PATCH 2/7] fix: run LearnerXBlockBound once per block and user, including blocks first bound while unknown Co-Authored-By: Claude Opus 5.5 (1M context) --- lms/djangoapps/courseware/block_render.py | 20 +++++++++---------- .../courseware/tests/test_block_render.py | 17 ++++++++++------ 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index d6e55d0b9449..a88099063004 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -462,9 +462,6 @@ def get_block_for_descriptor( block.runtime.set('position', position) - # bind_for_student is a no-op for a block already bound to this user, which - # has then already been through LearnerXBlockBound. - already_bound = block.scope_ids.user_id is not None and block.scope_ids.user_id == user.id block.bind_for_student( user.id, [ @@ -491,22 +488,25 @@ def get_block_for_descriptor( ) if access or caller_will_handle_access_error: block.has_access_error = bool(caller_will_handle_access_error) - return _run_learner_xblock_bound(block, user, course_key, already_bound) + return _run_learner_xblock_bound(block, user, course_key) return None - return _run_learner_xblock_bound(block, user, course_key, already_bound) + return _run_learner_xblock_bound(block, user, course_key) -def _run_learner_xblock_bound(block, user, course_key, already_bound): +def _run_learner_xblock_bound(block, user, course_key): """ - Run the LearnerXBlockBound filter once per binding, for known users only. + Run the LearnerXBlockBound filter once per block and user, for known users only. Unknown users are block structure collection (whose result is cached and shared by every learner) and no-auth callbacks, so their blocks are left as - authored. + authored. A block that was bound to this user while unknown is filtered the + first time it is returned to the known user. """ - if already_bound or not getattr(user, 'known', True): + if not getattr(user, 'known', True) or getattr(block, '_learner_xblock_bound_user_id', None) == user.id: return block - return LearnerXBlockBound.run_filter(block=block, user=user, course_key=course_key) + block = LearnerXBlockBound.run_filter(block=block, user=user, course_key=course_key) + block._learner_xblock_bound_user_id = user.id # pylint: disable=protected-access + return block def prepare_runtime_for_user( diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index ec81b548a2a2..3e7a92a676ad 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -476,25 +476,30 @@ def test_rebinding_same_user(self, block_type): course=self.toy_course ) - @ddt.data(True, False) + @ddt.data( + ([True, True], 1), + ([False, False], 0), + ([False, True, True], 1), + ) + @ddt.unpack @patch( 'lms.djangoapps.courseware.block_render.LearnerXBlockBound.run_filter', side_effect=lambda **kwargs: kwargs['block'], ) - def test_learner_xblock_bound_filter(self, known, run_filter): - """The bound filter runs once per binding, and only for known users.""" + def test_learner_xblock_bound_filter(self, known_per_call, expected_calls, run_filter): + """The bound filter runs once per block and user, and only for known users.""" request = self.request_factory.get('') request.user = self.mock_user - self.mock_user.known = known block = BlockFactory(category='html', parent=CourseFactory()) field_data_cache = FieldDataCache([self.toy_course, block], self.toy_course.id, self.mock_user) - for _ in range(2): + for known in known_per_call: + self.mock_user.known = known render.get_block_for_descriptor( self.mock_user, request, block, field_data_cache, self.toy_course.id, course=self.toy_course ) - assert run_filter.call_count == (1 if known else 0) + assert run_filter.call_count == expected_calls @override_settings(FIELD_OVERRIDE_PROVIDERS=( 'lms.djangoapps.courseware.student_field_overrides.IndividualStudentOverrideProvider', From aee6e4f5865b688791c7ea330bd9d0c86c06d75b Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Wed, 21 Oct 2026 17:09:51 +0500 Subject: [PATCH 3/7] fix: build shared course data from fresh block instances Block structure collection and course overviews are shared by every user, but they could read block instances that the current request had already loaded and bound to a user (and that LearnerXBlockBound steps may have changed in memory), or load children through a runtime prepared for that user. - Add a fresh_block_reads(course_key) modulestore context manager: a no-op by default, delegated by the mixed and CCX stores, and in split it sets aside the bulk operation's cached blocks and the request's runtimes. - Use it in BlockStructureManager._update_collected and CourseOverview.load_from_module_store. - Restore user.known in a finally block after collection. - Run LearnerXBlockBound for anonymous users too (their id is None). Co-Authored-By: Claude Opus 5.5 (1M context) --- lms/djangoapps/ccx/modulestore.py | 7 ++ .../ccx/tests/test_ccx_modulestore.py | 10 +++ lms/djangoapps/courseware/block_render.py | 9 ++- .../courseware/tests/test_block_render.py | 73 ++++++++++++++++++- .../content/block_structure/manager.py | 30 ++++---- .../content/block_structure/tests/helpers.py | 7 ++ .../block_structure/tests/test_manager.py | 27 +++++++ .../content/course_overviews/models.py | 4 +- .../tests/test_course_overviews.py | 22 ++++++ xmodule/modulestore/__init__.py | 10 +++ xmodule/modulestore/mixed.py | 10 +++ xmodule/modulestore/split_mongo/split.py | 48 ++++++++++-- 12 files changed, 235 insertions(+), 22 deletions(-) diff --git a/lms/djangoapps/ccx/modulestore.py b/lms/djangoapps/ccx/modulestore.py index d47a8396084f..b2cb414960bf 100644 --- a/lms/djangoapps/ccx/modulestore.py +++ b/lms/djangoapps/ccx/modulestore.py @@ -304,3 +304,10 @@ def bulk_operations(self, course_id, emit_signals=True, ignore_case=False): course_id, _ = strip_ccx(course_id) with self._modulestore.bulk_operations(course_id, emit_signals=emit_signals, ignore_case=ignore_case): yield + + @contextmanager + def fresh_block_reads(self, course_key): + """See the docs for xmodule.modulestore.mixed.MixedModuleStore""" + course_key, _ = strip_ccx(course_key) + with self._modulestore.fresh_block_reads(course_key): + yield diff --git a/lms/djangoapps/ccx/tests/test_ccx_modulestore.py b/lms/djangoapps/ccx/tests/test_ccx_modulestore.py index b3b36a96dc9a..ed94bac90296 100644 --- a/lms/djangoapps/ccx/tests/test_ccx_modulestore.py +++ b/lms/djangoapps/ccx/tests/test_ccx_modulestore.py @@ -126,6 +126,16 @@ def test_get_item(self): assert expected.display_name == actual.display_name assert expected.location == actual.location.to_block_locator() + def test_fresh_block_reads(self): + """verify that fresh block reads by ccx key ignore blocks changed in memory""" + expected = self.blocks[0] + block_key = self.ccx_locator.make_usage_key(expected.location.block_type, expected.location.block_id) + with self.store.bulk_operations(self.ccx_locator): + self.store.get_item(block_key).display_name = 'Changed in memory' + with self.store.fresh_block_reads(self.ccx_locator): + assert self.store.get_item(block_key).display_name == expected.display_name + assert self.store.get_item(block_key).display_name == 'Changed in memory' + def test_publication_api(self): """verify that we can correctly discern a published item by ccx key""" with self.store.bulk_operations(self.ccx_locator): diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index a88099063004..db81a6a0ce7a 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -117,6 +117,9 @@ log = logging.getLogger(__name__) +# Marks a block that LearnerXBlockBound has not run on yet (anonymous users have no id). +_NOT_FILTERED = object() + # TODO: course_id and course_key are used interchangeably in this file, which is wrong. # Some brave person should make the variable names consistently someday, but the code's # coupled enough that it's kind of tricky--you've been warned! @@ -495,14 +498,16 @@ def get_block_for_descriptor( def _run_learner_xblock_bound(block, user, course_key): """ - Run the LearnerXBlockBound filter once per block and user, for known users only. + Run the LearnerXBlockBound filter once each time a block is bound to a known user. + + Binding the same block instance to a different user runs it again. Unknown users are block structure collection (whose result is cached and shared by every learner) and no-auth callbacks, so their blocks are left as authored. A block that was bound to this user while unknown is filtered the first time it is returned to the known user. """ - if not getattr(user, 'known', True) or getattr(block, '_learner_xblock_bound_user_id', None) == user.id: + if not getattr(user, 'known', True) or getattr(block, '_learner_xblock_bound_user_id', _NOT_FILTERED) == user.id: return block block = LearnerXBlockBound.run_filter(block=block, user=user, course_key=course_key) block._learner_xblock_bound_user_id = user.id # pylint: disable=protected-access diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index 3e7a92a676ad..57de5b50aceb 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -1,6 +1,7 @@ """ Test for lms courseware app, block render unit """ +import contextlib import json import textwrap import warnings @@ -87,6 +88,8 @@ from lms.djangoapps.courseware.tests.tests import LoginEnrollmentTestCase from lms.djangoapps.lms_xblock.field_data import LmsFieldData from lms.djangoapps.verify_student.tests.factories import SoftwareSecurePhotoVerificationFactory +from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager +from openedx.core.djangoapps.content.block_structure.exceptions import BlockStructureNotFound from openedx.core.djangoapps.credit.api import set_credit_requirement_status, set_credit_requirements from openedx.core.djangoapps.credit.models import CreditCourse from openedx.core.djangoapps.oauth_dispatch.jwt import _create_jwt, create_jwt_for_user @@ -487,7 +490,7 @@ def test_rebinding_same_user(self, block_type): side_effect=lambda **kwargs: kwargs['block'], ) def test_learner_xblock_bound_filter(self, known_per_call, expected_calls, run_filter): - """The bound filter runs once per block and user, and only for known users.""" + """The bound filter runs once per binding to a known user, and not for unknown users.""" request = self.request_factory.get('') request.user = self.mock_user block = BlockFactory(category='html', parent=CourseFactory()) @@ -501,6 +504,74 @@ def test_learner_xblock_bound_filter(self, known_per_call, expected_calls, run_f assert run_filter.call_count == expected_calls + @patch( + 'lms.djangoapps.courseware.block_render.LearnerXBlockBound.run_filter', + side_effect=lambda **kwargs: kwargs['block'], + ) + def test_learner_xblock_bound_changes_are_not_collected(self, run_filter): + """ + Blocks changed by the bound filter, including children loaded later through the + learner's runtime, are not collected into the block structure shared by every user. + """ + def change_display_name(block, **kwargs): # pylint: disable=unused-argument + block.fields['display_name']._set_cached_value(block, 'Changed') # pylint: disable=protected-access + return block + + run_filter.side_effect = change_display_name + course = CourseFactory.create() + chapter = BlockFactory.create(parent=course, category='chapter', display_name='Authored') + BlockFactory.create(parent=chapter, category='sequential', display_name='Authored') + request = self.request_factory.get('') + request.user = self.mock_user + store = modulestore() + manager = get_block_structure_manager(course.id) + with contextlib.suppress(BlockStructureNotFound): + manager.clear() + + with store.bulk_operations(course.id): + course_block = store.get_course(course.id, depth=None) + field_data_cache = FieldDataCache([course_block], course.id, self.mock_user) + bound = render.get_block_for_descriptor( + self.mock_user, request, course_block, field_data_cache, course.id, course=course_block + ) + assert bound.display_name == 'Changed' + collected = manager.get_collected() + + assert 'Changed' not in {collected.get_xblock_field(key, 'display_name') for key in collected} + + @patch( + 'lms.djangoapps.courseware.block_render.LearnerXBlockBound.run_filter', + side_effect=lambda **kwargs: kwargs['block'], + ) + def test_learner_xblock_bound_filter_runs_again_for_another_user(self, run_filter): + """Binding the same block to a different user runs the bound filter again.""" + block = BlockFactory(category='html', parent=CourseFactory()) + other_user = UserFactory() + + for user in (self.mock_user, other_user): + request = self.request_factory.get('') + request.user = user + field_data_cache = FieldDataCache([self.toy_course, block], self.toy_course.id, user) + render.get_block_for_descriptor( + user, request, block, field_data_cache, self.toy_course.id, course=self.toy_course + ) + + assert [call.kwargs['user'] for call in run_filter.call_args_list] == [self.mock_user, other_user] + + @patch( + 'lms.djangoapps.courseware.block_render.LearnerXBlockBound.run_filter', + side_effect=lambda **kwargs: kwargs['block'], + ) + def test_learner_xblock_bound_filter_anonymous_user(self, run_filter): + """The bound filter runs once for an anonymous user, whose id is None.""" + block = BlockFactory(category='html', parent=CourseFactory()) + run_bound_filter = render._run_learner_xblock_bound # pylint: disable=protected-access + + for _ in range(2): + run_bound_filter(block, AnonymousUser(), self.toy_course.id) + + assert run_filter.call_count == 1 + @override_settings(FIELD_OVERRIDE_PROVIDERS=( 'lms.djangoapps.courseware.student_field_overrides.IndividualStudentOverrideProvider', )) diff --git a/openedx/core/djangoapps/content/block_structure/manager.py b/openedx/core/djangoapps/content/block_structure/manager.py index 5335182ba6e8..d260c057985e 100644 --- a/openedx/core/djangoapps/content/block_structure/manager.py +++ b/openedx/core/djangoapps/content/block_structure/manager.py @@ -112,8 +112,10 @@ def get_collected(self, user=None): # we do not want to perform access checks. Access checks result in inconsistent blocks where # inaccessible blocks are missing from the cache. Cached course blocks are then used for all the users. user.known = False - block_structure = self._update_collected() - user.known = True + try: + block_structure = self._update_collected() + finally: + user.known = True else: block_structure = self._update_collected() @@ -133,18 +135,20 @@ def _update_collected(self): The store is updated with newly collected transformers data from the modulestore. """ + course_key = self.root_block_usage_key.course_key with self._bulk_operations(): - # Always uses published-only branch regardless of CMS or LMS context. - with self.modulestore.branch_setting( - ModuleStoreEnum.Branch.published_only, - self.root_block_usage_key.course_key - ): - block_structure = BlockStructureFactory.create_from_modulestore( - self.root_block_usage_key, - self.modulestore, - ) - - BlockStructureTransformers.collect(block_structure) + # The collected structure is cached and shared by every user, so it is + # never read from block instances this request has already loaded, + # which may have been changed in memory for one user. + with self.modulestore.fresh_block_reads(course_key): + # Always uses published-only branch regardless of CMS or LMS context. + with self.modulestore.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + block_structure = BlockStructureFactory.create_from_modulestore( + self.root_block_usage_key, + self.modulestore, + ) + + BlockStructureTransformers.collect(block_structure) self.store.add(block_structure) return block_structure diff --git a/openedx/core/djangoapps/content/block_structure/tests/helpers.py b/openedx/core/djangoapps/content/block_structure/tests/helpers.py index 30899a87f398..4ba2cf539c07 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/helpers.py +++ b/openedx/core/djangoapps/content/block_structure/tests/helpers.py @@ -117,6 +117,13 @@ def branch_setting(self, branch_settings, course_id=None): # pylint: disable=un """ yield + @contextmanager + def fresh_block_reads(self, course_key): # pylint: disable=unused-argument + """ + A context manager for loading blocks as new instances. + """ + yield + class MockCache: """ diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py index 04a08accf6b3..f5e3b2a8a086 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py @@ -9,6 +9,9 @@ from django.test import TestCase from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory from ..block_structure import BlockStructureBlockData from ..exceptions import UsageKeyNotInBlockStructure @@ -254,3 +257,27 @@ def mock_branch_setting(branch, course_key): setattr(self.modulestore, attr_name, original_branch_setting) elif hasattr(self.modulestore, attr_name): delattr(self.modulestore, attr_name) + + +class TestBlockStructureManagerCollection(ModuleStoreTestCase): + """ + Tests for collecting block structures from the modulestore. + """ + + def test_collect_ignores_blocks_changed_in_memory(self): + """ + Block instances loaded and changed in memory earlier in a bulk operation are not + collected into the block structure, which is cached and shared by every user. + """ + course = CourseFactory.create() + chapter = BlockFactory.create(parent=course, category='chapter', display_name='Authored') + store = modulestore() + manager = BlockStructureManager(course.location, store, MockCache()) + + with store.bulk_operations(course.id): + loaded = store.get_course(course.id, depth=None).get_children()[0] + loaded.display_name = 'Changed in memory' + collected = manager.get_collected() + assert store.get_item(chapter.location).display_name == 'Changed in memory' + + assert collected.get_xblock_field(chapter.location, 'display_name') == 'Authored' diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 61dd8898755c..dae0103a6d4c 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -312,7 +312,9 @@ def load_from_module_store(cls, course_id): course_id, ) store = modulestore() - with store.bulk_operations(course_id): + # The overview is shared by every user, so it is never built from blocks this + # request has already loaded and may have changed in memory. + with store.bulk_operations(course_id), store.fresh_block_reads(course_id): course = store.get_course(course_id) if isinstance(course, CourseBlock): try: diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py index 95c9e2df965c..b4c5081335a5 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py @@ -1281,3 +1281,25 @@ def test_tabs_deletion_rollback_on_integrity_error(self): actual_tabs = {tab.tab_id for tab in course_overview.tab_set.all()} assert actual_tabs == expected_tabs assert course_overview.display_name != course.display_name + + +class CourseOverviewFreshReadTestCase(ModuleStoreTestCase): + """ + Tests that course overviews are built from the course as authored. + """ + + def test_load_ignores_course_changed_in_memory(self): + """ + A course instance loaded and changed in memory earlier in a bulk operation is not + saved into the course overview, which is shared by every user. + """ + course = CourseFactory.create(display_name='Authored') + store = modulestore() + + with store.bulk_operations(course.id): + loaded = store.get_course(course.id) + loaded.fields['display_name']._set_cached_value(loaded, 'Changed') # pylint: disable=protected-access + assert store.get_course(course.id).display_name == 'Changed' + overview = CourseOverview.load_from_module_store(course.id) + + assert overview.display_name == 'Authored' diff --git a/xmodule/modulestore/__init__.py b/xmodule/modulestore/__init__.py index 79496eb55e16..d11c7734f5f1 100644 --- a/xmodule/modulestore/__init__.py +++ b/xmodule/modulestore/__init__.py @@ -1039,6 +1039,16 @@ def bulk_operations(self, course_id, emit_signals=True, ignore_case=False): # """ yield + @contextmanager + def fresh_block_reads(self, course_key): # pylint: disable=unused-argument + """ + A context manager for loading the course's blocks as new instances, ignoring block + instances that this thread has already loaded (and may have changed in memory). + + The default is a no-op, for stores that do not reuse loaded block instances. + """ + yield + def ensure_indexes(self): """ Ensure that all appropriate indexes are created that are needed by this modulestore, or raise diff --git a/xmodule/modulestore/mixed.py b/xmodule/modulestore/mixed.py index 2a3a04699a79..9dfb09678421 100644 --- a/xmodule/modulestore/mixed.py +++ b/xmodule/modulestore/mixed.py @@ -1113,6 +1113,16 @@ def bulk_operations(self, course_id, emit_signals=True, ignore_case=False): with store.bulk_operations(course_id, emit_signals, ignore_case): yield + @contextmanager + def fresh_block_reads(self, course_key): + """ + A context manager for loading the course's blocks as new instances, ignoring block + instances that this thread has already loaded (and may have changed in memory). + """ + store = self._get_modulestore_for_courselike(course_key) + with store.fresh_block_reads(course_key): + yield + def ensure_indexes(self): """ Ensure that all appropriate indexes are created that are needed by this modulestore, or raise diff --git a/xmodule/modulestore/split_mongo/split.py b/xmodule/modulestore/split_mongo/split.py index 5a115dd16176..8687af36c0ed 100644 --- a/xmodule/modulestore/split_mongo/split.py +++ b/xmodule/modulestore/split_mongo/split.py @@ -59,6 +59,7 @@ import datetime import logging from collections import defaultdict +from contextlib import contextmanager from importlib import import_module from zoneinfo import ZoneInfo @@ -222,15 +223,52 @@ def _clear_bulk_ops_record(self, course_key): """ Clear the record for this course """ + del self._active_bulk_ops.records[self._bulk_ops_record_key(course_key)] + + def _bulk_ops_record_key(self, course_key): + """ + Return the key of this course's record in the active bulk operations. + """ if not isinstance(course_key, (CourseLocator, LibraryLocator)): raise TypeError(f'{course_key!r} is not a CourseLocator or LibraryLocator') if course_key.org and get_library_or_course_attribute(course_key) and course_key.run: - del self._active_bulk_ops.records[course_key.replace(branch=None, version_guid=None)] - else: - del self._active_bulk_ops.records[ - course_key.replace(org=None, course=None, run=None, branch=None) - ] + return course_key.replace(branch=None, version_guid=None) + return course_key.replace(org=None, course=None, run=None, branch=None) + + @contextmanager + def fresh_block_reads(self, course_key): + """ + A context manager for loading the course's blocks as new instances, ignoring block + instances that this thread has already loaded (and may have changed in memory). + + Set aside, and restored on exit: + + * the blocks cached by the course's active bulk operation, which hand back + already-loaded instances; + * the request's runtimes (for every course), which may have been prepared for a + user and then bind each child block they load to that user. + + Cached structures and definitions are still used. Meant for reads: blocks cached + by writes made inside the context are dropped on exit. + """ + # Looked up without indexing, which would add a record to the defaultdict. + bulk_ops_record = self._active_bulk_ops.records.get(self._bulk_ops_record_key(course_key)) + cache_data = self.request_cache.data if self.request_cache is not None else {} + saved_modules = bulk_ops_record.modules if bulk_ops_record is not None else None + saved_runtimes = cache_data.get('course_cache') + if bulk_ops_record is not None: + bulk_ops_record.modules = defaultdict(dict) + cache_data['course_cache'] = {} + try: + yield + finally: + if bulk_ops_record is not None: + bulk_ops_record.modules = saved_modules + if saved_runtimes is None: + cache_data.pop('course_cache', None) + else: + cache_data['course_cache'] = saved_runtimes def _start_outermost_bulk_operation(self, bulk_write_record, course_key, ignore_case=False): # pylint: disable=arguments-differ """ From 0d2503fe7b1c9528c6f688c57cdd9d802f6b58ff Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Wed, 21 Oct 2026 17:28:05 +0500 Subject: [PATCH 4/7] test: cover the new filters with configured pipelines Co-Authored-By: Claude Opus 5.5 (1M context) --- .../course_blocks/tests/test_api.py | 50 ++++++++++++++++++ .../course_metadata/tests/test_views.py | 37 +++++++++++++ .../outline/tests/test_view.py | 45 ++++++++++++++++ .../learning_sequences/tests/test_views.py | 52 +++++++++++++++++++ .../courseware_api/tests/test_views.py | 38 ++++++++++++++ 5 files changed, 222 insertions(+) diff --git a/lms/djangoapps/course_blocks/tests/test_api.py b/lms/djangoapps/course_blocks/tests/test_api.py index 3643c520b5a4..53bbacf169f2 100644 --- a/lms/djangoapps/course_blocks/tests/test_api.py +++ b/lms/djangoapps/course_blocks/tests/test_api.py @@ -7,16 +7,33 @@ import ddt from django.http.request import HttpRequest +from django.test import override_settings +from openedx_filters import PipelineStep from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.course_blocks.api import get_course_blocks +from lms.djangoapps.course_blocks.transformers.load_override_data import OverrideDataTransformer from lms.djangoapps.course_blocks.transformers.tests.helpers import CourseStructureTestCase from lms.djangoapps.course_blocks.transformers.tests.test_user_partitions import UserPartitionTestMixin from lms.djangoapps.courseware.block_render import make_track_function, prepare_runtime_for_user +from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort from xmodule.modulestore.django import modulestore +FILTERED_USERNAME = 'filtered_user' + + +class TestOverrideDisplayNameStep(PipelineStep): + """ + Pipeline step that overrides the course display name for a single user. + """ + + def run_filter(self, block_structure, user, starting_block_usage_key): # pylint: disable=arguments-differ + if user.username == FILTERED_USERNAME: + block_structure.override_xblock_field(starting_block_usage_key, 'display_name', 'Changed') + return {'block_structure': block_structure} + def get_block_side_effect(block_locator, user_known): """ @@ -147,3 +164,36 @@ def test_get_course_blocks(self, group_id, expected_blocks, user_known): set(block_structure.get_block_keys()), self.get_block_key_set(self.blocks, *expected_blocks) ) + + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + 'org.openedx.learning.course_blocks.transform.completed.v1': { + 'pipeline': ['lms.djangoapps.course_blocks.tests.test_api.TestOverrideDisplayNameStep'], + 'fail_silently': False, + }, + }, + ) + # Register a transformer that collects display_name so the collected structure carries the authored value. + @patch( + 'openedx.core.djangoapps.content.block_structure.transformer_registry.' + 'TransformerRegistry.get_registered_transformers', + Mock(return_value={OverrideDataTransformer}), + ) + def test_filter_overrides_do_not_leak_into_collected_structure(self): + """ + Tests that changes made by the transform completed filter are returned on cold and warm caches + without modifying the collected block structure or the blocks returned to other users. + """ + self.setup_partitions_and_course() + root_key = self.course.location + authored_name = self.course.display_name + filtered_user = UserFactory.create(username=FILTERED_USERNAME) + + for _ in range(2): # the first call collects the structure, the second reads it from the cache + block_structure = get_course_blocks(filtered_user, root_key, BlockStructureTransformers([])) + assert block_structure.get_xblock_field(root_key, 'display_name') == 'Changed' + + collected = get_block_structure_manager(self.course.id).get_collected() + assert collected.get_xblock_field(root_key, 'display_name') == authored_name + block_structure = get_course_blocks(self.user, root_key, BlockStructureTransformers([])) + assert block_structure.get_xblock_field(root_key, 'display_name') == authored_name diff --git a/lms/djangoapps/course_home_api/course_metadata/tests/test_views.py b/lms/djangoapps/course_home_api/course_metadata/tests/test_views.py index 2d0ac7b41b7f..a723aaaed833 100644 --- a/lms/djangoapps/course_home_api/course_metadata/tests/test_views.py +++ b/lms/djangoapps/course_home_api/course_metadata/tests/test_views.py @@ -9,6 +9,7 @@ from django.test.utils import override_settings from django.urls import reverse from edx_toggles.toggles.testutils import override_waffle_flag +from openedx_filters import PipelineStep from openedx_filters.learning.filters import CoursewareAccessChecksRequested from common.djangoapps.course_modes.models import CourseMode @@ -28,6 +29,19 @@ ) from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration +FILTERED_TABS = [{'tab_id': 'filtered', 'title': 'Filtered tab', 'url': 'https://example.com/filtered'}] +filter_users = [] + + +class TestReplaceMetadataStep(PipelineStep): + """ + Pipeline step that records the user it receives and replaces the title and tabs. + """ + + def run_filter(self, course_key, user, data): # pylint: disable=arguments-differ + filter_users.append(user) + return {'data': {**data, 'title': f'Filtered {course_key.course}', 'tabs': FILTERED_TABS}} + @ddt.ddt @override_waffle_flag(COURSEWARE_MICROFRONTEND_PROGRESS_MILESTONES, active=True) @@ -87,6 +101,29 @@ def test_get_masqueraded_user(self): self.update_masquerade(username=self.user.username) assert self.client.get(self.url).data['username'] == self.user.username + @override_settings(OPEN_EDX_FILTERS_CONFIG={ + 'org.openedx.learning.course_home_metadata.api.render.started.v1': { + 'pipeline': [f'{__name__}.TestReplaceMetadataStep'], + 'fail_silently': False, + }, + }) + def test_metadata_filter_replaces_data_for_masqueraded_user(self): + """ + Test that the response uses the data returned by the pipeline, which receives the masqueraded user. + """ + CourseEnrollment.enroll(self.user, self.course.id, CourseMode.VERIFIED) + self.client.logout() + self.client.login(username=self.staff_user.username, password='bar') + self.update_masquerade(username=self.user.username) + filter_users.clear() + + response = self.client.get(self.url) + + assert response.status_code == 200 + assert response.data['title'] == f'Filtered {self.course.id.course}' + assert response.data['tabs'] == FILTERED_TABS + assert filter_users == [self.user] + def test_get_unknown_course(self): self.client.logout() url = reverse('course-home:course-metadata', args=['course-v1:unknown+course+2T2020']) diff --git a/lms/djangoapps/course_home_api/outline/tests/test_view.py b/lms/djangoapps/course_home_api/outline/tests/test_view.py index b802a806eeca..b8eb36679241 100644 --- a/lms/djangoapps/course_home_api/outline/tests/test_view.py +++ b/lms/djangoapps/course_home_api/outline/tests/test_view.py @@ -13,7 +13,9 @@ from django.contrib.auth.models import AnonymousUser from django.test import override_settings from django.urls import reverse +from django.utils.translation import get_language from edx_toggles.toggles.testutils import override_waffle_flag +from openedx_filters import PipelineStep from cms.djangoapps.contentstore.outlines import update_outline_from_modulestore from common.djangoapps.course_modes.models import CourseMode @@ -38,6 +40,22 @@ from xmodule.course_block import COURSE_VISIBILITY_PUBLIC, COURSE_VISIBILITY_PUBLIC_OUTLINE from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory +block_filter_languages = [] + + +class TestLanguageBlockNamesStep(PipelineStep): + """ + Pipeline step that appends the active language to every block display name. + """ + + def run_filter(self, block_structure, user, starting_block_usage_key): # pylint: disable=arguments-differ + language = get_language() + block_filter_languages.append(language) + for block_key in block_structure: + display_name = block_structure.get_xblock_field(block_key, 'display_name') + block_structure.override_xblock_field(block_key, 'display_name', f'{display_name} [{language}]') + return {'block_structure': block_structure} + @ddt.ddt class OutlineTabTestViews(BaseCourseHomeTests): @@ -958,3 +976,30 @@ def test_navigation_does_not_cache_stale_data_after_publish(self): assert response.status_code == 200 sequential_data = response.data['blocks'][str(self.sequential.location)] assert str(self.vertical.location) not in sequential_data['children'] + + @override_settings(OPEN_EDX_FILTERS_CONFIG={ + 'org.openedx.learning.course_blocks.transform.completed.v1': { + 'pipeline': [f'{__name__}.TestLanguageBlockNamesStep'], + 'fail_silently': False, + }, + }) + def test_navigation_cache_is_separate_per_language(self): + """ + Test that block data that depends on the active language is cached separately for each language. + """ + self.add_blocks_to_course() + CourseEnrollment.enroll(self.user, self.course.id, CourseMode.VERIFIED) + # Build the block structure up front so its version, part of the cache key, stays the same. + update_course_in_cache(self.course.id) + block_filter_languages.clear() + + titles = {} + for language in ('en', 'es-419', 'en', 'es-419'): + response = self.client.get(self.url, HTTP_ACCEPT_LANGUAGE=language) + assert response.status_code == 200 + title = response.data['blocks'][str(self.sequential.location)]['display_name'] + assert titles.setdefault(language, title) == title + + assert titles == {'en': 'Test [en]', 'es-419': 'Test [es-419]'} + # Repeated requests in the same language are served from the cache. + assert block_filter_languages == ['en', 'es-419'] diff --git a/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py b/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py index 49d9fafa068f..e9d7beefce1d 100644 --- a/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py +++ b/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py @@ -16,7 +16,9 @@ from datetime import datetime, timezone import ddt +from django.test import override_settings from opaque_keys.edx.keys import CourseKey +from openedx_filters import PipelineStep from rest_framework.test import APIClient, APITestCase from common.djangoapps.student.models import CourseEnrollment @@ -31,6 +33,21 @@ from ..data import CourseOutlineData, CourseVisibility +class TestCourseOutlineRenderStep(PipelineStep): + """ + Utility pipeline step that records the user and changes sequence titles in place. + """ + + users = [] + + def run_filter(self, course_key, user, data): # pylint: disable=arguments-differ + """Record the user and override every sequence title.""" + self.users.append(user) + for sequence in data["outline"]["sequences"].values(): + sequence["title"] = "Filtered title" + return {"data": data} + + @skip_unless_lms class CourseOutlineViewTest(CacheIsolationTestCase, APITestCase): """ @@ -119,6 +136,41 @@ def test_outline_as_staff(self): assert len(data['outline']['sections'][1]['sequence_ids']) == 2 assert len(data['outline']['sequences']) == 4 + def test_outline_render_filter(self): + """ + The render filter can modify the response, receives the effective user, and does not change cached data. + """ + filters_config = { + "org.openedx.learning.course_outline.api.render.started.v1": { + "pipeline": [ + "openedx.core.djangoapps.content.learning_sequences.tests.test_views.TestCourseOutlineRenderStep", + ], + "fail_silently": False, + }, + } + CourseOverviewFactory(id=self.course_key) + CourseEnrollment.enroll(self.student, self.course_key) + TestCourseOutlineRenderStep.users = [] + with override_settings(OPEN_EDX_FILTERS_CONFIG=filters_config): + self.client.login(username='student', password='student_pass') + student_result = self.client.get(self.course_url) + self.client.login(username='staff', password='staff_pass') + staff_as_student_result = self.client.get(self.course_url, {'user': 'student'}) + + for result in [student_result, staff_as_student_result]: + assert result.status_code == 200 + titles = {seq['title'] for seq in result.data['outline']['sequences'].values()} + assert titles == {"Filtered title"} + assert TestCourseOutlineRenderStep.users == [self.student, self.student] + + unfiltered_result = self.client.get(self.course_url) + authored_titles = { + str(seq.usage_key): seq.title for section in self.outline.sections for seq in section.sequences + } + assert { + key: seq['title'] for key, seq in unfiltered_result.data['outline']['sequences'].items() + } == authored_titles + @ddt.ddt @skip_unless_lms diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index 3f4f5fae0ebc..22a5e87b1443 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -15,6 +15,7 @@ from django.test.client import RequestFactory from edx_django_utils.cache import TieredCache from edx_toggles.toggles.testutils import override_waffle_flag, override_waffle_switch +from openedx_filters import PipelineStep from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.course_modes.tests.factories import CourseModeFactory @@ -50,6 +51,18 @@ _NEXT_WEEK = datetime.now() + timedelta(days=7) +class TestReplaceCoursewareMetadata(PipelineStep): + """ + Utility class used when getting steps for pipeline. + """ + + def run_filter(self, course_key, user, data): # pylint: disable=arguments-differ + """ + Pipeline step that replaces the response body with the course key and the user it received. + """ + return {"data": {"course_key": str(course_key), "username": user.username}} + + @skip_unless_lms class BaseCoursewareTests(SharedModuleStoreTestCase): """ @@ -428,6 +441,31 @@ def test_can_access_proctored_exams_masquerading(self, masquerade_group_id, resu assert 'can_access_proctored_exams' in courseware_data assert courseware_data['can_access_proctored_exams'] == result + @ddt.data(False, True) + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.learning.courseware_metadata.api.render.started.v1": { + "pipeline": [ + "openedx.core.djangoapps.courseware_api.tests.test_views.TestReplaceCoursewareMetadata", + ], + "fail_silently": False, + }, + }, + ) + def test_courseware_metadata_filter_replaces_response(self, masquerade): + """ + The filter's returned data is sent to the client, and the filter receives the effective (masqueraded) user. + """ + CourseEnrollment.enroll(self.user, self.course.id, 'audit') + if masquerade: + self.client.login(username=self.instructor.username, password='foo') + self.update_masquerade(username=self.user.username) + + response = self.client.get(self.url) + + assert response.status_code == 200 + assert response.json() == {"course_key": str(self.course.id), "username": self.user.username} + @ddt.data( True, False From 837fdfbe533fbcbe44b89621f2cd5f0458611d34 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Wed, 21 Oct 2026 18:00:51 +0500 Subject: [PATCH 5/7] docs: state which modulestores need fresh_block_reads Co-Authored-By: Claude Opus 5.5 (1M context) --- xmodule/modulestore/__init__.py | 4 +++- xmodule/modulestore/xml.py | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/xmodule/modulestore/__init__.py b/xmodule/modulestore/__init__.py index d11c7734f5f1..bb1c6956b0ca 100644 --- a/xmodule/modulestore/__init__.py +++ b/xmodule/modulestore/__init__.py @@ -1045,7 +1045,9 @@ def fresh_block_reads(self, course_key): # pylint: disable=unused-argument A context manager for loading the course's blocks as new instances, ignoring block instances that this thread has already loaded (and may have changed in memory). - The default is a no-op, for stores that do not reuse loaded block instances. + The default is a no-op, which is only correct for stores that build new block instances on + every read. A store that hands back already-loaded instances and serves learners must override + it. """ yield diff --git a/xmodule/modulestore/xml.py b/xmodule/modulestore/xml.py index eddc525970f5..f283cff91b32 100644 --- a/xmodule/modulestore/xml.py +++ b/xmodule/modulestore/xml.py @@ -910,6 +910,16 @@ def make_course_usage_key(self, course_key): """ return BlockUsageLocator(course_key, 'course', course_key.run) + @contextmanager + def fresh_block_reads(self, course_key): # pylint: disable=unused-argument + """ + A no-op, although this store hands back the same block instances on every read. + + This store only feeds course import and management commands; it is never configured to + serve learners, so its blocks are not bound to users or changed in memory per user. + """ + yield + def get_courses(self, **kwargs): """ Returns a list of course blocks. If there were errors on loading, From c2f79f8df2d6984e3dfb2ed5f2105f48504188e0 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Thu, 22 Oct 2026 15:52:25 +0500 Subject: [PATCH 6/7] feat: call the course home progress, dates, outline and navigation filters Each view passes its serialized response through its new filter. The navigation sidebar runs it after the cache, so the cached blocks stay as built. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../course_home_api/dates/tests/test_views.py | 37 ++++++++ lms/djangoapps/course_home_api/dates/views.py | 4 +- .../outline/tests/test_view.py | 87 +++++++++++++++++++ .../course_home_api/outline/views.py | 13 ++- .../progress/tests/test_views.py | 38 ++++++++ .../course_home_api/progress/views.py | 4 +- 6 files changed, 179 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/course_home_api/dates/tests/test_views.py b/lms/djangoapps/course_home_api/dates/tests/test_views.py index 0e37fbcaafae..ac4864d6776e 100644 --- a/lms/djangoapps/course_home_api/dates/tests/test_views.py +++ b/lms/djangoapps/course_home_api/dates/tests/test_views.py @@ -5,13 +5,28 @@ from datetime import datetime import ddt +from django.test import override_settings from django.urls import reverse +from openedx_filters import PipelineStep from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.course_home_api.tests.utils import BaseCourseHomeTests from openedx.features.content_type_gating.models import ContentTypeGatingConfig +filter_users = [] + + +class TestReplaceDatesDataStep(PipelineStep): + """ + Pipeline step that records the user and replaces the response data. + """ + + def run_filter(self, course_key, user, data): # pylint: disable=arguments-differ + filter_users.append(user) + return {'data': {'course_key': str(course_key)}} + @ddt.ddt class DatesTabTestViews(BaseCourseHomeTests): @@ -69,3 +84,25 @@ def test_masquerade(self): self.update_masquerade(role='student') assert not self.client.get(self.url).data.get('learner_is_full_access') + + @override_settings(OPEN_EDX_FILTERS_CONFIG={ + 'org.openedx.learning.course_home_dates.api.render.started.v1': { + 'pipeline': [f'{__name__}.TestReplaceDatesDataStep'], + 'fail_silently': False, + }, + }) + def test_render_filter_receives_masqueraded_user(self): + """ + Test that the render filter can replace the response and receives the masqueraded user. + """ + learner = UserFactory() + CourseEnrollment.enroll(learner, self.course.id) + self.switch_to_staff() + self.update_masquerade(username=learner.username) + filter_users.clear() + + response = self.client.get(self.url) + + assert response.status_code == 200 + assert response.data == {'course_key': str(self.course.id)} + assert filter_users == [learner] diff --git a/lms/djangoapps/course_home_api/dates/views.py b/lms/djangoapps/course_home_api/dates/views.py index 64f8fc34428e..05deaef68cca 100644 --- a/lms/djangoapps/course_home_api/dates/views.py +++ b/lms/djangoapps/course_home_api/dates/views.py @@ -6,6 +6,7 @@ from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from opaque_keys.edx.keys import CourseKey +from openedx_filters.learning.filters import CourseHomeDatesAPIRenderStarted from rest_framework.generics import RetrieveAPIView from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response @@ -130,5 +131,6 @@ def get(self, request, *args, **kwargs): context = self.get_serializer_context() context['learner_is_full_access'] = learner_is_full_access serializer = self.get_serializer_class()(data, context=context) + data = CourseHomeDatesAPIRenderStarted.run_filter(course_key=course_key, user=request.user, data=serializer.data) - return Response(serializer.data) + return Response(data) diff --git a/lms/djangoapps/course_home_api/outline/tests/test_view.py b/lms/djangoapps/course_home_api/outline/tests/test_view.py index b8eb36679241..79b9cbbaeadc 100644 --- a/lms/djangoapps/course_home_api/outline/tests/test_view.py +++ b/lms/djangoapps/course_home_api/outline/tests/test_view.py @@ -57,6 +57,30 @@ def run_filter(self, block_structure, user, starting_block_usage_key): # pylint return {'block_structure': block_structure} +filter_users = [] + + +class TestReplaceOutlineDataStep(PipelineStep): + """ + Pipeline step that records the user and replaces the response data. + """ + + def run_filter(self, course_key, user, data): # pylint: disable=arguments-differ + filter_users.append(user) + return {'data': {'course_key': str(course_key)}} + + +class TestRenameNavigationBlocksStep(PipelineStep): + """ + Pipeline step that renames every block in place. + """ + + def run_filter(self, course_key, user, data): # pylint: disable=arguments-differ + for block in data['blocks'].values(): + block['display_name'] = 'Changed' + return {'data': data} + + @ddt.ddt class OutlineTabTestViews(BaseCourseHomeTests): """ @@ -496,6 +520,28 @@ def test_course_progress_analytics_disabled(self, mock_task): self.client.get(self.url) mock_task.assert_not_called() + @override_settings(OPEN_EDX_FILTERS_CONFIG={ + 'org.openedx.learning.course_home_outline.api.render.started.v1': { + 'pipeline': [f'{__name__}.TestReplaceOutlineDataStep'], + 'fail_silently': False, + }, + }) + def test_render_filter_receives_masqueraded_user(self): + """ + Test that the render filter can replace the response and receives the masqueraded user. + """ + learner = UserFactory() + CourseEnrollment.enroll(learner, self.course.id) + self.switch_to_staff() + self.update_masquerade(username=learner.username) + filter_users.clear() + + response = self.client.get(self.url) + + assert response.status_code == 200 + assert response.data == {'course_key': str(self.course.id)} + assert filter_users == [learner] + @ddt.ddt class SidebarBlocksTestViews(BaseCourseHomeTests): @@ -1003,3 +1049,44 @@ def test_navigation_cache_is_separate_per_language(self): assert titles == {'en': 'Test [en]', 'es-419': 'Test [es-419]'} # Repeated requests in the same language are served from the cache. assert block_filter_languages == ['en', 'es-419'] + + def test_render_filter_replaces_response(self): + """ + Test that the render filter can replace the response and receives the requesting user. + """ + CourseEnrollment.enroll(self.user, self.course.id) + filter_users.clear() + + with override_settings(OPEN_EDX_FILTERS_CONFIG={ + 'org.openedx.learning.course_navigation.api.render.started.v1': { + 'pipeline': [f'{__name__}.TestReplaceOutlineDataStep'], + 'fail_silently': False, + }, + }): + response = self.client.get(self.url) + + assert response.status_code == 200 + assert response.data == {'course_key': str(self.course.id)} + assert filter_users == [self.user] + + def test_render_filter_changes_are_not_cached(self): + """ + Test that changes made by the render filter do not reach the cached blocks. + """ + self.add_blocks_to_course() + CourseEnrollment.enroll(self.user, self.course.id, CourseMode.VERIFIED) + # Build the block structure up front so its version, part of the cache key, stays the same. + update_course_in_cache(self.course.id) + sequential_key = str(self.sequential.location) + + with override_settings(OPEN_EDX_FILTERS_CONFIG={ + 'org.openedx.learning.course_navigation.api.render.started.v1': { + 'pipeline': [f'{__name__}.TestRenameNavigationBlocksStep'], + 'fail_silently': False, + }, + }): + response = self.client.get(self.url) + assert response.data['blocks'][sequential_key]['display_name'] == 'Changed' + + response = self.client.get(self.url) + assert response.data['blocks'][sequential_key]['display_name'] == 'Test' diff --git a/lms/djangoapps/course_home_api/outline/views.py b/lms/djangoapps/course_home_api/outline/views.py index 51279195f3fe..2fc0d71cc72d 100644 --- a/lms/djangoapps/course_home_api/outline/views.py +++ b/lms/djangoapps/course_home_api/outline/views.py @@ -21,6 +21,10 @@ SessionAuthenticationAllowInactiveUser, # pylint: disable=wrong-import-order ) from opaque_keys.edx.keys import CourseKey # pylint: disable=wrong-import-order +from openedx_filters.learning.filters import ( # pylint: disable=wrong-import-order + CourseHomeOutlineAPIRenderStarted, + CourseNavigationAPIRenderStarted, +) from rest_framework.decorators import ( # pylint: disable=wrong-import-order api_view, authentication_classes, @@ -384,7 +388,8 @@ def get(self, request, *args, **kwargs): # pylint: disable=too-many-statements if send_course_progress_analytics_for_student_is_enabled(course_key) and not user_is_masquerading: collect_progress_for_user_in_course.delay(course_key_string, request.user.id) - return Response(serializer.data) + data = CourseHomeOutlineAPIRenderStarted.run_filter(course_key=course_key, user=request.user, data=serializer.data) + return Response(data) def finalize_response(self, request, response, *args, **kwargs): """ @@ -507,8 +512,12 @@ def get(self, request, *args, **kwargs): }) serializer = self.get_serializer_class()(course_blocks, context=context) + # Runs on the serialized response, after the cache, so the cached blocks stay as built. + data = CourseNavigationAPIRenderStarted.run_filter( + course_key=course_key, user=request.user, data=serializer.data + ) - return Response(serializer.data) + return Response(data) def filter_inaccessible_blocks(self, course_blocks, course_key): """ diff --git a/lms/djangoapps/course_home_api/progress/tests/test_views.py b/lms/djangoapps/course_home_api/progress/tests/test_views.py index 49ec170d97ef..2ca587ea587e 100644 --- a/lms/djangoapps/course_home_api/progress/tests/test_views.py +++ b/lms/djangoapps/course_home_api/progress/tests/test_views.py @@ -11,6 +11,7 @@ from django.urls import reverse from django.utils.timezone import now from edx_toggles.toggles.testutils import override_waffle_flag +from openedx_filters import PipelineStep from pytz import UTC from common.djangoapps.course_modes.models import CourseMode @@ -36,6 +37,18 @@ from openedx.features.course_duration_limits.models import CourseDurationLimitConfig from xmodule.modulestore.tests.factories import BlockFactory +filter_users = [] + + +class TestReplaceProgressDataStep(PipelineStep): + """ + Pipeline step that records the user and replaces the response data. + """ + + def run_filter(self, course_key, user, data): # pylint: disable=arguments-differ + filter_users.append(user) + return {'data': {'username': data['username'], 'course_key': str(course_key)}} + @ddt.ddt @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) @@ -362,3 +375,28 @@ def test_course_grade_considers_subsection_grade_visibility(self, is_staff, expe assert response.status_code == 200 assert response.data['course_grade']['percent'] == expected_percent assert response.data['course_grade']['is_passing'] == (expected_percent >= 0.5) + + @override_settings(OPEN_EDX_FILTERS_CONFIG={ + 'org.openedx.learning.course_home_progress.api.render.started.v1': { + 'pipeline': [f'{__name__}.TestReplaceProgressDataStep'], + 'fail_silently': False, + }, + }) + def test_render_filter_receives_learner(self): + """ + Test that the render filter can replace the response and receives the learner whose progress is shown. + """ + learner = self.user + CourseEnrollment.enroll(learner, self.course.id) + filter_users.clear() + + response = self.client.get(self.url) + assert response.status_code == 200 + assert response.data == {'username': learner.username, 'course_key': str(self.course.id)} + + self.switch_to_staff() + url = reverse('course-home:progress-tab-other-student', args=[self.course.id, learner.id]) + response = self.client.get(url) + assert response.status_code == 200 + assert response.data == {'username': learner.username, 'course_key': str(self.course.id)} + assert filter_users == [learner, learner] diff --git a/lms/djangoapps/course_home_api/progress/views.py b/lms/djangoapps/course_home_api/progress/views.py index 577392dc9527..07a090e456de 100644 --- a/lms/djangoapps/course_home_api/progress/views.py +++ b/lms/djangoapps/course_home_api/progress/views.py @@ -9,6 +9,7 @@ from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from opaque_keys.edx.keys import CourseKey +from openedx_filters.learning.filters import CourseHomeProgressAPIRenderStarted from rest_framework.generics import RetrieveAPIView from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response @@ -307,5 +308,6 @@ def get(self, request, *args, **kwargs): # pylint: disable=too-many-statement context['course_overview'] = course_overview context['enrollment'] = enrollment serializer = self.get_serializer_class()(data, context=context) + data = CourseHomeProgressAPIRenderStarted.run_filter(course_key=course_key, user=student, data=serializer.data) - return Response(serializer.data) + return Response(data) From 2b5a06070dd899725266a6af697d070cc591a5a1 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Thu, 22 Oct 2026 16:02:54 +0500 Subject: [PATCH 7/7] fix: return a fresh assignment colors list from the progress serializer Co-Authored-By: Claude Opus 5.5 (1M context) --- .../course_home_api/progress/serializers.py | 3 ++- .../progress/tests/test_views.py | 27 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/course_home_api/progress/serializers.py b/lms/djangoapps/course_home_api/progress/serializers.py index 04ddccd22399..25fdc09f81e1 100644 --- a/lms/djangoapps/course_home_api/progress/serializers.py +++ b/lms/djangoapps/course_home_api/progress/serializers.py @@ -128,7 +128,8 @@ def get_assignment_policies(self, grading_policy): } for assignment_policy in grading_policy['GRADER']] def get_assignment_colors(self, obj): - return self.ASSIGNMENT_COLORS + # A copy, so a change to one response does not reach the shared class list. + return list(self.ASSIGNMENT_COLORS) class CertificateDataSerializer(ReadOnlySerializer): diff --git a/lms/djangoapps/course_home_api/progress/tests/test_views.py b/lms/djangoapps/course_home_api/progress/tests/test_views.py index 2ca587ea587e..056926c7adce 100644 --- a/lms/djangoapps/course_home_api/progress/tests/test_views.py +++ b/lms/djangoapps/course_home_api/progress/tests/test_views.py @@ -50,6 +50,16 @@ def run_filter(self, course_key, user, data): # pylint: disable=arguments-diffe return {'data': {'username': data['username'], 'course_key': str(course_key)}} +class TestRecolorProgressStep(PipelineStep): + """ + Pipeline step that changes the assignment colors in place. + """ + + def run_filter(self, course_key, user, data): # pylint: disable=arguments-differ + data['grading_policy']['assignment_colors'][0] = '#000000' + return {'data': data} + + @ddt.ddt @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) class ProgressTabTestViews(BaseCourseHomeTests): @@ -400,3 +410,20 @@ def test_render_filter_receives_learner(self): assert response.status_code == 200 assert response.data == {'username': learner.username, 'course_key': str(self.course.id)} assert filter_users == [learner, learner] + + def test_render_filter_changes_do_not_reach_later_responses(self): + """ + Test that changing the response in place does not change the responses that follow. + """ + CourseEnrollment.enroll(self.user, self.course.id) + with override_settings(OPEN_EDX_FILTERS_CONFIG={ + 'org.openedx.learning.course_home_progress.api.render.started.v1': { + 'pipeline': [f'{__name__}.TestRecolorProgressStep'], + 'fail_silently': False, + }, + }): + filtered = self.client.get(self.url) + assert filtered.data['grading_policy']['assignment_colors'][0] == '#000000' + + response = self.client.get(self.url) + assert response.data['grading_policy']['assignment_colors'][0] == '#D24242'