diff --git a/openedx/core/djangoapps/content/block_structure/factory.py b/openedx/core/djangoapps/content/block_structure/factory.py index 3697f9e92f51..9dd2ce02d71e 100644 --- a/openedx/core/djangoapps/content/block_structure/factory.py +++ b/openedx/core/djangoapps/content/block_structure/factory.py @@ -31,7 +31,8 @@ def create_from_modulestore(cls, root_block_usage_key, modulestore): xmodule.modulestore.exceptions.ItemNotFoundError if a block for root_block_usage_key is not found in the modulestore. """ - block_structure = BlockStructureModulestoreData(root_block_usage_key) + root_xblock = modulestore.get_item(root_block_usage_key, depth=None, lazy=False) + block_structure = BlockStructureModulestoreData(root_block_usage_key.for_branch(None)) blocks_visited = set() def build_block_structure(xblock): @@ -41,19 +42,26 @@ def build_block_structure(xblock): """ # Check if the xblock was already visited (can happen in # DAGs). - if xblock.location in blocks_visited: + # Normalize location to remove branch/version information + # When create_from_modulestore is wrapped in published_only branch decorator, + # "xblock being changed" location contains branch and version info which causes + # mismatch when removing inaccessible blocks in + # CourseNavigationBlocksView.filter_inaccessible_blocks + # while fetching course navigation. + location = xblock.location.for_branch(None) + if location in blocks_visited: return # Add the xBlock. - blocks_visited.add(xblock.location) - block_structure._add_xblock(xblock.location, xblock) # pylint: disable=protected-access + blocks_visited.add(location) + block_structure._add_xblock(location, xblock) # pylint: disable=protected-access # Add relations with its children and recurse. for child in xblock.get_children(): - block_structure._add_relation(xblock.location, child.location) # pylint: disable=protected-access + child_location = child.location.for_branch(None) + block_structure._add_relation(location, child_location) # pylint: disable=protected-access build_block_structure(child) - root_xblock = modulestore.get_item(root_block_usage_key, depth=None, lazy=False) build_block_structure(root_xblock) return block_structure diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_factory.py b/openedx/core/djangoapps/content/block_structure/tests/test_factory.py index 00efa393d8fa..92050eef24b8 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_factory.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_factory.py @@ -6,12 +6,13 @@ from django.test import TestCase from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from xmodule.modulestore.exceptions import ItemNotFoundError from ..exceptions import BlockStructureNotFound from ..factory import BlockStructureFactory from ..store import BlockStructureStore -from .helpers import ChildrenMapTestMixin, MockCache, MockModulestoreFactory +from .helpers import ChildrenMapTestMixin, MockCache, MockModulestoreFactory, MockXBlock, MockModulestore class TestBlockStructureFactory(TestCase, ChildrenMapTestMixin): @@ -77,3 +78,78 @@ def test_new(self): block_structure._block_data_map, # pylint: disable=protected-access ) self.assert_block_structure(new_structure, self.children_map) + + def test_from_modulestore_normalizes_locations_with_branch_info(self): + """ + Test that locations with branch/version information are normalized + when building block structures. + + This test verifies the fix for PR #37866, which ensures that when + creating block structures within the published_only branch context, + locations are normalized by removing branch/version information. + This prevents comparison mismatches when filtering inaccessible blocks. + + Without the fix, locations with branch info would be stored as-is, + causing issues when comparing with normalized locations later. + """ + # Create a course key with branch information to simulate + # the published_only branch context + course_key_with_branch = CourseLocator('org', 'course', 'run', branch='published') + root_usage_key = BlockUsageLocator( + course_key=course_key_with_branch, + block_type='html', + block_id='0' + ) + + # Create a modulestore with xblocks that have locations containing branch info + modulestore = MockModulestore() + blocks = {} + children_map = self.SIMPLE_CHILDREN_MAP + + # Create blocks with branch information in their locations + for block_id, children in enumerate(children_map): + # Create location with branch info + block_location = BlockUsageLocator( + course_key=course_key_with_branch, + block_type='html', + block_id=str(block_id) + ) + # Create child locations with branch info + child_locations = [ + BlockUsageLocator( + course_key=course_key_with_branch, + block_type='html', + block_id=str(child_id) + ) + for child_id in children + ] + blocks[block_location] = MockXBlock( + location=block_location, + children=child_locations, + modulestore=modulestore + ) + modulestore.set_blocks(blocks) + + # Build block structure from modulestore + block_structure = BlockStructureFactory.create_from_modulestore( + root_block_usage_key=root_usage_key, + modulestore=modulestore + ) + + # Verify that all stored block keys are normalized (without branch info) + # This is the key assertion: with the fix, all keys should be normalized + for block_key in block_structure: + # The block_key should equal its normalized version + normalized_key = block_key.for_branch(None) + self.assertEqual( + block_key, + normalized_key, + f"Block key {block_key} should be normalized (without branch info). " + f"Normalized version: {normalized_key}" + ) + # Verify it doesn't have branch information in the course_key + if hasattr(block_key.course_key, 'branch'): + self.assertIsNone( + block_key.course_key.branch, + f"Block key {block_key} should not have branch information" + )