Skip to content
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
merged 11 commits into from
Jun 27, 2023
131 changes: 116 additions & 15 deletions cms/djangoapps/contentstore/helpers.py
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.
Expand Down Expand Up @@ -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
Expand All @@ -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):
Expand All @@ -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.
Comment on lines +266 to +268
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
Expand All @@ -257,7 +276,89 @@ 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)
contentstore().save(content)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
contentstore().save(content)
thumbnail_content, thumbnail_location = contentstore().generate_thumbnail(content)
if thumbnail_content is not None:
content.thumbnail_location = thumbnail_location
contentstore().save(content)

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):
Expand Down
75 changes: 72 additions & 3 deletions cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
"""
from opaque_keys.edx.keys import UsageKey
from rest_framework.test import APIClient
from xmodule.modulestore.django import modulestore
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import ToyCourseFactory
from xmodule.modulestore.django import contentstore, modulestore
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course
from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory

CLIPBOARD_ENDPOINT = "/api/content-staging/v1/clipboard/"
XBLOCK_ENDPOINT = "/xblock/"
Expand Down Expand Up @@ -60,3 +60,72 @@ def test_copy_and_paste_video(self):
assert new_video.youtube_id_1_0 == orig_video.youtube_id_1_0
# The new block should store a reference to where it was copied from
assert new_video.copied_from_block == str(video_key)

def test_paste_with_assets(self):
"""
When pasting into a different course, any required static assets should
be pasted too, unless they already exist in the destination course.
"""
dest_course_key, client = self._setup_course()
# Make sure some files exist in the source course to be copied:
source_course = CourseFactory.create()
upload_file_to_course(
course_key=source_course.id,
contentstore=contentstore(),
source_file='./common/test/data/static/picture1.jpg',
target_filename="picture1.jpg",
)
upload_file_to_course(
course_key=source_course.id,
contentstore=contentstore(),
source_file='./common/test/data/static/picture2.jpg',
target_filename="picture2.jpg",
)
source_html = BlockFactory.create(
parent_location=source_course.location,
category="html",
display_name="Some HTML",
data="""
<p>
<a href="/static/picture1.jpg">Picture 1</a>
<a href="/static/picture2.jpg">Picture 2</a>
</p>
""",
)

# Now, to test conflict handling, we also upload a CONFLICTING image to
# the destination course under the same filename.
upload_file_to_course(
course_key=dest_course_key,
contentstore=contentstore(),
# Note this is picture 3, not picture 2, but we save it as picture 2:
source_file='./common/test/data/static/picture3.jpg',
target_filename="picture2.jpg",
)

# Now copy the HTML block from the source cost and paste it into the destination:
copy_response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(source_html.location)}, format="json")
assert copy_response.status_code == 200

# Paste the video
dest_parent_key = dest_course_key.make_usage_key("vertical", "vertical_test")
paste_response = client.post(XBLOCK_ENDPOINT, {
"parent_locator": str(dest_parent_key),
"staged_content": "clipboard",
}, format="json")
assert paste_response.status_code == 200
static_file_notices = paste_response.json()["static_file_notices"]
assert static_file_notices == {
"error_files": [],
"new_files": ["picture1.jpg"],
# The new course already had a file named "picture2.jpg" with different md5 hash, so it's a conflict:
"conflicting_files": ["picture2.jpg"],
}

# Check that the files are as we expect:
source_pic1_hash = contentstore().find(source_course.id.make_asset_key("asset", "picture1.jpg")).content_digest
dest_pic1_hash = contentstore().find(dest_course_key.make_asset_key("asset", "picture1.jpg")).content_digest
assert source_pic1_hash == dest_pic1_hash
source_pic2_hash = contentstore().find(source_course.id.make_asset_key("asset", "picture2.jpg")).content_digest
dest_pic2_hash = contentstore().find(dest_course_key.make_asset_key("asset", "picture2.jpg")).content_digest
assert source_pic2_hash != dest_pic2_hash # Because there was a conflict, this file was unchanged.
14 changes: 7 additions & 7 deletions cms/djangoapps/contentstore/xblock_services/xblock_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from datetime import datetime
from uuid import uuid4

from attrs import asdict
from django.conf import settings
from django.contrib.auth.decorators import login_required
from django.contrib.auth.models import (User) # lint-amnesty, pylint: disable=imported-auth-user
Expand Down Expand Up @@ -562,7 +563,7 @@ def _create_block(request):
if request.json.get("staged_content") == "clipboard":
# Paste from the user's clipboard (content_staging app clipboard, not browser clipboard) into 'usage_key':
try:
created_xblock = import_staged_content_from_user_clipboard(
created_xblock, notices = import_staged_content_from_user_clipboard(
parent_key=usage_key, request=request
)
except Exception: # pylint: disable=broad-except
Expand All @@ -576,12 +577,11 @@ def _create_block(request):
return JsonResponse(
{"error": _("Your clipboard is empty or invalid.")}, status=400
)
return JsonResponse(
{
"locator": str(created_xblock.location),
"courseKey": str(created_xblock.location.course_key),
}
)
return JsonResponse({
"locator": str(created_xblock.location),
"courseKey": str(created_xblock.location.course_key),
"static_file_notices": asdict(notices),
})

category = request.json["category"]
if isinstance(usage_key, LibraryUsageLocator):
Expand Down
8 changes: 8 additions & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -2728,3 +2728,11 @@

DISCUSSIONS_INCONTEXT_FEEDBACK_URL = ''
DISCUSSIONS_INCONTEXT_LEARNMORE_URL = ''

OPEN_EDX_FILTERS_CONFIG = {
"org.openedx.content_authoring.staged_content.static_filter_source.v1": {
"pipeline": [
"openedx.core.djangoapps.content_staging.filters.IgnoreLargeFiles",
]
}
}
56 changes: 54 additions & 2 deletions cms/static/js/views/pages/container.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ define(['jquery', 'underscore', 'backbone', 'gettext', 'js/views/pages/base_page
'common/js/components/utils/view_utils', 'js/views/container', 'js/views/xblock',
'js/views/components/add_xblock', 'js/views/modals/edit_xblock', 'js/views/modals/move_xblock_modal',
'js/models/xblock_info', 'js/views/xblock_string_field_editor', 'js/views/xblock_access_editor',
'js/views/pages/container_subviews', 'js/views/unit_outline', 'js/views/utils/xblock_utils'],
'js/views/pages/container_subviews', 'js/views/unit_outline', 'js/views/utils/xblock_utils',
'common/js/components/views/feedback_notification', 'common/js/components/views/feedback_prompt',
],
function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView, AddXBlockComponent,
EditXBlockModal, MoveXBlockModal, XBlockInfo, XBlockStringFieldEditor, XBlockAccessEditor,
ContainerSubviews, UnitOutlineView, XBlockUtils) {
ContainerSubviews, UnitOutlineView, XBlockUtils, NotificationView, PromptView) {
'use strict';

var XBlockContainerPage = BasePage.extend({
Expand Down Expand Up @@ -255,10 +257,60 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
staged_content: "clipboard",
}).then((data) => {
this.onNewXBlock(placeholderElement, scrollOffset, false, data);
return data;
}).fail(() => {
// Remove the placeholder if the paste failed
placeholderElement.remove();
});
}).done((data) => {
const {
conflicting_files: conflictingFiles,
error_files: errorFiles,
new_files: newFiles,
} = data.static_file_notices;

const notices = [];
if (errorFiles.length) {
notices.push((next) => new PromptView.Error({
title: gettext("Some errors occurred"),
message: (
gettext("The following required files could not be added to the course:") +
" " + errorFiles.join(", ")
),
actions: {primary: {text: gettext("OK"), click: (x) => { x.hide(); next(); }}},
}));
}
if (conflictingFiles.length) {
notices.push((next) => new PromptView.Warning({
title: gettext("You may need to update a file(s) manually"),
message: (
gettext(
"The following files already exist in this course but don't match the " +
"version used by the component you pasted:"
) + " " + conflictingFiles.join(", ")
),
actions: {primary: {text: gettext("OK"), click: (x) => { x.hide(); next(); }}},
}));
}
if (newFiles.length) {
notices.push(() => new NotificationView.Confirmation({
title: gettext("New files were added to this course's Files & Uploads"),
message: (
gettext("The following required files were imported to this course:") +
" " + newFiles.join(", ")
),
closeIcon: true,
}));
}
if (notices.length) {
// Show the notices, one at a time:
const showNext = () => {
const view = notices.shift()(showNext);
view.show();
}
// Delay to avoid conflict with the "Pasting..." notification.
setTimeout(showNext, 1250);
}
});
},

Expand Down
Loading