diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 6a029fbe2832..fc98ffb372b0 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -112,7 +112,11 @@ def transform_block_filters(self, usage_info, block_structure): if block_class is None: logger.error('Failed to load block class for %s', block_key) continue - block_keys = block_class.make_selection(selected, library_children, max_count) + # Seed deterministically so this transformer and the XBlock runtime's + # selected_children() path converge on the same selection for the same + # (user, block). See issue #38027. + seed = f"{usage_info.user.id}:{block_key}" + block_keys = block_class.make_selection(selected, library_children, max_count, seed=seed) selected = block_keys['selected'] # Save back any changes diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index 5a5ab0400815..5c49885f3fb2 100644 --- a/xmodule/item_bank_block.py +++ b/xmodule/item_bank_block.py @@ -93,7 +93,7 @@ def completion_mode(cls): # pylint: disable=no-self-argument return XBlockCompletionMode.AGGREGATOR @classmethod - def make_selection(cls, selected, children, max_count): + def make_selection(cls, selected, children, max_count, *, seed=None): """ Dynamically selects block_ids indicating which of the possible children are displayed to the current user. @@ -101,6 +101,11 @@ def make_selection(cls, selected, children, max_count): selected - list of (block_type, block_id) pairs assigned to this student children - children of this block max_count - number of components to display to each student + seed - optional deterministic seed (any hashable). When provided, two calls with the + same seed, children, and max_count produce the same selection. Callers that run + in independent code paths (e.g. the XBlock runtime and the ContentLibraryTransformer) + MUST pass a stable seed derived from (user_id, block_usage_key) so that both paths + converge on the same selection. See issue #38027. NOTE/TODO: We like to treat `self.selected` as a list of 2-tuples, but when we load a block from persistence, all @@ -124,7 +129,7 @@ def make_selection(cls, selected, children, max_count): Order of invalid/overlimit/added is ALPHABETICAL, by type then id. This is arbitrary, but it makes the events more consistent and easier to test. """ - rand = random.Random() + rand = random.Random(seed) selected_keys = {tuple(k) for k in selected} # set of (block_type, block_id) tuples assigned to this student @@ -156,7 +161,7 @@ def make_selection(cls, selected, children, max_count): if any((invalid_block_keys, overlimit_block_keys, added_block_keys)): selected = list(selected_keys) - random.shuffle(selected) + rand.shuffle(selected) return { 'selected': selected, @@ -249,7 +254,13 @@ def selected_children(self): if max_count < 0: max_count = len(self.children) - block_keys = self.make_selection(self.selected, self.children, max_count) # pylint: disable=no-member + # Seed deterministically so the XBlock runtime and ContentLibraryTransformer, + # which both call make_selection independently on a page load, converge on the + # same selection. See issue #38027. + seed = f"{self.get_user_id()}:{self.usage_key}" + block_keys = self.make_selection( # pylint: disable=no-member + self.selected, self.children, max_count, seed=seed, + ) self.publish_selected_children_events( block_keys, diff --git a/xmodule/tests/test_item_bank.py b/xmodule/tests/test_item_bank.py index 2063381670d7..52def2852579 100644 --- a/xmodule/tests/test_item_bank.py +++ b/xmodule/tests/test_item_bank.py @@ -520,3 +520,84 @@ def shuffle(_self, selection): return mock_result return shuffle + + +class ItemBankMakeSelectionDeterminismTests(MixedSplitTestCase): + """ + Regression tests for bug #38027: make_selection must be deterministic + across independent call sites when a seed is supplied, so that the XBlock + runtime and ContentLibraryTransformer converge on the same random subset + for a given (user, block) pair. + """ + + @staticmethod + def _children_pool(count=10): + """Build a simple children pool for make_selection.""" + return [Mock(block_type="problem", block_id=f"p{i}") for i in range(count)] + + def test_unit_make_selection_with_seed_is_deterministic(self): + """Same seed produces the same selection on every call.""" + children = self._children_pool() + seed = "user-42:block-foo/bar" + first = ItemBankBlock.make_selection([], children, max_count=3, seed=seed) + for _ in range(5): + again = ItemBankBlock.make_selection([], children, max_count=3, seed=seed) + assert first['selected'] == again['selected'] + assert first['added'] == again['added'] + + def test_unit_make_selection_different_seeds_can_differ(self): + """Different seeds across many users should produce at least two distinct subsets.""" + children = self._children_pool() + subsets = set() + for uid in range(20): + result = ItemBankBlock.make_selection( + [], children, max_count=3, seed=f"user-{uid}:block-x", + ) + subsets.add(tuple(sorted(result['selected']))) + assert len(subsets) > 1 + + def test_make_selection_preserves_signature_without_seed(self): + """ + Backward compatibility: callers that don't pass seed still work + (keyword-only default is None). + """ + children = self._children_pool() + result = ItemBankBlock.make_selection([], children, max_count=2) + assert len(result['selected']) == 2 + + def test_integration_transformer_and_xblock_seed_match(self): + """ + Integration test for bug #38027: the XBlock runtime and transformer + produce the same selection for the same (user, block) pair because + they use the same seed format. + """ + children = self._children_pool(8) + user_id = 777 + block_key = "block-v1:Org+Course+Run+type@itembank+block@abc" + seed_from_xblock = f"{user_id}:{block_key}" + seed_from_transformer = f"{user_id}:{block_key}" + + xblock_result = ItemBankBlock.make_selection( + [], children, max_count=3, seed=seed_from_xblock, + ) + transformer_result = ItemBankBlock.make_selection( + [], children, max_count=3, seed=seed_from_transformer, + ) + assert xblock_result['selected'] == transformer_result['selected'] + + def test_bug_38027_regression_selection_stable_across_rerun(self): + """ + Regression for bug #38027: re-running make_selection with a non-empty + 'selected' from a previous run must be a no-op (no shuffle, no + additions), so the learner's checkmark does not disappear on refresh. + """ + children = self._children_pool(8) + seed = "user-777:block-abc" + first = ItemBankBlock.make_selection([], children, max_count=3, seed=seed) + second = ItemBankBlock.make_selection( + first['selected'], children, max_count=3, seed=seed, + ) + assert second['added'] == [] + assert second['invalid'] == [] + assert second['overlimit'] == [] + assert sorted(second['selected']) == sorted(first['selected'])