From 2349c0a5a98c892aed0ed045ec8890895b4c43eb Mon Sep 17 00:00:00 2001 From: Aditya Bhalsod Date: Sat, 24 Oct 2026 03:44:25 +0000 Subject: [PATCH 1/7] feat: incrementally load large assessment units Rendering every problem of a very large unit, such as a 90-question exam built from randomized question banks, in one response can exceed the upstream request timeout, so the unit never loads. Behind the new course waffle flag courseware.incremental_assessment_load (off by default), a unit with more than INCREMENTAL_LOAD_PROBLEM_THRESHOLD problems renders its first INCREMENTAL_LOAD_EAGER_COUNT children inline and leaves placeholders for the rest. The page then fills them in, in sequential batches of at most XBLOCK_CHILDREN_BATCH_MAX, from the new GET /api/courseware/xblock_children/{usage_key} endpoint, retrying transient failures. Mobile app requests and units at or below the threshold always render in full. With the flag off, rendering is unchanged. Ported from edx/edx-platform#445, originally written by Santhosh Kumar. This commit is the downstream change as it was merged there, except the hunks that do not apply to master. Those are added in "chore: adapt the incremental assessment load port to master". Co-Authored-By: Santhosh Kumar --- lms/djangoapps/courseware/block_render.py | 276 +++++++++++++++++- .../tests/test_incremental_render.py | 269 +++++++++++++++++ lms/djangoapps/courseware/toggles.py | 23 ++ lms/djangoapps/courseware/views/views.py | 8 + lms/envs/common.py | 18 ++ lms/templates/vert_module.html | 22 +- lms/templates/vert_module_lazy_bootstrap.html | 214 ++++++++++++++ .../courseware_api/tests/test_views.py | 94 ++++++ .../core/djangoapps/courseware_api/urls.py | 3 + .../core/djangoapps/courseware_api/views.py | 71 ++++- xmodule/vertical_block.py | 4 + 11 files changed, 999 insertions(+), 3 deletions(-) create mode 100644 lms/djangoapps/courseware/tests/test_incremental_render.py create mode 100644 lms/templates/vert_module_lazy_bootstrap.html diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 8ef637b82bf0..4a646fdcd9cd 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -861,10 +861,17 @@ def _get_block_by_usage_key(usage_key): def get_block_by_usage_id(request, course_id, usage_id, disable_staff_debug_info=False, course=None, - will_recheck_access=False): + will_recheck_access=False, field_data_cache_depth=None): """ Gets a block instance based on its `usage_id` in a course, for a given request/user + Arguments: + field_data_cache_depth: depth passed through to + FieldDataCache.cache_for_block_descendents. None (the default) prefetches every + descendant, matching historical behavior. Used by the incremental assessment load + (see render_xblock_children) so a batch of children only prefetches state for + themselves, not their own descendants. + Returns (instance, tracking_context) """ course_key = CourseKey.from_string(course_id) @@ -876,6 +883,7 @@ def get_block_by_usage_id(request, course_id, usage_id, disable_staff_debug_info course_key, user, block, + depth=field_data_cache_depth, read_only=CrawlersConfig.is_crawler(request), ) instance = get_block_for_descriptor( @@ -1081,3 +1089,269 @@ def append_data_to_webob_response(response, data): response_data.update(data) response.body = json.dumps(response_data).encode('utf-8') return response + + +# Leaf block types counted by count_renderable_descendants. Only types whose render cost +# is significant are counted -- a unit with fifty short HTML blocks renders quickly and +# gains nothing from being split up. +INCREMENTAL_LOAD_COUNTED_BLOCK_TYPES = frozenset(['problem']) + + +def count_renderable_descendants(block, depth=4): + """ + Estimate how many leaf blocks a learner will actually be shown under `block`. + + Used only to decide whether a unit is large enough to be worth loading incrementally. + For randomized banks (library_content / item_bank) this counts what the learner will + see -- `max_count`, or the whole pool when it is unset or -1 -- rather than the full + candidate pool, applying the same has_dynamic_children()/get_child_blocks() distinction + FieldDataCache._children_to_prefetch and VerticalBlock.block_has_access_error already + make elsewhere in this codebase. + """ + if block is None or depth < 0: + return 0 + + has_dynamic_children = getattr(block, 'has_dynamic_children', None) + if callable(has_dynamic_children) and has_dynamic_children(): + max_count = getattr(block, 'max_count', None) + pool_size = len(getattr(block, 'children', None) or []) + if isinstance(max_count, int) and max_count > 0: + return min(max_count, pool_size) if pool_size else max_count + # max_count of -1 (or unset) means "show all of them". + return pool_size + + block_type = getattr(getattr(block, 'location', None), 'block_type', None) + get_children = getattr(block, 'get_children', None) + if not callable(get_children): + return 1 if block_type in INCREMENTAL_LOAD_COUNTED_BLOCK_TYPES else 0 + + children = get_children() + if not children: + return 1 if block_type in INCREMENTAL_LOAD_COUNTED_BLOCK_TYPES else 0 + + return sum(count_renderable_descendants(child, depth=depth - 1) for child in children) + + +def should_use_incremental_load(request, block): + """ + Return True when `block` should be rendered as a shell -- the first + settings.INCREMENTAL_LOAD_EAGER_COUNT children rendered inline and the rest left as + placeholders for XBlockChildren to fill in batches -- instead of rendering every + child in one response. + + All of the following must hold: + 1. courseware.incremental_assessment_load is enabled for the course (kill switch, + off by default). + 2. `block` is a vertical -- the only block the courseware iframe renders as a unit, + and the only place a shell makes sense. + 3. The unit is big enough that a single render risks the upstream request timeout. + 4. The request is not from a mobile app -- mobile webviews have not been validated + against the incremental-load bootstrap. + """ + block_type = getattr(getattr(block, 'location', None), 'block_type', None) + if block_type not in ('vertical', 'unit'): + return False + + if is_request_from_mobile_app(request): + return False + + course_key = block.location.course_key + if not incremental_assessment_load_is_enabled(course_key): + return False + + return count_renderable_descendants(block) > settings.INCREMENTAL_LOAD_PROBLEM_THRESHOLD + + +def _allowed_child_usage_keys(parent): + """ + Return the set of usage keys the current learner is allowed to load under `parent`. + + This is the batch endpoint's security boundary: without it, a learner could ask for + any usage key under a randomized bank, including problems it never assigned them. + Dynamic blocks (library_content / item_bank) expose only the learner's persisted + selection via get_child_blocks(); everything else exposes its ordinary children -- + the same distinction FieldDataCache._children_to_prefetch and + VerticalBlock.block_has_access_error already make. + """ + has_dynamic_children = getattr(parent, 'has_dynamic_children', None) + get_child_blocks = getattr(parent, 'get_child_blocks', None) + if callable(has_dynamic_children) and has_dynamic_children() and callable(get_child_blocks): + children = get_child_blocks() + else: + children = parent.get_children() + return {child.location for child in children if child is not None} + + +def _lazy_child_student_view_context(request): + """ + Build the child render context used by the incremental load endpoint. + + Keep this aligned with render_xblock's student_view_context and + VerticalBlock._student_or_public_view's child context so lazily-rendered + children behave like children rendered inline by the unit. + """ + context = request.GET.dict() + context['show_bookmark_button'] = request.GET.get('show_bookmark_button', '0') == '1' + context['show_title'] = request.GET.get('show_title', '1') == '1' + context['hide_access_error_blocks'] = ( + is_request_from_learning_mfe(request) and request.GET.get('recheck_access') == '1' + ) + context['is_mobile_app'] = is_request_from_mobile_app(request) + context['child_of_vertical'] = True + return context + + +def render_xblock_children(request, parent_usage_key, child_usage_keys, course=None): + """ + Render a bounded batch of `parent`'s children and return their HTML. + + Backs the XBlockChildren API that the incremental assessment load bootstrap calls + repeatedly to fill in the placeholders VerticalBlock leaves for children past its + eager-render window. One FieldDataCache is built for the whole batch, and the + masquerade user is resolved once and reused for every child in it -- building a + separate cache, or re-resolving masquerade, per child would multiply exactly the + per-child render cost this feature exists to bound. + + A child that fails to render is reported in `errors` rather than failing the whole + batch, so one bad problem doesn't cost the learner the rest of the batch. + + Returns: + dict with `parent_usage_key`, `results` (list of `{usage_key, html}`) and + `errors` (list of `{usage_key, error, message}`). + + Raises: + ValueError: the request is malformed or the batch is too large. Callers should + map this to a 400. + """ + if not child_usage_keys: + raise ValueError('child_usage_keys is required') + + max_batch = settings.XBLOCK_CHILDREN_BATCH_MAX + if len(child_usage_keys) > max_batch: + raise ValueError(f'At most {max_batch} child_usage_keys are allowed per request') + + course_key = parent_usage_key.course_key + staff_access = bool(has_access(request.user, 'staff', course_key)) + _, user = setup_masquerade(request, course_key, staff_access) + + with modulestore().bulk_operations(course_key): + parent, _ = get_block_by_usage_id( + request, + str(course_key), + str(parent_usage_key), + course=course, + field_data_cache_depth=0, + will_recheck_access=True, + ) + + allowed_keys = _allowed_child_usage_keys(parent) + children_by_key = {child.location: child for child in parent.get_children() if child is not None} + + requested = [] + errors = [] + for child_key in child_usage_keys: + if child_key in allowed_keys: + requested.append(child_key) + else: + errors.append({ + 'usage_key': str(child_key), + 'error': 'forbidden', + 'message': "Block is not among this learner's children for that parent.", + }) + + results = [] + if not requested: + return {'parent_usage_key': str(parent_usage_key), 'results': results, 'errors': errors} + + # One cache for the whole batch, not one per child. Depth=1 pulls in each child's + # own selected problem (for library_content/item_bank children) without recursing + # further -- matching the shape FieldDataCache._children_to_prefetch already + # narrows to for such blocks. + field_data_cache = FieldDataCache([], course_key, user, read_only=CrawlersConfig.is_crawler(request)) + for child_key in requested: + child = children_by_key.get(child_key) + if child is None: + errors.append({ + 'usage_key': str(child_key), + 'error': 'not_found', + 'message': 'Block could not be loaded.', + }) + continue + field_data_cache.add_block_descendents(child, depth=1) + + # Both computed once for the whole batch, not per child, matching how + # VerticalBlock computes them once for its own eager-rendered children. + block_has_access_error = getattr(parent, 'block_has_access_error', None) + completion_service = parent.runtime.service(parent, 'completion') + child_blocks_to_complete_on_view = set() + complete_on_view_delay = None + if completion_service and completion_service.completion_tracking_enabled(): + requested_children = [ + children_by_key[child_key] for child_key in requested if child_key in children_by_key + ] + child_blocks_to_complete_on_view = completion_service.blocks_to_mark_complete_on_view( + requested_children + ) + complete_on_view_delay = completion_service.get_complete_on_view_delay_ms() + + for child_key in requested: + child = children_by_key.get(child_key) + if child is None: + continue + + child_context = _lazy_child_student_view_context(request) + if ( + child_context.get('hide_access_error_blocks') + and callable(block_has_access_error) + and block_has_access_error(child) + ): + # Matches VerticalBlock's own eager-render skip (block_has_access_error): + # a block gated for this learner must stay hidden whether it renders + # inline or gets lazy-loaded later. + results.append({'usage_key': str(child_key), 'html': ''}) + continue + if child in child_blocks_to_complete_on_view: + child_context['wrap_xblock_data'] = { + 'mark-completed-on-view-after-delay': complete_on_view_delay + } + + try: + instance = get_block_for_descriptor( + user, + request, + child, + field_data_cache, + course_key, + course=course, + will_recheck_access=True, + ) + if instance is None: + errors.append({ + 'usage_key': str(child_key), + 'error': 'forbidden', + 'message': 'Learner does not have access to this block.', + }) + continue + instance, child_context = VerticalBlockChildRenderStarted.run_filter( + block=instance, + context=child_context + ) + fragment = instance.render(STUDENT_VIEW, context=child_context) + results.append({'usage_key': str(child_key), 'html': fragment.content}) + except VerticalBlockChildRenderStarted.PreventChildBlockRender as exc: + log.info('Incremental load: skipping child %s. Reason: %s', child_key, exc.message) + errors.append({ + 'usage_key': str(child_key), + 'error': 'forbidden', + 'message': exc.message, + }) + except Exception as exc: # pylint: disable=broad-except + # One problem failing to render must not cost the learner the rest of the batch. + log.exception('Incremental load: child %s under %s failed to render', child_key, parent_usage_key) + errors.append({ + 'usage_key': str(child_key), + 'error': 'render_failed', + 'message': str(exc), + }) + + return {'parent_usage_key': str(parent_usage_key), 'results': results, 'errors': errors} diff --git a/lms/djangoapps/courseware/tests/test_incremental_render.py b/lms/djangoapps/courseware/tests/test_incremental_render.py new file mode 100644 index 000000000000..0d0581e9f8bb --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_incremental_render.py @@ -0,0 +1,269 @@ +""" +Tests for the incremental assessment load: descendant counting, shell eligibility, the +batch endpoint's access boundary, and VerticalBlock's eager-window rendering. +""" +from unittest.mock import Mock, patch + +from django.test import RequestFactory, TestCase, override_settings +from opaque_keys.edx.locator import CourseLocator + +from lms.djangoapps.courseware.block_render import ( + _allowed_child_usage_keys, + _lazy_child_student_view_context, + count_renderable_descendants, + should_use_incremental_load, +) + + +COURSE_KEY = CourseLocator('org', 'course', 'run') + + +def _block(block_type, children=None, dynamic=False, max_count=None, pool=0): + """ + Build a stand-in XBlock with just the attributes the helpers under test read. + """ + block = Mock() + block.location.block_type = block_type + block.location.course_key = COURSE_KEY + block.has_dynamic_children.return_value = dynamic + block.get_children.return_value = children or [] + if dynamic: + block.max_count = max_count + block.children = [Mock() for _ in range(pool)] + return block + + +class CountRenderableDescendantsTests(TestCase): + """ + Tests for count_renderable_descendants. + """ + + def test_counts_problems_in_a_flat_unit(self): + vertical = _block('vertical', children=[_block('problem'), _block('problem')]) + assert count_renderable_descendants(vertical) == 2 + + def test_ignores_cheap_leaf_types(self): + # A unit full of short HTML renders quickly and should not be split up. + vertical = _block('vertical', children=[_block('html'), _block('video'), _block('problem')]) + assert count_renderable_descendants(vertical) == 1 + + def test_randomized_bank_counts_what_the_learner_sees(self): + # This is the shape that caused the incident: a large pool, one problem shown. + bank = _block('library_content', dynamic=True, max_count=1, pool=40) + vertical = _block('vertical', children=[bank]) + assert count_renderable_descendants(vertical) == 1 + + def test_many_single_selection_banks_add_up(self): + banks = [_block('library_content', dynamic=True, max_count=1, pool=4) for _ in range(90)] + vertical = _block('vertical', children=banks) + assert count_renderable_descendants(vertical) == 90 + + def test_max_count_of_minus_one_counts_whole_pool(self): + # -1 means "show all of them", the most expensive configuration there is. + bank = _block('library_content', dynamic=True, max_count=-1, pool=30) + assert count_renderable_descendants(bank) == 30 + + def test_max_count_above_pool_is_capped_by_pool(self): + bank = _block('library_content', dynamic=True, max_count=50, pool=6) + assert count_renderable_descendants(bank) == 6 + + def test_depth_limit_stops_runaway_recursion(self): + deep = _block('problem') + for _unused in range(10): + deep = _block('vertical', children=[deep]) + assert count_renderable_descendants(deep) >= 0 + + +class ShouldUseIncrementalLoadTests(TestCase): + """ + Tests for should_use_incremental_load. All conditions must hold. + """ + + def setUp(self): + super().setUp() + self.request = Mock() + self.large_vertical = _block('vertical', children=[_block('problem') for _ in range(30)]) + + def _check(self, block, flag_enabled=True, mobile=False): + """ + Call should_use_incremental_load with the flag and mobile-app checks mocked. + """ + with patch( + 'lms.djangoapps.courseware.block_render.incremental_assessment_load_is_enabled', + return_value=flag_enabled, + ), patch( + 'lms.djangoapps.courseware.block_render.is_request_from_mobile_app', + return_value=mobile, + ): + return should_use_incremental_load(self.request, block) + + @override_settings(INCREMENTAL_LOAD_PROBLEM_THRESHOLD=20) + def test_large_vertical_with_flag_on(self): + assert self._check(self.large_vertical) is True + + @override_settings(INCREMENTAL_LOAD_PROBLEM_THRESHOLD=20) + def test_flag_off_is_the_kill_switch(self): + assert self._check(self.large_vertical, flag_enabled=False) is False + + @override_settings(INCREMENTAL_LOAD_PROBLEM_THRESHOLD=20) + def test_small_vertical_is_untouched_even_when_flagged(self): + small = _block('vertical', children=[_block('problem') for _ in range(5)]) + assert self._check(small) is False + + @override_settings(INCREMENTAL_LOAD_PROBLEM_THRESHOLD=20) + def test_non_vertical_blocks_never_shell(self): + assert self._check(_block('sequential', children=[_block('problem')] * 30)) is False + assert self._check(_block('problem')) is False + + @override_settings(INCREMENTAL_LOAD_PROBLEM_THRESHOLD=20) + def test_mobile_apps_are_excluded(self): + assert self._check(self.large_vertical, mobile=True) is False + + @override_settings(INCREMENTAL_LOAD_PROBLEM_THRESHOLD=100) + def test_threshold_is_respected(self): + assert self._check(self.large_vertical) is False + + +class AllowedChildUsageKeysTests(TestCase): + """ + Tests for the batch endpoint's access boundary. + """ + + def test_static_parent_exposes_its_children(self): + children = [_block('problem'), _block('problem')] + parent = _block('vertical', children=children) + assert _allowed_child_usage_keys(parent) == {child.location for child in children} + + def test_dynamic_parent_exposes_only_the_learner_selection(self): + # A learner must not be able to pull problems the bank did not assign them. + selected = _block('problem') + unselected = _block('problem') + parent = _block('library_content', children=[selected, unselected], dynamic=True, max_count=1, pool=2) + parent.get_child_blocks.return_value = [selected] + + allowed = _allowed_child_usage_keys(parent) + assert allowed == {selected.location} + assert unselected.location not in allowed + + def test_none_children_are_skipped(self): + good = _block('problem') + parent = _block('vertical', children=[good, None]) + assert _allowed_child_usage_keys(parent) == {good.location} + + +class LazyChildStudentViewContextTests(TestCase): + """ + Tests for the child render context used by incremental batch rendering. + """ + + def test_context_matches_unit_child_render_flags(self): + request = RequestFactory().get( + '/', + { + 'recheck_access': '1', + 'show_bookmark_button': '0', + 'show_title': '0', + 'view': 'student_view', + } + ) + + context = _lazy_child_student_view_context(request) + + assert context['child_of_vertical'] is True + assert context['show_bookmark_button'] is False + assert context['show_title'] is False + assert context['is_mobile_app'] is False + assert context['view'] == 'student_view' + + +class VerticalEagerWindowRenderTests(TestCase): + """ + Tests that VerticalBlock renders only the eager window and reports the rest as + placeholders. + """ + + def _render_context(self, eager_count, child_count=8): + """ + Run VerticalBlock._student_or_public_view with stubbed children and return the + fragment context it handed to the template. + """ + from xmodule.vertical_block import VerticalBlock + + children = [] + for index in range(child_count): + child = Mock() + child.location = f'block-v1:org+course+run+type@problem+block@p{index}' + rendered = Mock() + rendered.content = f'
problem {index}
' + rendered.resources = [] + child.render.return_value = rendered + children.append(child) + + # Not Mock(spec=VerticalBlock): `runtime` is set dynamically per-instance by the + # XBlock runtime binding, not declared on the class, so spec= would reject it. + block = Mock() + block.get_children.return_value = children + block.is_block_complete_for_assignments.return_value = None + block.due = None + block.location = 'block-v1:org+course+run+type@vertical+block@v1' + block.display_name_with_default = 'Unit' + + captured = {} + + def capture_render(_template, fragment_context): + captured.update(fragment_context) + return '' + + mako = Mock() + mako.render_lms_template.side_effect = capture_render + completion = Mock() + completion.completion_tracking_enabled.return_value = False + user_service = Mock() + user_service.get_current_user.return_value.opt_attrs = {'edx-platform.username': 'learner'} + + def service(_self, name): + return { + 'mako': mako, + 'completion': completion, + 'bookmarks': Mock(is_bookmarked=Mock(return_value=False)), + 'user': user_service, + 'call_to_action': None, + }.get(name) + + block.runtime.service.side_effect = service + + context = {'incremental_load_eager_count': eager_count} if eager_count is not None else {} + with patch('xmodule.vertical_block.add_webpack_js_to_fragment'): + VerticalBlock._student_or_public_view( # pylint: disable=protected-access + block, context, 'student_view' + ) + return captured, children + + def test_renders_only_the_eager_window(self): + captured, children = self._render_context(eager_count=5, child_count=8) + + assert len(captured['items']) == 5 + assert len(captured['lazy_children']) == 3 + for child in children[:5]: + child.render.assert_called_once() + for child in children[5:]: + child.render.assert_not_called() + + def test_placeholders_are_reported_in_order(self): + captured, children = self._render_context(eager_count=2, child_count=6) + assert captured['lazy_children'] == [str(child.location) for child in children[2:]] + assert captured['parent_usage_key'] is not None + + def test_without_eager_count_every_child_renders(self): + # The default path: flag off, no shell, behavior identical to before this feature. + captured, children = self._render_context(eager_count=None, child_count=6) + assert len(captured['items']) == 6 + assert captured['lazy_children'] == [] + assert captured['parent_usage_key'] is None + for child in children: + child.render.assert_called_once() + + def test_eager_count_at_or_above_child_count_renders_everything(self): + captured, children = self._render_context(eager_count=10, child_count=6) + assert len(captured['items']) == 6 + assert captured['lazy_children'] == [] diff --git a/lms/djangoapps/courseware/toggles.py b/lms/djangoapps/courseware/toggles.py index 3a51ff1f6d91..31076525048a 100644 --- a/lms/djangoapps/courseware/toggles.py +++ b/lms/djangoapps/courseware/toggles.py @@ -111,6 +111,29 @@ f'{WAFFLE_FLAG_NAMESPACE}.optimized_render_xblock', __name__ ) +# .. toggle_name: courseware.incremental_assessment_load +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: Render large assessment units as a shell -- the first few problems +# plus placeholders -- and load the remaining problems in batches from +# /api/courseware/xblock_children/. Units at or below +# settings.INCREMENTAL_LOAD_PROBLEM_THRESHOLD are unaffected even when this is on. Intended +# for assessments large enough that rendering every problem in one response risks exceeding +# the upstream request timeout. +# .. toggle_use_cases: opt_in +# .. toggle_creation_date: 2026-08-27 +# .. toggle_target_removal_date: None +COURSEWARE_INCREMENTAL_ASSESSMENT_LOAD = CourseWaffleFlag( + f'{WAFFLE_FLAG_NAMESPACE}.incremental_assessment_load', __name__ +) + + +def incremental_assessment_load_is_enabled(course_key=None): + """ + Return whether large assessment units in this course should load incrementally. + """ + return COURSEWARE_INCREMENTAL_ASSESSMENT_LOAD.is_enabled(course_key) + # .. toggle_name: COURSES_INVITE_ONLY # .. toggle_implementation: SettingToggle # .. toggle_type: feature_flag diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 4dcb50579251..cd866a7c82a1 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -1642,6 +1642,14 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_sta student_view_context['show_bookmark_button'] = request.GET.get('show_bookmark_button', '0') == '1' student_view_context['show_title'] = request.GET.get('show_title', '1') == '1' + # Large assessment units render the first few problems and leave placeholders + # for the rest, which the page then loads in batches from XBlockChildren. Off + # unless the course opts in, so every other unit renders in a single response + # exactly as it does today. + if should_use_incremental_load(request, block): + student_view_context['incremental_load_eager_count'] = settings.INCREMENTAL_LOAD_EAGER_COUNT + set_custom_attribute('incremental_assessment_load', True) + is_learning_mfe = is_request_from_learning_mfe(request) # Right now, we only care about this in regards to the Learning MFE because it results # in a bad UX if we display blocks with access errors (repeated upgrade messaging). diff --git a/lms/envs/common.py b/lms/envs/common.py index 1fc662d26e15..ac9e6acc6e15 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -918,6 +918,24 @@ SEARCH_COURSEWARE_CONTENT_LOG_PARAMS = False +############################# Incremental assessment load ##################### +# Only consulted when the courseware.incremental_assessment_load waffle flag is on for +# the course. See lms/djangoapps/courseware/block_render.py. + +# Units with more estimated problems than this are rendered as a shell -- the first +# INCREMENTAL_LOAD_EAGER_COUNT children inline, the rest as placeholders -- instead of +# rendering every child in one response. Units at or below it are unaffected. +INCREMENTAL_LOAD_PROBLEM_THRESHOLD = 20 + +# How many children a shell response renders inline before it starts emitting +# placeholders. These arrive with the page, so the learner can begin immediately, and +# rendering them also pulls in the CSS/JS the batch-loaded children will need. +INCREMENTAL_LOAD_EAGER_COUNT = 5 + +# Maximum children accepted by a single request to /api/courseware/xblock_children/. +# Requests above this are rejected with a 400. +XBLOCK_CHILDREN_BATCH_MAX = 10 + # .. setting_name: ELASTIC_SEARCH_INDEX_PREFIX # .. setting_default: '' # .. setting_description: Specifies the prefix used when naming elasticsearch indexes related to edx-search. diff --git a/lms/templates/vert_module.html b/lms/templates/vert_module.html index 909e7db829c1..9ea90614b2b5 100644 --- a/lms/templates/vert_module.html +++ b/lms/templates/vert_module.html @@ -59,7 +59,11 @@

${unit_title}

% endfor % endif -
+
% for idx, item in enumerate(items): % if item['content']:
@@ -67,8 +71,24 @@

${unit_title}

%endif % endfor +% if lazy_children: + ## Deliberately not rendered server-side -- vert_module_lazy_bootstrap.html fetches + ## these in batches from XBlockChildren and fills each placeholder in turn. + % for offset, usage_key in enumerate(lazy_children): +
+
+ ${_("Loading question {number}").format(number=len(items) + offset + 1)} +
+
+ % endfor +% endif
+% if lazy_children: + <%include file='vert_module_lazy_bootstrap.html' args="parent_usage_key=parent_usage_key, lazy_children=lazy_children"/> +% endif + % if reset_button:
diff --git a/lms/templates/vert_module_lazy_bootstrap.html b/lms/templates/vert_module_lazy_bootstrap.html new file mode 100644 index 000000000000..44b22e699511 --- /dev/null +++ b/lms/templates/vert_module_lazy_bootstrap.html @@ -0,0 +1,214 @@ +<%page expression_filter="h"/> + +<%! +from django.conf import settings +from django.utils.translation import gettext as _ + +from openedx.core.djangolib.js_utils import dump_js_escaped_json, js_escaped_string +%> + +
+ + diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index 3f4f5fae0ebc..a5c4e8e5b053 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -501,6 +501,100 @@ def test_hidden_after_due(self, is_past_due, masquerade_config, expected_hidden, assert bool(response.data['banner_text']) == expected_banner +class XBlockChildrenApiTestViews(BaseCoursewareTests): + """ + Tests for the xblock_children batch REST API (incremental assessment load) + """ + @classmethod + def setUpClass(cls): + super().setUpClass() + CourseEnrollment.enroll(cls.user, cls.course.id, 'audit') + cls.children = [ + BlockFactory(parent=cls.unit, category='html', display_name=f'child-{i}', data=f'

child {i}

') + for i in range(3) + ] + cls.url = f'/api/courseware/xblock_children/{cls.unit.location}' + + @staticmethod + def _child_keys_param(children): + return ','.join(str(child.location) for child in children) + + def test_batch_renders_requested_children(self): + response = self.client.get(self.url, {'child_usage_keys': self._child_keys_param(self.children)}) + assert response.status_code == 200 + data = response.json() + assert data['parent_usage_key'] == str(self.unit.location) + assert not data['errors'] + returned_keys = {result['usage_key'] for result in data['results']} + assert returned_keys == {str(child.location) for child in self.children} + for result in data['results']: + assert result['html'] + + def test_batch_requires_authentication(self): + self.client.logout() + response = self.client.get(self.url, {'child_usage_keys': self._child_keys_param(self.children)}) + assert response.status_code in (401, 403) + + def test_missing_child_usage_keys_is_a_400(self): + response = self.client.get(self.url) + assert response.status_code == 400 + + def test_invalid_child_usage_key_is_a_400(self): + response = self.client.get(self.url, {'child_usage_keys': 'not-a-usage-key'}) + assert response.status_code == 400 + + def test_oversized_batch_is_a_400(self): + with override_settings(XBLOCK_CHILDREN_BATCH_MAX=2): + response = self.client.get(self.url, {'child_usage_keys': self._child_keys_param(self.children)}) + assert response.status_code == 400 + + def test_child_not_under_this_parent_is_forbidden(self): + # A learner must not be able to pull HTML for a block that isn't among this + # parent's own children -- this is the batch endpoint's security boundary. + # Use parent_location=... (not parent=...) for these scaffolding blocks: passing + # the live object would mutate self.chapter's in-memory .children, leaking into + # other tests -- see SequenceApiTestViews.test_hidden_after_due for the same note. + other_sequence = BlockFactory(parent_location=self.chapter.location, category='sequential') + other_unit = BlockFactory(parent_location=other_sequence.location, category='vertical') + stray_child = BlockFactory(parent_location=other_unit.location, category='html', display_name='stray') + + response = self.client.get(self.url, {'child_usage_keys': str(stray_child.location)}) + assert response.status_code == 200 + data = response.json() + assert not data['results'] + assert data['errors'][0]['usage_key'] == str(stray_child.location) + assert data['errors'][0]['error'] == 'forbidden' + + def test_partial_failure_still_returns_the_rest(self): + # One bad key in the batch shouldn't cost the learner the other, valid ones. + # parent_location=... (not parent=...) for the same reason as + # test_child_not_under_this_parent_is_forbidden above. + other_sequence = BlockFactory(parent_location=self.chapter.location, category='sequential') + other_unit = BlockFactory(parent_location=other_sequence.location, category='vertical') + stray_child = BlockFactory(parent_location=other_unit.location, category='html') + response = self.client.get( + self.url, + {'child_usage_keys': self._child_keys_param(self.children) + ',' + str(stray_child.location)}, + ) + assert response.status_code == 200 + data = response.json() + assert len(data['results']) == len(self.children) + assert len(data['errors']) == 1 + assert data['errors'][0]['usage_key'] == str(stray_child.location) + + def test_parent_with_no_children_forbids_every_requested_key(self): + # A leaf block has no get_children() of its own, so nothing is in its allowed set. + response = self.client.get( + f'/api/courseware/xblock_children/{self.children[0].location}', + {'child_usage_keys': self._child_keys_param(self.children[1:])}, + ) + assert response.status_code == 200 + data = response.json() + assert not data['results'] + assert len(data['errors']) == len(self.children) - 1 + assert all(error['error'] == 'forbidden' for error in data['errors']) + + class ResumeApiTestViews(BaseCoursewareTests, CompletionWaffleTestMixin): """ Tests for the resume API diff --git a/openedx/core/djangoapps/courseware_api/urls.py b/openedx/core/djangoapps/courseware_api/urls.py index 12fdf8cff2b4..a5060dd729f2 100644 --- a/openedx/core/djangoapps/courseware_api/urls.py +++ b/openedx/core/djangoapps/courseware_api/urls.py @@ -15,6 +15,9 @@ re_path(fr'^sequence/{settings.USAGE_KEY_PATTERN}', views.SequenceMetadata.as_view(), name="sequence-api"), + re_path(fr'^xblock_children/{settings.USAGE_KEY_PATTERN}', + views.XBlockChildren.as_view(), + name="xblock-children-api"), re_path(fr'^resume/{settings.COURSE_KEY_PATTERN}', views.Resume.as_view(), name="resume-api"), diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index 1b0bd0c50f5f..cdafc920c7d4 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -13,7 +13,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from rest_framework import status -from rest_framework.exceptions import NotFound +from rest_framework.exceptions import NotFound, ValidationError from rest_framework.generics import RetrieveAPIView from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response @@ -821,6 +821,75 @@ def get(self, request, usage_key_string, *args, **kwargs): # pylint: disable=un return Response(sequence.get_metadata(view=view, context=context)) +class XBlockChildren(DeveloperErrorViewMixin, APIView): + """ + **Use Cases** + + Request a bounded batch of a unit's not-yet-rendered children, rendered as HTML. + Backs the incremental assessment load: VerticalBlock leaves placeholders for + children past its eager-render window, and the page's own JS calls this endpoint + repeatedly, in sequential batches, to fill them in. + + **Example Requests** + + GET /api/courseware/xblock_children/{usage_key}?child_usage_keys=key1,key2 + + **Response Values** + + Body consists of the following fields: + * parent_usage_key: the usage key passed in + * results: list of {usage_key, html} for children that rendered + * errors: list of {usage_key, error, message} for children that didn't + + **Returns** + + * 200 on success, including partial success (see `errors`). + * 400 if child_usage_keys is missing, malformed, or larger than + settings.XBLOCK_CHILDREN_BATCH_MAX. + * 404 if the parent is not available or not visible to this learner. + """ + + authentication_classes = ( + JwtAuthentication, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (IsAuthenticated,) + + def get(self, request, usage_key_string, *args, **kwargs): # lint-amnesty, pylint: disable=unused-argument + """ + Return response to a GET request. + """ + try: + parent_usage_key = UsageKey.from_string(usage_key_string) + except InvalidKeyError as exc: + raise NotFound(f"Invalid usage key: '{usage_key_string}'.") from exc + + child_usage_keys_param = request.GET.get('child_usage_keys', '') + try: + child_usage_keys = [ + UsageKey.from_string(key) for key in child_usage_keys_param.split(',') if key + ] + except InvalidKeyError as exc: + raise ValidationError(f"Invalid child usage key in '{child_usage_keys_param}'.") from exc + + try: + course = get_course_with_access( + request.user, + 'load', + parent_usage_key.course_key, + check_if_enrolled=True, + ) + except CourseAccessRedirect: + raise NotFound() # lint-amnesty, pylint: disable=raise-missing-from + + try: + result = render_xblock_children(request, parent_usage_key, child_usage_keys, course=course) + except ValueError as exc: + raise ValidationError(str(exc)) from exc + + return Response(result) + + class Resume(DeveloperErrorViewMixin, APIView): """ **Use Cases** diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py index 5ca26088f3a6..1b80610fb072 100644 --- a/xmodule/vertical_block.py +++ b/xmodule/vertical_block.py @@ -141,6 +141,8 @@ def _student_or_public_view(self, context, view): # pylint: disable=too-many-st cta_service = self.runtime.service(self, 'call_to_action') vertical_banner_ctas = cta_service.get_ctas(self, 'vertical_banner', completed) if cta_service else [] + lazy_children = [str(child.location) for child in lazy_blocks] + fragment_context = { 'items': contents, 'xblock_context': context, @@ -151,6 +153,8 @@ def _student_or_public_view(self, context, view): # pylint: disable=too-many-st 'has_assignments': completed is not None, 'subsection_format': context.get('format', ''), 'vertical_banner_ctas': vertical_banner_ctas, + 'lazy_children': lazy_children, + 'parent_usage_key': str(self.location) if lazy_children else None, } if view == STUDENT_VIEW: From ff09486d14c507f10accb6dc14f3aba5deb5f7bf Mon Sep 17 00:00:00 2001 From: Aditya Bhalsod Date: Sat, 24 Oct 2026 03:44:25 +0000 Subject: [PATCH 2/7] fix: hydrate learner state for incrementally loaded children Children rendered by the xblock_children endpoint did not see the learner's saved state, so an answered problem rendered as a fresh attempt and resubmitting it failed with "The state of this problem has changed since you loaded this page". Resolve each requested child to a fresh, unbound descriptor and bind it once, after one FieldDataCache populated for the whole batch through the new FieldDataCache.add_block_descendents_batch, so a batch costs a single learner-state lookup. The page now also shows a per-child error with a Retry button for children the endpoint reports in its errors list. Ported from edx/edx-platform#465, originally written by Santhosh Kumar. This commit is the downstream change as it was merged there, except the hunks that do not apply to master. Those are added in "chore: adapt the incremental assessment load port to master". Co-Authored-By: Santhosh Kumar --- lms/djangoapps/courseware/block_render.py | 135 ++++++++++++++---- lms/djangoapps/courseware/model_data.py | 18 +++ .../tests/test_incremental_render.py | 43 ++++++ lms/templates/vert_module_lazy_bootstrap.html | 39 +++++ .../courseware_api/tests/test_views.py | 40 ++++++ 5 files changed, 245 insertions(+), 30 deletions(-) diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 4a646fdcd9cd..c3e863902f4a 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -1177,11 +1177,53 @@ def _allowed_child_usage_keys(parent): get_child_blocks = getattr(parent, 'get_child_blocks', None) if callable(has_dynamic_children) and has_dynamic_children() and callable(get_child_blocks): children = get_child_blocks() - else: - children = parent.get_children() + return {child.location for child in children if child is not None} + + # ``children`` is a content-scoped field containing UsageKeys. Reading it does not + # instantiate/bind the child blocks. Calling ``parent.get_children()`` here would + # create child instances through the parent's shallow field-data cache; those + # instances must not later be rendered with a different learner-state cache. + child_usage_keys = getattr(parent, 'children', None) + if isinstance(child_usage_keys, (list, tuple)): + return {child_usage_key for child_usage_key in child_usage_keys if child_usage_key is not None} + + # Keep compatibility with XBlocks that do not expose their children as a normal + # UsageKey list (and with older test doubles). This fallback is only an access check; + # the endpoint still resolves the objects it renders from the modulestore below. + children = parent.get_children() return {child.location for child in children if child is not None} +def _load_unbound_child_descriptors(child_usage_keys, parent=None): + """ + Resolve child usage keys to fresh, unbound XBlock descriptors. + + The parent is intentionally not used for this lookup. A parent loaded with a shallow + FieldDataCache can already have child instances bound to that cache. Reusing one of + those instances and then passing a second cache to ``get_block_for_descriptor`` is + unsafe because ``bind_for_student`` skips rebinding when the user id is unchanged. + + ``parent`` is optional and is used only to carry over the runtime's export filesystem + setting, which ``XModuleMixin.get_child()`` normally copies to child instances. + """ + descriptors = {} + missing = [] + store = modulestore() + for child_usage_key in child_usage_keys: + try: + descriptor = store.get_item(child_usage_key) + except ItemNotFoundError: + missing.append(child_usage_key) + continue + if descriptor is None: + missing.append(child_usage_key) + continue + if parent is not None: + descriptor.runtime.export_fs = parent.runtime.export_fs + descriptors[child_usage_key] = descriptor + return descriptors, missing + + def _lazy_child_student_view_context(request): """ Build the child render context used by the incremental load endpoint. @@ -1244,11 +1286,9 @@ def render_xblock_children(request, parent_usage_key, child_usage_keys, course=N will_recheck_access=True, ) - allowed_keys = _allowed_child_usage_keys(parent) - children_by_key = {child.location: child for child in parent.get_children() if child is not None} - requested = [] errors = [] + allowed_keys = _allowed_child_usage_keys(parent) for child_key in child_usage_keys: if child_key in allowed_keys: requested.append(child_key) @@ -1263,21 +1303,72 @@ def render_xblock_children(request, parent_usage_key, child_usage_keys, course=N if not requested: return {'parent_usage_key': str(parent_usage_key), 'results': results, 'errors': errors} + # Resolve from the modulestore rather than reusing parent.get_children() instances. + # The latter may already be bound to the parent's depth-0 cache for this same user. + # Those instances would then skip rebinding when get_block_for_descriptor() is + # called with the batch cache, causing a valid StudentModule row to look absent. + children_by_key, missing_keys = _load_unbound_child_descriptors(requested, parent=parent) + for child_key in missing_keys: + errors.append({ + 'usage_key': str(child_key), + 'error': 'not_found', + 'message': 'Block could not be loaded.', + }) + + renderable_children = [ + children_by_key[child_key] for child_key in requested if child_key in children_by_key + ] + # One cache for the whole batch, not one per child. Depth=1 pulls in each child's # own selected problem (for library_content/item_bank children) without recursing # further -- matching the shape FieldDataCache._children_to_prefetch already - # narrows to for such blocks. + # narrows to for such blocks. The batch helper performs one state lookup for the + # complete set instead of one get_many() call per child. field_data_cache = FieldDataCache([], course_key, user, read_only=CrawlersConfig.is_crawler(request)) + field_data_cache.add_block_descendents_batch(renderable_children, depth=1) + + # Bind each fresh descriptor exactly once, after the cache containing the learner's + # state has been populated. Besides fixing state hydration, doing this before the + # completion/access pass keeps those checks aligned with the eventual render. + instances_by_key = {} for child_key in requested: child = children_by_key.get(child_key) if child is None: + continue + try: + instance = get_block_for_descriptor( + user, + request, + child, + field_data_cache, + course_key, + course=course, + will_recheck_access=True, + ) + except Exception as exc: # pylint: disable=broad-except + log.exception('Incremental load: child %s under %s failed to bind', child_key, parent_usage_key) + errors.append({ + 'usage_key': str(child_key), + 'error': 'render_failed', + 'message': str(exc), + }) + continue + if instance is None: errors.append({ 'usage_key': str(child_key), - 'error': 'not_found', - 'message': 'Block could not be loaded.', + 'error': 'forbidden', + 'message': 'Learner does not have access to this block.', }) continue - field_data_cache.add_block_descendents(child, depth=1) + log.debug( + 'Incremental load: bound child %s for parent %s with the batch state cache; ' + 'user_id before=%s after=%s', + child_key, + parent_usage_key, + getattr(child.scope_ids, 'user_id', None), + getattr(instance.scope_ids, 'user_id', None), + ) + instances_by_key[child_key] = instance # Both computed once for the whole batch, not per child, matching how # VerticalBlock computes them once for its own eager-rendered children. @@ -1287,7 +1378,7 @@ def render_xblock_children(request, parent_usage_key, child_usage_keys, course=N complete_on_view_delay = None if completion_service and completion_service.completion_tracking_enabled(): requested_children = [ - children_by_key[child_key] for child_key in requested if child_key in children_by_key + instances_by_key[child_key] for child_key in requested if child_key in instances_by_key ] child_blocks_to_complete_on_view = completion_service.blocks_to_mark_complete_on_view( requested_children @@ -1295,43 +1386,27 @@ def render_xblock_children(request, parent_usage_key, child_usage_keys, course=N complete_on_view_delay = completion_service.get_complete_on_view_delay_ms() for child_key in requested: - child = children_by_key.get(child_key) - if child is None: + instance = instances_by_key.get(child_key) + if instance is None: continue child_context = _lazy_child_student_view_context(request) if ( child_context.get('hide_access_error_blocks') and callable(block_has_access_error) - and block_has_access_error(child) + and block_has_access_error(instance) ): # Matches VerticalBlock's own eager-render skip (block_has_access_error): # a block gated for this learner must stay hidden whether it renders # inline or gets lazy-loaded later. results.append({'usage_key': str(child_key), 'html': ''}) continue - if child in child_blocks_to_complete_on_view: + if instance in child_blocks_to_complete_on_view: child_context['wrap_xblock_data'] = { 'mark-completed-on-view-after-delay': complete_on_view_delay } try: - instance = get_block_for_descriptor( - user, - request, - child, - field_data_cache, - course_key, - course=course, - will_recheck_access=True, - ) - if instance is None: - errors.append({ - 'usage_key': str(child_key), - 'error': 'forbidden', - 'message': 'Learner does not have access to this block.', - }) - continue instance, child_context = VerticalBlockChildRenderStarted.run_filter( block=instance, context=child_context diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 6218b0e9f3e7..21102776cb6f 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -733,6 +733,24 @@ def add_block_descendents(self, block, depth=None, block_filter=lambda block: Tr should be cached """ + self.add_block_descendents_batch([block], depth=depth, block_filter=block_filter) + + def add_block_descendents_batch(self, blocks, depth=None, block_filter=lambda block: True): + """ + Add descendants of several blocks in one state-cache population operation. + + This is the batch equivalent of :meth:`add_block_descendents`. It is useful for + endpoints that render several sibling blocks independently: walking each sibling + separately and calling ``add_blocks_to_cache`` each time causes one user-state + lookup per sibling even though all of the data belongs to the same request. + + The supplied blocks must belong to the same course as this cache. Duplicate usage + keys are removed before the fields are read so shared required descriptors do not + cause duplicate work. + """ + if not blocks: + return + def get_child_blocks(block, depth, block_filter): """ Return a list of all child blocks down to the specified depth diff --git a/lms/djangoapps/courseware/tests/test_incremental_render.py b/lms/djangoapps/courseware/tests/test_incremental_render.py index 0d0581e9f8bb..5d264814864e 100644 --- a/lms/djangoapps/courseware/tests/test_incremental_render.py +++ b/lms/djangoapps/courseware/tests/test_incremental_render.py @@ -6,9 +6,11 @@ from django.test import RequestFactory, TestCase, override_settings from opaque_keys.edx.locator import CourseLocator +from xmodule.modulestore.exceptions import ItemNotFoundError from lms.djangoapps.courseware.block_render import ( _allowed_child_usage_keys, + _load_unbound_child_descriptors, _lazy_child_student_view_context, count_renderable_descendants, should_use_incremental_load, @@ -134,6 +136,14 @@ def test_static_parent_exposes_its_children(self): parent = _block('vertical', children=children) assert _allowed_child_usage_keys(parent) == {child.location for child in children} + def test_static_parent_uses_content_keys_without_instantiating_children(self): + children = [_block('problem'), _block('problem')] + parent = _block('vertical') + parent.children = [child.location for child in children] + parent.get_children.side_effect = AssertionError('static children must not be instantiated') + + assert _allowed_child_usage_keys(parent) == {child.location for child in children} + def test_dynamic_parent_exposes_only_the_learner_selection(self): # A learner must not be able to pull problems the bank did not assign them. selected = _block('problem') @@ -151,6 +161,39 @@ def test_none_children_are_skipped(self): assert _allowed_child_usage_keys(parent) == {good.location} +class UnboundChildDescriptorTests(TestCase): + """Tests for resolving lazy children independently of a parent instance cache.""" + + def test_resolves_requested_keys_without_using_parent_children(self): + first_key = COURSE_KEY.make_usage_key('problem', 'first') + second_key = COURSE_KEY.make_usage_key('problem', 'second') + first_descriptor = Mock() + second_descriptor = Mock() + store = Mock() + store.get_item.side_effect = [first_descriptor, second_descriptor] + + with patch('lms.djangoapps.courseware.block_render.modulestore', return_value=store): + descriptors, missing = _load_unbound_child_descriptors([first_key, second_key]) + + assert list(descriptors) == [first_key, second_key] + assert descriptors[first_key] is first_descriptor + assert descriptors[second_key] is second_descriptor + assert not missing + + def test_reports_missing_descriptor_without_aborting_the_batch(self): + existing_key = COURSE_KEY.make_usage_key('problem', 'existing') + missing_key = COURSE_KEY.make_usage_key('problem', 'missing') + existing_descriptor = Mock() + store = Mock() + store.get_item.side_effect = [existing_descriptor, ItemNotFoundError()] + + with patch('lms.djangoapps.courseware.block_render.modulestore', return_value=store): + descriptors, missing = _load_unbound_child_descriptors([existing_key, missing_key]) + + assert descriptors == {existing_key: existing_descriptor} + assert missing == [missing_key] + + class LazyChildStudentViewContextTests(TestCase): """ Tests for the child render context used by incremental batch rendering. diff --git a/lms/templates/vert_module_lazy_bootstrap.html b/lms/templates/vert_module_lazy_bootstrap.html index 44b22e699511..e31772eed0ab 100644 --- a/lms/templates/vert_module_lazy_bootstrap.html +++ b/lms/templates/vert_module_lazy_bootstrap.html @@ -115,6 +115,42 @@ initializeChild(el); } + function markChildError(usageKey, message) { + var el = container.querySelector('[data-usage-key="' + usageKey + '"]'); + if (!el) { + return; + } + + el.innerHTML = ''; + el.classList.remove('vert-lazy-pending'); + el.classList.add('vert-lazy-error'); + el.setAttribute('aria-busy', 'false'); + el.setAttribute('role', 'alert'); + + var messageEl = document.createElement('span'); + messageEl.textContent = message || messages.failed; + el.appendChild(messageEl); + + var retryButton = document.createElement('button'); + retryButton.type = 'button'; + retryButton.className = 'btn btn-link vert-lazy-retry'; + retryButton.textContent = messages.retry; + retryButton.addEventListener('click', function () { + el.innerHTML = ''; + el.classList.remove('vert-lazy-error'); + el.classList.add('vert-lazy-pending'); + el.removeAttribute('role'); + el.setAttribute('aria-busy', 'true'); + reportProgress(); + loadBatch([usageKey]).catch(function (err) { + markChildError(usageKey, err.message); + reportProgress(); + }); + }); + el.appendChild(document.createTextNode(' ')); + el.appendChild(retryButton); + } + function fetchBatch(keys) { var url = endpoint + '?child_usage_keys=' + encodeURIComponent(keys.join(',')); var controller = window.AbortController ? new window.AbortController() : null; @@ -168,6 +204,9 @@ (data.results || []).forEach(function (result) { fillChild(result.usage_key, result.html); }); + (data.errors || []).forEach(function (error) { + markChildError(error.usage_key, error.message); + }); postResize(); reportProgress(); }, function (err) { diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index a5c4e8e5b053..0cf129c7493e 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -530,6 +530,46 @@ def test_batch_renders_requested_children(self): for result in data['results']: assert result['html'] + def test_batch_renders_persisted_problem_state(self): + """ + A problem rendered after the eager window must use the same StudentModule state + as the normal problem handler, rather than appearing as a fresh attempt. + """ + problem_xml = OptionResponseXMLFactory().build_xml( + question_text='The correct answer is Correct', + num_inputs=1, + weight=1, + options=['Correct', 'Incorrect'], + correct_option='Correct', + ) + problem = BlockFactory.create( + parent_location=self.unit.location, + category='problem', + data=problem_xml, + display_name='persisted-problem', + ) + + problem_check_url = reverse( + 'xblock_handler', + kwargs={ + 'course_id': str(self.course.id), + 'usage_id': quote_slashes(str(problem.location)), + 'handler': 'xmodule_handler', + 'suffix': 'problem_check', + }, + ) + answer_key = f'input_{problem.location.html_id()}_2_1' + submit_response = self.client.post(problem_check_url, {answer_key: 'Correct'}) + assert submit_response.status_code == 200 + + response = self.client.get(self.url, {'child_usage_keys': str(problem.location)}) + assert response.status_code == 200 + data = response.json() + assert not data['errors'] + rendered_html = data['results'][0]['html'] + assert 'data-attempts-used="1"' in rendered_html + assert answer_key in rendered_html + def test_batch_requires_authentication(self): self.client.logout() response = self.client.get(self.url, {'child_usage_keys': self._child_keys_param(self.children)}) From 47b319b0d08686fdb0be7d9cede93b53415321b7 Mon Sep 17 00:00:00 2001 From: Aditya Bhalsod Date: Sat, 24 Oct 2026 03:44:25 +0000 Subject: [PATCH 3/7] fix: hydrate item bank problems and staff debug in lazily loaded units Problems a randomized bank (item_bank / library_content) selected rendered as unanswered when they fell past the incremental-load eager window. The bank's selected problem is loaded through the runtime the parent vertical bound with its own depth-0 FieldDataCache, so it never saw the learner state prefetched into the separate batch cache. Resubmitting such a problem then failed with "The state of this problem has changed since you loaded this page". Bind the parent and the batch to one FieldDataCache so every block loaded through the shared runtime sees the same state. Lazily inserted children were also set with innerHTML, which never runs inline scripts, so setup_debug() from staff_problem_info.html never ran and Staff Debug Info / Submission History did nothing past the eager window. Insert them with edx.HtmlUtils.setHtml, which runs them as an inline render does and marks the server-rendered fragment trusted the same way capa does. Ported from edx/edx-platform#529. Co-Authored-By: Claude Opus 5.5 --- lms/djangoapps/courseware/block_render.py | 40 +++++++++------ lms/templates/vert_module_lazy_bootstrap.html | 12 ++++- .../courseware_api/tests/test_views.py | 49 ++++++++++++++++--- 3 files changed, 79 insertions(+), 22 deletions(-) diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index c3e863902f4a..6c27419e4a69 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -861,7 +861,7 @@ def _get_block_by_usage_key(usage_key): def get_block_by_usage_id(request, course_id, usage_id, disable_staff_debug_info=False, course=None, - will_recheck_access=False, field_data_cache_depth=None): + will_recheck_access=False, field_data_cache_depth=None, field_data_cache=None): """ Gets a block instance based on its `usage_id` in a course, for a given request/user @@ -871,6 +871,10 @@ def get_block_by_usage_id(request, course_id, usage_id, disable_staff_debug_info descendant, matching historical behavior. Used by the incremental assessment load (see render_xblock_children) so a batch of children only prefetches state for themselves, not their own descendants. + field_data_cache: optional FieldDataCache to populate and bind the block with, + instead of building a new one. The runtime binding the block is shared with + every block loaded through it later in the request, so a caller that adds more + state to this cache afterwards makes that state visible to those blocks too. Returns (instance, tracking_context) """ @@ -879,13 +883,16 @@ def get_block_by_usage_id(request, course_id, usage_id, disable_staff_debug_info block, tracking_context = _get_block_by_usage_key(usage_key) _, user = setup_masquerade(request, course_key, has_access(request.user, 'staff', block, course_key)) - field_data_cache = FieldDataCache.cache_for_block_descendents( - course_key, - user, - block, - depth=field_data_cache_depth, - read_only=CrawlersConfig.is_crawler(request), - ) + if field_data_cache is None: + field_data_cache = FieldDataCache.cache_for_block_descendents( + course_key, + user, + block, + depth=field_data_cache_depth, + read_only=CrawlersConfig.is_crawler(request), + ) + else: + field_data_cache.add_block_descendents(block, depth=field_data_cache_depth) instance = get_block_for_descriptor( user, request, @@ -1276,6 +1283,12 @@ def render_xblock_children(request, parent_usage_key, child_usage_keys, course=N staff_access = bool(has_access(request.user, 'staff', course_key)) _, user = setup_masquerade(request, course_key, staff_access) + # One cache for the parent and the whole batch. Binding the parent prepares the shared + # runtime around this cache, and blocks a child loads itself through that runtime -- + # e.g. the problem an item_bank / library_content child selected -- are bound with it + # too, so they only see the learner's state if it is added to this same cache. + field_data_cache = FieldDataCache([], course_key, user, read_only=CrawlersConfig.is_crawler(request)) + with modulestore().bulk_operations(course_key): parent, _ = get_block_by_usage_id( request, @@ -1284,6 +1297,7 @@ def render_xblock_children(request, parent_usage_key, child_usage_keys, course=N course=course, field_data_cache_depth=0, will_recheck_access=True, + field_data_cache=field_data_cache, ) requested = [] @@ -1319,12 +1333,10 @@ def render_xblock_children(request, parent_usage_key, child_usage_keys, course=N children_by_key[child_key] for child_key in requested if child_key in children_by_key ] - # One cache for the whole batch, not one per child. Depth=1 pulls in each child's - # own selected problem (for library_content/item_bank children) without recursing - # further -- matching the shape FieldDataCache._children_to_prefetch already - # narrows to for such blocks. The batch helper performs one state lookup for the - # complete set instead of one get_many() call per child. - field_data_cache = FieldDataCache([], course_key, user, read_only=CrawlersConfig.is_crawler(request)) + # One state lookup for the whole batch, not one per child. Depth=1 pulls in each + # child's own selected problem (for library_content/item_bank children) without + # recursing further -- matching the shape FieldDataCache._children_to_prefetch + # already narrows to for such blocks. field_data_cache.add_block_descendents_batch(renderable_children, depth=1) # Bind each fresh descriptor exactly once, after the cache containing the learner's diff --git a/lms/templates/vert_module_lazy_bootstrap.html b/lms/templates/vert_module_lazy_bootstrap.html index e31772eed0ab..2eaa315da551 100644 --- a/lms/templates/vert_module_lazy_bootstrap.html +++ b/lms/templates/vert_module_lazy_bootstrap.html @@ -109,7 +109,17 @@ if (!el) { return; } - el.innerHTML = html || ''; + // innerHTML never runs inline