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.

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):
         """

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()

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(),

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,

lms/djangoapps/course_blocks/transformers/tests/test_library_content.py

 """
 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)
+            )

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",