Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor!: use String field instead of Dict field to store top_level_downstream_parent_key (#37448)

Browse Source 
* 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.

* chore: add `top_level_parent` field in ComponentLink and ContainerLink admin

* refactor: use ":" as separator

* refactor: block key parsing and tests
This commit is contained in:
Navin Karkera
2025-10-09 22:03:23 +05:30
committed by GitHub
parent 9110ae0d71
commit 09e86e24b2
10 changed files with 80 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
cms/djangoapps/contentstore/admin.py
View File

@@ -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",

15
cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py
View File

@@ -11,11 +11,10 @@ from opaque_keys.edx.keys import UsageKey
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 @@ class CourseToLibraryTestCase(ContentLibrariesRestApiTest, ModuleStoreTestCase):
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 @@ class CourseToLibraryTestCase(ContentLibrariesRestApiTest, ModuleStoreTestCase):
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 @@ class CourseToLibraryTestCase(ContentLibrariesRestApiTest, ModuleStoreTestCase):
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

25
cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py
View File

@@ -16,7 +16,7 @@ from organizations.models import Organization
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
@@ -157,7 +157,7 @@ class _BaseDownstreamViewTestMixin:
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 +171,7 @@ class _BaseDownstreamViewTestMixin:
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 +180,7 @@ class _BaseDownstreamViewTestMixin:
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 +189,7 @@ class _BaseDownstreamViewTestMixin:
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 +455,14 @@ class DeleteDownstreamViewTest(SharedErrorTestCases, SharedModuleStoreTestCase):
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 +1246,6 @@ class GetUpstreamViewTest(
'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):

6
cms/djangoapps/contentstore/tasks.py
View File

@@ -92,6 +92,7 @@ from xmodule.modulestore.exceptions import DuplicateCourseError, InvalidProctori
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)

11
cms/djangoapps/contentstore/utils.py
View File

@@ -117,6 +117,7 @@ from xmodule.partitions.partitions_service import (
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(

4
cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py
View File

@@ -33,7 +33,7 @@ from opaque_keys.edx.locator import LibraryUsageLocator, LibraryUsageLocatorV2
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,
),
},

6
cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py
View File

@@ -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}`
Extract block key from UsageKey in string format: `html:my-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]:

8
cms/lib/xblock/upstream_sync.py
View File

@@ -26,7 +26,7 @@ from opaque_keys.edx.keys import CourseKey
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
@@ -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 "

27
xmodule/tests/test_util_keys.py
View File

@@ -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 @@ class TestDeriveKey(TestCase):
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

16
xmodule/util/keys.py
View File

@@ -4,8 +4,7 @@ Utilities for working with opaque-keys.
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,6 +27,19 @@ class BlockKey(NamedTuple):
def from_usage_key(cls, usage_key):
return cls(usage_key.block_type, usage_key.block_id)
def __str__(self) -> str:
return f"{self.type}:{self.id}"
@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])
def derive_key(source: UsageKey, dest_parent: BlockKey) -> BlockKey:
"""