Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lms/djangoapps/course_blocks/transformers/library_content.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 15 additions & 4 deletions xmodule/item_bank_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,19 @@ 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.

Arguments:
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
Expand 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

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
81 changes: 81 additions & 0 deletions xmodule/tests/test_item_bank.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'])