diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 8ef637b82bf0..4eecb605c974 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 VerticalBlockChildRenderStarted from rest_framework.decorators import api_view from rest_framework.exceptions import APIException from web_fragments.fragment import Fragment @@ -61,6 +62,7 @@ ) from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache from lms.djangoapps.courseware.services import UserStateService +from lms.djangoapps.courseware.toggles import incremental_assessment_load_is_enabled from lms.djangoapps.grades.api import GradesUtilService from lms.djangoapps.lms_xblock.field_data import LmsFieldData from lms.djangoapps.lms_xblock.runtime import UserTagsService, lms_applicable_aside_types, lms_wrappers_aside @@ -79,6 +81,7 @@ from openedx.core.lib.cache_utils import CacheService from openedx.core.lib.gating.services import GatingService from openedx.core.lib.license import wrap_with_license +from openedx.core.lib.mobile_utils import is_request_from_mobile_app from openedx.core.lib.url_utils import quote_slashes, unquote_slashes from openedx.core.lib.xblock_services.call_to_action import CallToActionService from openedx.core.lib.xblock_utils import ( @@ -91,6 +94,7 @@ from openedx.core.lib.xblock_utils import request_token as xblock_request_token from openedx.features.content_type_gating.services import ContentTypeGatingService from openedx.features.course_duration_limits.access import course_expiration_wrapper +from openedx.features.course_experience.url_helpers import is_request_from_learning_mfe from openedx.features.discounts.utils import offer_banner_wrapper from xmodule.contentstore.django import contentstore from xmodule.exceptions import NotFoundError as XModuleNotFoundError @@ -106,6 +110,7 @@ XQueueService, ) from xmodule.util.sandboxing import SandboxService +from xmodule.x_module import STUDENT_VIEW if TYPE_CHECKING: from rest_framework.request import Request @@ -861,10 +866,21 @@ 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, field_data_cache=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. + 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) """ course_key = CourseKey.from_string(course_id) @@ -872,12 +888,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, - 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, @@ -1081,3 +1101,344 @@ 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 (blocks whose has_dynamic_children() is True, such as + 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. + """ + 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 (has_dynamic_children() is True, such as library_content / item_bank) + expose only the learner's persisted selection via get_child_blocks(); everything else + exposes its ordinary children. + """ + 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() + 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. + + 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) + + # 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, + str(course_key), + str(parent_usage_key), + course=course, + field_data_cache_depth=0, + will_recheck_access=True, + field_data_cache=field_data_cache, + ) + + 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) + 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} + + # 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 state lookup for the whole batch, not one per child. Depth=1 also covers the + # problems inside each library_content/item_bank child, without recursing further. + 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': 'forbidden', + 'message': 'Learner does not have access to this block.', + }) + continue + 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. + 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 = [ + 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 + ) + complete_on_view_delay = completion_service.get_complete_on_view_delay_ms() + + for child_key in requested: + 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(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 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, 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/model_data.py b/lms/djangoapps/courseware/model_data.py index 6218b0e9f3e7..ec6c138d35e7 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 @@ -756,10 +774,16 @@ def get_child_blocks(block, depth, block_filter): return blocks - with modulestore().bulk_operations(block.location.course_key): - blocks = get_child_blocks(block, depth, block_filter) + blocks_to_cache = [] + seen_locations = set() + with modulestore().bulk_operations(blocks[0].location.course_key): + for block in blocks: + for descendant in get_child_blocks(block, depth, block_filter): + if descendant.location not in seen_locations: + seen_locations.add(descendant.location) + blocks_to_cache.append(descendant) - self.add_blocks_to_cache(blocks) + self.add_blocks_to_cache(blocks_to_cache) @classmethod def cache_for_block_descendents(cls, course_id, user, block, depth=None, 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..b9919838913c --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_incremental_render.py @@ -0,0 +1,311 @@ +""" +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, + _load_unbound_child_descriptors, + count_renderable_descendants, + should_use_incremental_load, +) +from xmodule.modulestore.exceptions import ItemNotFoundError + +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_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') + 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 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. + """ + + 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'
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 _create_problem(self, parent_location): + """ + Create a one-input option-response problem under `parent_location`. + """ + problem_xml = OptionResponseXMLFactory().build_xml( + question_text='The correct answer is Correct', + num_inputs=1, + weight=1, + options=['Correct', 'Incorrect'], + correct_option='Correct', + ) + return BlockFactory.create( + parent_location=parent_location, + category='problem', + data=problem_xml, + display_name='persisted-problem', + ) + + def _submit_correct_answer(self, problem): + """ + Submit `problem` through the normal problem_check handler; return its answer input key. + """ + 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 + return answer_key + + def _render_single_child(self, child): + """ + Fetch one child through the batch endpoint and return its rendered HTML. + """ + response = self.client.get(self.url, {'child_usage_keys': str(child.location)}) + assert response.status_code == 200 + data = response.json() + assert not data['errors'] + return data['results'][0]['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 = self._create_problem(self.unit.location) + answer_key = self._submit_correct_answer(problem) + + rendered_html = self._render_single_child(problem) + assert 'data-attempts-used="1"' in rendered_html + assert answer_key in rendered_html + + def test_batch_renders_persisted_item_bank_problem_state(self): + """ + A problem the learner was assigned from a randomized bank (item_bank / + library_content) is loaded by the bank through the shared runtime, not by this + endpoint, so it must still see the learner's saved state. Otherwise it renders as + unanswered, and resubmitting fails with "The state of this problem has changed". + """ + bank = BlockFactory.create(parent_location=self.unit.location, category='itembank', max_count=1) + problem = self._create_problem(bank.location) + + # First visit assigns and persists the learner's selection, as it would in courseware. + assert 'data-attempts-used="0"' in self._render_single_child(bank) + self._submit_correct_answer(problem) + + rendered_html = self._render_single_child(bank) + assert 'data-attempts-used="1"' 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)}) + 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..db22483bd056 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 @@ -34,7 +34,7 @@ from lms.djangoapps.course_goals.models import UserActivity from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.access_utils import check_public_access -from lms.djangoapps.courseware.block_render import get_block_by_usage_id +from lms.djangoapps.courseware.block_render import get_block_by_usage_id, render_xblock_children from lms.djangoapps.courseware.context_processor import user_timezone_locale_prefs from lms.djangoapps.courseware.courses import ( get_course_about_section, @@ -42,6 +42,7 @@ get_permission_for_course_about, ) from lms.djangoapps.courseware.entrance_exams import course_has_entrance_exam, user_has_passed_entrance_exam +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from lms.djangoapps.courseware.masquerade import ( is_masquerading_as_non_audit_enrollment, is_masquerading_as_specific_student, @@ -821,6 +822,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 as exc: + raise NotFound() from exc + + 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..8517f8f8741e 100644 --- a/xmodule/vertical_block.py +++ b/xmodule/vertical_block.py @@ -99,17 +99,29 @@ def _student_or_public_view(self, context, view): # pylint: disable=too-many-st child_blocks = self.get_children() # pylint: disable=no-member + # Large assessment units render only the first `incremental_load_eager_count` + # children here; the rest are left as placeholders (`lazy_children`) for the page's + # own JS to fill in via batched calls to XBlockChildren. Absent that context key, + # every child renders inline, exactly as before. + eager_count = (context or {}).get('incremental_load_eager_count') + if eager_count is not None and eager_count < len(child_blocks): + eager_blocks = child_blocks[:eager_count] + lazy_blocks = child_blocks[eager_count:] + else: + eager_blocks = child_blocks + lazy_blocks = [] + child_blocks_to_complete_on_view = set() completion_service = self.runtime.service(self, 'completion') if completion_service and completion_service.completion_tracking_enabled(): - child_blocks_to_complete_on_view = completion_service.blocks_to_mark_complete_on_view(child_blocks) + child_blocks_to_complete_on_view = completion_service.blocks_to_mark_complete_on_view(eager_blocks) complete_on_view_delay = completion_service.get_complete_on_view_delay_ms() child_context['child_of_vertical'] = True is_child_of_vertical = context.get('child_of_vertical', False) # pylint: disable=no-member - for child in child_blocks: + for child in eager_blocks: child_has_access_error = self.block_has_access_error(child) if context.get('hide_access_error_blocks') and child_has_access_error: continue @@ -141,6 +153,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 +165,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: