From 1cbf7ab0fb80cbf24190ea845d047763b6170ea2 Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Fri, 30 Mar 2018 13:08:33 -0400 Subject: [PATCH] EDUCATOR-1290 | Remove over-limit block keys randomly for library content blocks. --- .../xmodule/xmodule/library_content_module.py | 9 +++-- .../xmodule/tests/test_library_content.py | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 91cc473f6d..535b4ede57 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -152,6 +152,8 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): 'overlimit' (set) of dropped (block_type, block_id) tuples that were previously selected 'added' (set) of newly added (block_type, block_id) tuples """ + rand = random.Random() + 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: @@ -164,8 +166,10 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): # If max_count has been decreased, we may have to drop some previously selected blocks: overlimit_block_keys = set() - while len(selected) > max_count: - overlimit_block_keys.add(selected.pop()) + 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) @@ -176,7 +180,6 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): pool = valid_block_keys - selected if mode == "random": num_to_add = min(len(pool), num_to_add) - rand = random.Random() added_block_keys = set(rand.sample(pool, num_to_add)) # We now have the correct n random children to show for this user. else: diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index 87594fa3ba..6f54b46fde 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -241,6 +241,42 @@ class LibraryContentModuleTestMixin(object): self.assertIn(LibraryContentDescriptor.mode, non_editable_metadata_fields) self.assertNotIn(LibraryContentDescriptor.display_name, non_editable_metadata_fields) + def test_overlimit_blocks_chosen_randomly(self): + """ + Tests that blocks to remove from selected children are chosen + randomly when len(selected) > max_count. + """ + blocks_seen = set() + total_tries, max_tries = 0, 100 + + self.lc_block.refresh_children() + self.lc_block = self.store.get_item(self.lc_block.location) + self._bind_course_module(self.lc_block) + + # Eventually, we should see every child block selected + while len(blocks_seen) != len(self.lib_blocks): + self._change_count_and_refresh_children(len(self.lib_blocks)) + # Now set the number of selections to 1 + selected = self._change_count_and_refresh_children(1) + blocks_seen.update(selected) + total_tries += 1 + if total_tries >= max_tries: + assert False, "Max tries exceeded before seeing all blocks." + break + + 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. + """ + # 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) + return selected + @patch('xmodule.library_tools.SearchEngine.get_search_engine', Mock(return_value=None, autospec=True)) class TestLibraryContentModuleNoSearchIndex(LibraryContentModuleTestMixin, LibraryContentTest):