From d2af1d4c5b59cdc606a7eb34f1fa5c71c375a624 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 8 May 2019 12:42:44 -0400 Subject: [PATCH] Revert "Fix the unpredictable order randomization issue with randomized content blocks" --- .../xmodule/xmodule/library_content_module.py | 43 +++--- .../xmodule/tests/test_library_content.py | 13 +- lms/djangoapps/course_blocks/api.py | 1 - .../transformers/library_content.py | 47 ------- .../tests/test_library_content.py | 130 +----------------- setup.py | 1 - 6 files changed, 34 insertions(+), 201 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index ceef1560d831..09605ce45a0a 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -154,40 +154,37 @@ def make_selection(cls, selected, children, max_count, mode): """ rand = random.Random() - selected_keys = set(tuple(k) for k in selected) # set of (block_type, block_id) tuples assigned to this student + selected = 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_keys - valid_block_keys) + invalid_block_keys = (selected - valid_block_keys) if invalid_block_keys: - selected_keys -= invalid_block_keys + selected -= 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_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 + 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 # Do we have enough blocks now? - num_to_add = max_count - len(selected_keys) + num_to_add = max_count - len(selected) 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_keys + pool = valid_block_keys - selected 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_keys |= added_block_keys - - if any([invalid_block_keys, overlimit_block_keys, added_block_keys]): - selected = selected_keys + selected |= added_block_keys return { 'selected': selected, @@ -267,15 +264,19 @@ def publish_selected_children_events(cls, block_keys, format_block_keys, publish def selected_children(self): """ - Returns a list() of block_ids indicating which of the possible children + Returns a set() 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: the return value (self.selected) contains block_ids. To get + Note: self.selected and the return value contain 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: @@ -287,13 +288,13 @@ def selected_children(self): self._publish_event, ) - 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. + # 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 - return self.selected + return 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 b23705d8c779..f3a34d7512e5 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -269,8 +269,9 @@ def _change_count_and_refresh_children(self, count): 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. """ - # Construct the XModule for the descriptor, if not present already present pylint: disable=protected-access - self.lc_block._xmodule + # 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 self.lc_block.max_count = count selected = self.lc_block.get_child_descriptors() self.assertEqual(len(selected), count) @@ -393,6 +394,8 @@ def test_assigned_event(self): # 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) @@ -472,6 +475,8 @@ def test_removed_overlimit(self): 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() @@ -489,6 +494,8 @@ def test_removed_invalid(self): # 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. @@ -502,6 +509,8 @@ def test_removed_invalid(self): 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 b1acfdd058c2..9e8fc469411a 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -37,7 +37,6 @@ 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 4e77594a707b..de1a82580a46 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -2,7 +2,6 @@ Content Library Transformer. """ import json -import random from eventtracking import tracker @@ -100,7 +99,6 @@ def transform_block_filters(self, usage_info, block_structure): # 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, @@ -178,48 +176,3 @@ def publish_event(event_name, result, **kwargs): 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 815a3ada7589..44b0c1ea9277 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py @@ -1,14 +1,13 @@ """ 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, ContentLibraryOrderTransformer +from ..library_content import ContentLibraryTransformer from .helpers import CourseStructureTestCase @@ -181,130 +180,3 @@ def test_staff_access_to_library_content(self): 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 0a1f0dac0588..0088a6fe23b4 100644 --- a/setup.py +++ b/setup.py @@ -51,7 +51,6 @@ ], "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",