fix: apply library_content transformer to all ItemBankMixin xblocks (#37830)

Likely fixes: #38027

Author: MaferMazu

lms/djangoapps/course_blocks/transformers/library_content.py

@@ -1,20 +1,24 @@
 """
-Content Library Transformer.
+Item Bank Transformer.
+
+Transformers for handling item bank blocks (library_content, itembank, etc.)
+that use ItemBankMixin for randomized content selection.
 """
 
 
 import json
 import logging
 
 from eventtracking import tracker
+from xblock.core import XBlock
 
 from common.djangoapps.track import contexts
 from lms.djangoapps.courseware.models import StudentModule
 from openedx.core.djangoapps.content.block_structure.transformer import (
     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
@@ -25,10 +29,13 @@
 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,10 +68,10 @@ 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).
+        # If so add block analytics summary for each of its children.
         for block_key in block_structure.topological_traversal(
-                filter_func=lambda block_key: block_key.block_type == 'library_content',
+                filter_func=lambda block_key: issubclass(XBlock.load_class(block_key.block_type), ItemBankMixin),
                 yield_descendants_of_unyielded=True,
         ):
             xblock = block_structure.get_xblock(block_key)
@@ -76,7 +83,9 @@ 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':
+            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)
             if library_children:
@@ -98,7 +107,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 = XBlock.load_class(block_key.block_type)
+                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)
                 selected = block_keys['selected']
 
                 # Save back any changes
@@ -128,7 +142,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 +170,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 = XBlock.load_class(location.block_type)
+        if block_class is None:
+            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 +190,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 +205,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 +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:
-            if block_key.block_type != 'library_content':
+            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)
@@ -228,7 +252,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.

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