From d8536c183b75b678aee51370591f6f42bc205426 Mon Sep 17 00:00:00 2001 From: Maria Fernanda Magallanes Zubillaga Date: Wed, 31 Dec 2025 14:54:16 -0500 Subject: [PATCH 1/5] fix: apply library_content transformer to all ItemBankMixin xblocks --- .../transformers/library_content.py | 93 ++++++++++++++----- 1 file changed, 69 insertions(+), 24 deletions(-) diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 01b573168358..ae75f6b0181b 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -1,5 +1,8 @@ """ -Content Library Transformer. +Item Bank Transformer. + +Transformers for handling item bank blocks (library_content, itembank, etc.) +that use ItemBankMixin for randomized content selection. """ @@ -7,6 +10,7 @@ import logging from eventtracking import tracker +from xblock.core import XBlock from common.djangoapps.track import contexts from lms.djangoapps.courseware.models import StudentModule @@ -14,21 +18,40 @@ BlockStructureTransformer, FilteringTransformerMixin, ) -from xmodule.library_content_block import LegacyLibraryContentBlock # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.item_bank_block import ItemBankMixin # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from ..utils import get_student_module_as_dict logger = logging.getLogger(__name__) +# Module-level cache to avoid repeated XBlock.load_class() calls +_BLOCK_CLASS_CACHE = {} + + +def _get_block_class(block_type: str): + """ + Get the XBlock class from a block type. + """ + if block_type not in _BLOCK_CLASS_CACHE: + try: + _BLOCK_CLASS_CACHE[block_type] = XBlock.load_class(block_type) + except Exception: # lint-amnesty, pylint: disable=broad-except + logger.exception('Failed to load block class for type %s', block_type) + _BLOCK_CLASS_CACHE[block_type] = None + return _BLOCK_CLASS_CACHE[block_type] + class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransformer): """ A transformer that manipulates the block structure by removing all - blocks within a library_content block to which a user should not - have access. + blocks within item bank blocks (library_content, itembank, etc.) + to which a user should not have access. + + This transformer works with any XBlock that inherits from ItemBankMixin, + filtering children based on the selection logic defined by each block type. - Staff users are not to be exempted from library content pathways. + Staff users are not to be exempted from item bank pathways. """ WRITE_VERSION = 1 READ_VERSION = 1 @@ -61,22 +84,27 @@ def summarize_block(usage_key): "original_usage_version": str(orig_version) if orig_version else None, } - # For each block check if block is library_content. - # If library_content add children array to content_library_children field + # For each block check if block uses ItemBankMixin (e.g., library_content, itembank). + # Mark these blocks and collect analytics data for their children. for block_key in block_structure.topological_traversal( - filter_func=lambda block_key: block_key.block_type == 'library_content', yield_descendants_of_unyielded=True, ): - xblock = block_structure.get_xblock(block_key) - for child_key in xblock.children: - summary = summarize_block(child_key) - block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary) + block_class = _get_block_class(block_key.block_type) + is_item_bank = block_class and issubclass(block_class, ItemBankMixin) + + if is_item_bank: + block_structure.set_transformer_block_field(block_key, cls, 'is_item_bank_block', True) + xblock = block_structure.get_xblock(block_key) + for child_key in xblock.children: + summary = summarize_block(child_key) + block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary) def transform_block_filters(self, usage_info, block_structure): all_library_children = set() all_selected_children = set() for block_key in block_structure: - if block_key.block_type != 'library_content': + # Check if this block was marked as an ItemBankMixin block during collect + if not block_structure.get_transformer_block_field(block_key, self, 'is_item_bank_block'): continue library_children = block_structure.get_children(block_key) if library_children: @@ -98,7 +126,12 @@ def transform_block_filters(self, usage_info, block_structure): # Update selected previous_count = len(selected) - block_keys = LegacyLibraryContentBlock.make_selection(selected, library_children, max_count) + # Get the cached block class to call make_selection + block_class = _get_block_class(block_key.block_type) + if not block_class: + logger.error('Failed to load block class for %s', block_key) + continue + block_keys = block_class.make_selection(selected, library_children, max_count) selected = block_keys['selected'] # Save back any changes @@ -128,7 +161,7 @@ def check_child_removal(block_key): """ Return True if selected block should be removed. - Block is removed if it is part of library_content, but has + Block is removed if it is a child of an item bank block, but has not been selected for current user. """ if block_key not in all_library_children: @@ -156,6 +189,12 @@ def format_block_keys(keys): json_result.append(info) return json_result + # Get the cached block class to call publish_selected_children_events + block_class = _get_block_class(location.block_type) + if not block_class: + logger.error('Failed to load block class for publishing events: %s', location) + return + def publish_event(event_name, result, **kwargs): """ Helper function to publish an event for analytics purposes @@ -170,11 +209,12 @@ def publish_event(event_name, result, **kwargs): context = contexts.course_context_from_course_id(location.course_key) if user_id: context['user_id'] = user_id - full_event_name = f"edx.librarycontentblock.content.{event_name}" + event_prefix = block_class.get_selected_event_prefix() + full_event_name = f"{event_prefix}.{event_name}" with tracker.get_tracker().context(full_event_name, context): tracker.emit(full_event_name, event_data) - LegacyLibraryContentBlock.publish_selected_children_events( + block_class.publish_selected_children_events( block_keys, format_block_keys, publish_event, @@ -184,12 +224,14 @@ def publish_event(event_name, result, **kwargs): class ContentLibraryOrderTransformer(BlockStructureTransformer): """ A transformer that manipulates the block structure by modifying the order of the - selected blocks within a library_content block to match the order of the selections - made by the ContentLibraryTransformer or the corresponding XBlock. So this transformer - requires the selections for the randomized content block to be already - made either by the ContentLibraryTransformer or the XBlock. + selected blocks within item bank blocks (library_content, itembank, etc.) + to match the order of the selections made by the ContentLibraryTransformer or the + corresponding XBlock. This transformer requires the selections for the item bank block + to be already made either by the ContentLibraryTransformer or the XBlock. + + This transformer works with any XBlock that inherits from ItemBankMixin. - Staff users are *not* exempted from library content pathways. + Staff users are *not* exempted from item bank pathways. """ WRITE_VERSION = 1 READ_VERSION = 1 @@ -217,7 +259,10 @@ def transform(self, usage_info, block_structure): to match the order of the selections made and stored in the XBlock 'selected' field. """ for block_key in block_structure: - if block_key.block_type != 'library_content': + # Check if this block was marked as an ItemBankMixin block by ContentLibraryTransformer + if not block_structure.get_transformer_block_field( + block_key, ContentLibraryTransformer, 'is_item_bank_block' + ): continue library_children = block_structure.get_children(block_key) @@ -228,7 +273,7 @@ def transform(self, usage_info, block_structure): current_selected_blocks = {item[1] for item in state_dict.get('selected', [])} # As the selections should have already been made by the ContentLibraryTransformer, - # the current children of the library_content block should be the same as the stored + # the current children of the item bank block should be the same as the stored # selections. If they aren't, some other transformer that ran before this transformer # has modified those blocks (for example, content gating may have affected this). So do not # transform the order in that case. From fea0622e0ab3afbe047b0f84ca6935f930abbd2c Mon Sep 17 00:00:00 2001 From: Maria Fernanda Magallanes Zubillaga Date: Fri, 2 Jan 2026 12:10:19 -0500 Subject: [PATCH 2/5] fix: update the write version and add fallback for old cache --- .../transformers/library_content.py | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index ae75f6b0181b..3424fc1579ab 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -53,7 +53,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo Staff users are not to be exempted from item bank pathways. """ - WRITE_VERSION = 1 + WRITE_VERSION = 2 READ_VERSION = 1 @classmethod @@ -104,7 +104,13 @@ def transform_block_filters(self, usage_info, block_structure): all_selected_children = set() for block_key in block_structure: # Check if this block was marked as an ItemBankMixin block during collect - if not block_structure.get_transformer_block_field(block_key, self, 'is_item_bank_block'): + is_item_bank = block_structure.get_transformer_block_field(block_key, self, 'is_item_bank_block') + # Fallback for old cache that doesn't have the 'is_item_bank_block' field + if is_item_bank is None: + block_class = _get_block_class(block_key.block_type) + is_item_bank = block_class and issubclass(block_class, ItemBankMixin) + + if not is_item_bank: continue library_children = block_structure.get_children(block_key) if library_children: @@ -259,10 +265,19 @@ def transform(self, usage_info, block_structure): to match the order of the selections made and stored in the XBlock 'selected' field. """ for block_key in block_structure: - # Check if this block was marked as an ItemBankMixin block by ContentLibraryTransformer - if not block_structure.get_transformer_block_field( - block_key, ContentLibraryTransformer, 'is_item_bank_block' - ): + # Try to read from cache (collected by ContentLibraryTransformer) + is_item_bank = block_structure.get_transformer_block_field( + block_key, + ContentLibraryTransformer, + 'is_item_bank_block' + ) + + # Fallback for old cache that doesn't have the 'is_item_bank_block' field + if is_item_bank is None: + block_class = _get_block_class(block_key.block_type) + is_item_bank = block_class and issubclass(block_class, ItemBankMixin) + + if not is_item_bank: continue library_children = block_structure.get_children(block_key) From 5d0a030ec8c3de5eeeacb62ce8c3daea1bf611a8 Mon Sep 17 00:00:00 2001 From: Maria Fernanda Magallanes Zubillaga Date: Tue, 3 Feb 2026 12:26:22 -0500 Subject: [PATCH 3/5] fix: use load_class because it is already cached --- .../transformers/library_content.py | 26 ++++--------------- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 3424fc1579ab..1d3c5ec5e3e8 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -25,22 +25,6 @@ logger = logging.getLogger(__name__) -# Module-level cache to avoid repeated XBlock.load_class() calls -_BLOCK_CLASS_CACHE = {} - - -def _get_block_class(block_type: str): - """ - Get the XBlock class from a block type. - """ - if block_type not in _BLOCK_CLASS_CACHE: - try: - _BLOCK_CLASS_CACHE[block_type] = XBlock.load_class(block_type) - except Exception: # lint-amnesty, pylint: disable=broad-except - logger.exception('Failed to load block class for type %s', block_type) - _BLOCK_CLASS_CACHE[block_type] = None - return _BLOCK_CLASS_CACHE[block_type] - class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransformer): """ @@ -89,7 +73,7 @@ def summarize_block(usage_key): for block_key in block_structure.topological_traversal( yield_descendants_of_unyielded=True, ): - block_class = _get_block_class(block_key.block_type) + block_class = XBlock.load_class(block_key.block_type) is_item_bank = block_class and issubclass(block_class, ItemBankMixin) if is_item_bank: @@ -107,7 +91,7 @@ def transform_block_filters(self, usage_info, block_structure): is_item_bank = block_structure.get_transformer_block_field(block_key, self, 'is_item_bank_block') # Fallback for old cache that doesn't have the 'is_item_bank_block' field if is_item_bank is None: - block_class = _get_block_class(block_key.block_type) + block_class = XBlock.load_class(block_key.block_type) is_item_bank = block_class and issubclass(block_class, ItemBankMixin) if not is_item_bank: @@ -133,7 +117,7 @@ def transform_block_filters(self, usage_info, block_structure): # Update selected previous_count = len(selected) # Get the cached block class to call make_selection - block_class = _get_block_class(block_key.block_type) + block_class = XBlock.load_class(block_key.block_type) if not block_class: logger.error('Failed to load block class for %s', block_key) continue @@ -196,7 +180,7 @@ def format_block_keys(keys): return json_result # Get the cached block class to call publish_selected_children_events - block_class = _get_block_class(location.block_type) + block_class = XBlock.load_class(location.block_type) if not block_class: logger.error('Failed to load block class for publishing events: %s', location) return @@ -274,7 +258,7 @@ def transform(self, usage_info, block_structure): # Fallback for old cache that doesn't have the 'is_item_bank_block' field if is_item_bank is None: - block_class = _get_block_class(block_key.block_type) + block_class = XBlock.load_class(block_key.block_type) is_item_bank = block_class and issubclass(block_class, ItemBankMixin) if not is_item_bank: From 4d54ddfaddf397556338f73ac02bec72983e569d Mon Sep 17 00:00:00 2001 From: Maria Fernanda Magallanes Zubillaga Date: Mon, 6 Apr 2026 16:59:09 -0500 Subject: [PATCH 4/5] fix: use load_class and not a different field --- .../transformers/library_content.py | 46 ++++++------------- 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 1d3c5ec5e3e8..6a029fbe2832 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -37,7 +37,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo Staff users are not to be exempted from item bank pathways. """ - WRITE_VERSION = 2 + WRITE_VERSION = 1 READ_VERSION = 1 @classmethod @@ -69,32 +69,23 @@ def summarize_block(usage_key): } # For each block check if block uses ItemBankMixin (e.g., library_content, itembank). - # Mark these blocks and collect analytics data for their children. + # If so add block analytics summary for each of its children. for block_key in block_structure.topological_traversal( + filter_func=lambda block_key: issubclass(XBlock.load_class(block_key.block_type), ItemBankMixin), yield_descendants_of_unyielded=True, ): - block_class = XBlock.load_class(block_key.block_type) - is_item_bank = block_class and issubclass(block_class, ItemBankMixin) - - if is_item_bank: - block_structure.set_transformer_block_field(block_key, cls, 'is_item_bank_block', True) - xblock = block_structure.get_xblock(block_key) - for child_key in xblock.children: - summary = summarize_block(child_key) - block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary) + xblock = block_structure.get_xblock(block_key) + for child_key in xblock.children: + summary = summarize_block(child_key) + block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary) def transform_block_filters(self, usage_info, block_structure): all_library_children = set() all_selected_children = set() for block_key in block_structure: - # Check if this block was marked as an ItemBankMixin block during collect - is_item_bank = block_structure.get_transformer_block_field(block_key, self, 'is_item_bank_block') - # Fallback for old cache that doesn't have the 'is_item_bank_block' field - if is_item_bank is None: - block_class = XBlock.load_class(block_key.block_type) - is_item_bank = block_class and issubclass(block_class, ItemBankMixin) + block_class = XBlock.load_class(block_key.block_type) - if not is_item_bank: + if block_class is None or not issubclass(block_class, ItemBankMixin): continue library_children = block_structure.get_children(block_key) if library_children: @@ -118,7 +109,7 @@ def transform_block_filters(self, usage_info, block_structure): previous_count = len(selected) # Get the cached block class to call make_selection block_class = XBlock.load_class(block_key.block_type) - if not block_class: + if block_class is None: logger.error('Failed to load block class for %s', block_key) continue block_keys = block_class.make_selection(selected, library_children, max_count) @@ -181,7 +172,7 @@ def format_block_keys(keys): # Get the cached block class to call publish_selected_children_events block_class = XBlock.load_class(location.block_type) - if not block_class: + if block_class is None: logger.error('Failed to load block class for publishing events: %s', location) return @@ -249,19 +240,8 @@ def transform(self, usage_info, block_structure): to match the order of the selections made and stored in the XBlock 'selected' field. """ for block_key in block_structure: - # Try to read from cache (collected by ContentLibraryTransformer) - is_item_bank = block_structure.get_transformer_block_field( - block_key, - ContentLibraryTransformer, - 'is_item_bank_block' - ) - - # Fallback for old cache that doesn't have the 'is_item_bank_block' field - if is_item_bank is None: - block_class = XBlock.load_class(block_key.block_type) - is_item_bank = block_class and issubclass(block_class, ItemBankMixin) - - if not is_item_bank: + block_class = XBlock.load_class(block_key.block_type) + if block_class is None or not issubclass(block_class, ItemBankMixin): continue library_children = block_structure.get_children(block_key) From c64ffc42c55b355ca53f1ef254dd6c8ed3b3c31e Mon Sep 17 00:00:00 2001 From: Maria Fernanda Magallanes Zubillaga Date: Mon, 6 Apr 2026 18:04:18 -0500 Subject: [PATCH 5/5] test: add test for itembank --- .../tests/test_library_content.py | 54 +++++++++++++------ 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py index 367f5329a184..37cf7edd5658 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py @@ -4,6 +4,8 @@ from unittest import mock +from ddt import data, ddt + import openedx.core.djangoapps.content.block_structure.api as bs_api from common.djangoapps.student.tests.factories import CourseEnrollmentFactory from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache @@ -26,6 +28,7 @@ def __init__(self, state): self.state = state +@ddt class ContentLibraryTransformerTestCase(CourseStructureTestCase): """ ContentLibraryTransformer Test @@ -37,9 +40,14 @@ def setUp(self): Setup course structure and create user for content library transformer test. """ super().setUp() + self._initialize_course_hierarchy() + def _initialize_course_hierarchy(self, block_type='library_content'): + """ + Initialize course hierarchy with the given block type. + """ # Build course. - self.course_hierarchy = self.get_course_hierarchy() + self.course_hierarchy = self.get_course_hierarchy(block_type) self.blocks = self.build_course(self.course_hierarchy) self.course = self.blocks['course'] # Do this manually because publish signals are not fired by default in tests. @@ -49,14 +57,14 @@ def setUp(self): # Enroll user in course. CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id, is_active=True) - def get_course_hierarchy(self): + def get_course_hierarchy(self, block_type='library_content'): """ Get a course hierarchy to test with. """ return [{ 'org': 'ContentLibraryTransformer', 'course': 'CL101F', - 'run': 'test_run', + 'run': f'test_run_{block_type}', '#type': 'course', '#ref': 'course', '#children': [ @@ -73,8 +81,8 @@ def get_course_hierarchy(self): '#ref': 'vertical1', '#children': [ { - '#type': 'library_content', - '#ref': 'library_content1', + '#type': block_type, + '#ref': f'{block_type}1', '#children': [ { 'metadata': {'display_name': "CL Vertical 2"}, @@ -111,13 +119,18 @@ def get_course_hierarchy(self): ] }] - def test_content_library(self): + @data('library_content', 'itembank') + def test_content_library(self, block_type): """ Test when course has content library section. First test user can't see any content library section, and after that mock response from MySQL db. Check user can see mocked sections in content library. """ + # Re-initialize if testing with a different block type + if block_type != 'library_content': + self._initialize_course_hierarchy(block_type) + raw_block_structure = get_course_blocks( self.user, self.course.location, @@ -136,7 +149,7 @@ def test_content_library(self): # Should dynamically assign a block to student trans_keys = set(trans_block_structure.get_block_keys()) block_key_set = self.get_block_key_set( - self.blocks, 'course', 'chapter1', 'lesson1', 'vertical1', 'library_content1' + self.blocks, 'course', 'chapter1', 'lesson1', 'vertical1', f'{block_type}1' ) for key in block_key_set: assert key in trans_keys @@ -160,11 +173,12 @@ def test_content_library(self): assert set(trans_block_structure.get_block_keys()) == self.get_block_key_set(self.blocks, 'course', 'chapter1', 'lesson1', 'vertical1', - 'library_content1', + f'{block_type}1', selected_vertical, selected_child), f"Expected 'selected' equality failed in iteration {i}." # pylint: disable=line-too-long +@ddt class ContentLibraryOrderTransformerTestCase(CourseStructureTestCase): """ ContentLibraryOrderTransformer Test @@ -176,7 +190,13 @@ def setUp(self): Setup course structure and create user for content library order transformer test. """ super().setUp() - self.course_hierarchy = self.get_course_hierarchy() + self._initialize_course_hierarchy() + + def _initialize_course_hierarchy(self, block_type='library_content'): + """ + Initialize course hierarchy with the given block type. + """ + self.course_hierarchy = self.get_course_hierarchy(block_type) self.blocks = self.build_course(self.course_hierarchy) self.course = self.blocks['course'] bs_api.update_course_in_cache(self.course.id) @@ -185,14 +205,14 @@ def setUp(self): # Enroll user in course. CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id, is_active=True) - def get_course_hierarchy(self): + def get_course_hierarchy(self, block_type='library_content'): """ Get a course hierarchy to test with. """ return [{ 'org': 'ContentLibraryTransformer', 'course': 'CL101F', - 'run': 'test_run', + 'run': f'test_run_{block_type}', '#type': 'course', '#ref': 'course', '#children': [ @@ -209,8 +229,8 @@ def get_course_hierarchy(self): '#ref': 'vertical1', '#children': [ { - '#type': 'library_content', - '#ref': 'library_content1', + '#type': block_type, + '#ref': f'{block_type}1', '#children': [ { 'metadata': {'display_name': "CL Vertical 2"}, @@ -260,11 +280,15 @@ def get_course_hierarchy(self): }] @mock.patch('lms.djangoapps.course_blocks.transformers.library_content.get_student_module_as_dict') - def test_content_library_randomize(self, mocked): + @data('library_content', 'itembank') + def test_content_library_randomize(self, block_type, mocked): """ Test whether the order of the children blocks matches the order of the selected blocks when course has content library section """ + # Re-initialize if testing with a different block type + if block_type != 'library_content': + self._initialize_course_hierarchy(block_type) mocked.return_value = { 'selected': [ ['vertical', 'vertical_vertical3'], @@ -280,7 +304,7 @@ def test_content_library_randomize(self, mocked): ) children = [] for block_key in trans_block_structure.topological_traversal(): - if block_key.block_type == 'library_content': + if block_key.block_type == block_type: children = trans_block_structure.get_children(block_key) break