From 857d0b4aaeacf1728d3352e680dfa5f11b4e576d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 16 Jul 2025 20:44:13 -0300 Subject: [PATCH 01/18] feat: copy endpoint for Library Containers --- cms/djangoapps/contentstore/helpers.py | 16 +- .../content_libraries/api/__init__.py | 1 + .../content_libraries/api/block_metadata.py | 4 +- .../api/container_metadata.py | 145 ++++++++++++++++ .../content_libraries/api/containers.py | 163 ++++-------------- .../content_libraries/api/serializers.py | 58 +++++++ .../content_libraries/rest_api/containers.py | 29 +++- .../core/djangoapps/content_libraries/urls.py | 1 + .../core/djangoapps/content_staging/api.py | 151 ++++++++++++---- .../core/djangoapps/content_staging/models.py | 1 + .../core/djangoapps/content_staging/views.py | 3 +- openedx/core/djangoapps/xblock/api.py | 4 +- .../lib/xblock_serializer/block_serializer.py | 14 +- 13 files changed, 413 insertions(+), 177 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/api/container_metadata.py create mode 100644 openedx/core/djangoapps/content_libraries/api/serializers.py diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index ff2020afd89f..a92a22dacc8b 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -376,7 +376,10 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> store, user=request.user, slug_hint=user_clipboard.source_usage_key.block_id, - copied_from_block=str(user_clipboard.source_usage_key), + # WIP: Remove this + copied_from_block=( + str(user_clipboard.source_usage_key) if "fake" not in str(user_clipboard.source_usage_key) else None + ), copied_from_version_num=user_clipboard.content.version_num, tags=user_clipboard.content.tags, ) @@ -508,6 +511,8 @@ def _import_xml_node_to_parent( runtime = parent_xblock.runtime parent_key = parent_xblock.scope_ids.usage_id block_type = node.tag + upstream = node.attrib.get('upstream', None) + upstream_version = node.attrib.get('upstream_version', None) # Modulestore's IdGenerator here is SplitMongoIdManager which is assigned # by CachingDescriptorSystem Runtime and since we need our custom ImportIdGenerator @@ -567,6 +572,8 @@ def _import_xml_node_to_parent( raise NotImplementedError("We don't yet support pasting XBlocks with children") if copied_from_block: _fetch_and_set_upstream_link(copied_from_block, copied_from_version_num, temp_xblock, user) + elif upstream: + _fetch_and_set_upstream_link(upstream, upstream_version, temp_xblock, user) # Save the XBlock into modulestore. We need to save the block and its parent for this to work: new_xblock = store.update_item(temp_xblock, user.id, allow_not_found=True) new_xblock.parent = parent_key @@ -582,13 +589,16 @@ def _import_xml_node_to_parent( if not children_handled: for child_node in child_nodes: - child_copied_from = _get_usage_key_from_node(child_node, copied_from_block) if copied_from_block else None + child_copied_from = str( + _get_usage_key_from_node(child_node, copied_from_block) + ) if copied_from_block else None + _import_xml_node_to_parent( child_node, new_xblock, store, user=user, - copied_from_block=str(child_copied_from), + copied_from_block=child_copied_from, tags=tags, ) diff --git a/openedx/core/djangoapps/content_libraries/api/__init__.py b/openedx/core/djangoapps/content_libraries/api/__init__.py index 6c5cbce2a2fa..5f7db8b17f72 100644 --- a/openedx/core/djangoapps/content_libraries/api/__init__.py +++ b/openedx/core/djangoapps/content_libraries/api/__init__.py @@ -3,6 +3,7 @@ """ from .block_metadata import * from .collections import * +from .container_metadata import * from .containers import * from .courseware_import import * from .exceptions import * diff --git a/openedx/core/djangoapps/content_libraries/api/block_metadata.py b/openedx/core/djangoapps/content_libraries/api/block_metadata.py index ec5c43b1218b..f117d2762949 100644 --- a/openedx/core/djangoapps/content_libraries/api/block_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/block_metadata.py @@ -1,7 +1,5 @@ """ -Content libraries API methods related to XBlocks/Components. - -These methods don't enforce permissions (only the REST APIs do). +Content libraries data classes related to XBlocks/Components. """ from __future__ import annotations from dataclasses import dataclass diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py new file mode 100644 index 000000000000..46eb0d62b8ac --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -0,0 +1,145 @@ +""" +Content libraries data classes related to Containers. +""" +from __future__ import annotations + +from dataclasses import dataclass +from enum import Enum + +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2 +from openedx_learning.api import authoring as authoring_api +from openedx_learning.api.authoring_models import Container + +from openedx.core.djangoapps.content_tagging.api import get_object_tag_counts + +from .libraries import PublishableItem + +# The public API is only the following symbols: +__all__ = [ + # Models + "ContainerMetadata", + "ContainerType", + # Methods + "library_container_locator", +] + + +class ContainerType(Enum): + """ + The container types supported by content_libraries, and logic to map them to OLX. + """ + Unit = "unit" + Subsection = "subsection" + Section = "section" + + @property + def olx_tag(self) -> str: + """ + Canonical XML tag to use when representing this container as OLX. + + For example, Units are encoded as .... + + These tag names are historical. We keep them around for the backwards compatibility of OLX + and for easier interaction with legacy modulestore-powered structural XBlocks + (e.g., copy-paste of Units between courses and V2 libraries). + """ + match self: + case self.Unit: + return "vertical" + case self.Subsection: + return "sequential" + case self.Section: + return "chapter" + raise TypeError(f"unexpected ContainerType: {self!r}") + + @classmethod + def from_source_olx_tag(cls, olx_tag: str) -> 'ContainerType': + """ + Get the ContainerType that this OLX tag maps to. + """ + if olx_tag == "unit": + # There is an alternative implementation to VerticalBlock called UnitBlock whose + # OLX tag is . When converting from OLX, we want to handle both + # and as Unit containers, although the canonical serialization is still . + return cls.Unit + try: + return next(ct for ct in cls if olx_tag == ct.olx_tag) + except StopIteration: + raise ValueError(f"no container_type for XML tag: <{olx_tag}>") from None + + +@dataclass(frozen=True, kw_only=True) +class ContainerMetadata(PublishableItem): + """ + Class that represents the metadata about a Container (e.g. Unit) in a content library. + """ + container_key: LibraryContainerLocator + container_type: ContainerType + container_pk: int + + @classmethod + def from_container(cls, library_key, container: Container, associated_collections=None): + """ + Construct a ContainerMetadata object from a Container object. + """ + last_publish_log = container.versioning.last_publish_log + container_key = library_container_locator( + library_key, + container=container, + ) + container_type = ContainerType(container_key.container_type) + published_by = "" + if last_publish_log and last_publish_log.published_by: + published_by = last_publish_log.published_by.username + + draft = container.versioning.draft + published = container.versioning.published + last_draft_created = draft.created if draft else None + if draft and draft.publishable_entity_version.created_by: + last_draft_created_by = draft.publishable_entity_version.created_by.username + else: + last_draft_created_by = "" + tags = get_object_tag_counts(str(container_key), count_implicit=True) + + return cls( + container_key=container_key, + container_type=container_type, + container_pk=container.pk, + display_name=draft.title, + created=container.created, + modified=draft.created, + draft_version_num=draft.version_num, + published_version_num=published.version_num if published else None, + published_display_name=published.title if published else None, + last_published=None if last_publish_log is None else last_publish_log.published_at, + published_by=published_by, + last_draft_created=last_draft_created, + last_draft_created_by=last_draft_created_by, + has_unpublished_changes=authoring_api.contains_unpublished_changes(container.pk), + tags_count=tags.get(str(container_key), 0), + collections=associated_collections or [], + ) + + +def library_container_locator( + library_key: LibraryLocatorV2, + container: Container, +) -> LibraryContainerLocator: + """ + Returns a LibraryContainerLocator for the given library + container. + """ + container_type = None + if hasattr(container, 'unit'): + container_type = ContainerType.Unit + elif hasattr(container, 'subsection'): + container_type = ContainerType.Subsection + elif hasattr(container, 'section'): + container_type = ContainerType.Section + + assert container_type is not None + + return LibraryContainerLocator( + library_key, + container_type=container_type.value, + container_id=container.publishable_entity.key, + ) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 5e576a92f117..7f2be415ebd5 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -3,11 +3,10 @@ """ from __future__ import annotations -from dataclasses import dataclass from datetime import datetime, timezone -from enum import Enum import logging from uuid import uuid4 +import typing from django.utils.text import slugify from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 @@ -26,158 +25,40 @@ from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import Container, ContainerVersion, Component from openedx.core.djangoapps.content_libraries.api.collections import library_collection_locator -from openedx.core.djangoapps.content_tagging.api import get_object_tag_counts from openedx.core.djangoapps.xblock.api import get_component_from_usage_key from ..models import ContentLibrary -from .exceptions import ContentLibraryContainerNotFound -from .libraries import PublishableItem from .block_metadata import LibraryXBlockMetadata +from .container_metadata import ContainerMetadata, ContainerType, library_container_locator +from .exceptions import ContentLibraryContainerNotFound +from .serializers import ContainerSerializer + from .. import tasks +if typing.TYPE_CHECKING: + from openedx.core.djangoapps.content_staging.api import UserClipboardData + + # The public API is only the following symbols: __all__ = [ - # Models - "ContainerMetadata", - "ContainerType", - # API methods "get_container", "create_container", "get_container_children", "get_container_children_count", - "library_container_locator", "update_container", "delete_container", "restore_container", "update_container_children", "get_containers_contains_item", "publish_container_changes", + "copy_container", + "library_container_locator", ] log = logging.getLogger(__name__) -class ContainerType(Enum): - """ - The container types supported by content_libraries, and logic to map them to OLX. - """ - Unit = "unit" - Subsection = "subsection" - Section = "section" - - @property - def olx_tag(self) -> str: - """ - Canonical XML tag to use when representing this container as OLX. - - For example, Units are encoded as .... - - These tag names are historical. We keep them around for the backwards compatibility of OLX - and for easier interaction with legacy modulestore-powered structural XBlocks - (e.g., copy-paste of Units between courses and V2 libraries). - """ - match self: - case self.Unit: - return "vertical" - case self.Subsection: - return "sequential" - case self.Section: - return "chapter" - raise TypeError(f"unexpected ContainerType: {self!r}") - - @classmethod - def from_source_olx_tag(cls, olx_tag: str) -> 'ContainerType': - """ - Get the ContainerType that this OLX tag maps to. - """ - if olx_tag == "unit": - # There is an alternative implementation to VerticalBlock called UnitBlock whose - # OLX tag is . When converting from OLX, we want to handle both - # and as Unit containers, although the canonical serialization is still . - return cls.Unit - try: - return next(ct for ct in cls if olx_tag == ct.olx_tag) - except StopIteration: - raise ValueError(f"no container_type for XML tag: <{olx_tag}>") from None - - -@dataclass(frozen=True, kw_only=True) -class ContainerMetadata(PublishableItem): - """ - Class that represents the metadata about a Container (e.g. Unit) in a content library. - """ - container_key: LibraryContainerLocator - container_type: ContainerType - container_pk: int - - @classmethod - def from_container(cls, library_key, container: Container, associated_collections=None): - """ - Construct a ContainerMetadata object from a Container object. - """ - last_publish_log = container.versioning.last_publish_log - container_key = library_container_locator( - library_key, - container=container, - ) - container_type = ContainerType(container_key.container_type) - published_by = "" - if last_publish_log and last_publish_log.published_by: - published_by = last_publish_log.published_by.username - - draft = container.versioning.draft - published = container.versioning.published - last_draft_created = draft.created if draft else None - if draft and draft.publishable_entity_version.created_by: - last_draft_created_by = draft.publishable_entity_version.created_by.username - else: - last_draft_created_by = "" - tags = get_object_tag_counts(str(container_key), count_implicit=True) - - return cls( - container_key=container_key, - container_type=container_type, - container_pk=container.pk, - display_name=draft.title, - created=container.created, - modified=draft.created, - draft_version_num=draft.version_num, - published_version_num=published.version_num if published else None, - published_display_name=published.title if published else None, - last_published=None if last_publish_log is None else last_publish_log.published_at, - published_by=published_by, - last_draft_created=last_draft_created, - last_draft_created_by=last_draft_created_by, - has_unpublished_changes=authoring_api.contains_unpublished_changes(container.pk), - tags_count=tags.get(str(container_key), 0), - collections=associated_collections or [], - ) - - -def library_container_locator( - library_key: LibraryLocatorV2, - container: Container, -) -> LibraryContainerLocator: - """ - Returns a LibraryContainerLocator for the given library + container. - """ - if hasattr(container, 'unit'): - container_type = ContainerType.Unit - elif hasattr(container, 'subsection'): - container_type = ContainerType.Subsection - elif hasattr(container, 'section'): - container_type = ContainerType.Section - - assert container_type is not None - - return LibraryContainerLocator( - library_key, - container_type=container_type.value, - container_id=container.publishable_entity.key, - ) - - def _get_container_from_key(container_key: LibraryContainerLocator, isDeleted=False) -> Container: """ Internal method to fetch the Container object from its LibraryContainerLocator @@ -728,3 +609,25 @@ def publish_container_changes(container_key: LibraryContainerLocator, user_id: i # Update the search index (and anything else) for the affected container + blocks # This is mostly synchronous but may complete some work asynchronously if there are a lot of changes. tasks.wait_for_post_publish_events(publish_log, library_key) + + +def copy_container(container_key: LibraryContainerLocator, user_id: int) -> UserClipboardData: + """ + Copy a container (a Section, Subsection, or Unit) to the content staging. + """ + container_metadata = get_container(container_key) + container_olx = ContainerSerializer(container_metadata).olx_str + block_type = ContainerType(container_key.container_type).olx_tag + + from openedx.core.djangoapps.content_staging import api as content_staging_api + + return content_staging_api.save_content_to_user_clipboard( + user_id=user_id, + block_type=block_type, + olx=container_olx, + display_name=container_metadata.display_name, + suggested_url_name=str(container_key), + source_usage_key=str(container_key), + tags=None, # WIP: Handle tags + version_num=container_metadata.published_version_num, + ) diff --git a/openedx/core/djangoapps/content_libraries/api/serializers.py b/openedx/core/djangoapps/content_libraries/api/serializers.py new file mode 100644 index 000000000000..d44d66e8e8b5 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/api/serializers.py @@ -0,0 +1,58 @@ +""" +Serializer classes for containers +""" +from lxml import etree + +from openedx.core.djangoapps.xblock import api as xblock_api +from openedx.core.lib.xblock_serializer.api import StaticFile, XBlockSerializer + +from . import containers as container_api + + +class ContainerSerializer: + """ + Serializes a container (a Section, Subsection, or Unit) to OLX. + """ + static_files: list[StaticFile] + + def __init__(self, container_metadata: container_api.ContainerMetadata): + self.container_metadata = container_metadata + self.static_files = [] + olx_node = self._serialize_container(container_metadata) + + self.olx_str = etree.tostring(olx_node, encoding="unicode", pretty_print=True) + + def _serialize_container(self, container_metadata: container_api.ContainerMetadata) -> etree.Element: + """ + Serialize the given container to OLX. + """ + # Create an XML node to hold the exported data + container_type = container_api.ContainerType(container_metadata.container_key.container_type) + + olx = etree.Element(container_type.olx_tag) + olx.attrib["upstream"] = str(container_metadata.container_key) + olx.attrib["upstream_version"] = str(container_metadata.draft_version_num) + + # Serialize the container's metadata + olx.attrib["display_name"] = container_metadata.display_name + + children = container_api.get_container_children(container_metadata.container_key) + for child in children: + if isinstance(child, container_api.ContainerMetadata): + # If the child is a container, serialize it recursively + child_node = self._serialize_container(child) + olx.append(child_node) + elif isinstance(child, container_api.LibraryXBlockMetadata): + xblock = xblock_api.load_block( + child.usage_key, + user=None, + ) + xblock_serializer = XBlockSerializer( + xblock, + fetch_asset_data=True, + add_upstream=True, + ) + olx.append(xblock_serializer.olx_node) + self.static_files.extend(xblock_serializer.static_files) + + return olx diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index d861cbceecfd..ac79133d5137 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -9,8 +9,7 @@ from django.db.transaction import non_atomic_requests from django.utils.decorators import method_decorator from drf_yasg.utils import swagger_auto_schema - -from opaque_keys.edx.locator import LibraryLocatorV2, LibraryContainerLocator +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2 from openedx_learning.api import authoring as authoring_api from rest_framework.generics import GenericAPIView from rest_framework.response import Response @@ -19,6 +18,7 @@ from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.lib.api.view_utils import view_auth_classes from openedx.core.types.http import RestRequest + from . import serializers from .utils import convert_exceptions @@ -350,3 +350,28 @@ def post(self, request: RestRequest, container_key: LibraryContainerLocator) -> # If we need to in the future, we could return a list of all the child containers/components that were # auto-published as a result. return Response({}) + + +@view_auth_classes() +class LibraryContainerCopyView(GenericAPIView): + """ + View to copy a container to clipboard + """ + @convert_exceptions + def post(self, request: RestRequest, container_key: LibraryContainerLocator) -> Response: + """ + Copy a Container to clipboard + """ + api.require_permission_for_library_key( + container_key.lib_key, + request.user, + permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, + ) + assert request.user.id is not None, "User must be authenticated to copy a container" + + api.copy_container( + container_key, + user_id=request.user.id, + ) + + return Response({}) diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 2b9cd59af7e9..aa3dfc469b8e 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -86,6 +86,7 @@ path('collections/', containers.LibraryContainerCollectionsView.as_view(), name='update-collections-ct'), # Publish a container (or reset to last published) path('publish/', containers.LibraryContainerPublishView.as_view()), + path('copy/', containers.LibraryContainerCopyView.as_view()), ])), re_path(r'^lti/1.3/', include([ path('login/', libraries.LtiToolLoginView.as_view(), name='lti-login'), diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 7baae10baed4..653b4c686c55 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -18,26 +18,36 @@ from xmodule.contentstore.content import StaticContent from xmodule.contentstore.django import contentstore -from .data import ( - CLIPBOARD_PURPOSE, - StagedContentData, - StagedContentFileData, - StagedContentStatus, - UserClipboardData, -) -from .models import ( - UserClipboard as _UserClipboard, - StagedContent as _StagedContent, - StagedContentFile as _StagedContentFile, -) -from .serializers import ( - UserClipboardSerializer as _UserClipboardSerializer, -) +from .data import CLIPBOARD_PURPOSE, StagedContentData, StagedContentFileData, StagedContentStatus, UserClipboardData +from .models import StagedContent as _StagedContent +from .models import StagedContentFile as _StagedContentFile +from .models import UserClipboard as _UserClipboard +from .serializers import UserClipboardSerializer as _UserClipboardSerializer from .tasks import delete_expired_clipboards log = logging.getLogger(__name__) +def _get_expired_staged_content_ids(user_id: int) -> list[int]: + """ + Mark all of the user's existing StagedContent rows as EXPIRED. + This is used when the user clears their clipboard. + """ + expired_ids: list[int] = [] + to_expire = _StagedContent.objects.filter( + user_id=user_id, + purpose=CLIPBOARD_PURPOSE, + ).exclude( + status=StagedContentStatus.EXPIRED, + ) # WIP: Should we exclude or only return the ids? + for sc in to_expire: + expired_ids.append(sc.id) + sc.status = StagedContentStatus.EXPIRED + sc.save() + + return expired_ids + + def _save_xblock_to_staged_content( block: XBlock, user_id: int, purpose: str, version_num: int | None = None ) -> _StagedContent: @@ -51,20 +61,10 @@ def _save_xblock_to_staged_content( ) usage_key = block.usage_key - expired_ids = [] + expired_ids = None with transaction.atomic(): if purpose == CLIPBOARD_PURPOSE: - # Mark all of the user's existing StagedContent rows as EXPIRED - to_expire = _StagedContent.objects.filter( - user_id=user_id, - purpose=purpose, - ).exclude( - status=StagedContentStatus.EXPIRED, - ) - for sc in to_expire: - expired_ids.append(sc.id) - sc.status = StagedContentStatus.EXPIRED - sc.save() + expired_ids = _get_expired_staged_content_ids(user_id) # Insert a new StagedContent row for this staged_content = _StagedContent.objects.create( @@ -89,11 +89,61 @@ def _save_xblock_to_staged_content( except Exception: # pylint: disable=broad-except log.exception(f"Unable to copy static files to staged content for component {usage_key}") - # Enqueue a (potentially slow) task to delete the old staged content - try: - delete_expired_clipboards.delay(expired_ids) - except Exception: # pylint: disable=broad-except - log.exception(f"Unable to enqueue cleanup task for StagedContents: {','.join(str(x) for x in expired_ids)}") + if expired_ids: + # Enqueue a (potentially slow) task to delete the old staged content + try: + # WIP: Won't this already be deleted by _get_expired_staged_content_ids? + delete_expired_clipboards.delay(expired_ids) + except Exception: # pylint: disable=broad-except + log.exception(f"Unable to enqueue cleanup task for StagedContents: {','.join(str(x) for x in expired_ids)}") + + return staged_content + + +# WIP: Merge this with _save_xblock_to_staged_content? +def _save_data_to_staged_content( + user_id: int, + purpose: str, + block_type: str, + olx: str, + display_name: str, + suggested_url_name: str, + tags: dict[str, str] | None = None, + version_num: int | None = None, +) -> _StagedContent: + """ + Save arbitrary OLX data to staged content. + This is used by the library sync functionality to save OLX data + that is not associated with any XBlock. + """ + + expired_ids = None + with transaction.atomic(): + if purpose == CLIPBOARD_PURPOSE: + expired_ids = _get_expired_staged_content_ids(user_id) + + # Insert a new StagedContent row for this + staged_content = _StagedContent.objects.create( + user_id=user_id, + purpose=purpose, + status=StagedContentStatus.READY, + block_type=block_type, + olx=olx, + display_name=display_name, + suggested_url_name=suggested_url_name, + tags=tags or {}, + version_num=(version_num or 0), + ) + + # Log an event so we can analyze how this feature is used: + log.info(f'Saved {block_type} content to staged content for {purpose}.') + + if expired_ids: + # Enqueue a (potentially slow) task to delete the old staged content + try: + delete_expired_clipboards.delay(expired_ids) + except Exception: # pylint: disable=broad-except + log.exception(f"Unable to enqueue cleanup task for StagedContents: {','.join(str(x) for x in expired_ids)}") return staged_content @@ -166,6 +216,43 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int return _user_clipboard_model_to_data(clipboard) +# WIP: Merge this with save_xblock_to_user_clipboard? +def save_content_to_user_clipboard( + user_id: int, + block_type: str, + olx: str, + display_name: str, + suggested_url_name: str, + source_usage_key: str, + tags: dict[str, str] | None = None, + version_num: int | None = None, +) -> UserClipboardData: + """ + Copy arbitrary OLX data to the user's clipboard. + """ + staged_content = _save_data_to_staged_content( + user_id=user_id, + purpose=CLIPBOARD_PURPOSE, + block_type=block_type, + olx=olx, + display_name=display_name, + suggested_url_name=suggested_url_name, + tags=tags, + version_num=version_num, + ) + + # Create/update the clipboard entry + (clipboard, _created) = _UserClipboard.objects.update_or_create( + user_id=user_id, + defaults={ + "content": staged_content, + "source_usage_key": "lb:orgA:lib1:unit:fake", # WIP: This shouldn't be a usage key + }, + ) + + return _user_clipboard_model_to_data(clipboard) + + def stage_xblock_temporarily( block: XBlock, user_id: int, purpose: str, version_num: int | None = None, ) -> _StagedContent: diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 1fd02cb43784..49100a2887ae 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -110,6 +110,7 @@ class UserClipboard(models.Model): # previously copied items are not kept. user = models.OneToOneField(User, on_delete=models.CASCADE, primary_key=True) content = models.ForeignKey(StagedContent, on_delete=models.CASCADE) + # WIP: We need to change this type to support ContainerKeys source_usage_key = UsageKeyField( max_length=255, help_text=_("Original usage key/ID of the thing that is in the clipboard."), diff --git a/openedx/core/djangoapps/content_staging/views.py b/openedx/core/djangoapps/content_staging/views.py index f0a20540ba8d..37bfa54d7df2 100644 --- a/openedx/core/djangoapps/content_staging/views.py +++ b/openedx/core/djangoapps/content_staging/views.py @@ -16,7 +16,6 @@ from rest_framework.views import APIView from common.djangoapps.student.auth import has_studio_read_access -from openedx.core.djangoapps.content_libraries import api as lib_api from openedx.core.djangoapps.xblock import api as xblock_api from openedx.core.lib.api.view_utils import view_auth_classes from xmodule.modulestore.django import modulestore @@ -110,6 +109,8 @@ def post(self, request): version_num = None elif isinstance(course_key, LibraryLocatorV2): + from openedx.core.djangoapps.content_libraries import api as lib_api + lib_api.require_permission_for_library_key( course_key, request.user, diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index c806fefc87c5..c0293f614b00 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -50,7 +50,7 @@ log = logging.getLogger(__name__) -def get_runtime(user: UserType): +def get_runtime(user: UserType | None) -> LearningCoreXBlockRuntime: """ Return a new XBlockRuntime. @@ -71,7 +71,7 @@ def get_runtime(user: UserType): def load_block( usage_key: UsageKeyV2, - user: UserType, + user: UserType | None, *, check_permission: CheckPerm | None = CheckPerm.CAN_LEARN, version: int | LatestVersion = LatestVersion.AUTO, diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index b8eabfcb4f11..16c31a6f5eab 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -21,20 +21,23 @@ class XBlockSerializer: """ static_files: list[StaticFile] tags: TagValuesByObjectIdDict + olx_node: etree.Element + olx_str: str - def __init__(self, block, write_url_name=True, fetch_asset_data=False): + def __init__(self, block, write_url_name=True, fetch_asset_data=False, add_upstream=False): """ Serialize an XBlock to an OLX string + supporting files, and store the resulting data in this object. """ self.write_url_name = write_url_name + self.add_upstream = add_upstream self.orig_block_key = block.scope_ids.usage_id self.static_files = [] self.tags = {} - olx_node = self._serialize_block(block) + self.olx_node = self._serialize_block(block) - self.olx_str = etree.tostring(olx_node, encoding="unicode", pretty_print=True) + self.olx_str = etree.tostring(self.olx_node, encoding="unicode", pretty_print=True) course_key = self.orig_block_key.course_key # Search the OLX for references to files stored in the course's @@ -131,7 +134,7 @@ def _serialize_normal_block(self, block) -> etree.Element: return olx_node - def _serialize_children(self, block, parent_olx_node): + def _serialize_children(self, block, parent_olx_node) -> None: """ Recursively serialize the children of XBlock 'block'. Subclasses may override this. @@ -146,6 +149,9 @@ def _serialize_html_block(self, block) -> etree.Element: """ olx_node = etree.Element("html") olx_node.attrib["url_name"] = block.scope_ids.usage_id.block_id + if self.add_upstream: + olx_node.attrib["upstream"] = str(block.scope_ids.usage_id) + olx_node.attrib["upstream_version"] = str(block.scope_ids.version) if block.display_name: olx_node.attrib["display_name"] = block.display_name if block.fields["editor"].is_set_on(block): From 5ac9bcf1caf9a11d0425fbd7b18a5b7d9727a97c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 24 Jul 2025 16:17:25 -0300 Subject: [PATCH 02/18] fix: make source_usage_key optional and removing upstram info for xblock olx --- cms/djangoapps/contentstore/helpers.py | 47 +++++++----- .../content_libraries/api/blocks.py | 6 +- .../content_libraries/api/containers.py | 8 +- .../content_libraries/api/serializers.py | 10 ++- .../core/djangoapps/content_staging/api.py | 74 ++++++++----------- .../core/djangoapps/content_staging/data.py | 8 +- ...06_alter_userclipboard_source_usage_key.py | 19 +++++ .../core/djangoapps/content_staging/models.py | 8 +- .../djangoapps/content_staging/serializers.py | 2 + .../lib/xblock_serializer/block_serializer.py | 6 +- 10 files changed, 104 insertions(+), 84 deletions(-) create mode 100644 openedx/core/djangoapps/content_staging/migrations/0006_alter_userclipboard_source_usage_key.py diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index a92a22dacc8b..d7820eb317c6 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -286,6 +286,27 @@ class StaticFileNotices: error_files: list[str] = Factory(list) +def _rewrite_static_asset_references(downstream_xblock: XBlock, substitutions: dict[str, str], user_id: int) -> None: + """ + Rewrite the static asset references in the OLX string to point to the new locations in the course. + """ + store = modulestore() + if hasattr(downstream_xblock, "data"): + data_with_substitutions = downstream_xblock.data + for old_static_ref, new_static_ref in substitutions.items(): + data_with_substitutions = _replace_strings( + data_with_substitutions, + old_static_ref, + new_static_ref, + ) + downstream_xblock.data = data_with_substitutions + if store is not None: + store.update_item(downstream_xblock, user_id) + + for child in downstream_xblock.get_children(): + _rewrite_static_asset_references(child, substitutions, user_id) + + def _insert_static_files_into_downstream_xblock( downstream_xblock: XBlock, staged_content_id: int, request ) -> StaticFileNotices: @@ -308,21 +329,12 @@ def _insert_static_files_into_downstream_xblock( static_files=static_files, ) - # Rewrite the OLX's static asset references to point to the new - # locations for those assets. See _import_files_into_course for more - # info on why this is necessary. - store = modulestore() - if hasattr(downstream_xblock, "data") and substitutions: - data_with_substitutions = downstream_xblock.data - for old_static_ref, new_static_ref in substitutions.items(): - data_with_substitutions = _replace_strings( - data_with_substitutions, - old_static_ref, - new_static_ref, - ) - downstream_xblock.data = data_with_substitutions - if store is not None: - store.update_item(downstream_xblock, request.user.id) + if substitutions: + # Rewrite the OLX's static asset references to point to the new + # locations for those assets. See _import_files_into_course for more + # info on why this is necessary. + _rewrite_static_asset_references(downstream_xblock, substitutions, request.user.id) + return notices @@ -375,10 +387,9 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> parent_xblock, store, user=request.user, - slug_hint=user_clipboard.source_usage_key.block_id, - # WIP: Remove this + slug_hint=user_clipboard.source_usage_key.block_id if user_clipboard.source_usage_key else None, copied_from_block=( - str(user_clipboard.source_usage_key) if "fake" not in str(user_clipboard.source_usage_key) else None + str(user_clipboard.source_usage_key) if user_clipboard.source_usage_key else None ), copied_from_version_num=user_clipboard.content.version_num, tags=user_clipboard.content.tags, diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 3eff4c6c92d0..e3511dbc517d 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -343,7 +343,7 @@ def _import_staged_block( block_type: str, olx_str: str, library_key: LibraryLocatorV2, - source_context_key: LearningContextKey, + source_context_key: LearningContextKey | None, user, staged_content_id: int, staged_content_files: list[StagedContentFileData], @@ -472,7 +472,7 @@ def _import_staged_block( def _import_staged_block_as_container( olx_str: str, library_key: LibraryLocatorV2, - source_context_key: LearningContextKey, + source_context_key: LearningContextKey | None, user, staged_content_id: int, staged_content_files: list[StagedContentFileData], @@ -538,7 +538,7 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use raise ValidationError("The user's clipboard is empty") staged_content_id = user_clipboard.content.id - source_context_key: LearningContextKey = user_clipboard.source_context_key + source_context_key: LearningContextKey | None = user_clipboard.source_context_key staged_content_files = content_staging_api.get_staged_content_static_files(staged_content_id) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 7f2be415ebd5..340e0bba1000 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -616,7 +616,7 @@ def copy_container(container_key: LibraryContainerLocator, user_id: int) -> User Copy a container (a Section, Subsection, or Unit) to the content staging. """ container_metadata = get_container(container_key) - container_olx = ContainerSerializer(container_metadata).olx_str + container_serializer = ContainerSerializer(container_metadata) block_type = ContainerType(container_key.container_type).olx_tag from openedx.core.djangoapps.content_staging import api as content_staging_api @@ -624,10 +624,10 @@ def copy_container(container_key: LibraryContainerLocator, user_id: int) -> User return content_staging_api.save_content_to_user_clipboard( user_id=user_id, block_type=block_type, - olx=container_olx, + olx=container_serializer.olx_str, display_name=container_metadata.display_name, suggested_url_name=str(container_key), - source_usage_key=str(container_key), - tags=None, # WIP: Handle tags + tags=container_serializer.tags, version_num=container_metadata.published_version_num, + static_files=container_serializer.static_files, ) diff --git a/openedx/core/djangoapps/content_libraries/api/serializers.py b/openedx/core/djangoapps/content_libraries/api/serializers.py index d44d66e8e8b5..54c77e3a8c89 100644 --- a/openedx/core/djangoapps/content_libraries/api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/api/serializers.py @@ -5,6 +5,7 @@ from openedx.core.djangoapps.xblock import api as xblock_api from openedx.core.lib.xblock_serializer.api import StaticFile, XBlockSerializer +from openedx.core.djangoapps.content_tagging.api import TagValuesByObjectIdDict, get_all_object_tags from . import containers as container_api @@ -14,10 +15,12 @@ class ContainerSerializer: Serializes a container (a Section, Subsection, or Unit) to OLX. """ static_files: list[StaticFile] + tags: TagValuesByObjectIdDict def __init__(self, container_metadata: container_api.ContainerMetadata): self.container_metadata = container_metadata self.static_files = [] + self.tags = {} olx_node = self._serialize_container(container_metadata) self.olx_str = etree.tostring(olx_node, encoding="unicode", pretty_print=True) @@ -28,13 +31,16 @@ def _serialize_container(self, container_metadata: container_api.ContainerMetada """ # Create an XML node to hold the exported data container_type = container_api.ContainerType(container_metadata.container_key.container_type) + container_key = container_metadata.container_key olx = etree.Element(container_type.olx_tag) - olx.attrib["upstream"] = str(container_metadata.container_key) + olx.attrib["upstream"] = str(container_key) olx.attrib["upstream_version"] = str(container_metadata.draft_version_num) # Serialize the container's metadata olx.attrib["display_name"] = container_metadata.display_name + container_tags, _ = get_all_object_tags(content_key=container_key) + self.tags.update(container_tags) children = container_api.get_container_children(container_metadata.container_key) for child in children: @@ -50,9 +56,9 @@ def _serialize_container(self, container_metadata: container_api.ContainerMetada xblock_serializer = XBlockSerializer( xblock, fetch_asset_data=True, - add_upstream=True, ) olx.append(xblock_serializer.olx_node) self.static_files.extend(xblock_serializer.static_files) + self.tags.update(xblock_serializer.tags) return olx diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 653b4c686c55..5c291deb5036 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -13,6 +13,7 @@ from opaque_keys.edx.keys import AssetKey, UsageKey from xblock.core import XBlock +from openedx.core.djangoapps.content_tagging.api import TagValuesByObjectIdDict from openedx.core.lib.xblock_serializer.api import StaticFile, XBlockSerializer from xmodule import block_metadata_utils from xmodule.contentstore.content import StaticContent @@ -61,46 +62,22 @@ def _save_xblock_to_staged_content( ) usage_key = block.usage_key - expired_ids = None - with transaction.atomic(): - if purpose == CLIPBOARD_PURPOSE: - expired_ids = _get_expired_staged_content_ids(user_id) - - # Insert a new StagedContent row for this - staged_content = _StagedContent.objects.create( - user_id=user_id, - purpose=purpose, - status=StagedContentStatus.READY, - block_type=usage_key.block_type, - olx=block_data.olx_str, - display_name=block_metadata_utils.display_name_with_default(block), - suggested_url_name=usage_key.block_id, - tags=block_data.tags or {}, - version_num=(version_num or 0), - ) - - # Log an event so we can analyze how this feature is used: - log.info(f'Saved {usage_key.block_type} component "{usage_key}" to staged content for {purpose}.') - - # Try to copy the static files. If this fails, we still consider the overall save attempt to have succeeded, - # because intra-course operations will still work fine, and users can manually resolve file issues. - try: - _save_static_assets_to_staged_content(block_data.static_files, usage_key, staged_content) - except Exception: # pylint: disable=broad-except - log.exception(f"Unable to copy static files to staged content for component {usage_key}") - - if expired_ids: - # Enqueue a (potentially slow) task to delete the old staged content - try: - # WIP: Won't this already be deleted by _get_expired_staged_content_ids? - delete_expired_clipboards.delay(expired_ids) - except Exception: # pylint: disable=broad-except - log.exception(f"Unable to enqueue cleanup task for StagedContents: {','.join(str(x) for x in expired_ids)}") + staged_content = _save_data_to_staged_content( + user_id=user_id, + purpose=purpose, + block_type=usage_key.block_type, + olx=block_data.olx_str, + display_name=block_metadata_utils.display_name_with_default(block), + suggested_url_name=usage_key.block_id, + tags=block_data.tags or {}, + version_num=(version_num or 0), + usage_key=usage_key, + static_files=block_data.static_files, + ) return staged_content -# WIP: Merge this with _save_xblock_to_staged_content? def _save_data_to_staged_content( user_id: int, purpose: str, @@ -108,8 +85,10 @@ def _save_data_to_staged_content( olx: str, display_name: str, suggested_url_name: str, - tags: dict[str, str] | None = None, + tags: TagValuesByObjectIdDict, version_num: int | None = None, + usage_key: UsageKey | None = None, + static_files: list[StaticFile] | None = None, ) -> _StagedContent: """ Save arbitrary OLX data to staged content. @@ -135,8 +114,13 @@ def _save_data_to_staged_content( version_num=(version_num or 0), ) - # Log an event so we can analyze how this feature is used: - log.info(f'Saved {block_type} content to staged content for {purpose}.') + if static_files: + # Try to copy the static files. If this fails, we still consider the overall save attempt to have succeeded, + # because intra-course operations will still work fine, and users can manually resolve file issues. + try: + _save_static_assets_to_staged_content(static_files, usage_key, staged_content) + except Exception: # pylint: disable=broad-except + log.exception(f"Unable to copy static files to staged content for component {usage_key}") if expired_ids: # Enqueue a (potentially slow) task to delete the old staged content @@ -149,7 +133,7 @@ def _save_data_to_staged_content( def _save_static_assets_to_staged_content( - static_files: list[StaticFile], usage_key: UsageKey, staged_content: _StagedContent + static_files: list[StaticFile], usage_key: UsageKey | None, staged_content: _StagedContent ): """ Helper method for saving static files into staged content. @@ -157,7 +141,7 @@ def _save_static_assets_to_staged_content( """ for f in static_files: source_key = ( - StaticContent.get_asset_key_from_path(usage_key.context_key, f.url) + StaticContent.get_asset_key_from_path(usage_key.context_key if usage_key else "", f.url) if (f.url and f.url.startswith('/')) else None ) # Compute the MD5 hash and get the content: @@ -216,16 +200,15 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int return _user_clipboard_model_to_data(clipboard) -# WIP: Merge this with save_xblock_to_user_clipboard? def save_content_to_user_clipboard( user_id: int, block_type: str, olx: str, display_name: str, suggested_url_name: str, - source_usage_key: str, - tags: dict[str, str] | None = None, + tags: TagValuesByObjectIdDict, version_num: int | None = None, + static_files: list[StaticFile] | None = None, ) -> UserClipboardData: """ Copy arbitrary OLX data to the user's clipboard. @@ -239,6 +222,7 @@ def save_content_to_user_clipboard( suggested_url_name=suggested_url_name, tags=tags, version_num=version_num, + static_files=static_files, ) # Create/update the clipboard entry @@ -246,7 +230,7 @@ def save_content_to_user_clipboard( user_id=user_id, defaults={ "content": staged_content, - "source_usage_key": "lb:orgA:lib1:unit:fake", # WIP: This shouldn't be a usage key + "source_usage_key": None, }, ) diff --git a/openedx/core/djangoapps/content_staging/data.py b/openedx/core/djangoapps/content_staging/data.py index d095f2506b17..20a8292d77a8 100644 --- a/openedx/core/djangoapps/content_staging/data.py +++ b/openedx/core/djangoapps/content_staging/data.py @@ -69,10 +69,12 @@ class StagedContentFileData: class UserClipboardData: """ Read-only data model for User Clipboard data (copied OLX) """ content: StagedContentData = field(validator=validators.instance_of(StagedContentData)) - source_usage_key: UsageKey = field(validator=validators.instance_of(UsageKey)) # type: ignore[type-abstract] + source_usage_key: UsageKey | None = field( + validator=validators.optional(validators.instance_of(UsageKey)) # type: ignore[type-abstract] + ) source_context_title: str @property - def source_context_key(self) -> LearningContextKey: + def source_context_key(self) -> LearningContextKey | None: """ Get the context (course/library) that this was copied from """ - return self.source_usage_key.context_key + return self.source_usage_key.context_key if self.source_usage_key else None diff --git a/openedx/core/djangoapps/content_staging/migrations/0006_alter_userclipboard_source_usage_key.py b/openedx/core/djangoapps/content_staging/migrations/0006_alter_userclipboard_source_usage_key.py new file mode 100644 index 000000000000..b0ce836de6b8 --- /dev/null +++ b/openedx/core/djangoapps/content_staging/migrations/0006_alter_userclipboard_source_usage_key.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.22 on 2025-07-24 19:45 + +from django.db import migrations +import opaque_keys.edx.django.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('content_staging', '0005_stagedcontent_version_num'), + ] + + operations = [ + migrations.AlterField( + model_name='userclipboard', + name='source_usage_key', + field=opaque_keys.edx.django.models.UsageKeyField(blank=True, help_text='Original usage key/ID of the thing that is in the clipboard.', max_length=255), + ), + ] diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 49100a2887ae..3f284e6f80b2 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -110,20 +110,20 @@ class UserClipboard(models.Model): # previously copied items are not kept. user = models.OneToOneField(User, on_delete=models.CASCADE, primary_key=True) content = models.ForeignKey(StagedContent, on_delete=models.CASCADE) - # WIP: We need to change this type to support ContainerKeys source_usage_key = UsageKeyField( max_length=255, help_text=_("Original usage key/ID of the thing that is in the clipboard."), + blank=True, ) @property - def source_context_key(self) -> LearningContextKey: + def source_context_key(self) -> LearningContextKey | None: """ Get the context (course/library) that this was copied from """ - return self.source_usage_key.context_key + return self.source_usage_key.context_key if self.source_usage_key else None def get_source_context_title(self) -> str: """ Get the title of the source context, if any """ - if self.source_context_key.is_course: + if self.source_context_key and self.source_context_key.is_course: course_overview = get_course_overview_or_none(self.source_context_key) if course_overview: return course_overview.display_name_with_default diff --git a/openedx/core/djangoapps/content_staging/serializers.py b/openedx/core/djangoapps/content_staging/serializers.py index 753e040972c1..27e87143fef7 100644 --- a/openedx/core/djangoapps/content_staging/serializers.py +++ b/openedx/core/djangoapps/content_staging/serializers.py @@ -57,6 +57,8 @@ def get_source_edit_url(self, obj) -> str: user = request.user if request else None if not user: return "" + if not obj.source_usage_key: + return "" if not obj.source_usage_key.context_key.is_course: return "" # Linking back to libraries is not implemented yet if not has_studio_read_access(user, obj.source_usage_key.course_key): diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index 16c31a6f5eab..9b8124f4a508 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -24,13 +24,12 @@ class XBlockSerializer: olx_node: etree.Element olx_str: str - def __init__(self, block, write_url_name=True, fetch_asset_data=False, add_upstream=False): + def __init__(self, block, write_url_name=True, fetch_asset_data=False): """ Serialize an XBlock to an OLX string + supporting files, and store the resulting data in this object. """ self.write_url_name = write_url_name - self.add_upstream = add_upstream self.orig_block_key = block.scope_ids.usage_id self.static_files = [] @@ -149,9 +148,6 @@ def _serialize_html_block(self, block) -> etree.Element: """ olx_node = etree.Element("html") olx_node.attrib["url_name"] = block.scope_ids.usage_id.block_id - if self.add_upstream: - olx_node.attrib["upstream"] = str(block.scope_ids.usage_id) - olx_node.attrib["upstream_version"] = str(block.scope_ids.version) if block.display_name: olx_node.attrib["display_name"] = block.display_name if block.fields["editor"].is_set_on(block): From 7e78a72d68500aebcc1d55d95d69a388ed7d256d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 28 Jul 2025 15:08:43 -0300 Subject: [PATCH 03/18] test: add tests --- .../content_libraries/tests/base.py | 5 ++ .../tests/test_containers.py | 73 +++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 0036c208b0c7..f3ae4749e523 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -42,6 +42,7 @@ URL_LIB_CONTAINER_RESTORE = URL_LIB_CONTAINER + 'restore/' # Restore a deleted container URL_LIB_CONTAINER_COLLECTIONS = URL_LIB_CONTAINER + 'collections/' # Handle associated collections URL_LIB_CONTAINER_PUBLISH = URL_LIB_CONTAINER + 'publish/' # Publish changes to the specified container + children +URL_LIB_CONTAINER_COPY = URL_LIB_CONTAINER + 'copy/' # Copy the specified container to the clipboard URL_LIB_COLLECTION = URL_LIB_COLLECTIONS + '{collection_key}/' # Get a collection in this library URL_LIB_COLLECTION_ITEMS = URL_LIB_COLLECTION + 'items/' # Get a collection in this library @@ -465,6 +466,10 @@ def _publish_container(self, container_key: ContainerKey | str, expect_response= """ Publish all changes in the specified container + children """ return self._api('post', URL_LIB_CONTAINER_PUBLISH.format(container_key=container_key), None, expect_response) + def _copy_container(self, container_key: ContainerKey | str, expect_response=200): + """ Copy the specified container to the clipboard """ + return self._api('post', URL_LIB_CONTAINER_COPY.format(container_key=container_key), None, expect_response) + def _create_collection( self, lib_key: LibraryLocatorV2 | str, diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 90b0c717946c..493d0c487334 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -2,6 +2,7 @@ Tests for Learning-Core-based Content Libraries """ from datetime import datetime, timezone +import textwrap import ddt from freezegun import freeze_time @@ -657,3 +658,75 @@ def test_publish_container(self) -> None: # pylint: disable=too-many-statements assert c2_components_after[1]["id"] == html_block_3["id"] assert c2_components_after[1]["has_unpublished_changes"] # unaffected assert c2_components_after[1]["published_by"] is None + + def test_copy_container(self) -> None: + """ + Test that we can copy a container and its children. + """ + tagging_api.tag_object( + self.section_with_subsections["id"], + self.taxonomy, + ['one', 'three', 'four'], + ) + tagging_api.tag_object( + self.subsection_with_units["id"], + self.taxonomy, + ['one', 'two'], + ) + tagging_api.tag_object( + self.unit_with_components["id"], + self.taxonomy, + ['one'], + ) + self._copy_container(self.section_with_subsections["id"]) + + from openedx.core.djangoapps.content_staging import api as staging_api + + clipboard_data = staging_api.get_user_clipboard(self.user.id) + + assert clipboard_data is not None + assert clipboard_data.content.display_name == "Section with subsections" + assert clipboard_data.content.status == "ready" + assert clipboard_data.content.purpose == "clipboard" + assert clipboard_data.content.block_type == "chapter" + # We don't set source_usage_key for containers, as it should be a UsageKey, and containers don't have one. + assert clipboard_data.source_usage_key is None + + # Check the tags on the clipboard content: + assert clipboard_data.content.tags == { + 'lb:CL-TEST:containers:html:Html1': {}, + 'lb:CL-TEST:containers:html:Html2': {}, + 'lb:CL-TEST:containers:problem:Problem1': {}, + 'lb:CL-TEST:containers:problem:Problem2': {}, + self.section_with_subsections["id"]: { + str(self.taxonomy.id): ['one', 'three', 'four'], + }, + self.subsection_with_units["id"]: { + str(self.taxonomy.id): ['one', 'two'], + }, + self.unit_with_components["id"]: { + str(self.taxonomy.id): ['one'], + }, + } + + # Test the actual OLX in the clipboard: + olx_data = staging_api.get_staged_content_olx(clipboard_data.content.id) + assert olx_data is not None + assert olx_data == textwrap.dedent(f"""\ + + + + + + + + + + + + + + + + + """) From 1ae434e0176e2b052193669308c1862e4ed15eed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 28 Jul 2025 17:15:13 -0300 Subject: [PATCH 04/18] refactor: remove unecessary changes to reduce diff --- .../content_libraries/rest_api/containers.py | 3 +- .../core/djangoapps/content_staging/api.py | 54 +++++++++---------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index ac79133d5137..9ed113905ab2 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -9,7 +9,8 @@ from django.db.transaction import non_atomic_requests from django.utils.decorators import method_decorator from drf_yasg.utils import swagger_auto_schema -from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2 + +from opaque_keys.edx.locator import LibraryLocatorV2, LibraryContainerLocator from openedx_learning.api import authoring as authoring_api from rest_framework.generics import GenericAPIView from rest_framework.response import Response diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 5c291deb5036..ac361d3ba879 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -19,36 +19,26 @@ from xmodule.contentstore.content import StaticContent from xmodule.contentstore.django import contentstore -from .data import CLIPBOARD_PURPOSE, StagedContentData, StagedContentFileData, StagedContentStatus, UserClipboardData -from .models import StagedContent as _StagedContent -from .models import StagedContentFile as _StagedContentFile -from .models import UserClipboard as _UserClipboard -from .serializers import UserClipboardSerializer as _UserClipboardSerializer +from .data import ( + CLIPBOARD_PURPOSE, + StagedContentData, + StagedContentFileData, + StagedContentStatus, + UserClipboardData, +) +from .models import ( + UserClipboard as _UserClipboard, + StagedContent as _StagedContent, + StagedContentFile as _StagedContentFile, +) +from .serializers import ( + UserClipboardSerializer as _UserClipboardSerializer, +) from .tasks import delete_expired_clipboards log = logging.getLogger(__name__) -def _get_expired_staged_content_ids(user_id: int) -> list[int]: - """ - Mark all of the user's existing StagedContent rows as EXPIRED. - This is used when the user clears their clipboard. - """ - expired_ids: list[int] = [] - to_expire = _StagedContent.objects.filter( - user_id=user_id, - purpose=CLIPBOARD_PURPOSE, - ).exclude( - status=StagedContentStatus.EXPIRED, - ) # WIP: Should we exclude or only return the ids? - for sc in to_expire: - expired_ids.append(sc.id) - sc.status = StagedContentStatus.EXPIRED - sc.save() - - return expired_ids - - def _save_xblock_to_staged_content( block: XBlock, user_id: int, purpose: str, version_num: int | None = None ) -> _StagedContent: @@ -96,10 +86,20 @@ def _save_data_to_staged_content( that is not associated with any XBlock. """ - expired_ids = None + expired_ids = [] with transaction.atomic(): if purpose == CLIPBOARD_PURPOSE: - expired_ids = _get_expired_staged_content_ids(user_id) + # Mark all of the user's existing StagedContent rows as EXPIRED + to_expire = _StagedContent.objects.filter( + user_id=user_id, + purpose=purpose, + ).exclude( + status=StagedContentStatus.EXPIRED, + ) # WIP: Should we exclude or only return the ids? + for sc in to_expire: + expired_ids.append(sc.id) + sc.status = StagedContentStatus.EXPIRED + sc.save() # Insert a new StagedContent row for this staged_content = _StagedContent.objects.create( From f6e46b3ceabe825732d17ad0fb3be7efb3d68a5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Jul 2025 18:50:29 -0300 Subject: [PATCH 05/18] fix: change assert --- .../djangoapps/content_libraries/api/container_metadata.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py index 46eb0d62b8ac..841a51b625ff 100644 --- a/openedx/core/djangoapps/content_libraries/api/container_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -135,8 +135,10 @@ def library_container_locator( container_type = ContainerType.Subsection elif hasattr(container, 'section'): container_type = ContainerType.Section - - assert container_type is not None + else: + # This should never happen, but we assert to ensure that we handle all cases. + # If this fails, it means that a new Container type was added without updating this code. + raise ValueError(f"Unexpected container type: {container!r}") return LibraryContainerLocator( library_key, From ca9ab981bc04c76f5d2a72fb6dca4544c8875f75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Jul 2025 19:06:53 -0300 Subject: [PATCH 06/18] feat: add `write_upstream` field to ContainerSerializer --- .../core/djangoapps/content_libraries/api/serializers.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/serializers.py b/openedx/core/djangoapps/content_libraries/api/serializers.py index 54c77e3a8c89..7b258ef28b22 100644 --- a/openedx/core/djangoapps/content_libraries/api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/api/serializers.py @@ -17,8 +17,9 @@ class ContainerSerializer: static_files: list[StaticFile] tags: TagValuesByObjectIdDict - def __init__(self, container_metadata: container_api.ContainerMetadata): + def __init__(self, container_metadata: container_api.ContainerMetadata, write_upstream: bool = True): self.container_metadata = container_metadata + self.write_upstream = write_upstream self.static_files = [] self.tags = {} olx_node = self._serialize_container(container_metadata) @@ -34,8 +35,10 @@ def _serialize_container(self, container_metadata: container_api.ContainerMetada container_key = container_metadata.container_key olx = etree.Element(container_type.olx_tag) - olx.attrib["upstream"] = str(container_key) - olx.attrib["upstream_version"] = str(container_metadata.draft_version_num) + + if self.write_upstream: + olx.attrib["upstream"] = str(container_key) + olx.attrib["upstream_version"] = str(container_metadata.draft_version_num) # Serialize the container's metadata olx.attrib["display_name"] = container_metadata.display_name From 8482ddb5db6c0af0c13dadc2b8fd73e1596448cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 5 Aug 2025 11:52:26 -0300 Subject: [PATCH 07/18] fix: remove comment --- openedx/core/djangoapps/content_staging/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index ac361d3ba879..5ccdb08426a4 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -95,7 +95,7 @@ def _save_data_to_staged_content( purpose=purpose, ).exclude( status=StagedContentStatus.EXPIRED, - ) # WIP: Should we exclude or only return the ids? + ) for sc in to_expire: expired_ids.append(sc.id) sc.status = StagedContentStatus.EXPIRED From 857a7da45ab2be091ebb652f90bf5e78726a9cdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 5 Aug 2025 13:29:57 -0300 Subject: [PATCH 08/18] refactor: change `source_usage_key` type and more --- cms/djangoapps/contentstore/helpers.py | 55 ++++---------- .../content_libraries/api/blocks.py | 6 +- .../content_libraries/api/containers.py | 1 + .../content_libraries/api/serializers.py | 8 +-- .../tests/test_containers.py | 29 ++++---- .../content_libraries/tests/test_runtime.py | 4 +- .../core/djangoapps/content_staging/api.py | 16 +++-- .../core/djangoapps/content_staging/data.py | 14 ++-- ...06_alter_userclipboard_source_usage_key.py | 12 ++-- .../core/djangoapps/content_staging/models.py | 34 ++++++--- .../djangoapps/content_staging/serializers.py | 2 - .../content_staging/tests/test_clipboard.py | 9 ++- .../core/djangoapps/content_tagging/api.py | 2 - openedx/core/lib/xblock_serializer/api.py | 2 +- .../lib/xblock_serializer/block_serializer.py | 16 +++-- .../core/lib/xblock_serializer/test_api.py | 71 +++++++++++++------ 16 files changed, 156 insertions(+), 125 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index d7820eb317c6..a1e4641dc03e 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -16,6 +16,7 @@ from django.utils.translation import gettext as _ from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import DefinitionLocator, LocalId +from openedx.core.djangoapps.content_tagging.types import TagValuesByObjectIdDict from xblock.core import XBlock from xblock.fields import ScopeIds from xblock.runtime import IdGenerator @@ -387,10 +388,10 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> parent_xblock, store, user=request.user, - slug_hint=user_clipboard.source_usage_key.block_id if user_clipboard.source_usage_key else None, - copied_from_block=( - str(user_clipboard.source_usage_key) if user_clipboard.source_usage_key else None - ), + slug_hint=user_clipboard.source_usage_key.block_id + if isinstance(user_clipboard.source_usage_key, UsageKey) + else None, + copied_from_block=str(user_clipboard.source_usage_key), copied_from_version_num=user_clipboard.content.version_num, tags=user_clipboard.content.tags, ) @@ -455,7 +456,7 @@ def _fetch_and_set_upstream_link( Fetch and set upstream link for the given xblock which is being pasted. This function handles following cases: * the xblock is copied from a v2 library; the library block is set as upstream. * the xblock is copied from a course; no upstream is set, only copied_from_block is set. - * the xblock is copied from a course where the source block was imported from a library; the original libary block + * the xblock is copied from a course where the source block was imported from a library; the original library block is set as upstream. """ # Try to link the pasted block (downstream) to the copied block (upstream). @@ -511,7 +512,7 @@ def _import_xml_node_to_parent( # Zero if not applicable (e.g., course block). copied_from_version_num: int = 0, # Content tags applied to the source XBlock(s) - tags: dict[str, str] | None = None, + tags: TagValuesByObjectIdDict | None = None, ) -> XBlock: """ Given an XML node representing a serialized XBlock (OLX), import it into modulestore 'store' as a child of the @@ -522,8 +523,8 @@ def _import_xml_node_to_parent( runtime = parent_xblock.runtime parent_key = parent_xblock.scope_ids.usage_id block_type = node.tag - upstream = node.attrib.get('upstream', None) - upstream_version = node.attrib.get('upstream_version', None) + source_key = node.attrib.get('source_key', None) + source_version = node.attrib.get('source_version', None) # Modulestore's IdGenerator here is SplitMongoIdManager which is assigned # by CachingDescriptorSystem Runtime and since we need our custom ImportIdGenerator @@ -581,10 +582,11 @@ def _import_xml_node_to_parent( if xblock_class.has_children and temp_xblock.children: raise NotImplementedError("We don't yet support pasting XBlocks with children") - if copied_from_block: + if source_key: + _fetch_and_set_upstream_link(source_key, source_version, temp_xblock, user) + elif copied_from_block: + # Use the copied_from_block field only if the source_key is not set. _fetch_and_set_upstream_link(copied_from_block, copied_from_version_num, temp_xblock, user) - elif upstream: - _fetch_and_set_upstream_link(upstream, upstream_version, temp_xblock, user) # Save the XBlock into modulestore. We need to save the block and its parent for this to work: new_xblock = store.update_item(temp_xblock, user.id, allow_not_found=True) new_xblock.parent = parent_key @@ -600,29 +602,23 @@ def _import_xml_node_to_parent( if not children_handled: for child_node in child_nodes: - child_copied_from = str( - _get_usage_key_from_node(child_node, copied_from_block) - ) if copied_from_block else None - _import_xml_node_to_parent( child_node, new_xblock, store, user=user, - copied_from_block=child_copied_from, tags=tags, ) # Copy content tags to the new xblock if new_xblock.upstream: - # If this block is synced from an upstream (e.g. library content), # copy the tags from the upstream as ready-only content_tagging_api.copy_tags_as_read_only( new_xblock.upstream, new_xblock.location, ) - elif copied_from_block and tags: - object_tags = tags.get(str(copied_from_block)) + elif tags and (source_key or copied_from_block): + object_tags = tags.get(source_key or copied_from_block) if object_tags: content_tagging_api.set_all_object_tags( content_key=new_xblock.location, @@ -815,27 +811,6 @@ def is_item_in_course_tree(item): return ancestor is not None -def _get_usage_key_from_node(node, parent_id: str) -> UsageKey | None: - """ - Returns the UsageKey for the given node and parent ID. - - If the parent_id is not a valid UsageKey, or there's no "url_name" attribute in the node, then will return None. - """ - parent_key = UsageKey.from_string(parent_id) - parent_context = parent_key.context_key - usage_key = None - block_id = node.attrib.get("url_name") - block_type = node.tag - - if parent_context and block_id and block_type: - usage_key = parent_context.make_usage_key( - block_type=block_type, - block_id=block_id, - ) - - return usage_key - - def concat_static_file_notices(notices: list[StaticFileNotices]) -> StaticFileNotices: """Combines multiple static file notices into a single object diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index e3511dbc517d..9980323447a1 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -343,7 +343,7 @@ def _import_staged_block( block_type: str, olx_str: str, library_key: LibraryLocatorV2, - source_context_key: LearningContextKey | None, + source_context_key: LearningContextKey, user, staged_content_id: int, staged_content_files: list[StagedContentFileData], @@ -472,7 +472,7 @@ def _import_staged_block( def _import_staged_block_as_container( olx_str: str, library_key: LibraryLocatorV2, - source_context_key: LearningContextKey | None, + source_context_key: LearningContextKey, user, staged_content_id: int, staged_content_files: list[StagedContentFileData], @@ -538,7 +538,7 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use raise ValidationError("The user's clipboard is empty") staged_content_id = user_clipboard.content.id - source_context_key: LearningContextKey | None = user_clipboard.source_context_key + source_context_key = user_clipboard.source_context_key staged_content_files = content_staging_api.get_staged_content_static_files(staged_content_id) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 340e0bba1000..788e9116b7e1 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -628,6 +628,7 @@ def copy_container(container_key: LibraryContainerLocator, user_id: int) -> User display_name=container_metadata.display_name, suggested_url_name=str(container_key), tags=container_serializer.tags, + copied_from=container_key, version_num=container_metadata.published_version_num, static_files=container_serializer.static_files, ) diff --git a/openedx/core/djangoapps/content_libraries/api/serializers.py b/openedx/core/djangoapps/content_libraries/api/serializers.py index 7b258ef28b22..7e7fcb4111c2 100644 --- a/openedx/core/djangoapps/content_libraries/api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/api/serializers.py @@ -17,9 +17,8 @@ class ContainerSerializer: static_files: list[StaticFile] tags: TagValuesByObjectIdDict - def __init__(self, container_metadata: container_api.ContainerMetadata, write_upstream: bool = True): + def __init__(self, container_metadata: container_api.ContainerMetadata): self.container_metadata = container_metadata - self.write_upstream = write_upstream self.static_files = [] self.tags = {} olx_node = self._serialize_container(container_metadata) @@ -36,9 +35,8 @@ def _serialize_container(self, container_metadata: container_api.ContainerMetada olx = etree.Element(container_type.olx_tag) - if self.write_upstream: - olx.attrib["upstream"] = str(container_key) - olx.attrib["upstream_version"] = str(container_metadata.draft_version_num) + olx.attrib["source_key"] = str(container_key) + olx.attrib["source_version"] = str(container_metadata.draft_version_num) # Serialize the container's metadata olx.attrib["display_name"] = container_metadata.display_name diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 493d0c487334..c90bb8f1e936 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -689,8 +689,7 @@ def test_copy_container(self) -> None: assert clipboard_data.content.status == "ready" assert clipboard_data.content.purpose == "clipboard" assert clipboard_data.content.block_type == "chapter" - # We don't set source_usage_key for containers, as it should be a UsageKey, and containers don't have one. - assert clipboard_data.source_usage_key is None + assert str(clipboard_data.source_usage_key) == self.section_with_subsections["id"] # Check the tags on the clipboard content: assert clipboard_data.content.tags == { @@ -713,20 +712,20 @@ def test_copy_container(self) -> None: olx_data = staging_api.get_staged_content_olx(clipboard_data.content.id) assert olx_data is not None assert olx_data == textwrap.dedent(f"""\ - - - - - - - - - + + + + + + + + + - - + + - - + + """) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py index 5fe5bb4eb1fc..a14b10ddf346 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py @@ -96,6 +96,7 @@ def test_html_round_trip(self): olx_1 = f'''\