From 51f58a46635ce16644fdcdb81d65f1f23bc896d3 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 8 Oct 2025 21:00:19 +0530 Subject: [PATCH 1/4] refactor!: use String field instead of Dict field to store top_level_downstream_parent_key Since this is a new field no production instance should have this field yet. Developers need to delete their old courses as this change will raise error in all course pages. --- .../tests/test_downstream_sync_integration.py | 15 +++++------ .../v2/views/tests/test_downstreams.py | 26 ++++++++----------- cms/djangoapps/contentstore/tasks.py | 6 +++-- cms/djangoapps/contentstore/utils.py | 11 +++++--- .../xblock_storage_handlers/view_handlers.py | 4 +-- .../xblock_storage_handlers/xblock_helpers.py | 4 +-- cms/lib/xblock/upstream_sync.py | 6 ++--- xmodule/util/keys.py | 8 ++++++ 8 files changed, 44 insertions(+), 36 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py index e5cd063e8181..78730fe6a0dc 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py @@ -11,11 +11,10 @@ from freezegun import freeze_time from openedx.core.djangoapps.content_libraries.tests import ContentLibrariesRestApiTest -from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_dict +from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_string from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory -from xmodule.xml_block import serialize_field @ddt.ddt @@ -296,9 +295,9 @@ def test_unit_sync(self): parent_usage_key=str(self.course_subsection.usage_key), upstream_key=self.upstream_unit["id"], ) - downstream_unit_block_key = serialize_field(get_block_key_dict( + downstream_unit_block_key = get_block_key_string( UsageKey.from_string(downstream_unit["locator"]), - )).replace('"', '"') + ) status = self._get_sync_status(downstream_unit["locator"]) self.assertDictContainsEntries(status, { 'upstream_ref': self.upstream_unit["id"], # e.g. 'lct:CL-TEST:testlib:unit:u1' @@ -898,9 +897,9 @@ def test_unit_sync_with_modified_downstream(self): parent_usage_key=str(self.course_subsection.usage_key), upstream_key=self.upstream_unit["id"], ) - downstream_unit_block_key = serialize_field(get_block_key_dict( + downstream_unit_block_key = get_block_key_string( UsageKey.from_string(downstream_unit["locator"]), - )).replace('"', '"') + ) status = self._get_sync_status(downstream_unit["locator"]) self.assertDictContainsEntries(status, { 'upstream_ref': self.upstream_unit["id"], # e.g. 'lct:CL-TEST:testlib:unit:u1' @@ -1259,9 +1258,9 @@ def test_unit_decline_sync(self): parent_usage_key=str(self.course_subsection.usage_key), upstream_key=self.upstream_unit["id"], ) - downstream_unit_block_key = serialize_field(get_block_key_dict( + downstream_unit_block_key = get_block_key_string( UsageKey.from_string(downstream_unit["locator"]), - )).replace('"', '"') + ) children_downstream_keys = self._get_course_block_children(downstream_unit["locator"]) downstream_problem1 = children_downstream_keys[1] assert "type@problem" in downstream_problem1 diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index b8f2c8f41057..b532081b5a7d 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -16,7 +16,7 @@ from cms.djangoapps.contentstore.helpers import StaticFileNotices from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.xblock_storage_handlers import view_handlers as xblock_view_handlers -from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_dict +from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_string from cms.lib.xblock.upstream_sync import BadUpstream, UpstreamLink from common.djangoapps.student.auth import add_users from common.djangoapps.student.roles import CourseStaffRole @@ -25,6 +25,7 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory +from xmodule.util.keys import BlockKey from .. import downstreams as downstreams_views @@ -157,7 +158,7 @@ def setUp(self): parent=self.top_level_downstream_unit, upstream=self.html_lib_id_2, upstream_version=1, - top_level_downstream_parent_key=get_block_key_dict( + top_level_downstream_parent_key=get_block_key_string( self.top_level_downstream_unit.usage_key, ) ).usage_key @@ -171,7 +172,7 @@ def setUp(self): parent=self.top_level_downstream_chapter, upstream=self.top_level_subsection_id, upstream_version=1, - top_level_downstream_parent_key=get_block_key_dict( + top_level_downstream_parent_key=get_block_key_string( self.top_level_downstream_chapter.usage_key, ), ) @@ -180,7 +181,7 @@ def setUp(self): parent=self.top_level_downstream_sequential, upstream=self.top_level_unit_id_2, upstream_version=1, - top_level_downstream_parent_key=get_block_key_dict( + top_level_downstream_parent_key=get_block_key_string( self.top_level_downstream_chapter.usage_key, ), ) @@ -189,7 +190,7 @@ def setUp(self): parent=self.top_level_downstream_unit_2, upstream=self.video_lib_id_2, upstream_version=1, - top_level_downstream_parent_key=get_block_key_dict( + top_level_downstream_parent_key=get_block_key_string( self.top_level_downstream_chapter.usage_key, ) ).usage_key @@ -455,17 +456,14 @@ def test_unlink_parent_should_update_children_top_level_parent(self): unit = modulestore().get_item(self.top_level_downstream_unit_2.usage_key) # The sequential is the top-level parent for the unit - assert unit.top_level_downstream_parent_key == { - "id": str(self.top_level_downstream_sequential.usage_key.block_id), - "type": str(self.top_level_downstream_sequential.usage_key.block_type), - } + sequential_block_key = get_block_key_string( + self.top_level_downstream_sequential.usage_key + ) + assert unit.top_level_downstream_parent_key == sequential_block_key video = modulestore().get_item(self.top_level_downstream_video_key) # The sequential is the top-level parent for the video - assert video.top_level_downstream_parent_key == { - "id": str(self.top_level_downstream_sequential.usage_key.block_id), - "type": str(self.top_level_downstream_sequential.usage_key.block_type), - } + assert video.top_level_downstream_parent_key == sequential_block_key all_downstreams = self.client.get( "/api/contentstore/v2/downstreams/", @@ -1249,8 +1247,6 @@ def test_200_get_ready_to_sync_top_level_parents_with_components(self): 'downstream_is_modified': False, }, ] - print(data["results"]) - print(expected) self.assertListEqual(data["results"], expected) def test_200_get_ready_to_sync_top_level_parents_with_containers(self): diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 71b86acca201..91b49b8a37c7 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -92,6 +92,7 @@ from xmodule.modulestore.xml_exporter import export_course_to_xml, export_library_to_xml from xmodule.modulestore.xml_importer import CourseImportException, import_course_from_xml, import_library_from_xml from xmodule.tabs import StaticTab +from xmodule.util.keys import BlockKey from .models import ComponentLink, ContainerLink, LearningContextLinksStatus, LearningContextLinksStatusChoices from .outlines import update_outline_from_modulestore @@ -1649,10 +1650,11 @@ def handle_create_xblock_upstream_link(usage_key): if not xblock.upstream or not xblock.upstream_version: return if xblock.top_level_downstream_parent_key is not None: + block_key = BlockKey.from_string(xblock.top_level_downstream_parent_key) top_level_parent_usage_key = BlockUsageLocator( xblock.course_id, - xblock.top_level_downstream_parent_key.get('type'), - xblock.top_level_downstream_parent_key.get('id'), + block_key.type, + block_key.id, ) try: ContainerLink.get_by_downstream_usage_key(top_level_parent_usage_key) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 3ca1d20bf564..c6861b134dd8 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -117,6 +117,7 @@ get_all_partitions_for_course, # lint-amnesty, pylint: disable=wrong-import-order ) from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService +from xmodule.util.keys import BlockKey from .models import ComponentLink, ContainerLink @@ -2411,10 +2412,11 @@ def _create_or_update_component_link(created: datetime | None, xblock): top_level_parent_usage_key = None if xblock.top_level_downstream_parent_key is not None: + block_key = BlockKey.from_string(xblock.top_level_downstream_parent_key) top_level_parent_usage_key = BlockUsageLocator( xblock.usage_key.course_key, - xblock.top_level_downstream_parent_key.get('type'), - xblock.top_level_downstream_parent_key.get('id'), + block_key.type, + block_key.id, ) ComponentLink.update_or_create( @@ -2444,10 +2446,11 @@ def _create_or_update_container_link(created: datetime | None, xblock): top_level_parent_usage_key = None if xblock.top_level_downstream_parent_key is not None: + block_key = BlockKey.from_string(xblock.top_level_downstream_parent_key) top_level_parent_usage_key = BlockUsageLocator( xblock.usage_key.course_key, - xblock.top_level_downstream_parent_key.get('type'), - xblock.top_level_downstream_parent_key.get('id'), + block_key.type, + block_key.id, ) ContainerLink.update_or_create( diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index ae7492ddbc91..78c393532305 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -33,7 +33,7 @@ from pytz import UTC from xblock.core import XBlock from xblock.fields import Scope -from .xblock_helpers import get_block_key_dict +from .xblock_helpers import get_block_key_string from cms.djangoapps.contentstore.config.waffle import SHOW_REVIEW_RULES_FLAG from cms.djangoapps.contentstore.helpers import StaticFileNotices @@ -602,7 +602,7 @@ def sync_library_content( block_id=f"{block_type}{uuid4().hex[:8]}", fields={ "upstream": upstream_key, - "top_level_downstream_parent_key": get_block_key_dict( + "top_level_downstream_parent_key": get_block_key_string( top_level_downstream_parent.usage_key, ), }, diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py index 322bf530ab84..1ee837bb83ba 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py @@ -18,11 +18,11 @@ def usage_key_with_run(usage_key_string: str) -> UsageKey: return usage_key -def get_block_key_dict(usage_key: UsageKey) -> dict: +def get_block_key_string(usage_key: UsageKey) -> str: """ Converts the usage_key in a dict with the form: `{"type": block_type, "id": block_id}` """ - return BlockKey.from_usage_key(usage_key)._asdict() + return str(BlockKey.from_usage_key(usage_key)) def get_tags_count(xblock: XBlock, include_children=False) -> dict[str, int]: diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index da9a60b422bc..7874fcc1d0ec 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -337,7 +337,7 @@ def decline_sync(downstream: XBlock, user_id=None) -> None: def _update_children_top_level_parent( downstream: XBlock, - new_top_level_parent_key: dict[str, str] | None + new_top_level_parent_key: str | None, ) -> list[XBlock]: """ Given a new top-level parent block, update the `top_level_downstream_parent_key` field on the downstream block @@ -357,7 +357,7 @@ def _update_children_top_level_parent( # If the `new_top_level_parent_key` is None, the current level assume the top-level # parent key for its children. child_top_level_parent_key = new_top_level_parent_key if new_top_level_parent_key is not None else ( - BlockKey.from_usage_key(child.usage_key)._asdict() + str(BlockKey.from_usage_key(child.usage_key)) ) affected_blocks.extend(_update_children_top_level_parent(child, child_top_level_parent_key)) @@ -466,7 +466,7 @@ class UpstreamSyncMixin(XBlockMixin): default=None, scope=Scope.settings, hidden=True, enforce_type=True, ) - top_level_downstream_parent_key = Dict( + top_level_downstream_parent_key = String( help=( "The block key ('block_type@block_id') of the downstream block that is the top-level parent of " "this block. This is present if the creation of this block is a consequence of " diff --git a/xmodule/util/keys.py b/xmodule/util/keys.py index ceb35b269eb5..e5d19ac3930f 100644 --- a/xmodule/util/keys.py +++ b/xmodule/util/keys.py @@ -28,6 +28,14 @@ class BlockKey(NamedTuple): def from_usage_key(cls, usage_key): return cls(usage_key.block_type, usage_key.block_id) + def __str__(self): + return f"{self.type}@{self.id}" + + @classmethod + def from_string(cls, s): + parts = s.split('@') + return cls(parts[0], parts[1]) + def derive_key(source: UsageKey, dest_parent: BlockKey) -> BlockKey: """ From 3c24211d511e04defd1b934e5e4d29ccc5c2e584 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 8 Oct 2025 21:08:30 +0530 Subject: [PATCH 2/4] chore: add `top_level_parent` field in ComponentLink and ContainerLink admin --- cms/djangoapps/contentstore/admin.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cms/djangoapps/contentstore/admin.py b/cms/djangoapps/contentstore/admin.py index 67bb39b7a32a..ac11ea42d0a4 100644 --- a/cms/djangoapps/contentstore/admin.py +++ b/cms/djangoapps/contentstore/admin.py @@ -100,6 +100,7 @@ class ComponentLinkAdmin(admin.ModelAdmin): "upstream_context_key", "downstream_usage_key", "downstream_context_key", + "top_level_parent", "version_synced", "version_declined", "created", @@ -139,6 +140,7 @@ class ContainerLinkAdmin(admin.ModelAdmin): "upstream_context_key", "downstream_usage_key", "downstream_context_key", + "top_level_parent", "version_synced", "version_declined", "created", From d5b01003f91262186ad47b838da4772bac6a903c Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 8 Oct 2025 21:58:38 +0530 Subject: [PATCH 3/4] refactor: use ":" as separator --- .../rest_api/v2/views/tests/test_downstreams.py | 1 - .../xblock_storage_handlers/xblock_helpers.py | 2 +- cms/lib/xblock/upstream_sync.py | 2 +- xmodule/util/keys.py | 11 +++++------ 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index b532081b5a7d..c6f24496f241 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -25,7 +25,6 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory -from xmodule.util.keys import BlockKey from .. import downstreams as downstreams_views diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py index 1ee837bb83ba..82ed7297d5af 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py @@ -20,7 +20,7 @@ def usage_key_with_run(usage_key_string: str) -> UsageKey: def get_block_key_string(usage_key: UsageKey) -> str: """ - Converts the usage_key in a dict with the form: `{"type": block_type, "id": block_id}` + Extract block key from UsageKey in string format: `html:my-id`. """ return str(BlockKey.from_usage_key(usage_key)) diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 7874fcc1d0ec..5e812f7035f5 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -26,7 +26,7 @@ from opaque_keys.edx.locator import LibraryContainerLocator, LibraryUsageLocatorV2 from opaque_keys.edx.keys import UsageKey from xblock.exceptions import XBlockNotFoundError -from xblock.fields import Scope, String, Integer, Dict, List +from xblock.fields import Scope, String, Integer, List from xblock.core import XBlockMixin, XBlock from xmodule.util.keys import BlockKey diff --git a/xmodule/util/keys.py b/xmodule/util/keys.py index e5d19ac3930f..804223c38f1c 100644 --- a/xmodule/util/keys.py +++ b/xmodule/util/keys.py @@ -4,8 +4,7 @@ Consider moving these into opaque-keys if they generalize well. """ import hashlib -from typing import NamedTuple - +from typing import NamedTuple, Self from opaque_keys.edx.keys import UsageKey @@ -28,12 +27,12 @@ class BlockKey(NamedTuple): def from_usage_key(cls, usage_key): return cls(usage_key.block_type, usage_key.block_id) - def __str__(self): - return f"{self.type}@{self.id}" + def __str__(self) -> str: + return f"{self.type}:{self.id}" @classmethod - def from_string(cls, s): - parts = s.split('@') + def from_string(cls, s: str) -> Self: + parts = s.split(':') return cls(parts[0], parts[1]) From 76f75e129af82158d3f4123d66ca84a85d183934 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 9 Oct 2025 12:43:37 +0530 Subject: [PATCH 4/4] refactor: block key parsing and tests --- xmodule/tests/test_util_keys.py | 27 +++++++++++++++++++++++++++ xmodule/util/keys.py | 5 +++++ 2 files changed, 32 insertions(+) diff --git a/xmodule/tests/test_util_keys.py b/xmodule/tests/test_util_keys.py index dcd16d6b7873..ceda27847830 100644 --- a/xmodule/tests/test_util_keys.py +++ b/xmodule/tests/test_util_keys.py @@ -2,6 +2,7 @@ Tests for xmodule/util/keys.py """ import ddt +import pytest from unittest import TestCase from unittest.mock import Mock @@ -43,3 +44,29 @@ def test_derive_key(self, source, parent, expected): Test that derive_key returns the expected value. """ assert derive_key(source, parent) == expected + + +@ddt.ddt +class TestBlockKeyParsing(TestCase): + """ + Tests for parsing BlockKeys. + """ + + @ddt.data(['chapter:some-id', 'chapter', 'some-id'], ['section:one-more-id', 'section', 'one-more-id']) + @ddt.unpack + def test_block_key_from_string(self, block_key_str, blockType, blockId): + block_key = BlockKey.from_string(block_key_str) + assert block_key.type == blockType + assert block_key.id == blockId + + @ddt.data('chapter:invalid:some-id', 'sectionone-more-id') + def test_block_key_from_string_error(self, block_key_str): + with pytest.raises(ValueError): + BlockKey.from_string(block_key_str) + + @ddt.data( + [BlockKey('chapter', 'some-id'), 'chapter:some-id'], [BlockKey('section', 'one-more-id'), 'section:one-more-id'] + ) + @ddt.unpack + def test_block_key_to_string(self, block_key, block_key_str): + assert str(block_key) == block_key_str diff --git a/xmodule/util/keys.py b/xmodule/util/keys.py index 804223c38f1c..9570079200cc 100644 --- a/xmodule/util/keys.py +++ b/xmodule/util/keys.py @@ -32,7 +32,12 @@ def __str__(self) -> str: @classmethod def from_string(cls, s: str) -> Self: + """ + Convert a BlockKey string into a BlockKey object. + """ parts = s.split(':') + if len(parts) != 2 or not parts[0] or not parts[1]: + raise ValueError(f"Invalid string format for BlockKey: {s}") return cls(parts[0], parts[1])