Skip to content
Merged
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
20 changes: 14 additions & 6 deletions openedx/core/djangoapps/content/block_structure/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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"
)
Loading