-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FC-0009] Copy/Paste associated static assets #32346
Merged
bradenmacdonald
merged 11 commits into
openedx:master
from
open-craft:copy-asset-dependencies
Jun 27, 2023
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
dd4213c
refactor: improve typing of StaticFile named tuple
bradenmacdonald 7505855
feat: copy static asset files into the clipboard
bradenmacdonald 5f75a83
feat: save the source key in StagedContentFile
bradenmacdonald ee6e698
feat: paste static assets
bradenmacdonald ed91565
feat: show notification in studio about pasted assets
bradenmacdonald 5c2ad99
fix: HTML XBlocks would lose the editor="raw" setting when copy-pasted.
bradenmacdonald 63d152d
feat: copy python_lib.zip to the clipboard when it seems to be in use
bradenmacdonald 780383a
feat: add tests for copying static assets
bradenmacdonald bb30799
fix: address review comments
bradenmacdonald c7774fe
fix: address further review comments
bradenmacdonald 07700f1
docs: address review comment
bradenmacdonald File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,31 +1,33 @@ | ||||||||||||||||
""" | ||||||||||||||||
Helper methods for Studio views. | ||||||||||||||||
""" | ||||||||||||||||
|
||||||||||||||||
from __future__ import annotations | ||||||||||||||||
import logging | ||||||||||||||||
import urllib | ||||||||||||||||
from lxml import etree | ||||||||||||||||
from mimetypes import guess_type | ||||||||||||||||
|
||||||||||||||||
from attrs import frozen, Factory | ||||||||||||||||
from django.utils.translation import gettext as _ | ||||||||||||||||
from opaque_keys.edx.keys import UsageKey | ||||||||||||||||
from opaque_keys.edx.keys import AssetKey, CourseKey, UsageKey | ||||||||||||||||
from opaque_keys.edx.locator import DefinitionLocator, LocalId | ||||||||||||||||
from xblock.core import XBlock | ||||||||||||||||
from xblock.fields import ScopeIds | ||||||||||||||||
from xblock.runtime import IdGenerator | ||||||||||||||||
from xmodule.contentstore.content import StaticContent | ||||||||||||||||
from xmodule.contentstore.django import contentstore | ||||||||||||||||
from xmodule.exceptions import NotFoundError | ||||||||||||||||
from xmodule.modulestore.django import modulestore | ||||||||||||||||
|
||||||||||||||||
# from cms.djangoapps.contentstore.views.preview import _load_preview_block | ||||||||||||||||
from cms.djangoapps.models.settings.course_grading import CourseGradingModel | ||||||||||||||||
from common.djangoapps.student import auth | ||||||||||||||||
from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole | ||||||||||||||||
|
||||||||||||||||
try: | ||||||||||||||||
# Technically this is a django app plugin, so we should not error if it's not installed: | ||||||||||||||||
import openedx.core.djangoapps.content_staging.api as content_staging_api | ||||||||||||||||
except ImportError: | ||||||||||||||||
content_staging_api = None | ||||||||||||||||
import openedx.core.djangoapps.content_staging.api as content_staging_api | ||||||||||||||||
|
||||||||||||||||
from .utils import reverse_course_url, reverse_library_url, reverse_usage_url | ||||||||||||||||
|
||||||||||||||||
log = logging.getLogger(__name__) | ||||||||||||||||
|
||||||||||||||||
# Note: Grader types are used throughout the platform but most usages are simply in-line | ||||||||||||||||
# strings. In addition, new grader types can be defined on the fly anytime one is needed | ||||||||||||||||
# (because they're just strings). This dict is an attempt to constrain the sprawl in Studio. | ||||||||||||||||
|
@@ -210,13 +212,24 @@ def create_definition(self, block_type, slug=None) -> DefinitionLocator: | |||||||||||||||
return DefinitionLocator(block_type, LocalId(block_type)) | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def import_staged_content_from_user_clipboard(parent_key: UsageKey, request): | ||||||||||||||||
@frozen | ||||||||||||||||
class StaticFileNotices: | ||||||||||||||||
""" Information about what static files were updated (or not) when pasting content into another course """ | ||||||||||||||||
new_files: list[str] = Factory(list) | ||||||||||||||||
conflicting_files: list[str] = Factory(list) | ||||||||||||||||
error_files: list[str] = Factory(list) | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> tuple[XBlock | None, StaticFileNotices]: | ||||||||||||||||
""" | ||||||||||||||||
Import a block (and any children it has) from "staged" OLX. | ||||||||||||||||
Import a block (along with its children and any required static assets) from | ||||||||||||||||
the "staged" OLX in the user's clipboard. | ||||||||||||||||
|
||||||||||||||||
Does not deal with permissions or REST stuff - do that before calling this. | ||||||||||||||||
|
||||||||||||||||
Returns the newly created block on success or None if the clipboard is | ||||||||||||||||
empty. | ||||||||||||||||
Returns (1) the newly created block on success or None if the clipboard is | ||||||||||||||||
empty, and (2) a summary of changes made to static files in the destination | ||||||||||||||||
course. | ||||||||||||||||
""" | ||||||||||||||||
|
||||||||||||||||
from cms.djangoapps.contentstore.views.preview import _load_preview_block | ||||||||||||||||
|
@@ -226,9 +239,10 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request): | |||||||||||||||
user_clipboard = content_staging_api.get_user_clipboard(request.user.id) | ||||||||||||||||
if not user_clipboard: | ||||||||||||||||
# Clipboard is empty or expired/error/loading | ||||||||||||||||
return None | ||||||||||||||||
return None, StaticFileNotices() | ||||||||||||||||
block_type = user_clipboard.content.block_type | ||||||||||||||||
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): | ||||||||||||||||
|
@@ -247,6 +261,11 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request): | |||||||||||||||
# 'keys' and uses 'id_generator', but the default XBlock parse_xml ignores 'id_generator' and uses 'keys'. | ||||||||||||||||
# For children of this block, obviously only id_generator is used. | ||||||||||||||||
xblock_class = runtime.load_block_type(block_type) | ||||||||||||||||
# Note: if we find a case where any XBlock needs access to the block-specific static files that were saved to | ||||||||||||||||
# export_fs during copying, we could make them available here via runtime.resources_fs before calling parse_xml. | ||||||||||||||||
# However, currently the only known case for that is video block's transcript files, and those will | ||||||||||||||||
# automatically be "carried over" to the new XBlock even in a different course because the video ID is the same, | ||||||||||||||||
# and VAL will thus make the transcript available. | ||||||||||||||||
temp_xblock = xblock_class.parse_xml(node, runtime, keys, id_generator) | ||||||||||||||||
if xblock_class.has_children and temp_xblock.children: | ||||||||||||||||
raise NotImplementedError("We don't yet support pasting XBlocks with children") | ||||||||||||||||
|
@@ -257,7 +276,93 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request): | |||||||||||||||
new_xblock = store.update_item(temp_xblock, request.user.id, allow_not_found=True) | ||||||||||||||||
parent_xblock.children.append(new_xblock.location) | ||||||||||||||||
store.update_item(parent_xblock, request.user.id) | ||||||||||||||||
return new_xblock | ||||||||||||||||
|
||||||||||||||||
# 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, | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
return new_xblock, notices | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def _import_files_into_course( | ||||||||||||||||
course_key: CourseKey, | ||||||||||||||||
staged_content_id: int, | ||||||||||||||||
static_files: list[content_staging_api.StagedContentFileData], | ||||||||||||||||
) -> StaticFileNotices: | ||||||||||||||||
""" | ||||||||||||||||
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. | ||||||||||||||||
""" | ||||||||||||||||
# 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 = [] | ||||||||||||||||
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) | ||||||||||||||||
if result is True: | ||||||||||||||||
new_files.append(file_data_obj.filename) | ||||||||||||||||
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( | ||||||||||||||||
new_files=new_files, | ||||||||||||||||
conflicting_files=conflicting_files, | ||||||||||||||||
error_files=error_files, | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def _import_file_into_course( | ||||||||||||||||
course_key: CourseKey, | ||||||||||||||||
staged_content_id: int, | ||||||||||||||||
file_data_obj: content_staging_api.StagedContentFileData, | ||||||||||||||||
) -> bool | None: | ||||||||||||||||
""" | ||||||||||||||||
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) | ||||||||||||||||
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) | ||||||||||||||||
if data is None: | ||||||||||||||||
raise NotFoundError(file_data_obj.source_key) | ||||||||||||||||
content = StaticContent(new_key, name=filename, content_type=content_type, data=data) | ||||||||||||||||
# 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) | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Optional. Currently, the thumbnails for copied files are missing. |
||||||||||||||||
return True | ||||||||||||||||
elif current_file.content_digest == file_data_obj.md5_hash: | ||||||||||||||||
# The file already exists and matches exactly, so no action is needed | ||||||||||||||||
return None | ||||||||||||||||
else: | ||||||||||||||||
# There is a conflict with some other file that has the same name. | ||||||||||||||||
return False | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def is_item_in_course_tree(item): | ||||||||||||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with VideoBlocks that don't use edx-val, i.e. those with
.srt
files that were manually uploaded? The VideoBlock has its own weird logic to do those file mappings, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'm okay with merging this even if it doesn't work quite right for non-pipeline video, but I want to make sure I'm understanding things correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. The video block's .srt files are copied into the clipboard in any case, since the logic was already in the serializer, but when I was testing things I couldn't find any use case to actually make them available at paste time. So I just added this comment and left it at that for now. I guess that if VAL is disabled, then this would be useful. Do you think that's something important to support in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think so, yeah. Many folks won't use the video pipeline because they prefer to manage their videos through other services, and the new VideoBlock work is going to preserve support for different sources.
But like I said, not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openedx/modular-learning#71