From c9553673d54a7e549e5b9251ee97ba33f5d9db03 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sun, 20 Oct 2024 00:29:12 -0400 Subject: [PATCH] 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 e9f599772d3e..c80b4399f3ed 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 @@ -277,7 +278,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): @@ -294,12 +294,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 @@ -444,7 +464,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 @@ -456,17 +477,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: @@ -474,25 +509,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: @@ -500,22 +552,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 5efaa28d670c..ad2139b7cbbf 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 d7dcb70d53b6..667280ceed4b 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: