diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 4366f11d81ac..e67e337e55fa 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 @@ -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 @@ -192,6 +193,8 @@ def xblock_type_display_name(xblock, default_display_name=None): return _('Problem') elif category == 'library_v2': return _('Library Content') + elif category == 'itembank': + return _('Problem Bank') component_class = XBlock.load_class(category) if hasattr(component_class, 'display_name') and component_class.display_name.default: return _(component_class.display_name.default) # lint-amnesty, pylint: disable=translation-of-non-string @@ -278,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): @@ -295,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 @@ -454,11 +473,21 @@ 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 = [] @@ -466,17 +495,25 @@ 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): - # 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: @@ -484,25 +521,45 @@ 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, +) -> 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: @@ -510,22 +567,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/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index d40a5ec79475..964f0c57e757 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -436,7 +436,7 @@ def get_library_content_picker_url(course_locator) -> str: content_picker_url = None if libraries_v2_enabled(): mfe_base_url = get_course_authoring_url(course_locator) - content_picker_url = f'{mfe_base_url}/component-picker' + content_picker_url = f'{mfe_base_url}/component-picker?variant=published' return content_picker_url diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index b89bef0f6709..2405af2479ec 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -47,6 +47,7 @@ 'discussion', 'library', 'library_v2', # Not an XBlock + 'itembank', 'html', 'openassessment', 'problem', @@ -262,6 +263,7 @@ def create_support_legend_dict(): 'openassessment': _("Open Response"), 'library': _("Legacy Library"), 'library_v2': _("Library Content"), + 'itembank': _("Problem Bank"), 'drag-and-drop-v2': _("Drag and Drop"), } @@ -488,6 +490,7 @@ def _filter_disabled_blocks(all_blocks): disabled_block_names = [block.name for block in disabled_xblocks()] if not libraries_v2_enabled(): disabled_block_names.append('library_v2') + disabled_block_names.append('itembank') return [block_name for block_name in all_blocks if block_name not in disabled_block_names] diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index acc5fc95dfe3..aa7421bb87b3 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -300,8 +300,9 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): selected_groups_label = _('Access restricted to: {list_of_groups}').format(list_of_groups=selected_groups_label) # lint-amnesty, pylint: disable=line-too-long course = modulestore().get_course(xblock.location.course_key) can_edit = context.get('can_edit', True) + can_add = context.get('can_add', True) # Is this a course or a library? - is_course = xblock.scope_ids.usage_id.context_key.is_course + is_course = xblock.context_key.is_course tags_count_map = context.get('tags_count_map') tags_count = 0 if tags_count_map: @@ -320,7 +321,10 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): 'is_selected': context.get('is_selected', False), 'selectable': context.get('selectable', False), 'selected_groups_label': selected_groups_label, - 'can_add': context.get('can_add', True), + 'can_add': can_add, + # Generally speaking, "if you can add, you can delete". One exception is itembank (Problem Bank) + # which has its own separate "add" workflow but uses the normal delete workflow for its child blocks. + 'can_delete': can_add or (root_xblock and root_xblock.scope_ids.block_type == "itembank" and can_edit), 'can_move': context.get('can_move', is_course), 'language': getattr(course, 'language', None), 'is_course': is_course, diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index a864b29025e0..a818da81d10f 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -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, """ + +

Including this totally real image:

+ + + + Wrong + Right + + + + """) # 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): """ diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index a4adbb0d4cbe..d0d414cbb46a 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -594,9 +594,13 @@ def _create_block(request): # Set `created_block.upstream` and then sync this with the upstream (library) version. created_block.upstream = upstream_ref sync_from_upstream(downstream=created_block, user=request.user) - except BadUpstream: + except BadUpstream as exc: _delete_item(created_block.location, request.user) - return JsonResponse({"error": _("Invalid library xblock reference.")}, status=400) + log.exception( + f"Could not sync to new block at '{created_block.usage_key}' " + f"using provided library_content_key='{upstream_ref}'" + ) + return JsonResponse({"error": str(exc)}, status=400) modulestore().update_item(created_block, request.user.id) return JsonResponse( diff --git a/cms/envs/common.py b/cms/envs/common.py index dc719e196b32..ea374bca8bb3 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1653,6 +1653,9 @@ 'corsheaders', 'openedx.core.djangoapps.cors_csrf', + # Provides the 'django_markup' template library so we can use 'interpolate_html' in django templates + 'xss_utils', + # History tables 'simple_history', diff --git a/cms/static/images/large-itembank-icon.png b/cms/static/images/large-itembank-icon.png new file mode 100644 index 000000000000..655ef1531336 Binary files /dev/null and b/cms/static/images/large-itembank-icon.png differ diff --git a/cms/static/js/views/components/add_library_content.js b/cms/static/js/views/components/add_library_content.js index ee1894b8aa9d..278717ba9212 100644 --- a/cms/static/js/views/components/add_library_content.js +++ b/cms/static/js/views/components/add_library_content.js @@ -1,6 +1,13 @@ /** * Provides utilities to open and close the library content picker. + * This is for adding a single, selected, non-randomized component (XBlock) + * from the library into the course. It achieves the same effect as copy-pasting + * the block from a library into the course. The block will remain synced with + * the "upstream" library version. * + * Compare cms/static/js/views/modals/select_v2_library_content.js which uses + * a multi-select modal to add component(s) to a Problem Bank (for + * randomization). */ define(['jquery', 'underscore', 'gettext', 'js/views/modals/base_modal'], function($, _, gettext, BaseModal) { diff --git a/cms/static/js/views/modals/select_v2_library_content.js b/cms/static/js/views/modals/select_v2_library_content.js new file mode 100644 index 000000000000..76fb301540be --- /dev/null +++ b/cms/static/js/views/modals/select_v2_library_content.js @@ -0,0 +1,88 @@ +/** + * Provides utilities to open and close the library content picker. + * This is for adding multiple components to a Problem Bank (for randomization). + * + * Compare cms/static/js/views/components/add_library_content.js which uses + * a single-select modal to add one component to a course (non-randomized). + */ +define(['jquery', 'underscore', 'gettext', 'js/views/modals/base_modal'], +function($, _, gettext, BaseModal) { + 'use strict'; + + var SelectV2LibraryContent = BaseModal.extend({ + options: $.extend({}, BaseModal.prototype.options, { + modalName: 'add-components-from-library', + modalSize: 'lg', + view: 'studio_view', + viewSpecificClasses: 'modal-add-component-picker confirm', + titleFormat: gettext('Add library content'), + addPrimaryActionButton: false, + }), + + events: { + 'click .action-add': 'addSelectedComponents', + 'click .action-cancel': 'cancel', + }, + + initialize: function() { + BaseModal.prototype.initialize.call(this); + this.selections = []; + // Add event listen to close picker when the iframe tells us to + const handleMessage = (event) => { + if (event.data?.type === 'pickerSelectionChanged') { + this.selections = event.data.selections; + if (this.selections.length > 0) { + this.enableActionButton('add'); + } else { + this.disableActionButton('add'); + } + } + }; + this.messageListener = window.addEventListener("message", handleMessage); + this.cleanupListener = () => { window.removeEventListener("message", handleMessage) }; + }, + + hide: function() { + BaseModal.prototype.hide.call(this); + this.cleanupListener(); + }, + + /** + * Adds the action buttons to the modal. + */ + addActionButtons: function() { + this.addActionButton('add', gettext('Add selected components'), true); + this.addActionButton('cancel', gettext('Cancel')); + this.disableActionButton('add'); + }, + + /** Handler when the user clicks the "Add Selected Components" primary button */ + addSelectedComponents: function(event) { + if (event) { + event.preventDefault(); + event.stopPropagation(); // Make sure parent modals don't see the click + } + this.hide(); + this.callback(this.selections); + }, + + /** + * Show a component picker modal from library. + * @param contentPickerUrl Url for component picker + * @param callback A function to call with the selected block(s) + */ + showComponentPicker: function(contentPickerUrl, callback) { + this.contentPickerUrl = contentPickerUrl; + this.callback = callback; + + this.render(); + this.show(); + }, + + getContentHtml: function() { + return `