diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 9c204a0c9b..fd58beaeea 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -160,37 +160,41 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): """ rand = random.Random() - selected = set(tuple(k) for k in selected) # set of (block_type, block_id) tuples assigned to this student + selected_keys = set(tuple(k) for k in selected) # set of (block_type, block_id) tuples assigned to this student # Determine which of our children we will show: - valid_block_keys = set([(c.block_type, c.block_id) for c in children]) + valid_block_keys = set((c.block_type, c.block_id) for c in children) # Remove any selected blocks that are no longer valid: - invalid_block_keys = (selected - valid_block_keys) + invalid_block_keys = (selected_keys - valid_block_keys) if invalid_block_keys: - selected -= invalid_block_keys + selected_keys -= invalid_block_keys # If max_count has been decreased, we may have to drop some previously selected blocks: overlimit_block_keys = set() - if len(selected) > max_count: - num_to_remove = len(selected) - max_count - overlimit_block_keys = set(rand.sample(selected, num_to_remove)) - selected -= overlimit_block_keys + if len(selected_keys) > max_count: + num_to_remove = len(selected_keys) - max_count + overlimit_block_keys = set(rand.sample(selected_keys, num_to_remove)) + selected_keys -= overlimit_block_keys # Do we have enough blocks now? - num_to_add = max_count - len(selected) + num_to_add = max_count - len(selected_keys) added_block_keys = None if num_to_add > 0: # We need to select [more] blocks to display to this user: - pool = valid_block_keys - selected + pool = valid_block_keys - selected_keys if mode == "random": num_to_add = min(len(pool), num_to_add) added_block_keys = set(rand.sample(pool, num_to_add)) # We now have the correct n random children to show for this user. else: raise NotImplementedError("Unsupported mode.") - selected |= added_block_keys + selected_keys |= added_block_keys + + if any([invalid_block_keys, overlimit_block_keys, added_block_keys]): + selected = list(selected_keys) + random.shuffle(selected) return { 'selected': selected, @@ -270,19 +274,15 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): def selected_children(self): """ - Returns a set() of block_ids indicating which of the possible children + Returns a list() of block_ids indicating which of the possible children have been selected to display to the current user. This reads and updates the "selected" field, which has user_state scope. - Note: self.selected and the return value contain block_ids. To get + Note: the return value (self.selected) contains block_ids. To get actual BlockUsageLocators, it is necessary to use self.children, because the block_ids alone do not specify the block type. """ - if hasattr(self, "_selected_set"): - # Already done: - return self._selected_set # pylint: disable=access-member-before-definition - block_keys = self.make_selection(self.selected, self.children, self.max_count, "random") # pylint: disable=no-member # Publish events for analytics purposes: @@ -294,13 +294,12 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): self._publish_event, ) - # Save our selections to the user state, to ensure consistency: - selected = block_keys['selected'] - self.selected = list(selected) # TODO: this doesn't save from the LMS "Progress" page. - # Cache the results - self._selected_set = selected # pylint: disable=attribute-defined-outside-init + if 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 selected + return self.selected def _get_selected_child_blocks(self): """ diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index d885d9129f..888dd4cfad 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -272,9 +272,8 @@ class LibraryContentModuleTestMixin(object): Helper method that changes the max_count of self.lc_block, refreshes children, and asserts that the number of selected children equals the count provided. """ - # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access - if hasattr(self.lc_block._xmodule, '_selected_set'): - del self.lc_block._xmodule._selected_set + # Construct the XModule for the descriptor, if not present already. + self.lc_block._xmodule # pylint: disable=pointless-statement,protected-access self.lc_block.max_count = count selected = self.lc_block.get_child_descriptors() self.assertEqual(len(selected), count) @@ -287,7 +286,7 @@ class TestLibraryContentModuleNoSearchIndex(LibraryContentModuleTestMixin, Libra Tests for library container when no search index is available. Tests fallback low-level CAPA problem introspection """ - pass + pass # pylint:disable=unnecessary-pass search_index_mock = Mock(spec=SearchEngine) # pylint: disable=invalid-name @@ -365,8 +364,8 @@ class TestLibraryContentAnalytics(LibraryContentTest): Check that a LibraryContentModule analytics event was published by self.lc_block. """ self.assertTrue(self.publisher.called) - self.assertTrue(len(self.publisher.call_args[0]), 3) - _, event_name, event_data = self.publisher.call_args[0] + self.assertTrue(len(self.publisher.call_args[0]), 3) # pylint:disable=unsubscriptable-object + _, event_name, event_data = self.publisher.call_args[0] # pylint:disable=unsubscriptable-object self.assertEqual(event_name, "edx.librarycontentblock.content.{}".format(event_type)) self.assertEqual(event_data["location"], six.text_type(self.lc_block.location)) return event_data @@ -397,8 +396,6 @@ class TestLibraryContentAnalytics(LibraryContentTest): # Now increase max_count so that one more child will be added: self.lc_block.max_count = 2 - # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access - del self.lc_block._xmodule._selected_set children = self.lc_block.get_child_descriptors() self.assertEqual(len(children), 2) child, new_child = children if children[0].location == child.location else reversed(children) @@ -478,8 +475,6 @@ class TestLibraryContentAnalytics(LibraryContentTest): self.lc_block.get_child_descriptors() # This line is needed in the test environment or the change has no effect self.publisher.reset_mock() # Clear the "assigned" event that was just published. self.lc_block.max_count = 0 - # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access - del self.lc_block._xmodule._selected_set # Check that the event says that one block was removed, leaving no blocks left: children = self.lc_block.get_child_descriptors() @@ -497,8 +492,6 @@ class TestLibraryContentAnalytics(LibraryContentTest): # Start by assigning two blocks to the student: self.lc_block.get_child_descriptors() # This line is needed in the test environment or the change has no effect self.lc_block.max_count = 2 - # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access - del self.lc_block._xmodule._selected_set initial_blocks_assigned = self.lc_block.get_child_descriptors() self.assertEqual(len(initial_blocks_assigned), 2) self.publisher.reset_mock() # Clear the "assigned" event that was just published. @@ -512,8 +505,6 @@ class TestLibraryContentAnalytics(LibraryContentTest): self.library.children = [keep_block_lib_usage_key] self.store.update_item(self.library, self.user_id) self.lc_block.refresh_children() - # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access - del self.lc_block._xmodule._selected_set # Check that the event says that one block was removed, leaving one block left: children = self.lc_block.get_child_descriptors() diff --git a/lms/djangoapps/course_blocks/api.py b/lms/djangoapps/course_blocks/api.py index 569eeb947e..a8eb5bf0e5 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -40,6 +40,7 @@ def get_course_block_access_transformers(user): """ course_block_access_transformers = [ library_content.ContentLibraryTransformer(), + library_content.ContentLibraryOrderTransformer(), start_date.StartDateTransformer(), ContentTypeGateTransformer(), user_partitions.UserPartitionTransformer(), diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 004177d602..59e0d5f640 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -4,6 +4,7 @@ Content Library Transformer. import json +import logging import six from eventtracking import tracker @@ -19,6 +20,8 @@ from xmodule.modulestore.django import modulestore from ..utils import get_student_module_as_dict +logger = logging.getLogger(__name__) + class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransformer): """ @@ -101,7 +104,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo # Save back any changes if any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')): - state_dict['selected'] = list(selected) + state_dict['selected'] = selected StudentModule.save_state( student=usage_info.user, course_id=usage_info.course_key, @@ -177,3 +180,66 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo format_block_keys, publish_event, ) + + +class ContentLibraryOrderTransformer(BlockStructureTransformer): + """ + A transformer that manipulates the block structure by modifying the order of the + selected blocks within a library_content module 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. + + Staff users are *not* exempted from library content pathways. + """ + WRITE_VERSION = 1 + READ_VERSION = 1 + + @classmethod + def name(cls): + """ + Unique identifier for the transformer's class; + same identifier used in setup.py + """ + return "library_content_randomize" + + @classmethod + def collect(cls, block_structure): + """ + Collects any information that's necessary to execute this + transformer's transform method. + """ + # There is nothing to collect + pass # pylint:disable=unnecessary-pass + + def transform(self, usage_info, block_structure): + """ + Transforms the order of the children of the randomized content block + 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': + continue + + library_children = block_structure.get_children(block_key) + + if library_children: + state_dict = get_student_module_as_dict(usage_info.user, usage_info.course_key, block_key) + current_children_blocks = set(block.block_id for block in library_children) + current_selected_blocks = set(item[1] for item in state_dict['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 + # 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. + if current_children_blocks != current_selected_blocks: + logger.info( + u'Mismatch between the children of %s in the stored state and the actual children for user %s. ' + 'Continuing without order transformation.', + str(block_key), + usage_info.user.username + ) + else: + ordering_data = {block[1]: position for position, block in enumerate(state_dict['selected'])} + library_children.sort(key=lambda block, data=ordering_data: data[block.block_id]) 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 ecf6eac39a..bb3cbad872 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py @@ -4,13 +4,14 @@ Tests for ContentLibraryTransformer. from six.moves import range +import mock from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers from student.tests.factories import CourseEnrollmentFactory from ...api import get_course_blocks -from ..library_content import ContentLibraryTransformer +from ..library_content import ContentLibraryTransformer, ContentLibraryOrderTransformer from .helpers import CourseStructureTestCase @@ -167,3 +168,183 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase): ), u"Expected 'selected' equality failed in iteration {}.".format(i) ) + + +class ContentLibraryOrderTransformerTestCase(CourseStructureTestCase): + """ + ContentLibraryOrderTransformer Test + """ + TRANSFORMER_CLASS_TO_TEST = ContentLibraryOrderTransformer + + def setUp(self): + """ + Setup course structure and create user for content library order transformer test. + """ + super(ContentLibraryOrderTransformerTestCase, self).setUp() + self.course_hierarchy = self.get_course_hierarchy() + self.blocks = self.build_course(self.course_hierarchy) + self.course = self.blocks['course'] + clear_course_from_cache(self.course.id) + + # Enroll user in course. + CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id, is_active=True) + + def get_course_hierarchy(self): + """ + Get a course hierarchy to test with. + """ + return [{ + 'org': 'ContentLibraryTransformer', + 'course': 'CL101F', + 'run': 'test_run', + '#type': 'course', + '#ref': 'course', + '#children': [ + { + '#type': 'chapter', + '#ref': 'chapter1', + '#children': [ + { + '#type': 'sequential', + '#ref': 'lesson1', + '#children': [ + { + '#type': 'vertical', + '#ref': 'vertical1', + '#children': [ + { + 'metadata': {'category': 'library_content'}, + '#type': 'library_content', + '#ref': 'library_content1', + '#children': [ + { + 'metadata': {'display_name': "CL Vertical 2"}, + '#type': 'vertical', + '#ref': 'vertical2', + '#children': [ + { + 'metadata': {'display_name': "HTML1"}, + '#type': 'html', + '#ref': 'html1', + } + ] + }, + { + 'metadata': {'display_name': "CL Vertical 3"}, + '#type': 'vertical', + '#ref': 'vertical3', + '#children': [ + { + 'metadata': {'display_name': "HTML2"}, + '#type': 'html', + '#ref': 'html2', + } + ] + }, + { + 'metadata': {'display_name': "CL Vertical 4"}, + '#type': 'vertical', + '#ref': 'vertical4', + '#children': [ + { + 'metadata': {'display_name': "HTML3"}, + '#type': 'html', + '#ref': 'html3', + } + ] + } + ] + } + ], + } + ], + } + ], + } + ] + }] + + @mock.patch('lms.djangoapps.course_blocks.transformers.library_content.get_student_module_as_dict') + def test_content_library_randomize(self, mocked): + """ + Test whether the order of the children blocks matches the order of the selected blocks when + course has content library section + """ + mocked.return_value = { + 'selected': [ + ['vertical', 'vertical_vertical3'], + ['vertical', 'vertical_vertical2'], + ['vertical', 'vertical_vertical4'], + ] + } + for i in range(5): + trans_block_structure = get_course_blocks( + self.user, + self.course.location, + self.transformers, + ) + children = [] + for block_key in trans_block_structure.topological_traversal(): + if block_key.block_type == 'library_content': + children = trans_block_structure.get_children(block_key) + break + + expected_children = ['vertical_vertical3', 'vertical_vertical2', 'vertical_vertical4'] + self.assertEqual( + expected_children, + [child.block_id for child in children], + u"Expected 'selected' equality failed in iteration {}.".format(i) + ) + + @mock.patch('lms.djangoapps.course_blocks.transformers.library_content.get_student_module_as_dict') + def test_content_library_randomize_selected_blocks_mismatch(self, mocked): + """ + Test and verify that the ContentLibraryOrderTransformer's order transformation doesn't + happen when the current children blocks no longer match the selections made in the ContentLibraryTransformer + and stored in the database. + + There are two types of block structure transformers - filtering and non-filtering. The filtering transformers + can only filter blocks from the block structure but cannot perform any other transformations. The + non-filtering transformers can transform the blocks in the block structure. + + The ContentLibraryTransformer is a filtering transformer that selects the children blocks of the randomized + content blocks, saves the selection in the database and filters the blocks that are not selected. + + The ContentLibraryOrderTransformer is a non-filtering transformer which transforms the order of the children + blocks of the randomized content block based on the order of the stored selection made by the + ContentLibraryTransformer. + + The 'transform()' methods of all the filtering transformers are combined into a single transformation function + and run in a single block structure traversal. The non-filtering block structure transformers are run + after this. + + When some filtering transformers like those for content visibility, gating etc. run after the + ContentLibraryTransformer and remove some/all of the selected children blocks before the + ContentLibraryTransformer runs, there will be a mismatch between the stored selected children blocks and + the current children blocks. When this happens, the ContentLibraryOrderTransformer shouldn't + transform the order. + """ + mocked.return_value = { + 'selected': [ + ['vertical', 'vertical_vertical3'], + ] + } + + expected_children_without_hiding_or_gating = ['vertical_vertical3', ] + + for _ in range(5): + trans_block_structure = get_course_blocks( + self.user, + self.course.location, + self.transformers, + ) + children = [] + for block_key in trans_block_structure.topological_traversal(): + if block_key.block_type == 'library_content': + children = trans_block_structure.get_children(block_key) + break + + self.assertNotEqual( + expected_children_without_hiding_or_gating, + [child.block_id for child in children], + ) diff --git a/setup.py b/setup.py index ada332a49d..f21177e1b0 100644 --- a/setup.py +++ b/setup.py @@ -53,6 +53,7 @@ setup( ], "openedx.block_structure_transformer": [ "library_content = lms.djangoapps.course_blocks.transformers.library_content:ContentLibraryTransformer", + "library_content_randomize = lms.djangoapps.course_blocks.transformers.library_content:ContentLibraryOrderTransformer", "split_test = lms.djangoapps.course_blocks.transformers.split_test:SplitTestTransformer", "start_date = lms.djangoapps.course_blocks.transformers.start_date:StartDateTransformer", "user_partitions = lms.djangoapps.course_blocks.transformers.user_partitions:UserPartitionTransformer",