diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 9933b6af70..29cd1f5934 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -154,37 +154,40 @@ 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]) # 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 = selected_keys return { 'selected': selected, @@ -264,19 +267,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: @@ -288,13 +287,13 @@ 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 = list(block_keys['selected']) + random.shuffle(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 f3a34d7512..b23705d8c7 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -269,9 +269,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 present pylint: disable=protected-access + self.lc_block._xmodule self.lc_block.max_count = count selected = self.lc_block.get_child_descriptors() self.assertEqual(len(selected), count) @@ -394,8 +393,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) @@ -475,8 +472,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() @@ -494,8 +489,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. @@ -509,8 +502,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 9e8fc46941..b1acfdd058 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -37,6 +37,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 de1a82580a..4e77594a70 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -2,6 +2,7 @@ Content Library Transformer. """ import json +import random from eventtracking import tracker @@ -99,6 +100,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) + random.shuffle(state_dict['selected']) StudentModule.save_state( student=usage_info.user, course_id=usage_info.course_key, @@ -176,3 +178,48 @@ 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 + + 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 + + state_dict = get_student_module_as_dict(usage_info.user, usage_info.course_key, block_key) + library_children = block_structure.get_children(block_key) + 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 44b0c1ea92..815a3ada75 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py @@ -1,13 +1,14 @@ """ Tests for ContentLibraryTransformer. """ +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 @@ -180,3 +181,130 @@ class ContentLibraryTransformerTestCase(CourseStructureTestCase): transformers=self.transformers ) self.assertEqual(len(list(transformed_blocks.get_block_keys())), len(self.blocks)) + + +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) + ) diff --git a/setup.py b/setup.py index 0088a6fe23..0a1f0dac05 100644 --- a/setup.py +++ b/setup.py @@ -51,6 +51,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",