From 5a2b607ea60e14cac60fce33fbb4388197c6ef38 Mon Sep 17 00:00:00 2001 From: Guruprasad Lakshmi Narayanan Date: Mon, 23 Jul 2018 13:37:04 +0530 Subject: [PATCH 1/3] Fix the order randomization behaviour of Randomized Content Block The Randomized Content Block XBlock only randomizes the selection of the children blocks and has unpredictable randomization of the order of the selected child blocks due to the usage of sets, which are unordered, for storing the selected blocks. This becomes apparent when all the available child blocks in a library are chosen for a Randomized Content Block, to randomize just the order of the child blocks and not just the selection of the blocks. The order of the selected blocks ends up being similar for multiple learners. This change modifies the XBlock to store the selected child blocks in a list, instead of a set, after randomly shuffling them. --- .../xmodule/xmodule/library_content_module.py | 43 +++--- .../xmodule/tests/test_library_content.py | 19 +-- lms/djangoapps/course_blocks/api.py | 1 + .../transformers/library_content.py | 4 +- .../tests/test_library_content.py | 130 +++++++++++++++++- setup.py | 1 + 6 files changed, 160 insertions(+), 38 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 077d457fce..36d1aea950 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -158,37 +158,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, @@ -268,19 +271,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: @@ -292,13 +291,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 cfe4ce2b5d..3e5fbd6fd4 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..68962b2d37 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 random import six from eventtracking import tracker @@ -102,7 +103,8 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo # Save back any changes if any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')): state_dict['selected'] = list(selected) - StudentModule.save_state( + random.shuffle(state_dict['selected']) + StudentModule.save_state( # pylint: disable=no-value-for-parameter student=usage_info.user, course_id=usage_info.course_key, module_state_key=block_key, 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..9164bdc8e8 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py @@ -1,6 +1,7 @@ """ Tests for ContentLibraryTransformer. """ +import mock from six.moves import range @@ -10,7 +11,7 @@ from openedx.core.djangoapps.content.block_structure.transformers import BlockSt 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,130 @@ 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) + ) diff --git a/setup.py b/setup.py index 257131cb2e..4b56c4861d 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", From c739151a27b7ddf2e37a98e7f5797a8cd7b0132a Mon Sep 17 00:00:00 2001 From: Guruprasad Lakshmi Narayanan Date: Wed, 29 May 2019 18:08:20 +0530 Subject: [PATCH 2/3] Select children again when current children don't match the selections This prevents errors when the current children do not match the selected children any more. --- .../transformers/library_content.py | 116 +++++++++++++++--- .../tests/test_library_content.py | 2 +- 2 files changed, 103 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 68962b2d37..bc0023e0b9 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 random import six @@ -20,6 +21,8 @@ from xmodule.modulestore.django import modulestore from ..utils import get_student_module_as_dict +logger = logging.getLogger(__name__) + class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransformer): """ @@ -41,17 +44,8 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo return "library_content" @classmethod - def collect(cls, block_structure): - """ - Collects any information that's necessary to execute this - transformer's transform method. - """ - block_structure.request_xblock_fields('mode') - block_structure.request_xblock_fields('max_count') - block_structure.request_xblock_fields('category') - store = modulestore() - - # needed for analytics purposes + def set_block_analytics_summary(cls, block_structure, store): + """Set the block analytics summary information in the children fields.""" def summarize_block(usage_key): """ Basic information about the given block """ orig_key, orig_version = store.get_block_original_usage(usage_key) @@ -72,6 +66,20 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo summary = summarize_block(child_key) block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary) + @classmethod + def collect(cls, block_structure): + """ + Collects any information that's necessary to execute this + transformer's transform method. + """ + block_structure.request_xblock_fields('mode') + block_structure.request_xblock_fields('max_count') + block_structure.request_xblock_fields('category') + store = modulestore() + + # needed for analytics purposes + cls.set_block_analytics_summary(block_structure, store) + def transform_block_filters(self, usage_info, block_structure): all_library_children = set() all_selected_children = set() @@ -104,7 +112,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo if any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')): state_dict['selected'] = list(selected) random.shuffle(state_dict['selected']) - StudentModule.save_state( # pylint: disable=no-value-for-parameter + StudentModule.save_state( student=usage_info.user, course_id=usage_info.course_key, module_state_key=block_key, @@ -114,7 +122,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo ) # publish events for analytics - self._publish_events( + self.publish_events( block_structure, block_key, previous_count, @@ -139,7 +147,8 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo return [block_structure.create_removal_filter(check_child_removal)] - def _publish_events(self, block_structure, location, previous_count, max_count, block_keys, user_id): + @staticmethod + def publish_events(block_structure, location, previous_count, max_count, block_keys, user_id): """ Helper method to publish events for analytics purposes """ @@ -179,3 +188,82 @@ 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. + """ + block_structure.request_xblock_fields('mode') + block_structure.request_xblock_fields('max_count') + + ContentLibraryTransformer.set_block_analytics_summary(block_structure, modulestore()) + + 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']) + + # Ensure that the current children blocks are the same as the stored selections + # before ordering the blocks. + if current_children_blocks != current_selected_blocks: + logger.info( + u'Mismatch between the children of %s in the stored state and the actual children', + str(block_key) + ) + previous_count = len(current_selected_blocks.intersection(current_children_blocks)) + mode = block_structure.get_xblock_field(block_key, 'mode') + max_count = block_structure.get_xblock_field(block_key, 'max_count') + block_keys = LibraryContentModule.make_selection( + state_dict['selected'], library_children, max_count, mode + ) + state_dict['selected'] = block_keys['selected'] + random.shuffle(state_dict['selected']) + StudentModule.save_state( + student=usage_info.user, + course_id=usage_info.course_key, + module_state_key=block_key, + defaults={ + 'state': json.dumps(state_dict) + }, + ) + ContentLibraryTransformer.publish_events( + block_structure, block_key, previous_count, max_count, block_keys, usage_info.user.id + ) + + 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 9164bdc8e8..2b5b4c3c27 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py @@ -1,10 +1,10 @@ """ Tests for ContentLibraryTransformer. """ -import mock 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 a7bd0050a637dd93bbd094e47576d25590c2391e Mon Sep 17 00:00:00 2001 From: Guruprasad Lakshmi Narayanan Date: Tue, 19 May 2020 20:43:25 +0530 Subject: [PATCH 3/3] Log the username to help with investigating the edge cases --- lms/djangoapps/course_blocks/transformers/library_content.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index bc0023e0b9..729d7953b7 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -242,8 +242,9 @@ class ContentLibraryOrderTransformer(BlockStructureTransformer): # before ordering the blocks. if current_children_blocks != current_selected_blocks: logger.info( - u'Mismatch between the children of %s in the stored state and the actual children', - str(block_key) + u'Mismatch between the children of %s in the stored state and the actual children for user %s', + str(block_key), + usage_info.user.username ) previous_count = len(current_selected_blocks.intersection(current_children_blocks)) mode = block_structure.get_xblock_field(block_key, 'mode')