Skip to content

Commit

Permalink
Support static assets when copy/pasting between courses and libraries (
Browse files Browse the repository at this point in the history
…#35668)

The biggest challenge is dealing with the mismatch between how Libraries store
assets (per-Component) and how Courses store assets (global Files and Uploads
space). To bridge this, we're going to kludge a component-local namespace in
Files and Uploads by making use of the obscure feature that you can create
folders there at an API level, even if no such UI exists.

In this commit:
* Assets work when copy-pasting between library components.
* Assets work when copy-pasting from a library to a course, with the convention
  being to put that file in a subdirectory of the form:
  components/{block_type}/{block_id}/file.
  Note that the Studio course Files page still just shows the filename.
* Assets work when copy-pasting from a course to a library.
  Top level assets are put into a static folder in the Component, per Learning
  Core conventions.

Limitations:
* Roundtrips don't work properly.
* There's no normalized form, so directories will start nesting if you copy
  from library and paste into course, then copy the pasted thing and paste back
  into library, etc. This was deemed acceptable for Sumac.

Low level stuff:
* XBlockSerializerForLearningCore has been removed, with the url_name stripping
  functionality added as an optional param to XBlockSerializer (the other stuff
  was for children and "vertical" -> "unit" conversion, neither of which are
  relevant now).
* url_name is now stripped out of anything added to the clipboard, so that we
  don't end up writing it in block.xml when it is redundant (and would be
  stripped out with the next write anyway).

For the Libraries Relaunch Beta. This should not affect any site which
has kept New Libraries disabled.

Issue: openedx/frontend-app-authoring#1170
  • Loading branch information
ormsbee authored Oct 23, 2024
1 parent 085b15a commit d25e651
Show file tree
Hide file tree
Showing 11 changed files with 311 additions and 162 deletions.
113 changes: 87 additions & 26 deletions cms/djangoapps/contentstore/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""
from __future__ import annotations
import logging
import pathlib
import urllib
from lxml import etree
from mimetypes import guess_type
Expand All @@ -11,7 +12,7 @@
from django.conf import settings
from django.contrib.auth import get_user_model
from django.utils.translation import gettext as _
from opaque_keys.edx.keys import AssetKey, CourseKey, UsageKey
from opaque_keys.edx.keys import CourseKey, UsageKey
from opaque_keys.edx.locator import DefinitionLocator, LocalId
from xblock.core import XBlock
from xblock.fields import ScopeIds
Expand Down Expand Up @@ -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):
Expand All @@ -297,12 +297,29 @@ 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,
)

# 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():
data_with_substitutions = data_with_substitutions.replace(
old_static_ref,
new_static_ref,
)
new_xblock.data = data_with_substitutions
store.update_item(new_xblock, request.user.id)

return new_xblock, notices


Expand Down Expand Up @@ -456,78 +473,122 @@ def _import_files_into_course(
course_key: CourseKey,
staged_content_id: int,
static_files: list[content_staging_api.StagedContentFileData],
) -> StaticFileNotices:
usage_key: UsageKey,
) -> tuple[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 = []
# List of files that conflicted with identically named files already in the destination 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):
# 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 = _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:
conflicting_files.append(file_data_obj.filename)
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,
) -> tuple[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)
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("/", "_"))

try:
current_file = contentstore().find(new_key)
except NotFoundError:
current_file = None
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):
Expand Down
58 changes: 58 additions & 0 deletions cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,64 @@ def test_paste_from_library_read_only_tags(self):
assert object_tag.value in self.lib_block_tags
assert object_tag.is_copied

def test_paste_from_library_copies_asset(self):
"""
Assets from a library component copied into a subdir of Files & Uploads.
"""
# This is the binary for a real, 1px webp file – we need actual image
# data because contentstore will try to make a thumbnail and grab
# metadata.
webp_raw_data = b'RIFF\x16\x00\x00\x00WEBPVP8L\n\x00\x00\x00/\x00\x00\x00\x00E\xff#\xfa\x1f'

# First add the asset.
library_api.add_library_block_static_asset_file(
self.lib_block_key,
"static/1px.webp",
webp_raw_data,
) # v==4

# Now add the reference to the asset
library_api.set_library_block_olx(self.lib_block_key, """
<problem display_name="MCQ-draft" max_attempts="5">
<p>Including this totally real image: <img src="/static/1px.webp" /></p>
<multiplechoiceresponse>
<label>Q</label>
<choicegroup type="MultipleChoice">
<choice correct="false">Wrong</choice>
<choice correct="true">Right</choice>
</choicegroup>
</multiplechoiceresponse>
</problem>
""") # v==5

copy_response = self.client.post(
CLIPBOARD_ENDPOINT,
{"usage_key": str(self.lib_block_key)},
format="json"
)
assert copy_response.status_code == 200

paste_response = self.client.post(XBLOCK_ENDPOINT, {
"parent_locator": str(self.course.usage_key),
"staged_content": "clipboard",
}, format="json")
assert paste_response.status_code == 200

new_block_key = UsageKey.from_string(paste_response.json()["locator"])
new_block = modulestore().get_item(new_block_key)

# Check that the substitution worked.
expected_import_path = f"components/{new_block_key.block_type}/{new_block_key.block_id}/1px.webp"
assert f"/static/{expected_import_path}" in new_block.data

# Check that the asset was copied over properly
image_asset = contentstore().find(
self.course.id.make_asset_key("asset", expected_import_path.replace('/', '_'))
)
assert image_asset.import_path == expected_import_path
assert image_asset.name == "1px.webp"
assert image_asset.length == len(webp_raw_data)


class ClipboardPasteFromV1LibraryTestCase(ModuleStoreTestCase):
"""
Expand Down
Loading

0 comments on commit d25e651

Please sign in to comment.