From b1f3d2cef2eab02e5a4ea5613b6906f607de271d Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 25 Jul 2019 13:42:20 -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 | 122 +++------------- .../tests/test_library_content.py | 130 +----------------- setup.py | 1 - 6 files changed, 50 insertions(+), 260 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 1b08eddb08..f5cb4be411 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -158,40 +158,37 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): """ 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, @@ -271,15 +268,19 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): 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: @@ -291,13 +292,13 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): 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 45c5567876..7943084009 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -272,8 +272,9 @@ 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. """ - # 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) @@ -396,6 +397,8 @@ 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,6 +478,8 @@ 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() @@ -492,6 +497,8 @@ 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. @@ -505,6 +512,8 @@ 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 7a64d1972f..685cdc9e0e 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -39,7 +39,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 6c5548a426..4f782c317a 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -4,8 +4,6 @@ Content Library Transformer. from __future__ import absolute_import import json -import logging -import random import six from eventtracking import tracker @@ -21,8 +19,6 @@ from xmodule.modulestore.django import modulestore from ..utils import get_student_module_as_dict -logger = logging.getLogger(__name__) - class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransformer): """ @@ -44,15 +40,24 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo return "library_content" @classmethod - def set_block_analytics_summary(cls, block_structure, store): - """Set the block analytics summary information in the children fields.""" + 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 summarize_block(usage_key): """ Basic information about the given block """ orig_key, orig_version = store.get_block_original_usage(usage_key) return { - "usage_key": unicode(usage_key), - "original_usage_key": unicode(orig_key) if orig_key else None, - "original_usage_version": unicode(orig_version) if orig_version else None, + "usage_key": six.text_type(usage_key), + "original_usage_key": six.text_type(orig_key) if orig_key else None, + "original_usage_version": six.text_type(orig_version) if orig_version else None, } # For each block check if block is library_content. @@ -66,20 +71,6 @@ 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() @@ -111,7 +102,6 @@ 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, @@ -122,7 +112,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo ) # publish events for analytics - self.publish_events( + self._publish_events( block_structure, block_key, previous_count, @@ -147,8 +137,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo return [block_structure.create_removal_filter(check_child_removal)] - @staticmethod - def publish_events(block_structure, location, previous_count, max_count, block_keys, user_id): + def _publish_events(self, block_structure, location, previous_count, max_count, block_keys, user_id): """ Helper method to publish events for analytics purposes """ @@ -188,82 +177,3 @@ 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 26cdb0591e..4d9985f2dc 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py @@ -5,14 +5,13 @@ Tests for ContentLibraryTransformer. from __future__ import absolute_import 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, ContentLibraryOrderTransformer +from ..library_content import ContentLibraryTransformer from .helpers import CourseStructureTestCase @@ -169,130 +168,3 @@ 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 5a8544643b..b5a14562e0 100644 --- a/setup.py +++ b/setup.py @@ -51,7 +51,6 @@ 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",