diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 59fcc725ed36..4868e7c4736c 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -660,6 +660,27 @@ def _cache_key_for_kvs_key(self, key): return key.field_name +def _children_for_field_data_cache(block, read_only=False): + """ + Return child blocks whose field data should be prefetched for ``block``. + + Dynamic blocks such as library_content expose a learner-specific subset via + ``get_child_blocks()``. Prefetching all modulestore children (``get_children()``) + loads user state for blocks that will never render for the current learner. + """ + get_child_blocks = getattr(block, 'get_child_blocks', None) + get_child_blocks_for_prefetch = getattr(block, 'get_child_blocks_for_prefetch', None) + has_dynamic_children = getattr(block, 'has_dynamic_children', None) + if callable(has_dynamic_children) and has_dynamic_children(): + if read_only and callable(get_child_blocks_for_prefetch): + return list(get_child_blocks_for_prefetch()) + + if callable(get_child_blocks): + return list(get_child_blocks()) + + return list(block.get_children()) + list(block.get_required_block_descriptors()) + + class FieldDataCache: """ A cache of django model objects needed to supply the data @@ -731,7 +752,7 @@ def add_block_descendents(self, block, depth=None, block_filter=lambda block: Tr should be cached """ - def get_child_blocks(block, depth, block_filter): + def collect_descendant_blocks(block, depth, block_filter): """ Return a list of all child blocks down to the specified depth that match the block filter. Includes `block` @@ -749,13 +770,15 @@ def get_child_blocks(block, depth, block_filter): if depth is None or depth > 0: new_depth = depth - 1 if depth is not None else depth - for child in block.get_children() + block.get_required_block_descriptors(): - blocks.extend(get_child_blocks(child, new_depth, block_filter)) + for child in _children_for_field_data_cache(block, read_only=self.read_only): + if child is None: + continue + blocks.extend(collect_descendant_blocks(child, new_depth, block_filter)) return blocks with modulestore().bulk_operations(block.location.course_key): - blocks = get_child_blocks(block, depth, block_filter) + blocks = collect_descendant_blocks(block, depth, block_filter) self.add_blocks_to_cache(blocks) diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 6c763d57b338..9ba608c7af6d 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -13,7 +13,13 @@ from xblock.fields import BlockScope, Scope, ScopeIds from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache, InvalidScopeError +from lms.djangoapps.courseware.model_data import ( + DjangoKeyValueStore, + FieldDataCache, + InvalidScopeError, + UserStateCache, + _children_for_field_data_cache, +) from lms.djangoapps.courseware.models import ( StudentModule, XModuleStudentInfoField, @@ -444,3 +450,118 @@ class TestStudentInfoStorage(OtherUserFailureTestMixin, StorageTestBase, TestCas storage_class = XModuleStudentInfoField other_key_factory = partial(DjangoKeyValueStore.Key, Scope.user_info, 2, 'mock_problem') # user_id=2, not 1 existing_field_name = "existing_field" + + +class TestFieldDataCacheDynamicChildren(TestCase): + """Tests for dynamic-child handling in FieldDataCache descendant prefetch.""" + + def test_children_for_field_data_cache_uses_get_child_blocks(self): + """ + Dynamic blocks should only expose learner-selected children for prefetch. + """ + selected_child = Mock(name='selected_child') + dynamic_block = Mock(name='dynamic_block') + dynamic_block.has_dynamic_children.return_value = True + dynamic_block.get_child_blocks.return_value = [selected_child] + dynamic_block.get_children.side_effect = AssertionError( + 'get_children should not be called for dynamic blocks' + ) + + assert _children_for_field_data_cache(dynamic_block) == [selected_child] + dynamic_block.get_child_blocks.assert_called_once_with() + dynamic_block.get_children.assert_not_called() + + def test_children_for_field_data_cache_uses_prefetch_api_in_read_only_mode(self): + """ + Read-only prefetch should use a side-effect-free dynamic-child API when available. + """ + selected_child = Mock(name='selected_child') + dynamic_block = Mock(name='dynamic_block') + dynamic_block.has_dynamic_children.return_value = True + dynamic_block.get_child_blocks_for_prefetch.return_value = [selected_child] + dynamic_block.get_child_blocks.side_effect = AssertionError( + 'get_child_blocks should not be called for read-only prefetch' + ) + dynamic_block.get_children.side_effect = AssertionError( + 'get_children should not be called for dynamic blocks' + ) + + assert _children_for_field_data_cache(dynamic_block, read_only=True) == [selected_child] + dynamic_block.get_child_blocks_for_prefetch.assert_called_once_with() + dynamic_block.get_child_blocks.assert_not_called() + dynamic_block.get_children.assert_not_called() + + def test_children_for_field_data_cache_uses_get_children_for_static_blocks(self): + """ + Static blocks should continue to prefetch all modulestore children. + """ + static_child = Mock(name='static_child') + static_block = Mock(name='static_block') + static_block.has_dynamic_children.return_value = False + static_block.get_children.return_value = [static_child] + static_block.get_required_block_descriptors.return_value = [] + + assert _children_for_field_data_cache(static_block) == [static_child] + static_block.get_children.assert_called_once_with() + static_block.get_required_block_descriptors.assert_called_once_with() + + @patch('lms.djangoapps.courseware.model_data.modulestore') + def test_add_block_descendents_prefetches_only_selected_dynamic_children(self, mock_modulestore): + """ + add_block_descendents should not walk unselected modulestore children. + """ + mock_modulestore.return_value.bulk_operations.return_value.__enter__ = Mock(return_value=None) + mock_modulestore.return_value.bulk_operations.return_value.__exit__ = Mock(return_value=False) + + user_state_field = mock_field(Scope.user_state, 'state') + + def configure_static_block(block): + block.get_children.return_value = [] + block.get_required_block_descriptors.return_value = [] + block.has_dynamic_children.return_value = False + block.fields.values.return_value = [user_state_field] + block.has_score = False + block.location = LOCATION('usage_id') + + unselected_children = [Mock(name=f'unselected_{index}') for index in range(3)] + selected_children = [Mock(name='selected_0'), Mock(name='selected_1')] + for child in selected_children + unselected_children: + configure_static_block(child) + + library_content = Mock(name='library_content') + library_content.has_dynamic_children.return_value = True + library_content.get_child_blocks.return_value = selected_children + library_content.get_children.return_value = unselected_children + selected_children + library_content.get_required_block_descriptors.return_value = [] + library_content.fields.values.return_value = [user_state_field] + library_content.has_score = False + library_content.location = Mock(course_key=COURSE_KEY) + + vertical = Mock(name='vertical') + vertical.has_dynamic_children.return_value = False + vertical.get_children.return_value = [library_content] + vertical.get_required_block_descriptors.return_value = [] + vertical.fields.values.return_value = [user_state_field] + vertical.has_score = False + vertical.location = Mock(course_key=COURSE_KEY) + + user = UserFactory.create(username='dynamic_children_user') + field_data_cache = FieldDataCache([], COURSE_KEY, user) + + cached_blocks = [] + + def capture_cache_fields(fields, blocks, aside_types): # lint-amnesty, pylint: disable=unused-argument + cached_blocks.extend(blocks) + + with patch.object(UserStateCache, 'cache_fields', side_effect=capture_cache_fields): + field_data_cache.add_block_descendents(vertical) + + cached_block_names = {block._mock_name for block in cached_blocks} # pylint: disable=protected-access + assert 'vertical' in cached_block_names + assert 'library_content' in cached_block_names + assert 'selected_0' in cached_block_names + assert 'selected_1' in cached_block_names + assert 'unselected_0' not in cached_block_names + assert 'unselected_1' not in cached_block_names + assert 'unselected_2' not in cached_block_names + library_content.get_child_blocks.assert_called_once_with() diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index b53617e2c8e4..8b2c717511a6 100644 --- a/xmodule/item_bank_block.py +++ b/xmodule/item_bank_block.py @@ -238,7 +238,7 @@ def publish_selected_children_events(cls, block_keys, format_block_keys, publish added=format_block_keys(block_keys['added']) ) - def selected_children(self): + def selected_children(self, read_only=False): """ Returns a [] of block_ids indicating which of the possible children have been selected to display to the current user. @@ -254,19 +254,20 @@ def selected_children(self): max_count = len(self.children) block_keys = self.make_selection(self.selected, self.children, max_count) # pylint: disable=no-member + selected = block_keys['selected'] - self.publish_selected_children_events( - block_keys, - self.format_block_keys_for_analytics, - self._publish_event, - ) + if not read_only: + self.publish_selected_children_events( + block_keys, + self.format_block_keys_for_analytics, + self._publish_event, + ) - if any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')): + if not read_only and any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')): # Save our selections to the user state, to ensure consistency: - selected = block_keys['selected'] self.selected = selected # TODO: this doesn't save from the LMS "Progress" page. - return self.selected + return selected if read_only else self.selected def format_block_keys_for_analytics(self, block_keys: list[tuple[str, str]]) -> list[dict]: """ @@ -303,14 +304,20 @@ def reset_selected_children(self, _, __): self.selected = [] return Response(json.dumps(self.student_view({}).content)) - def _get_selected_child_blocks(self): + def _get_selected_child_blocks(self, read_only=False): """ Generator returning XBlock instances of the children selected for the current user. """ - for block_type, block_id in self.selected_children(): + for block_type, block_id in self.selected_children(read_only=read_only): yield self.runtime.get_block(self.context_key.make_usage_key(block_type, block_id)) + def get_child_blocks_for_prefetch(self): + """ + Return the learner-selected child blocks without publishing analytics or mutating user state. + """ + return list(self._get_selected_child_blocks(read_only=True)) + def student_view(self, context): # lint-amnesty, pylint: disable=missing-function-docstring fragment = Fragment() contents = [] diff --git a/xmodule/tests/test_item_bank.py b/xmodule/tests/test_item_bank.py index c27412c8b709..f0c54b498398 100644 --- a/xmodule/tests/test_item_bank.py +++ b/xmodule/tests/test_item_bank.py @@ -232,6 +232,18 @@ def test_children_seen_by_a_user(self): # Check that get_content_titles() doesn't return titles for hidden/unused children assert len(self.item_bank.get_content_titles()) == 1 + def test_prefetch_child_blocks_does_not_publish_or_persist_selection(self): + """ + Read-only child selection should avoid analytics events and user-state writes. + """ + self._bind_course_block(self.item_bank) + + selected_children = self.item_bank.get_child_blocks_for_prefetch() + + assert len(selected_children) == 1 + self.publisher.assert_not_called() + assert self.item_bank.selected == [] + def test_overlimit_blocks_chosen_randomly(self): """ Tests that blocks to remove from selected children are chosen