diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 10ef8c2138b6..3252738e1372 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,7 +18,7 @@ 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. 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 5a4d7a0de11a..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,11 +4,13 @@ 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 from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers -import openedx.core.djangoapps.content.block_structure.api as bs_api from ...api import get_course_blocks from ..library_content import ContentLibraryOrderTransformer, ContentLibraryTransformer from .helpers import CourseStructureTestCase @@ -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