From 8182fbf9d213fee5d7854434f8b1cfa2b67d1da2 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 17 Oct 2024 14:28:36 -0400 Subject: [PATCH 01/20] temp: dummy first commit to make the PR --- openedx/core/djangoapps/content_libraries/api.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index ede11dc5101a..5e4e88324a71 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -917,6 +917,8 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use Create a new library block and populate it with staged content from clipboard Returns the newly created library block + + TODO: Handle static assets. """ from openedx.core.djangoapps.content_staging import api as content_staging_api if not content_staging_api: From b405cc44be3fafd6f9ae756e2dd6be997b6a8ea0 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 18 Oct 2024 15:41:28 -0400 Subject: [PATCH 02/20] feat: asset support for course -> library copy+paste --- .../core/djangoapps/content_libraries/api.py | 89 ++++++++++++++++--- .../djangoapps/content_libraries/views.py | 2 +- .../xblock/runtime/learning_core_runtime.py | 9 +- 3 files changed, 86 insertions(+), 14 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 5e4e88324a71..7d175f3e30c8 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -761,9 +761,6 @@ def set_library_block_olx(usage_key, new_olx_str) -> int: # because this old pylint can't understand attr.ib() objects, pylint: disable=no-member assert isinstance(usage_key, LibraryUsageLocatorV2) - # Make sure the block exists: - _block_metadata = get_library_block(usage_key) - # Verify that the OLX parses, at least as generic XML, and the root tag is correct: node = etree.fromstring(new_olx_str) if node.tag != usage_key.block_type: @@ -809,7 +806,7 @@ def set_library_block_olx(usage_key, new_olx_str) -> int: ) ) - return new_component_version.version_num + return new_component_version def library_component_usage_key( @@ -917,8 +914,6 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use Create a new library block and populate it with staged content from clipboard Returns the newly created library block - - TODO: Handle static assets. """ from openedx.core.djangoapps.content_staging import api as content_staging_api if not content_staging_api: @@ -928,9 +923,9 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use if not user_clipboard: return None - olx_str = content_staging_api.get_staged_content_olx(user_clipboard.content.id) - - # TODO: Handle importing over static assets + staged_content_id = user_clipboard.content.id + olx_str = content_staging_api.get_staged_content_olx(staged_content_id) + staged_content_files = content_staging_api.get_staged_content_static_files(staged_content_id) content_library, usage_key = validate_can_add_block_to_library( library_key, @@ -938,9 +933,77 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use block_id ) + now = datetime.now(tz=timezone.utc) + # Create component for block then populate it with clipboard data - _create_component_for_block(content_library, usage_key, user.id) - set_library_block_olx(usage_key, olx_str) + with transaction.atomic(): + # First create the Component, but do not initialize it to anything (i.e. + # no ComponentVersion). + component_type = authoring_api.get_or_create_component_type( + "xblock.v1", usage_key.block_type + ) + component = authoring_api.create_component( + content_library.learning_package.id, + component_type=component_type, + local_key=usage_key.block_id, + created=now, + created_by=user.id, + ) + + # This will create the first component version and set the OLX/title + # appropriately. It will not publish. Once we get the newly created + # ComponentVersion back from this, we can attach all our files to it. + component_version = set_library_block_olx(usage_key, olx_str) + + for staged_content_file_data in staged_content_files: + # The ``data`` attribute is going to be None because the clipboard + # is optimized to not do redundant file copying when copying/pasting + # within the same course (where all the Files and Uploads are + # shared). Learning Core backed content Components will always store + # a Component-local "copy" of the data, and rely on lower-level + # deduplication to happen in the ``contents`` app. + filename = staged_content_file_data.filename + + # Grab our byte data for the file... + file_data = content_staging_api.get_staged_content_static_file_data( + staged_content_id, + filename, + ) + + # Courses don't support having assets that are local to a specific + # component, and instead store all their content together in a + # shared Files and Uploads namespace. If we're pasting that into a + # Learning Core backed data model (v2 Libraries), then we want to + # prepend "static/" to the filename. This will need to get updated + # when we start moving courses over to Learning Core, or if we start + # storing course component assets in sub-directories of Files and + # Uploads. + # + # The reason we don't just search for a "static/" prefix is that + # Learning Core components can store other kinds of files if they + # wish (though none currently do). + source_assumes_global_assets = not isinstance( + user_clipboard.source_context_key, LibraryLocatorV2 + ) + if source_assumes_global_assets: + filename = f"static/{filename}" + + # Now construct the Learning Core data models for it... + media_type_str, _encoding = mimetypes.guess_type(filename) + media_type = authoring_api.get_or_create_media_type(media_type_str) + content = authoring_api.get_or_create_file_content( + content_library.learning_package_id, + media_type.id, + data=file_data, + created=now, + ) + authoring_api.create_component_version_content( + component_version.pk, + content.id, + key=filename, + learner_downloadable=True, + ) + # Emit library block created event LIBRARY_BLOCK_CREATED.send_event( @@ -968,7 +1031,7 @@ def get_or_create_olx_media_type(block_type: str) -> MediaType: def _create_component_for_block(content_lib, usage_key, user_id=None): """ - Create a Component for an XBlock type, and initialize it. + Create a Component for an XBlock type, initialize it, and return the ComponentVersion. This will create a Component, along with its first ComponentVersion. The tag in the OLX will have no attributes, e.g. ``. This first version @@ -1012,6 +1075,8 @@ def _create_component_for_block(content_lib, usage_key, user_id=None): learner_downloadable=False ) + return component_version + def delete_library_block(usage_key, remove_from_parent=True): """ diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 50b532f25bc2..9357d54ca7ce 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -737,7 +737,7 @@ def post(self, request, usage_key_str): serializer.is_valid(raise_exception=True) new_olx_str = serializer.validated_data["olx"] try: - version_num = api.set_library_block_olx(key, new_olx_str) + version_num = api.set_library_block_olx(key, new_olx_str).version_num except ValueError as err: raise ValidationError(detail=str(err)) # lint-amnesty, pylint: disable=raise-missing-from return Response(LibraryXBlockOlxSerializer({"olx": new_olx_str, "version_num": version_num}).data) diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index 1d1df738ab37..b530bce0ec13 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -248,14 +248,21 @@ def get_block_assets(self, block): component_version .componentversioncontent_set .filter(content__has_file=True) + .select_related('content') .order_by('key') ) + # TODO: We're returning both the URL and the data here because this is + # invoked from the XBlockSerializer for both saving a new version of a + # block, as well as for saving into the clipboard. The clipboard needs + # the actual static asset data to be dumped in here, but saving a new + # block version doesn't. We should clean this up later so we're not + # doing the unnecessary read() calls for saving new versions. return [ StaticFile( name=cvc.key, url=self._absolute_url_for_asset(component_version, cvc.key), - data=None, + data=cvc.content.read_file().read(), ) for cvc in cvc_list ] From 19267fad8008e4bd5dfa6e7aaecf7bd11b8dfa74 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sun, 20 Oct 2024 00:29:12 -0400 Subject: [PATCH 03/20] feat: add params to XBlockSerializer for clipboard use case --- cms/djangoapps/contentstore/helpers.py | 98 +++++++++++++++---- .../core/djangoapps/content_staging/api.py | 9 +- .../xblock/runtime/learning_core_runtime.py | 17 ++-- openedx/core/lib/xblock_serializer/api.py | 3 + .../lib/xblock_serializer/block_serializer.py | 18 +++- 5 files changed, 111 insertions(+), 34 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 5a4f3c652347..9875b3295cc7 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -3,6 +3,7 @@ """ from __future__ import annotations import logging +import pathlib import urllib from lxml import etree from mimetypes import guess_type @@ -280,7 +281,6 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> # Clipboard is empty or expired/error/loading return None, StaticFileNotices() olx_str = content_staging_api.get_staged_content_olx(user_clipboard.content.id) - static_files = content_staging_api.get_staged_content_static_files(user_clipboard.content.id) node = etree.fromstring(olx_str) store = modulestore() with store.bulk_operations(parent_key.course_key): @@ -297,12 +297,32 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> copied_from_version_num=user_clipboard.content.version_num, tags=user_clipboard.content.tags, ) - # Now handle static files that need to go into Files & Uploads: - notices = _import_files_into_course( - course_key=parent_key.context_key, - staged_content_id=user_clipboard.content.id, - static_files=static_files, - ) + + # Now handle static files that need to go into Files & Uploads. + static_files = content_staging_api.get_staged_content_static_files(user_clipboard.content.id) + notices, substitutions = _import_files_into_course( + course_key=parent_key.context_key, + staged_content_id=user_clipboard.content.id, + static_files=static_files, + usage_key=new_xblock.scope_ids.usage_id, + ) + + if hasattr(new_xblock, 'data') and substitutions: + data_with_substitutions = new_xblock.data + + print(f"Substitutions: {substitutions}") + + for old_static_ref, new_static_ref in substitutions.items(): + data_with_substitutions = data_with_substitutions.replace( + old_static_ref, + new_static_ref, + ) + + print(f"data_with_substitutions: {data_with_substitutions}") + + new_xblock.data = data_with_substitutions + store.update_item(new_xblock, request.user.id) + return new_xblock, notices @@ -456,7 +476,8 @@ def _import_files_into_course( course_key: CourseKey, staged_content_id: int, static_files: list[content_staging_api.StagedContentFileData], -) -> StaticFileNotices: + usage_key: UsageKey, +) -> (StaticFileNotices, dict[str, str]): """ For the given staged static asset files (which are in "Staged Content" such as the user's clipbaord, but which need to end up in the course's Files & Uploads page), import them into the destination course, unless they already @@ -468,17 +489,31 @@ def _import_files_into_course( conflicting_files = [] # List of files that had an error (shouldn't happen unless we have some kind of bug) error_files = [] + + # Store a mapping of asset URLs that need to be modified for the destination + # assets. This is necessary when you take something from a library and paste + # it into a course, because we need to translate Component-local static + # assets and shove them into the Course's global Files & Uploads space in a + # nested directory structure. + substitutions = {} for file_data_obj in static_files: - if not isinstance(file_data_obj.source_key, AssetKey): +# if not isinstance(file_data_obj.source_key, AssetKey): # This static asset was managed by the XBlock and instead of being added to "Files & Uploads", it is stored # using some other system. We could make it available via runtime.resources_fs during XML parsing, but it's # not needed here. - continue +# continue + # At this point, we know this is a "Files & Uploads" asset that we may need to copy into the course: try: - result = _import_file_into_course(course_key, staged_content_id, file_data_obj) + result, substitution_for_file = _import_file_into_course( + course_key, + staged_content_id, + file_data_obj, + usage_key, + ) if result is True: new_files.append(file_data_obj.filename) + substitutions.update(substitution_for_file) elif result is None: pass # This file already exists; no action needed. else: @@ -486,25 +521,42 @@ def _import_files_into_course( except Exception: # lint-amnesty, pylint: disable=broad-except error_files.append(file_data_obj.filename) log.exception(f"Failed to import Files & Uploads file {file_data_obj.filename}") - return StaticFileNotices( + + notices = StaticFileNotices( new_files=new_files, conflicting_files=conflicting_files, error_files=error_files, ) + return notices, substitutions + def _import_file_into_course( course_key: CourseKey, staged_content_id: int, file_data_obj: content_staging_api.StagedContentFileData, -) -> bool | None: + usage_key: UsageKey, +) -> (bool | None, dict): """ Import a single staged static asset file into the course, unless it already exists. Returns True if it was imported, False if there's a conflict, or None if the file already existed (no action needed). """ - filename = file_data_obj.filename - new_key = course_key.make_asset_key("asset", filename) + # If this came from a library (need to adjust this condition later) + clipboard_file_path = file_data_obj.filename + if clipboard_file_path.startswith('static/'): + file_path = clipboard_file_path.lstrip('static/') + import_path = f"components/{usage_key.block_type}/{usage_key.block_id}/{file_path}" + filename = pathlib.Path(file_path).name + new_key = course_key.make_asset_key("asset", import_path.replace("/", "_")) + else: + file_path = clipboard_file_path + import_path = None + filename = pathlib.Path(file_path).name + new_key = course_key.make_asset_key("asset", file_path.replace("/", "_")) + + # Yeah, I'd prefer a different delimiter, but this is what we already do + # during file import. try: current_file = contentstore().find(new_key) except NotFoundError: @@ -512,22 +564,28 @@ def _import_file_into_course( if not current_file: # This static asset should be imported into the new course: content_type = guess_type(filename)[0] - data = content_staging_api.get_staged_content_static_file_data(staged_content_id, filename) + data = content_staging_api.get_staged_content_static_file_data(staged_content_id, clipboard_file_path) if data is None: raise NotFoundError(file_data_obj.source_key) - content = StaticContent(new_key, name=filename, content_type=content_type, data=data) + content = StaticContent( + new_key, + name=filename, + content_type=content_type, + data=data, + import_path=import_path + ) # If it's an image file, also generate the thumbnail: thumbnail_content, thumbnail_location = contentstore().generate_thumbnail(content) if thumbnail_content is not None: content.thumbnail_location = thumbnail_location contentstore().save(content) - return True + return True, {clipboard_file_path: f"static/{import_path}"} elif current_file.content_digest == file_data_obj.md5_hash: # The file already exists and matches exactly, so no action is needed - return None + return None, {} else: # There is a conflict with some other file that has the same name. - return False + return False, {} def is_item_in_course_tree(item): diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 3912cb51c396..5794902359d6 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -13,7 +13,7 @@ from opaque_keys.edx.keys import AssetKey, UsageKey from xblock.core import XBlock -from openedx.core.lib.xblock_serializer.api import serialize_xblock_to_olx, StaticFile +from openedx.core.lib.xblock_serializer.api import StaticFile, XBlockSerializer from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none from xmodule import block_metadata_utils from xmodule.contentstore.content import StaticContent @@ -38,7 +38,12 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int """ Copy an XBlock's OLX to the user's clipboard. """ - block_data = serialize_xblock_to_olx(block) + block_data = XBlockSerializer( + block, + write_url_name=False, + fetch_asset_data=True, + normalize_asset_refs=True, + ) usage_key = block.usage_key expired_ids = [] diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index b530bce0ec13..fd2e867a3a8f 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -233,10 +233,17 @@ def get_block(self, usage_key, for_parent=None, *, version: int | LatestVersion return block - def get_block_assets(self, block): + def get_block_assets(self, block, fetch_asset_data): """ Return a list of StaticFile entries. + If ``fetch_data`` is True, we will read the actual asset file data from + storage and return it as part of the ``StaticFiles``. This is expensive, + and not necessary for something like writing a new version of the OLX in + response to a "Save" in the editor. But it is necessary for something + like serializing to the clipboard, where we make full copies of the + assets. + TODO: When we want to copy a whole Section at a time, doing these lookups one by one is going to get slow. At some point we're going to want something to look up a bunch of blocks at once. @@ -252,17 +259,11 @@ def get_block_assets(self, block): .order_by('key') ) - # TODO: We're returning both the URL and the data here because this is - # invoked from the XBlockSerializer for both saving a new version of a - # block, as well as for saving into the clipboard. The clipboard needs - # the actual static asset data to be dumped in here, but saving a new - # block version doesn't. We should clean this up later so we're not - # doing the unnecessary read() calls for saving new versions. return [ StaticFile( name=cvc.key, url=self._absolute_url_for_asset(component_version, cvc.key), - data=cvc.content.read_file().read(), + data=cvc.content.read_file().read() if fetch_asset_data else None, ) for cvc in cvc_list ] diff --git a/openedx/core/lib/xblock_serializer/api.py b/openedx/core/lib/xblock_serializer/api.py index 8ac1cd5717c3..b23a45c13bb5 100644 --- a/openedx/core/lib/xblock_serializer/api.py +++ b/openedx/core/lib/xblock_serializer/api.py @@ -10,6 +10,9 @@ def serialize_xblock_to_olx(block): This class will serialize an XBlock, producing: (1) an XML string defining the XBlock and all of its children (inline) (2) a list of any static files required by the XBlock and their URL + + This calls XBlockSerializer with all default options. To actually tweak the + output, instantiate XBlockSerializer directly. """ return XBlockSerializer(block) diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index 4f94b1acb11b..56c7a9541006 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -22,15 +22,19 @@ class XBlockSerializer: static_files: list[StaticFile] tags: TagValuesByObjectIdDict - def __init__(self, block): + def __init__(self, block, write_url_name=True, fetch_asset_data=False, normalize_asset_refs=False): """ Serialize an XBlock to an OLX string + supporting files, and store the resulting data in this object. """ + self.normalize_asset_refs = normalize_asset_refs + self.write_url_name = write_url_name + self.orig_block_key = block.scope_ids.usage_id self.static_files = [] self.tags = {} olx_node = self._serialize_block(block) + self.olx_str = etree.tostring(olx_node, encoding="unicode", pretty_print=True) course_key = self.orig_block_key.course_key @@ -44,7 +48,7 @@ def __init__(self, block): # Learning Core backed content supports this, which currently means # v2 Content Libraries. self.static_files.extend( - block.runtime.get_block_assets(block) + block.runtime.get_block_assets(block, fetch_asset_data) ) else: # Otherwise, we have to scan the content to extract associated asset @@ -71,6 +75,12 @@ def _serialize_block(self, block) -> etree.Element: else: olx = self._serialize_normal_block(block) + # The url_name attribute can come either because it was already in the + # block's field data, or because this class adds it in the calls above. + # However it gets set though, we can remove it here: + if not self.write_url_name: + olx.attrib.pop("url_name", None) + # Store the block's tags block_key = block.scope_ids.usage_id block_id = str(block_key) @@ -161,12 +171,12 @@ class XBlockSerializerForLearningCore(XBlockSerializer): (3) a list of any static files required by the XBlock and their URL """ - def __init__(self, block): + def __init__(self, block, *args, **kwargs): """ Serialize an XBlock to an OLX string + supporting files, and store the resulting data in this object. """ - super().__init__(block) + super().__init__(block, *args, **kwargs) self.def_id = utils.learning_core_def_key_from_modulestore_usage_key(self.orig_block_key) def _serialize_block(self, block) -> etree.Element: From 475a9cd05b4ab7a472c884818ba6a61c2d001109 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sun, 20 Oct 2024 23:53:03 -0400 Subject: [PATCH 04/20] temp: almost add has_normalized_asset_refs to content_staging models --- openedx/core/djangoapps/content_staging/models.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 2eab7954e826..326922c175ef 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -69,6 +69,11 @@ class Meta: # Tags applied to the original source block(s) will be copied to the new block(s) on paste. tags = models.JSONField(null=True, help_text=_("Content tags applied to these blocks")) + # Static assets. + # + # File paths start with "static/", references + # has_normalized_asset_refs = models.BooleanField(default=False) + @property def olx_filename(self) -> str: """ Get a filename that can be used for the OLX content of this staged content """ From b991a1e88f1c6e62515e17d69086da5a719b0d4a Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sun, 20 Oct 2024 23:54:21 -0400 Subject: [PATCH 05/20] refactor: remove XBlockSerializerForLearningCore The XBlockSerializerForLearningCore class existed to account for the differences in how Blockstore and later Learning Core stored OLX. This involved doing things like: * removing the url_name attribute (because it's encoded elsewhere) * special case conversion from tags to * other handling over child definition identifiers Stripping the url_name was moved to the XBlockSerializer superclass because it was useful for copy-paste functionality. The other two are no longer relevant because Learning Core won't internally model a Unit as an XBlock, and the Learning Core XBlock Runtime doesn't support having children. --- openedx/core/lib/xblock_serializer/api.py | 4 +- .../lib/xblock_serializer/block_serializer.py | 74 ------------------- 2 files changed, 2 insertions(+), 76 deletions(-) diff --git a/openedx/core/lib/xblock_serializer/api.py b/openedx/core/lib/xblock_serializer/api.py index b23a45c13bb5..f9ded3dbc55b 100644 --- a/openedx/core/lib/xblock_serializer/api.py +++ b/openedx/core/lib/xblock_serializer/api.py @@ -2,7 +2,7 @@ Public python API for serializing XBlocks to OLX """ # pylint: disable=unused-import -from .block_serializer import StaticFile, XBlockSerializer, XBlockSerializerForLearningCore +from .block_serializer import StaticFile, XBlockSerializer def serialize_xblock_to_olx(block): @@ -32,4 +32,4 @@ def serialize_modulestore_block_for_learning_core(block): we have around how we should rewrite this (e.g. are we going to remove ?). """ - return XBlockSerializerForLearningCore(block) + return XBlockSerializer(block, write_url_name=False) diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index 56c7a9541006..fc9a43e0cf8c 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -158,77 +158,3 @@ def _serialize_html_block(self, block) -> etree.Element: escaped_block_data = block.data.replace("]]>", "]]>") olx_node.text = etree.CDATA(escaped_block_data) return olx_node - - -class XBlockSerializerForLearningCore(XBlockSerializer): - """ - This class will serialize an XBlock, producing: - (1) A new definition ID for use in Learning Core - (2) an XML string defining the XBlock and referencing the IDs of its - children using syntax (which doesn't actually - contain the OLX of its children, just refers to them, so you have to - separately serialize them.) - (3) a list of any static files required by the XBlock and their URL - """ - - def __init__(self, block, *args, **kwargs): - """ - Serialize an XBlock to an OLX string + supporting files, and store the - resulting data in this object. - """ - super().__init__(block, *args, **kwargs) - self.def_id = utils.learning_core_def_key_from_modulestore_usage_key(self.orig_block_key) - - def _serialize_block(self, block) -> etree.Element: - """ Serialize an XBlock to OLX/XML. """ - olx_node = super()._serialize_block(block) - # Apply some transformations to the OLX: - self._transform_olx(olx_node, usage_id=block.scope_ids.usage_id) - return olx_node - - def _serialize_children(self, block, parent_olx_node): - """ - Recursively serialize the children of XBlock 'block'. - Subclasses may override this. - """ - for child_id in block.children: - # In modulestore, the "definition key" is a MongoDB ObjectID - # kept in split's definitions table, which theoretically allows - # the same block to be used in many places (each with a unique - # usage key). However, that functionality is not exposed in - # Studio (other than via content libraries). So when we import - # into Learning Core, we assume that each usage is unique, don't - # generate a usage key, and create a new "definition key" from - # the original usage key. - # So modulestore usage key - # block-v1:A+B+C+type@html+block@introduction - # will become Learning Core definition key - # html+introduction - # - # If we needed the real definition key, we could get it via - # child = block.runtime.get_block(child_id) - # child_def_id = str(child.scope_ids.def_id) - # and then use - # - def_id = utils.learning_core_def_key_from_modulestore_usage_key(child_id) - parent_olx_node.append(parent_olx_node.makeelement("xblock-include", {"definition": def_id})) - - def _transform_olx(self, olx_node, usage_id): - """ - Apply transformations to the given OLX etree Node. - """ - # Remove 'url_name' - we store the definition key in the folder name - # that holds the OLX and the usage key elsewhere, so specifying it - # within the OLX file is redundant and can lead to issues if the file is - # copied and pasted elsewhere in the bundle with a new definition key. - olx_node.attrib.pop('url_name', None) - # Convert to the new tag/block - if olx_node.tag == 'vertical': - olx_node.tag = 'unit' - for key in olx_node.attrib.keys(): - if key not in ('display_name', 'url_name'): - log.warning( - ' tag attribute "%s" will be ignored after conversion to (in %s)', - key, - str(usage_id) - ) From 2ff8cd1f884c91a5d7867a65575cef4ebfb1f931 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 21 Oct 2024 12:32:48 -0400 Subject: [PATCH 06/20] temp: give up on a normalized form for static asset refs for now --- cms/djangoapps/contentstore/helpers.py | 3 --- openedx/core/djangoapps/content_staging/api.py | 1 - openedx/core/djangoapps/content_staging/models.py | 5 ----- openedx/core/lib/xblock_serializer/block_serializer.py | 3 +-- 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 9875b3295cc7..626560f3e456 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -309,9 +309,6 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> if hasattr(new_xblock, 'data') and substitutions: data_with_substitutions = new_xblock.data - - print(f"Substitutions: {substitutions}") - for old_static_ref, new_static_ref in substitutions.items(): data_with_substitutions = data_with_substitutions.replace( old_static_ref, diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 5794902359d6..84dd6c756b93 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -42,7 +42,6 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int block, write_url_name=False, fetch_asset_data=True, - normalize_asset_refs=True, ) usage_key = block.usage_key diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 326922c175ef..2eab7954e826 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -69,11 +69,6 @@ class Meta: # Tags applied to the original source block(s) will be copied to the new block(s) on paste. tags = models.JSONField(null=True, help_text=_("Content tags applied to these blocks")) - # Static assets. - # - # File paths start with "static/", references - # has_normalized_asset_refs = models.BooleanField(default=False) - @property def olx_filename(self) -> str: """ Get a filename that can be used for the OLX content of this staged content """ diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index fc9a43e0cf8c..53be26937d4a 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -22,12 +22,11 @@ class XBlockSerializer: static_files: list[StaticFile] tags: TagValuesByObjectIdDict - def __init__(self, block, write_url_name=True, fetch_asset_data=False, normalize_asset_refs=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.normalize_asset_refs = normalize_asset_refs self.write_url_name = write_url_name self.orig_block_key = block.scope_ids.usage_id From 7a9a8ff660a6d24e5a89f969d4ba71e4568ca63e Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 21 Oct 2024 12:44:51 -0400 Subject: [PATCH 07/20] temp: fixups --- cms/djangoapps/contentstore/helpers.py | 34 +++++++++++++++----------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 626560f3e456..378b1de80509 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -307,6 +307,9 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> usage_key=new_xblock.scope_ids.usage_id, ) + # 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. if hasattr(new_xblock, 'data') and substitutions: data_with_substitutions = new_xblock.data for old_static_ref, new_static_ref in substitutions.items(): @@ -314,9 +317,6 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> old_static_ref, new_static_ref, ) - - print(f"data_with_substitutions: {data_with_substitutions}") - new_xblock.data = data_with_substitutions store.update_item(new_xblock, request.user.id) @@ -476,9 +476,18 @@ def _import_files_into_course( usage_key: UsageKey, ) -> (StaticFileNotices, dict[str, str]): """ - For the given staged static asset files (which are in "Staged Content" such as the user's clipbaord, but which - need to end up in the course's Files & Uploads page), import them into the destination course, unless they already + For the given staged static asset files (which are in "Staged Content" such + as the user's clipbaord, but which need to end up in the course's Files & + Uploads page), import them into the destination course, unless they already exist. + + This function returns a tuple of StaticFileNotices (assets added, errors, + conflicts), and static asset path substitutions that should be made in the + OLX in order to paste this content into this course. The latter is for the + case in which we're brining content in from a v2 library, which stores + static assets locally to a Component and needs to go into a subdirectory + when pasting into a course to avoid overwriting commonly named things, e.g. + "figure1.png". """ # List of files that were newly added to the destination course new_files = [] @@ -494,12 +503,6 @@ def _import_files_into_course( # nested directory structure. substitutions = {} for file_data_obj in static_files: -# if not isinstance(file_data_obj.source_key, AssetKey): - # This static asset was managed by the XBlock and instead of being added to "Files & Uploads", it is stored - # using some other system. We could make it available via runtime.resources_fs during XML parsing, but it's - # not needed here. -# continue - # At this point, we know this is a "Files & Uploads" asset that we may need to copy into the course: try: result, substitution_for_file = _import_file_into_course( @@ -539,21 +542,24 @@ def _import_file_into_course( Returns True if it was imported, False if there's a conflict, or None if the file already existed (no action needed). """ - # If this came from a library (need to adjust this condition later) clipboard_file_path = file_data_obj.filename + + # We need to generate an AssetKey to add an asset to a course. The mapping + # of directories '/' -> '_' is a long-existing contentstore convention that + # we're not going to attempt to change. if clipboard_file_path.startswith('static/'): + # If it's in this form, it came from a library and assumes component-local assets file_path = clipboard_file_path.lstrip('static/') import_path = f"components/{usage_key.block_type}/{usage_key.block_id}/{file_path}" filename = pathlib.Path(file_path).name new_key = course_key.make_asset_key("asset", import_path.replace("/", "_")) else: + # Otherwise it came from a course... file_path = clipboard_file_path import_path = None filename = pathlib.Path(file_path).name new_key = course_key.make_asset_key("asset", file_path.replace("/", "_")) - # Yeah, I'd prefer a different delimiter, but this is what we already do - # during file import. try: current_file = contentstore().find(new_key) except NotFoundError: From 95960baee09f4d3cc3356fb637d747f4b2cecf51 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 22 Oct 2024 01:06:07 -0400 Subject: [PATCH 08/20] test: remove learning-core container export test --- .../core/lib/xblock_serializer/test_api.py | 27 ++----------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/openedx/core/lib/xblock_serializer/test_api.py b/openedx/core/lib/xblock_serializer/test_api.py index 8078595b0ea9..d82a51bd73fc 100644 --- a/openedx/core/lib/xblock_serializer/test_api.py +++ b/openedx/core/lib/xblock_serializer/test_api.py @@ -144,7 +144,8 @@ def test_html_with_static_asset_learning_core(self): serialized_learning_core = api.serialize_modulestore_block_for_learning_core(html_block) self.assertXmlEqual( serialized_learning_core.olx_str, - # For learning core, OLX should never contain "url_name" as that ID is specified by the filename: + # For learning core, OLX should never contain "url_name" as that ID + # is specified by the Component key: """ @@ -154,8 +155,6 @@ def test_html_with_static_asset_learning_core(self): self.assertIn("CDATA", serialized.olx_str) # Static files should be identical: self.assertEqual(serialized.static_files, serialized_learning_core.static_files) - # This is the only other difference - an extra field with the learning-core-specific definition ID: - self.assertEqual(serialized_learning_core.def_id, "html/just_img") def test_html_with_fields(self): """ Test an HTML Block with non-default fields like editor='raw' """ @@ -193,28 +192,6 @@ def test_export_sequential(self): self.assertXmlEqual(serialized.olx_str, EXPECTED_SEQUENTIAL_OLX) - def test_export_sequential_learning_core(self): - """ - Export a sequential from the toy course, formatted for learning core. - """ - sequential_id = self.course.id.make_usage_key('sequential', 'Toy_Videos') # see sample_courses.py - sequential = modulestore().get_item(sequential_id) - serialized = api.serialize_modulestore_block_for_learning_core(sequential) - - self.assertXmlEqual(serialized.olx_str, """ - - - - - - - - - - - - """) - def test_capa_python_lib(self): """ Test capa problem blocks with and without python_lib.zip """ course = CourseFactory.create(display_name='Python Testing course', run="PY") From 6e2d251582272675bf5ff2e388b8ef8c29397a2e Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 22 Oct 2024 12:24:52 -0400 Subject: [PATCH 09/20] temp: remove test of unit export (our runtime no longer supports it) --- .../djangoapps/olx_rest_api/test_views.py | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/openedx/core/djangoapps/olx_rest_api/test_views.py b/openedx/core/djangoapps/olx_rest_api/test_views.py index c91cb6ff3ce8..bde877a457ae 100644 --- a/openedx/core/djangoapps/olx_rest_api/test_views.py +++ b/openedx/core/djangoapps/olx_rest_api/test_views.py @@ -26,7 +26,7 @@ def setUpClass(cls): with cls.store.default_store(ModuleStoreEnum.Type.split): cls.course = ToyCourseFactory.create(modulestore=cls.store) assert str(cls.course.id).startswith("course-v1:"), "This test is for split mongo course exports only" - cls.unit_key = cls.course.id.make_usage_key('vertical', 'vertical_test') + cls.video_key = cls.course.id.make_usage_key('video', 'sample_video') def setUp(self): """ @@ -56,7 +56,7 @@ def test_no_permission(self): A regular user enrolled in the course (but not part of the authoring team) should not be able to use the API. """ - response = self.get_olx_response_for_block(self.unit_key) + response = self.get_olx_response_for_block(self.video_key) assert response.status_code == 403 assert response.json()['detail'] ==\ 'You must be a member of the course team in Studio to export OLX using this API.' @@ -67,24 +67,14 @@ def test_export(self): the course. """ CourseStaffRole(self.course.id).add_users(self.user) - - response = self.get_olx_response_for_block(self.unit_key) + response = self.get_olx_response_for_block(self.video_key) assert response.status_code == 200 - assert response.json()['root_block_id'] == str(self.unit_key) + assert response.json()['root_block_id'] == str(self.video_key) blocks = response.json()['blocks'] - # Check the OLX of the root block: - self.assertXmlEqual( - blocks[str(self.unit_key)]['olx'], - '\n' - ' \n' - ' \n' - ' \n' - ' \n' - '\n' - ) + # Check the OLX of a video self.assertXmlEqual( - blocks[str(self.course.id.make_usage_key('video', 'sample_video'))]['olx'], + blocks[str(self.video_key)]['olx'], '