From c2e1d249adbee09975c1c60d416a387b3f0796da Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 27 Sep 2024 18:50:11 -0400 Subject: [PATCH 1/8] feat: very hacky first pass at getting static assets to show in v2 libs --- common/djangoapps/static_replace/__init__.py | 1 - common/djangoapps/static_replace/services.py | 1 + lms/urls.py | 9 +++ .../core/djangoapps/content_libraries/api.py | 32 ++++++++- .../core/djangoapps/content_libraries/urls.py | 5 ++ .../djangoapps/content_libraries/views.py | 72 ++++++++++++++++++- .../xblock/runtime/learning_core_runtime.py | 19 ++++- .../lib/xblock_serializer/block_serializer.py | 9 +-- 8 files changed, 139 insertions(+), 9 deletions(-) diff --git a/common/djangoapps/static_replace/__init__.py b/common/djangoapps/static_replace/__init__.py index 310ad8343242..893349e76c27 100644 --- a/common/djangoapps/static_replace/__init__.py +++ b/common/djangoapps/static_replace/__init__.py @@ -172,7 +172,6 @@ def replace_static_urls( xblock: xblock where the static assets are stored lookup_url_func: Lookup function which returns the correct path of the asset """ - if static_paths_out is None: static_paths_out = [] diff --git a/common/djangoapps/static_replace/services.py b/common/djangoapps/static_replace/services.py index 80c284a9b526..27caa0c1f2b4 100644 --- a/common/djangoapps/static_replace/services.py +++ b/common/djangoapps/static_replace/services.py @@ -51,6 +51,7 @@ def replace_urls(self, text, static_replace_only=False): static_replace_only: If True, only static urls will be replaced """ block = self.xblock() + if self.lookup_asset_url: text = replace_static_urls(text, xblock=block, lookup_asset_url=self.lookup_asset_url) else: diff --git a/lms/urls.py b/lms/urls.py index f97bc7f0d04e..e7d935dbdc49 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -92,10 +92,19 @@ ), ] +from openedx.core.djangoapps.content_libraries.views import component_version_asset urlpatterns = [ path('', branding_views.index, name='root'), # Main marketing page, or redirect to courseware + # this is definitely wrong for now, but I need it to test stuff end-to-end. + path( + 'library_assets//', + component_version_asset, + name='library-assets', + ), + + path('', include('common.djangoapps.student.urls')), # TODO: Move lms specific student views out of common code re_path(r'^dashboard/?$', student_views.student_dashboard, name='dashboard'), diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 3dc33aec9616..99d19e750395 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -67,6 +67,7 @@ from django.db import IntegrityError, transaction from django.db.models import Q, QuerySet from django.utils.translation import gettext as _ +from django.urls import reverse from edx_rest_api_client.client import OAuthAPIClient from lxml import etree from opaque_keys.edx.keys import BlockTypeKey, UsageKey, UsageKeyV2 @@ -997,7 +998,36 @@ def get_library_block_static_asset_files(usage_key) -> list[LibraryXBlockStaticF TODO: This is not yet implemented for Learning Core backed libraries. TODO: Should this be in the general XBlock API rather than the libraries API? """ - return [] + component = get_component_from_usage_key(usage_key) + component_version = component.versioning.draft + + # If there is no Draft version, then this was soft-deleted + if component_version is None: + return [] + + # cvc = the ComponentVersionContent through table + cvc_set = ( + component_version + .componentversioncontent_set + .filter(content__has_file=True) + .order_by('key') + .select_related('content') + ) + + return [ + LibraryXBlockStaticFile( + path=cvc.key, + size=cvc.content.size, + url=reverse( + 'content_libraries:library-assets', + kwargs={ + 'component_version_uuid': component_version.uuid, + 'asset_path': cvc.key, + } + ), + ) + for cvc in cvc_set + ] def add_library_block_static_asset_file(usage_key, file_name, file_content) -> LibraryXBlockStaticFile: diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 9455f0de5e61..ec7da1993950 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -73,4 +73,9 @@ path('pub/jwks/', views.LtiToolJwksView.as_view(), name='lti-pub-jwks'), ])), ])), + path( + 'library_assets//', + views.component_version_asset, + name='library-assets', + ), ] diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 835a5de1f1f2..e9e41d27727d 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -72,13 +72,14 @@ from django.contrib.auth import authenticate, get_user_model, login from django.contrib.auth.models import Group from django.db.transaction import atomic, non_atomic_requests -from django.http import Http404, HttpResponseBadRequest, JsonResponse +from django.http import Http404, HttpResponse, HttpResponseBadRequest, JsonResponse, StreamingHttpResponse from django.shortcuts import get_object_or_404 from django.urls import reverse from django.utils.decorators import method_decorator from django.utils.translation import gettext as _ from django.views.decorators.clickjacking import xframe_options_exempt from django.views.decorators.csrf import csrf_exempt +from django.views.decorators.http import require_safe from django.views.generic.base import TemplateResponseMixin, View from pylti1p3.contrib.django import DjangoCacheDataStorage, DjangoDbToolConf, DjangoMessageLaunch, DjangoOIDCLogin from pylti1p3.exception import LtiException, OIDCException @@ -86,6 +87,7 @@ import edx_api_doc_tools as apidocs from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 +from openedx_learning.api import authoring as authoring_api from organizations.api import ensure_organization from organizations.exceptions import InvalidOrganizationException from organizations.models import Organization @@ -1107,3 +1109,71 @@ def get(self, request): Return the JWKS. """ return JsonResponse(self.lti_tool_config.get_jwks(), safe=False) + + +@require_safe +def component_version_asset(request, component_version_uuid, asset_path): + """ + Serves static assets associated with particular Component versions. + + Important notes: + + * This is meant for Studio/authoring use ONLY. It requires read access to + the content library. + * It uses the UUID because that's easier to parse than the key field (which + could be part of an OpaqueKey, but could also be almost anything else). + * This is not very performant, and we still want to use the X-Accel-Redirect + method for serving LMS traffic in the longer term (and probably Studio + eventually). + """ + try: + component_version = authoring_api.get_component_version_by_uuid( + component_version_uuid + ) + except ObjectDoesNotExist: + raise Http404() + + learning_package = component_version.component.learning_package + library_key = LibraryLocatorV2.from_string(learning_package.key) + + api.require_permission_for_library_key( + library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, + ) + + # We already have logic for getting the correct content and generating the + # proper headers in Learning Core, but the response generated here is an + # X-Accel-Redirect and lacks the actual content. We eventually want to use + # this response in conjunction with a media reverse proxy (Caddy or Nginx), + # but in the short term we're just going to remove the redirect and stream + # the content directly. + redirect_response = authoring_api.get_redirect_response_for_component_asset( + component_version_uuid, + asset_path, + public=False, + learner_downloadable_only=False, + ) + + # If there was any error, we return that response because it will have the + # correct headers set and won't have any X-Accel-Redirect header set. + if redirect_response.status_code != 200: + return redirect_response + + cv_content = component_version.componentversioncontent_set.get(key=asset_path) + content = cv_content.content + + # Delete the re-direct part of the response headers. We'll copy the rest. + headers = redirect_response.headers + headers.pop('X-Accel-Redirect') + + # We need to set the content size header manually because this is a + # streaming response. It's not included in the redirect headers because it's + # not needed there (the reverse-proxy would have direct access to the file). + headers['Content-Length'] = content.size + + if request.method == "HEAD": + return HttpResponse(headers=headers) + + return StreamingHttpResponse( + content.read_file().chunks(), + headers=redirect_response.headers, + ) diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index 26aa7af60f0b..e3eaa8b266a9 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -9,6 +9,7 @@ from django.core.exceptions import ObjectDoesNotExist from django.db.transaction import atomic +from django.urls import reverse from openedx_learning.api import authoring as authoring_api @@ -291,6 +292,20 @@ def _lookup_asset_url(self, block: XBlock, asset_path: str) -> str | None: # py This is called by the XBlockRuntime superclass in the .runtime module. - TODO: Implement as part of larger static asset effort. + TODO: Like get_block, we currently assume that we're using the Draft + version. This should be a runtime parameter. """ - return None + usage_key = block.scope_ids.usage_id + component = self._get_component_from_usage_key(usage_key) + component_version = component.versioning.draft + if component_version is None: + # This could happen if a Component was soft-deleted. + raise NoSuchUsage(usage_key) + + return reverse( + 'library-assets', + kwargs={ + 'component_version_uuid': component_version.uuid, + 'asset_path': asset_path, + } + ) diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index 966380f25061..e45e1392b143 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -34,13 +34,14 @@ def __init__(self, block): self.olx_str = etree.tostring(olx_node, encoding="unicode", pretty_print=True) course_key = self.orig_block_key.course_key + # Search the OLX for references to files stored in the course's # "Files & Uploads" (contentstore): self.olx_str = utils.rewrite_absolute_static_urls(self.olx_str, course_key) - for asset in utils.collect_assets_from_text(self.olx_str, course_key): - path = asset['path'] - if path not in [sf.name for sf in self.static_files]: - self.static_files.append(StaticFile(name=path, url=asset['url'], data=None)) + # for asset in utils.collect_assets_from_text(self.olx_str, course_key): + # path = asset['path'] + # if path not in [sf.name for sf in self.static_files]: + # self.static_files.append(StaticFile(name=path, url=asset['url'], data=None)) if block.scope_ids.usage_id.block_type in ['problem', 'vertical']: py_lib_zip_file = utils.get_python_lib_zip_if_using(self.olx_str, course_key) From 55e7efcb2757c42abe51039f1fb5d6de2ec44d1f Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 28 Sep 2024 13:01:46 -0400 Subject: [PATCH 2/8] fix: prevent the 'static/' prefix from getting stripped in asset references --- .../xblock/runtime/learning_core_runtime.py | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index e3eaa8b266a9..ec5fb73f49c6 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -306,6 +306,30 @@ def _lookup_asset_url(self, block: XBlock, asset_path: str) -> str | None: # py 'library-assets', kwargs={ 'component_version_uuid': component_version.uuid, - 'asset_path': asset_path, + + # The standard XBlock "static/{asset_path}"" substitution + # strips out the leading "static/" part because it assumes that + # all files will exist in a flat namespace. Learning Core treats + # directories differently, and there may one day be other files + # that are not meant to be externally downloadable at the root + # (e.g. LaTeX source files or graders). + # + # So the transformation looks like this: + # + # 1. The Learning Core ComponentVersion has an asset stored as + # "static/test.png". + # 1. The original OLX content we store references + # "/static/test.png" or "static/test.png". + # 2. The ReplaceURLService XBlock runtime service invokes + # static_replace and strips out "/static/". + # 3. The method we're in now is invoked with a static_path of + # just "test.png", because that's the transformation that + # works for ModuleStore-based courses. + # 4. This method then builds a URL that adds the "static/" + # prefix back when creating the URL. + # + # Which is a very long explanation for why we're re-adding the + # "static/" to the asset path below. + 'asset_path': f"static/{asset_path}", } ) From fd211247ecaa8542351b94d767902e186801cc17 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 28 Sep 2024 14:34:04 -0400 Subject: [PATCH 3/8] feat: get copy to take Learning Core static assets and put them into staged content --- .../xblock/runtime/learning_core_runtime.py | 37 +++++++++++++++++++ .../lib/xblock_serializer/block_serializer.py | 23 ++++++++++-- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index ec5fb73f49c6..b95de8ae71d1 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -21,6 +21,7 @@ from xblock.field_data import FieldData from openedx.core.lib.xblock_serializer.api import serialize_modulestore_block_for_learning_core +from openedx.core.lib.xblock_serializer.data import StaticFile from ..learning_context.manager import get_learning_context_impl from .runtime import XBlockRuntime @@ -216,6 +217,42 @@ def get_block(self, usage_key, for_parent=None): return block + def get_block_assets(self, usage_key): + """ + This currently doesn't copy any + """ + component = self._get_component_from_usage_key(usage_key) + component_version = component.versioning.draft + + # If there is no Draft version, then this was soft-deleted + if component_version is None: + return [] + + # cvc = the ComponentVersionContent through table + cvc_set = ( + component_version + .componentversioncontent_set + .filter(content__has_file=True) + .order_by('key') + .select_related('content') + ) + + return [ + StaticFile( + name=cvc.key, + url=None, +# url=reverse( +# 'content_libraries:library-assets', +# kwargs={ +# 'component_version_uuid': component_version.uuid, +# 'asset_path': cvc.key, +# } +# ), + data=cvc.content.read_file().read() + ) + for cvc in cvc_set + ] + def save_block(self, block): """ Save any pending field data values to Learning Core data models. diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index e45e1392b143..4b5678b13625 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -38,10 +38,25 @@ def __init__(self, block): # Search the OLX for references to files stored in the course's # "Files & Uploads" (contentstore): self.olx_str = utils.rewrite_absolute_static_urls(self.olx_str, course_key) - # for asset in utils.collect_assets_from_text(self.olx_str, course_key): - # path = asset['path'] - # if path not in [sf.name for sf in self.static_files]: - # self.static_files.append(StaticFile(name=path, url=asset['url'], data=None)) + + # If a block supports explicit assets, there's no need to scan the + # content looking for static assets that it *might* be using. Learning + # Core backed content supports this, which currently means v2 Content + # Libraries. + runtime_supports_explicit_assets = hasattr(block.runtime, 'get_block_assets') + if runtime_supports_explicit_assets: + self.static_files.extend( + block.runtime.get_block_assets(block.scope_ids.usage_id) + ) + print(f"static_files: {self.static_files}") + return + + # Otherwise, we have to scan the content to extract associated asset + # files that are stored in the course-global Files and Uploads. + for asset in utils.collect_assets_from_text(self.olx_str, course_key): + path = asset['path'] + if path not in [sf.name for sf in self.static_files]: + self.static_files.append(StaticFile(name=path, url=asset['url'], data=None)) if block.scope_ids.usage_id.block_type in ['problem', 'vertical']: py_lib_zip_file = utils.get_python_lib_zip_if_using(self.olx_str, course_key) From 2da6e8a1a007c205c95f272c315cc04b99ac2ce5 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 28 Sep 2024 23:55:32 -0400 Subject: [PATCH 4/8] feat: working paste between v2 libraries and from course -> v2 library --- .../core/djangoapps/content_libraries/api.py | 75 +++++++++++++++++-- .../xblock/runtime/learning_core_runtime.py | 12 +-- 2 files changed, 73 insertions(+), 14 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 99d19e750395..e44fd9b52598 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -56,6 +56,7 @@ import base64 import hashlib import logging +import mimetypes import attr import requests @@ -888,9 +889,9 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use if not user_clipboard: return None - olx_str = content_staging_api.get_staged_content_olx(user_clipboard.content.id) - - # TODO: Handle importing over static assets + staged_content_id = user_clipboard.content.id + olx_str = content_staging_api.get_staged_content_olx(staged_content_id) + staged_content_files = content_staging_api.get_staged_content_static_files(staged_content_id) content_library, usage_key = validate_can_add_block_to_library( library_key, @@ -898,9 +899,70 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use block_id ) + now = datetime.now(tz=timezone.utc) + # Create component for block then populate it with clipboard data - _create_component_for_block(content_library, usage_key, user.id) - set_library_block_olx(usage_key, olx_str) + with transaction.atomic(): + component_version = _create_component_for_block( + content_library, + usage_key, + user.id, + ) + + for staged_content_file_data in staged_content_files: + # The ``data`` attribute is going to be None because the clipboard + # is optimized to not do redundant file copying when copying/pasting + # within the same course (where all the Files and Uploads are + # shared). Learning Core backed content Components will always store + # a Component-local "copy" of the data, and rely on lower-level + # deduplication to happen in the ``contents`` app. + filename = staged_content_file_data.filename + + # Grab our byte data for the file... + file_data = content_staging_api.get_staged_content_static_file_data( + staged_content_id, + filename, + ) + + # Courses don't support having assets that are local to a specific + # component, and instead store all their content together in a + # shared Files and Uploads namespace. If we're pasting that into a + # Learning Core backed data model (v2 Libraries), then we want to + # prepend "static/" to the filename. This will need to get updated + # when we start moving courses over to Learning Core, or if we start + # storing course component assets in sub-directories of Files and + # Uploads. + # + # The reason we don't just search for a "static/" prefix is that + # Learning Core components can store other kinds of files if they + # wish (though none currently do). + source_assumes_global_assets = not isinstance( + user_clipboard.source_context_key, LibraryLocatorV2 + ) + if source_assumes_global_assets: + filename = f"static/{filename}" + + # Now construct the Learning Core data models for it... + media_type_str, _encoding = mimetypes.guess_type(filename) + media_type = authoring_api.get_or_create_media_type(media_type_str) + content = authoring_api.get_or_create_file_content( + content_library.learning_package_id, + media_type.id, + data=file_data, + created=now, + ) + authoring_api.create_component_version_content( + component_version.pk, + content.id, + key=filename, + learner_downloadable=True, + ) + + # Right now the ordering of this is important and set_library_block_olx + # makes a new version (so pasting makes two version instead of one). + # Get this more cleanly refactored later. + set_library_block_olx(usage_key, olx_str) + # Emit library block created event LIBRARY_BLOCK_CREATED.send_event( @@ -928,7 +990,7 @@ def get_or_create_olx_media_type(block_type: str) -> MediaType: def _create_component_for_block(content_lib, usage_key, user_id=None): """ - Create a Component for an XBlock type, and initialize it. + Create a Component for an XBlock type, initialize it, and return the ComponentVersion. This will create a Component, along with its first ComponentVersion. The tag in the OLX will have no attributes, e.g. ``. This first version @@ -971,6 +1033,7 @@ def _create_component_for_block(content_lib, usage_key, user_id=None): key="block.xml", learner_downloadable=False ) + return component_version def delete_library_block(usage_key, remove_from_parent=True): diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index b95de8ae71d1..e53fd19d391e 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -219,7 +219,10 @@ def get_block(self, usage_key, for_parent=None): def get_block_assets(self, usage_key): """ - This currently doesn't copy any + Return a list of StaticFile entries. + + TODO: When we want to copy a whole Section at a time, doing these + lookups one by one is going to get slow. """ component = self._get_component_from_usage_key(usage_key) component_version = component.versioning.draft @@ -241,13 +244,6 @@ def get_block_assets(self, usage_key): StaticFile( name=cvc.key, url=None, -# url=reverse( -# 'content_libraries:library-assets', -# kwargs={ -# 'component_version_uuid': component_version.uuid, -# 'asset_path': cvc.key, -# } -# ), data=cvc.content.read_file().read() ) for cvc in cvc_set From aef0bc81e229b8d5a4bfa00825d873e71009c5b0 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 30 Sep 2024 10:28:04 -0400 Subject: [PATCH 5/8] temp: move the preview library XBlock rendering to happen via Studio --- .../templates/xblock_v2/xblock_iframe.html | 0 lms/urls.py | 10 ---------- openedx/core/djangoapps/content_libraries/api.py | 10 ++++++++-- .../xblock/runtime/learning_core_runtime.py | 2 +- xmodule/video_block/transcripts_utils.py | 11 ++++++++--- 5 files changed, 17 insertions(+), 16 deletions(-) rename {lms => common}/templates/xblock_v2/xblock_iframe.html (100%) diff --git a/lms/templates/xblock_v2/xblock_iframe.html b/common/templates/xblock_v2/xblock_iframe.html similarity index 100% rename from lms/templates/xblock_v2/xblock_iframe.html rename to common/templates/xblock_v2/xblock_iframe.html diff --git a/lms/urls.py b/lms/urls.py index e7d935dbdc49..f47a6dc72cf2 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -92,19 +92,9 @@ ), ] -from openedx.core.djangoapps.content_libraries.views import component_version_asset - urlpatterns = [ path('', branding_views.index, name='root'), # Main marketing page, or redirect to courseware - # this is definitely wrong for now, but I need it to test stuff end-to-end. - path( - 'library_assets//', - component_version_asset, - name='library-assets', - ), - - path('', include('common.djangoapps.student.urls')), # TODO: Move lms specific student views out of common code re_path(r'^dashboard/?$', student_views.student_dashboard, name='dashboard'), diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index e44fd9b52598..97eaffed7e9c 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -102,7 +102,11 @@ from xblock.core import XBlock from xblock.exceptions import XBlockNotFoundError -from openedx.core.djangoapps.xblock.api import get_component_from_usage_key, xblock_type_display_name +from openedx.core.djangoapps.xblock.api import ( + get_component_from_usage_key, + get_xblock_app_config, + xblock_type_display_name, +) from openedx.core.lib.xblock_serializer.api import serialize_modulestore_block_for_learning_core from xmodule.library_root_xblock import LibraryRoot as LibraryRootV1 from xmodule.modulestore import ModuleStoreEnum @@ -1077,11 +1081,13 @@ def get_library_block_static_asset_files(usage_key) -> list[LibraryXBlockStaticF .select_related('content') ) + site_root_url = get_xblock_app_config().get_site_root_url() + return [ LibraryXBlockStaticFile( path=cvc.key, size=cvc.content.size, - url=reverse( + url=site_root_url + reverse( 'content_libraries:library-assets', kwargs={ 'component_version_uuid': component_version.uuid, diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index e53fd19d391e..75039e3b0431 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -336,7 +336,7 @@ def _lookup_asset_url(self, block: XBlock, asset_path: str) -> str | None: # py raise NoSuchUsage(usage_key) return reverse( - 'library-assets', + 'content_libraries:library-assets', kwargs={ 'component_version_uuid': component_version.uuid, diff --git a/xmodule/video_block/transcripts_utils.py b/xmodule/video_block/transcripts_utils.py index e41f925295f0..f7bcdaa51f5b 100644 --- a/xmodule/video_block/transcripts_utils.py +++ b/xmodule/video_block/transcripts_utils.py @@ -1041,6 +1041,13 @@ def get_transcript_from_learning_core(video_block, language, output_format, tran """ Get video transcript from Learning Core. + This whole tangle of transcript data is a crazy web of backwards + compatibility affordances with our bespoke SJSON format, which we should + completely abandon. We really shouldn't have to do anything other that + make the transcript files available to the frontend and let the VideoBlock + choose. But we'll jump through these hoops for now to avoid the pain of that + refactor a little longer. :-P + HISTORIC INFORMATION FROM WHEN THIS FUNCTION WAS `get_transcript_from_blockstore`: Blockstore expects video transcripts to be placed into the 'static/' @@ -1072,9 +1079,7 @@ def get_transcript_from_learning_core(video_block, language, output_format, tran Returns: tuple containing content, filename, mimetype """ - # TODO: Update to use Learning Core data models once static assets support - # has been added. - raise NotImplementedError("Transcripts not supported.") + raise NotImplementedError def get_transcript(video, lang=None, output_format=Transcript.SRT, youtube_id=None): From 5f743486bee1bd2674f16cee80e27f7448255711 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 30 Sep 2024 13:18:22 -0400 Subject: [PATCH 6/8] temp: fix it so that paste happens in one version instead of making two --- .../core/djangoapps/content_libraries/api.py | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 97eaffed7e9c..9ae2713eeef9 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -744,9 +744,6 @@ def set_library_block_olx(usage_key, new_olx_str): # because this old pylint can't understand attr.ib() objects, pylint: disable=no-member assert isinstance(usage_key, LibraryUsageLocatorV2) - # Make sure the block exists: - _block_metadata = get_library_block(usage_key) - # Verify that the OLX parses, at least as generic XML, and the root tag is correct: node = etree.fromstring(new_olx_str) if node.tag != usage_key.block_type: @@ -776,7 +773,7 @@ def set_library_block_olx(usage_key, new_olx_str): text=new_olx_str, created=now, ) - authoring_api.create_next_version( + component_version = authoring_api.create_next_component_version( component.pk, title=new_title, content_to_replace={ @@ -792,6 +789,8 @@ def set_library_block_olx(usage_key, new_olx_str): ) ) + return component_version + def validate_can_add_block_to_library( library_key: LibraryLocatorV2, @@ -907,11 +906,23 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use # Create component for block then populate it with clipboard data with transaction.atomic(): - component_version = _create_component_for_block( - content_library, - usage_key, - user.id, + # First create the Component, but do not initialize it to anything (i.e. + # no ComponentVersion). + component_type = authoring_api.get_or_create_component_type( + "xblock.v1", usage_key.block_type ) + component = authoring_api.create_component( + content_library.learning_package.id, + component_type=component_type, + local_key=usage_key.block_id, + created=now, + created_by=user.id, + ) + + # This will create the first component version and set the OLX/title + # appropriately. It will not publish. Once we get the newly created + # ComponentVersion back from this, we can attach all our files to it. + component_version = set_library_block_olx(usage_key, olx_str) for staged_content_file_data in staged_content_files: # The ``data`` attribute is going to be None because the clipboard @@ -962,12 +973,6 @@ def import_staged_content_from_user_clipboard(library_key: LibraryLocatorV2, use learner_downloadable=True, ) - # Right now the ordering of this is important and set_library_block_olx - # makes a new version (so pasting makes two version instead of one). - # Get this more cleanly refactored later. - set_library_block_olx(usage_key, olx_str) - - # Emit library block created event LIBRARY_BLOCK_CREATED.send_event( library_block=LibraryBlockData( From 4013341305a640510a6bf5f99c9b70300a23ace8 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 2 Oct 2024 16:28:09 -0400 Subject: [PATCH 7/8] feat: pasting from libraries into courses works now, and will created nested directories --- cms/djangoapps/contentstore/helpers.py | 100 +++++++++++++++---- common/djangoapps/static_replace/__init__.py | 1 + common/djangoapps/static_replace/services.py | 1 - lms/urls.py | 1 + 4 files changed, 82 insertions(+), 21 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index a4ece6c85d59..ff1f27ed81bf 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 @@ -270,8 +271,8 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> if not user_clipboard: # 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): @@ -287,16 +288,40 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> copied_from_block=str(user_clipboard.source_usage_key), 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 +def _generate_usage_key(block_type: str) -> UsageKey: + + pass + + def _import_xml_node_to_parent( node, parent_xblock: XBlock, @@ -415,7 +440,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 @@ -427,17 +453,30 @@ 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: @@ -445,25 +484,40 @@ 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: @@ -471,22 +525,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/common/djangoapps/static_replace/__init__.py b/common/djangoapps/static_replace/__init__.py index 893349e76c27..310ad8343242 100644 --- a/common/djangoapps/static_replace/__init__.py +++ b/common/djangoapps/static_replace/__init__.py @@ -172,6 +172,7 @@ def replace_static_urls( xblock: xblock where the static assets are stored lookup_url_func: Lookup function which returns the correct path of the asset """ + if static_paths_out is None: static_paths_out = [] diff --git a/common/djangoapps/static_replace/services.py b/common/djangoapps/static_replace/services.py index 27caa0c1f2b4..80c284a9b526 100644 --- a/common/djangoapps/static_replace/services.py +++ b/common/djangoapps/static_replace/services.py @@ -51,7 +51,6 @@ def replace_urls(self, text, static_replace_only=False): static_replace_only: If True, only static urls will be replaced """ block = self.xblock() - if self.lookup_asset_url: text = replace_static_urls(text, xblock=block, lookup_asset_url=self.lookup_asset_url) else: diff --git a/lms/urls.py b/lms/urls.py index f47a6dc72cf2..f97bc7f0d04e 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -92,6 +92,7 @@ ), ] + urlpatterns = [ path('', branding_views.index, name='root'), # Main marketing page, or redirect to courseware From cbe1792b763ae7394cc1f20203c6e306e68121f6 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 3 Oct 2024 00:26:17 -0400 Subject: [PATCH 8/8] feat: add import_path and expanded_path to asset metadata JSON --- cms/djangoapps/contentstore/asset_storage_handlers.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/asset_storage_handlers.py b/cms/djangoapps/contentstore/asset_storage_handlers.py index 281258e03a8d..d9901202d8ed 100644 --- a/cms/djangoapps/contentstore/asset_storage_handlers.py +++ b/cms/djangoapps/contentstore/asset_storage_handlers.py @@ -467,6 +467,7 @@ def _get_assets_in_json_format(assets, course_key, assets_usage_locations_map): course_key, asset_file_size, usage_locations, + asset['import_path'], ) assets_in_json_format.append(asset_in_json) @@ -709,7 +710,7 @@ def _delete_thumbnail(thumbnail_location, course_key, asset_key): # lint-amnest def get_asset_json(display_name, content_type, date, location, thumbnail_location, - locked, course_key, file_size=None, usage=None): + locked, course_key, file_size=None, usage=None, import_path=None): ''' Helper method for formatting the asset information to send to client. ''' @@ -731,4 +732,6 @@ def get_asset_json(display_name, content_type, date, location, thumbnail_locatio 'id': str(location), 'file_size': file_size, 'usage_locations': usage_locations, + 'import_path': import_path, + 'expanded_path': import_path if import_path else display_name, }