From a730dad8fcda35e5675da376715c5d480a337dff Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Fri, 24 Jan 2025 22:52:46 -0300 Subject: [PATCH 1/7] fix: static assets in problem bank and library content block --- cms/djangoapps/contentstore/helpers.py | 43 +++++ .../rest_api/v2/views/downstreams.py | 10 +- .../xblock_storage_handlers/view_handlers.py | 5 +- cms/lib/xblock/upstream_sync.py | 3 +- .../core/djangoapps/content_staging/api.py | 161 +++++++++++++----- .../core/djangoapps/content_staging/data.py | 16 ++ .../migrations/0006_userlibrarysync.py | 25 +++ .../core/djangoapps/content_staging/models.py | 42 ++++- .../djangoapps/content_staging/serializers.py | 10 ++ 9 files changed, 272 insertions(+), 43 deletions(-) create mode 100644 openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index e40eddb6c99e..3cb371d122fc 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -323,6 +323,49 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> return new_xblock, notices +def import_staged_content_for_library_sync(new_xblock: XBlock, lib_block: XBlock, request) -> StaticFileNotices: + """ + 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 (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. + """ + if not content_staging_api: + raise RuntimeError("The required content_staging app is not installed") + library_sync = content_staging_api.save_xblock_to_user_library_sync(lib_block, request.user.id) + if not library_sync: + # expired/error/loading + return StaticFileNotices() + + store = modulestore() + with store.bulk_operations(new_xblock.scope_ids.usage_id.context_key): + # Now handle static files that need to go into Files & Uploads. + static_files = content_staging_api.get_staged_content_static_files(library_sync.content.id) + notices, substitutions = _import_files_into_course( + course_key=new_xblock.scope_ids.usage_id.context_key, + staged_content_id=library_sync.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 + + return notices + def _fetch_and_set_upstream_link( copied_from_block: str, diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 8c0d651910a6..057a21d40b48 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -56,8 +56,10 @@ "ready_to_sync": Boolean } """ +from dataclasses import asdict import logging +from attrs import asdict as attrs_asdict from django.contrib.auth.models import User # pylint: disable=imported-auth-user from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey @@ -71,6 +73,7 @@ UpstreamLink, UpstreamLinkException, NoUpstream, BadUpstream, BadDownstream, fetch_customizable_fields, sync_from_upstream, decline_sync, sever_upstream_link ) +from cms.djangoapps.contentstore.helpers import import_staged_content_for_library_sync from common.djangoapps.student.auth import has_studio_write_access, has_studio_read_access from openedx.core.lib.api.view_utils import ( DeveloperErrorViewMixin, @@ -195,7 +198,8 @@ def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respons """ downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) try: - sync_from_upstream(downstream, request.user) + upstream = sync_from_upstream(downstream, request.user) + static_file_notices = import_staged_content_for_library_sync(downstream, upstream, request) except UpstreamLinkException as exc: logger.exception( "Could not sync from upstream '%s' to downstream '%s'", @@ -206,7 +210,9 @@ def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respons modulestore().update_item(downstream, request.user.id) # Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the # upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen. - return Response(UpstreamLink.get_for_block(downstream).to_json()) + response = UpstreamLink.get_for_block(downstream).to_json() + response["static_file_notices"] = attrs_asdict(static_file_notices) + return Response(response) def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response: """ diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 3b9ba5798a01..c30b7ad44acf 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -80,6 +80,7 @@ from ..helpers import ( get_parent_xblock, import_staged_content_from_user_clipboard, + import_staged_content_for_library_sync, is_unit, xblock_embed_lms_url, xblock_lms_url, @@ -598,7 +599,7 @@ def _create_block(request): try: # 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) + lib_block = sync_from_upstream(downstream=created_block, user=request.user) except BadUpstream as exc: _delete_item(created_block.location, request.user) log.exception( @@ -606,8 +607,10 @@ def _create_block(request): f"using provided library_content_key='{upstream_ref}'" ) return JsonResponse({"error": str(exc)}, status=400) + static_file_notices = import_staged_content_for_library_sync(created_block, lib_block, request) modulestore().update_item(created_block, request.user.id) response['upstreamRef'] = upstream_ref + response['static_file_notices'] = asdict(static_file_notices) return JsonResponse(response) diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 0d95931ce29d..418b95a39fe3 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -186,7 +186,7 @@ def get_for_block(cls, downstream: XBlock) -> t.Self: ) -def sync_from_upstream(downstream: XBlock, user: User) -> None: +def sync_from_upstream(downstream: XBlock, user: User) -> XBlock: """ Update `downstream` with content+settings from the latest available version of its linked upstream content. @@ -200,6 +200,7 @@ def sync_from_upstream(downstream: XBlock, user: User) -> None: _update_non_customizable_fields(upstream=upstream, downstream=downstream) _update_tags(upstream=upstream, downstream=downstream) downstream.upstream_version = link.version_available + return upstream def fetch_customizable_fields(*, downstream: XBlock, user: User, upstream: XBlock | None = None) -> None: diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 5f85d701faa5..c5ba2e824d95 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -21,22 +21,33 @@ from .data import ( CLIPBOARD_PURPOSE, - StagedContentData, StagedContentFileData, StagedContentStatus, UserClipboardData + LIBRARY_SYNC_PURPOSE, + StagedContentData, StagedContentFileData, StagedContentStatus, UserClipboardData, UserLibrarySyncData, ) from .models import ( UserClipboard as _UserClipboard, StagedContent as _StagedContent, StagedContentFile as _StagedContentFile, + UserLibrarySync as _UserLibrarySync, +) +from .serializers import ( + UserClipboardSerializer as _UserClipboardSerializer, + UserLibrarySyncSerializer as _UserLibrarySyncSerializer, ) -from .serializers import UserClipboardSerializer as _UserClipboardSerializer from .tasks import delete_expired_clipboards log = logging.getLogger(__name__) -def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int | None = None) -> UserClipboardData: +def _save_xblock_to_staged_content( + block: XBlock, + user_id: int, + purpose: str, + version_num: int | None = None +) -> _StagedContent: """ - Copy an XBlock's OLX to the user's clipboard. + Generic function to save an XBlock's OLX to staged content. + Used by both clipboard and library sync functionality. """ block_data = XBlockSerializer( block, @@ -49,7 +60,7 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int # Mark all of the user's existing StagedContent rows as EXPIRED to_expire = _StagedContent.objects.filter( user_id=user_id, - purpose=CLIPBOARD_PURPOSE, + purpose=purpose, ).exclude( status=StagedContentStatus.EXPIRED, ) @@ -60,7 +71,7 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int # Insert a new StagedContent row for this staged_content = _StagedContent.objects.create( user_id=user_id, - purpose=CLIPBOARD_PURPOSE, + purpose=purpose, status=StagedContentStatus.READY, block_type=usage_key.block_type, olx=block_data.olx_str, @@ -69,23 +80,16 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int tags=block_data.tags or {}, version_num=(version_num or 0), ) - (clipboard, _created) = _UserClipboard.objects.update_or_create(user_id=user_id, defaults={ - "content": staged_content, - "source_usage_key": usage_key, - }) # Log an event so we can analyze how this feature is used: - log.info(f"Copied {usage_key.block_type} component \"{usage_key}\" to their clipboard.") + log.info(f"Saved {usage_key.block_type} component \"{usage_key}\" to staged content for {purpose}.") - # Try to copy the static files. If this fails, we still consider the overall copy attempt to have succeeded, - # because intra-course pasting will still work fine, and in any case users can manually resolve the file issue. + # Try to copy the static files. If this fails, we still consider the overall save attempt to have succeeded, + # because intra-course operations will still work fine, and users can manually resolve file issues. try: - _save_static_assets_to_user_clipboard(block_data.static_files, usage_key, staged_content) + _save_static_assets_to_staged_content(block_data.static_files, usage_key, staged_content) except Exception: # pylint: disable=broad-except - # Regardless of what happened, with get_asset_key_from_path or contentstore or run_filter, we don't want the - # whole "copy to clipboard" operation to fail, which would be a bad user experience. For copying and pasting - # within a single course, static assets don't even matter. So any such errors become warnings here. - log.exception(f"Unable to copy static files to clipboard for component {usage_key}") + log.exception(f"Unable to copy static files to staged content for component {usage_key}") # Enqueue a (potentially slow) task to delete the old staged content try: @@ -93,14 +97,15 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int except Exception: # pylint: disable=broad-except log.exception(f"Unable to enqueue cleanup task for StagedContents: {','.join(str(x) for x in expired_ids)}") - return _user_clipboard_model_to_data(clipboard) + return staged_content -def _save_static_assets_to_user_clipboard( +def _save_static_assets_to_staged_content( static_files: list[StaticFile], usage_key: UsageKey, staged_content: _StagedContent ): """ - Helper method for save_xblock_to_user_clipboard endpoint. This deals with copying static files into the clipboard. + Helper method for saving static files into staged content. + Used by both clipboard and library sync functionality. """ for f in static_files: source_key = ( @@ -144,6 +149,46 @@ def _save_static_assets_to_user_clipboard( log.exception(f"Unable to copy static file {f.name} to clipboard for component {usage_key}") +def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int | None = None) -> UserClipboardData: + """ + Copy an XBlock's OLX to the user's clipboard. + """ + staged_content = _save_xblock_to_staged_content(block, user_id, CLIPBOARD_PURPOSE, version_num) + usage_key = block.usage_key + + # Create/update the clipboard entry + (clipboard, _created) = _UserClipboard.objects.update_or_create( + user_id=user_id, + defaults={ + "content": staged_content, + "source_usage_key": usage_key, + } + ) + + return _user_clipboard_model_to_data(clipboard) + +def save_xblock_to_user_library_sync( + block: XBlock, + user_id: int, + version_num: int | None = None +) -> UserLibrarySyncData: + """ + Save an XBlock's OLX for library sync. + """ + staged_content = _save_xblock_to_staged_content(block, user_id, LIBRARY_SYNC_PURPOSE, version_num) + usage_key = block.usage_key + + # Create/update the library sync entry + (sync, _created) = _UserLibrarySync.objects.update_or_create( + user_id=user_id, + defaults={ + "content": staged_content, + "source_usage_key": usage_key, + } + ) + + return _user_library_sync_model_to_data(sync) + def get_user_clipboard(user_id: int, only_ready: bool = True) -> UserClipboardData | None: """ Get the details of the user's clipboard. @@ -190,32 +235,72 @@ def get_user_clipboard_json(user_id: int, request: HttpRequest | None = None): return serializer.data +def get_user_library_sync_json(user_id: int, request: HttpRequest | None = None): + """ + Get the detailed status of the user's library sync, in exactly the same format + as returned from the + /api/content-staging/v1/library-sync/ + REST API endpoint. This version of the API is meant for "preloading" that + REST API endpoint so it can be embedded in a larger response sent to the + user's browser. + + (request is optional; including it will make the "olx_url" absolute instead + of relative.) + """ + try: + sync = _UserLibrarySync.objects.get(user_id=user_id) + except _UserLibrarySync.DoesNotExist: + # This user does not have any library sync content. + return { + "content": None, + "source_usage_key": "", + "source_context_title": "", + "source_edit_url": "" + } + + serializer = _UserLibrarySyncSerializer( + _user_library_sync_model_to_data(sync), + context={'request': request}, + ) + return serializer.data + + +def _staged_content_to_data(content: _StagedContent) -> StagedContentData: + """ + Convert a StagedContent model instance to an immutable data object. + """ + return StagedContentData( + id=content.id, + user_id=content.user_id, + created=content.created, + purpose=content.purpose, + status=content.status, + block_type=content.block_type, + display_name=content.display_name, + tags=content.tags or {}, + version_num=content.version_num, + ) + def _user_clipboard_model_to_data(clipboard: _UserClipboard) -> UserClipboardData: """ Convert a UserClipboard model instance to an immutable data object. """ - content = clipboard.content - source_context_key = clipboard.source_usage_key.context_key - if source_context_key.is_course and (course_overview := get_course_overview_or_none(source_context_key)): - source_context_title = course_overview.display_name_with_default - else: - source_context_title = str(source_context_key) # Fall back to stringified context key as a title return UserClipboardData( - content=StagedContentData( - id=content.id, - user_id=content.user_id, - created=content.created, - purpose=content.purpose, - status=content.status, - block_type=content.block_type, - display_name=content.display_name, - tags=content.tags or {}, - version_num=content.version_num, - ), + content=_staged_content_to_data(clipboard.content), source_usage_key=clipboard.source_usage_key, source_context_title=clipboard.get_source_context_title(), ) +def _user_library_sync_model_to_data(sync: _UserLibrarySync) -> UserLibrarySyncData: + """ + Convert a UserLibrarySync model instance to an immutable data object. + """ + return UserLibrarySyncData( + content=_staged_content_to_data(sync.content), + source_usage_key=sync.source_usage_key, + source_context_title=sync.get_source_context_title(), + ) + def get_staged_content_olx(staged_content_id: int) -> str | None: """ diff --git a/openedx/core/djangoapps/content_staging/data.py b/openedx/core/djangoapps/content_staging/data.py index d077d05a0aa4..64b5ef3eec1f 100644 --- a/openedx/core/djangoapps/content_staging/data.py +++ b/openedx/core/djangoapps/content_staging/data.py @@ -25,6 +25,10 @@ class StagedContentStatus(TextChoices): # Value of the "purpose" field on StagedContent objects used for clipboards. CLIPBOARD_PURPOSE = "clipboard" + +# Value of the "purpose" field on StagedContent objects used for library to course sync. +LIBRARY_SYNC_PURPOSE = "library_sync" + # There may be other valid values of "purpose" which aren't defined within this app. @@ -72,3 +76,15 @@ class UserClipboardData: def source_context_key(self) -> LearningContextKey: """ Get the context (course/library) that this was copied from """ return self.source_usage_key.context_key + +@frozen +class UserLibrarySyncData: + """ Read-only data model for User Library Sync data """ + content: StagedContentData = field(validator=validators.instance_of(StagedContentData)) + source_usage_key: UsageKey = field(validator=validators.instance_of(UsageKey)) + source_context_title: str + + @property + def source_context_key(self) -> LearningContextKey: + """ Get the context (course/library) that this was copied from """ + return self.source_usage_key.context_key diff --git a/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py b/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py new file mode 100644 index 000000000000..1a0e3dd3c3c6 --- /dev/null +++ b/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py @@ -0,0 +1,25 @@ +# Generated by Django 4.2.17 on 2025-01-25 01:10 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import opaque_keys.edx.django.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('auth', '0012_alter_user_first_name_max_length'), + ('content_staging', '0005_stagedcontent_version_num'), + ] + + operations = [ + migrations.CreateModel( + name='UserLibrarySync', + fields=[ + ('user', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to=settings.AUTH_USER_MODEL)), + ('source_usage_key', opaque_keys.edx.django.models.UsageKeyField(help_text='Original usage key/ID of the thing that is being synced.', max_length=255)), + ('content', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='content_staging.stagedcontent')), + ], + ), + ] diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 5e007bc4485a..42695898732f 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -14,7 +14,7 @@ from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none -from .data import CLIPBOARD_PURPOSE, StagedContentStatus +from .data import CLIPBOARD_PURPOSE, LIBRARY_SYNC_PURPOSE, StagedContentStatus log = logging.getLogger(__name__) @@ -144,3 +144,43 @@ def save(self, *args, **kwargs): # Enforce checks on save: self.full_clean() return super().save(*args, **kwargs) + + +class UserLibrarySync(models.Model): + """ + Each user can trigger a sync from a library component to that component in a course. + This model is used to facilitate that and to ease tracking. + """ + user = models.OneToOneField(User, on_delete=models.CASCADE, primary_key=True) + content = models.ForeignKey(StagedContent, on_delete=models.CASCADE) + source_usage_key = UsageKeyField( + max_length=255, + help_text=_("Original usage key/ID of the thing that is being synced."), + ) + + + @property + def source_context_key(self) -> LearningContextKey: + """ Get the context (library) that this was copied from """ + return self.source_usage_key.context_key + + def get_source_context_title(self) -> str: + """ Get the title of the source context, if any """ + # Just return the ID as the name, since it can only be a library + return str(self.source_context_key) + + def clean(self): + """ Check that this model is being used correctly. """ + # These could probably be replaced with constraints in Django 4.1+ + if self.user.id != self.content.user.id: + raise ValidationError("User ID mismatch.") + if self.content.purpose != LIBRARY_SYNC_PURPOSE: + raise ValidationError( + f"StagedContent.purpose must be '{CLIPBOARD_PURPOSE}' to use it as clipboard content." + ) + + def save(self, *args, **kwargs): + """ Save this model instance """ + # Enforce checks on save: + self.full_clean() + return super().save(*args, **kwargs) diff --git a/openedx/core/djangoapps/content_staging/serializers.py b/openedx/core/djangoapps/content_staging/serializers.py index fff9ff316e6f..e37fb0078e3e 100644 --- a/openedx/core/djangoapps/content_staging/serializers.py +++ b/openedx/core/djangoapps/content_staging/serializers.py @@ -77,3 +77,13 @@ class PostToClipboardSerializer(serializers.Serializer): user's clipboard. """ usage_key = serializers.CharField(help_text="Usage key to copy into the clipboard") + + +class UserLibrarySyncSerializer(serializers.Serializer): + """ + Serializer for the status of the user's library sync (a UserLibrarySyncData instance) + """ + content = StagedContentSerializer(allow_null=True) + source_usage_key = serializers.CharField(allow_blank=True) + # The title of the course that the content came from originally, if relevant + source_context_title = serializers.CharField(allow_blank=True) From f12ff12c706ea1770bd80afee7b5320f36e12e41 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Mon, 27 Jan 2025 09:20:28 -0300 Subject: [PATCH 2/7] fix: linter and tests --- cms/djangoapps/contentstore/helpers.py | 3 +- .../rest_api/v2/views/downstreams.py | 2 +- .../v2/views/tests/test_downstreams.py | 5 ++- .../xblock_storage_handlers/view_handlers.py | 4 +-- .../core/djangoapps/content_staging/api.py | 35 +++++++++---------- .../core/djangoapps/content_staging/data.py | 8 +++-- .../migrations/0006_userlibrarysync.py | 28 +++++++++++---- .../core/djangoapps/content_staging/models.py | 10 +++--- .../djangoapps/content_staging/serializers.py | 1 + 9 files changed, 58 insertions(+), 38 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 3cb371d122fc..10ddf984b4b8 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -323,6 +323,7 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> return new_xblock, notices + def import_staged_content_for_library_sync(new_xblock: XBlock, lib_block: XBlock, request) -> StaticFileNotices: """ Import a block (along with its children and any required static assets) from @@ -355,7 +356,7 @@ def import_staged_content_for_library_sync(new_xblock: XBlock, lib_block: XBlock # 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: + 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( diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 057a21d40b48..da92fb464788 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -56,7 +56,7 @@ "ready_to_sync": Boolean } """ -from dataclasses import asdict + import logging from attrs import asdict as attrs_asdict diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index 92da28bde989..f0e3dfc8e1a4 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -4,6 +4,7 @@ from unittest.mock import patch from django.conf import settings +from cms.djangoapps.contentstore.helpers import StaticFileNotices from cms.lib.xblock.upstream_sync import UpstreamLink, BadUpstream from common.djangoapps.student.tests.factories import UserFactory from xmodule.modulestore.django import modulestore @@ -247,7 +248,8 @@ def call_api(self, usage_key_string): @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) @patch.object(downstreams_views, "sync_from_upstream") - def test_200(self, mock_sync_from_upstream): + @patch.object(downstreams_views, "import_staged_content_for_library_sync", return_value=StaticFileNotices()) + def test_200(self, mock_sync_from_upstream, mock_import_staged_content): """ Does the happy path work? """ @@ -255,6 +257,7 @@ def test_200(self, mock_sync_from_upstream): response = self.call_api(self.downstream_video_key) assert response.status_code == 200 assert mock_sync_from_upstream.call_count == 1 + assert mock_import_staged_content.call_count == 1 class DeleteDownstreamSyncViewtest(_DownstreamSyncViewTestMixin, SharedModuleStoreTestCase): diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index c30b7ad44acf..8d10858b4e40 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -609,8 +609,8 @@ def _create_block(request): return JsonResponse({"error": str(exc)}, status=400) static_file_notices = import_staged_content_for_library_sync(created_block, lib_block, request) modulestore().update_item(created_block, request.user.id) - response['upstreamRef'] = upstream_ref - response['static_file_notices'] = asdict(static_file_notices) + response["upstreamRef"] = upstream_ref + response["static_file_notices"] = asdict(static_file_notices) return JsonResponse(response) diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index c5ba2e824d95..8ebdc1c9f243 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -14,7 +14,6 @@ from xblock.core import XBlock from openedx.core.lib.xblock_serializer.api import StaticFile, XBlockSerializer -from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none from xmodule import block_metadata_utils from xmodule.contentstore.content import StaticContent from xmodule.contentstore.django import contentstore @@ -22,7 +21,11 @@ from .data import ( CLIPBOARD_PURPOSE, LIBRARY_SYNC_PURPOSE, - StagedContentData, StagedContentFileData, StagedContentStatus, UserClipboardData, UserLibrarySyncData, + StagedContentData, + StagedContentFileData, + StagedContentStatus, + UserClipboardData, + UserLibrarySyncData, ) from .models import ( UserClipboard as _UserClipboard, @@ -40,10 +43,7 @@ def _save_xblock_to_staged_content( - block: XBlock, - user_id: int, - purpose: str, - version_num: int | None = None + block: XBlock, user_id: int, purpose: str, version_num: int | None = None ) -> _StagedContent: """ Generic function to save an XBlock's OLX to staged content. @@ -82,7 +82,7 @@ def _save_xblock_to_staged_content( ) # Log an event so we can analyze how this feature is used: - log.info(f"Saved {usage_key.block_type} component \"{usage_key}\" to staged content for {purpose}.") + log.info(f'Saved {usage_key.block_type} component "{usage_key}" to staged content for {purpose}.') # Try to copy the static files. If this fails, we still consider the overall save attempt to have succeeded, # because intra-course operations will still work fine, and users can manually resolve file issues. @@ -162,15 +162,14 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int defaults={ "content": staged_content, "source_usage_key": usage_key, - } + }, ) return _user_clipboard_model_to_data(clipboard) + def save_xblock_to_user_library_sync( - block: XBlock, - user_id: int, - version_num: int | None = None + block: XBlock, user_id: int, version_num: int | None = None ) -> UserLibrarySyncData: """ Save an XBlock's OLX for library sync. @@ -184,11 +183,12 @@ def save_xblock_to_user_library_sync( defaults={ "content": staged_content, "source_usage_key": usage_key, - } + }, ) return _user_library_sync_model_to_data(sync) + def get_user_clipboard(user_id: int, only_ready: bool = True) -> UserClipboardData | None: """ Get the details of the user's clipboard. @@ -251,16 +251,11 @@ def get_user_library_sync_json(user_id: int, request: HttpRequest | None = None) sync = _UserLibrarySync.objects.get(user_id=user_id) except _UserLibrarySync.DoesNotExist: # This user does not have any library sync content. - return { - "content": None, - "source_usage_key": "", - "source_context_title": "", - "source_edit_url": "" - } + return {"content": None, "source_usage_key": "", "source_context_title": "", "source_edit_url": ""} serializer = _UserLibrarySyncSerializer( _user_library_sync_model_to_data(sync), - context={'request': request}, + context={"request": request}, ) return serializer.data @@ -281,6 +276,7 @@ def _staged_content_to_data(content: _StagedContent) -> StagedContentData: version_num=content.version_num, ) + def _user_clipboard_model_to_data(clipboard: _UserClipboard) -> UserClipboardData: """ Convert a UserClipboard model instance to an immutable data object. @@ -291,6 +287,7 @@ def _user_clipboard_model_to_data(clipboard: _UserClipboard) -> UserClipboardDat source_context_title=clipboard.get_source_context_title(), ) + def _user_library_sync_model_to_data(sync: _UserLibrarySync) -> UserLibrarySyncData: """ Convert a UserLibrarySync model instance to an immutable data object. diff --git a/openedx/core/djangoapps/content_staging/data.py b/openedx/core/djangoapps/content_staging/data.py index 64b5ef3eec1f..4d535c9fcfcf 100644 --- a/openedx/core/djangoapps/content_staging/data.py +++ b/openedx/core/djangoapps/content_staging/data.py @@ -77,14 +77,16 @@ def source_context_key(self) -> LearningContextKey: """ Get the context (course/library) that this was copied from """ return self.source_usage_key.context_key + @frozen class UserLibrarySyncData: - """ Read-only data model for User Library Sync data """ + """Read-only data model for User Library Sync data""" + content: StagedContentData = field(validator=validators.instance_of(StagedContentData)) - source_usage_key: UsageKey = field(validator=validators.instance_of(UsageKey)) + source_usage_key: UsageKey = field(validator=validators.instance_of(UsageKey)) # type: ignore[type-abstract] source_context_title: str @property def source_context_key(self) -> LearningContextKey: - """ Get the context (course/library) that this was copied from """ + """Get the context (course/library) that this was copied from""" return self.source_usage_key.context_key diff --git a/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py b/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py index 1a0e3dd3c3c6..99ee67b18892 100644 --- a/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py +++ b/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py @@ -9,17 +9,33 @@ class Migration(migrations.Migration): dependencies = [ - ('auth', '0012_alter_user_first_name_max_length'), - ('content_staging', '0005_stagedcontent_version_num'), + ("auth", "0012_alter_user_first_name_max_length"), + ("content_staging", "0005_stagedcontent_version_num"), ] operations = [ migrations.CreateModel( - name='UserLibrarySync', + name="UserLibrarySync", fields=[ - ('user', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to=settings.AUTH_USER_MODEL)), - ('source_usage_key', opaque_keys.edx.django.models.UsageKeyField(help_text='Original usage key/ID of the thing that is being synced.', max_length=255)), - ('content', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='content_staging.stagedcontent')), + ( + "user", + models.OneToOneField( + on_delete=django.db.models.deletion.CASCADE, + primary_key=True, + serialize=False, + to=settings.AUTH_USER_MODEL, + ), + ), + ( + "source_usage_key", + opaque_keys.edx.django.models.UsageKeyField( + help_text="Original usage key/ID of the thing that is being synced.", max_length=255 + ), + ), + ( + "content", + models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="content_staging.stagedcontent"), + ), ], ), ] diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 42695898732f..65e8c8326a44 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -151,6 +151,7 @@ class UserLibrarySync(models.Model): Each user can trigger a sync from a library component to that component in a course. This model is used to facilitate that and to ease tracking. """ + user = models.OneToOneField(User, on_delete=models.CASCADE, primary_key=True) content = models.ForeignKey(StagedContent, on_delete=models.CASCADE) source_usage_key = UsageKeyField( @@ -158,19 +159,18 @@ class UserLibrarySync(models.Model): help_text=_("Original usage key/ID of the thing that is being synced."), ) - @property def source_context_key(self) -> LearningContextKey: - """ Get the context (library) that this was copied from """ + """Get the context (library) that this was copied from""" return self.source_usage_key.context_key def get_source_context_title(self) -> str: - """ Get the title of the source context, if any """ + """Get the title of the source context, if any""" # Just return the ID as the name, since it can only be a library return str(self.source_context_key) def clean(self): - """ Check that this model is being used correctly. """ + """Check that this model is being used correctly.""" # These could probably be replaced with constraints in Django 4.1+ if self.user.id != self.content.user.id: raise ValidationError("User ID mismatch.") @@ -180,7 +180,7 @@ def clean(self): ) def save(self, *args, **kwargs): - """ Save this model instance """ + """Save this model instance""" # Enforce checks on save: self.full_clean() return super().save(*args, **kwargs) diff --git a/openedx/core/djangoapps/content_staging/serializers.py b/openedx/core/djangoapps/content_staging/serializers.py index e37fb0078e3e..ca1847ca854c 100644 --- a/openedx/core/djangoapps/content_staging/serializers.py +++ b/openedx/core/djangoapps/content_staging/serializers.py @@ -83,6 +83,7 @@ class UserLibrarySyncSerializer(serializers.Serializer): """ Serializer for the status of the user's library sync (a UserLibrarySyncData instance) """ + content = StagedContentSerializer(allow_null=True) source_usage_key = serializers.CharField(allow_blank=True) # The title of the course that the content came from originally, if relevant From 5a9b68ff9e2990657f5af042972b21cdd3dd0de4 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Wed, 29 Jan 2025 09:10:32 -0300 Subject: [PATCH 3/7] feat: remove user library sync --- cms/djangoapps/contentstore/helpers.py | 99 +++++++++---------- .../core/djangoapps/content_staging/api.py | 64 ++---------- .../core/djangoapps/content_staging/data.py | 14 --- .../migrations/0006_userlibrarysync.py | 41 -------- .../core/djangoapps/content_staging/models.py | 40 -------- .../djangoapps/content_staging/serializers.py | 11 --- 6 files changed, 57 insertions(+), 212 deletions(-) delete mode 100644 openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 10ddf984b4b8..31d5a17d0708 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -262,6 +262,36 @@ class StaticFileNotices: error_files: list[str] = Factory(list) +def _insert_static_files_into_downstream_xblock(downstream_xblock: XBlock, staged_content_id: int, request) -> StaticFileNotices: + """ + Gets static files from staged content, and inserts them into the downstream XBlock. + """ + static_files = content_staging_api.get_staged_content_static_files(staged_content_id) + notices, substitutions = _import_files_into_course( + course_key=downstream_xblock.context_key, + staged_content_id=staged_content_id, + static_files=static_files, + usage_key=downstream_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. + store = modulestore() + if hasattr(downstream_xblock, "data") and substitutions: + data_with_substitutions = downstream_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, + ) + downstream_xblock.data = data_with_substitutions + if store is not None: + store.update_item(downstream_xblock, request.user.id) + return notices + + def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> tuple[XBlock | None, StaticFileNotices]: """ Import a block (along with its children and any required static assets) from @@ -299,71 +329,37 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> tags=user_clipboard.content.tags, ) - # 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) + notices = _insert_static_files_into_downstream_xblock(new_xblock, user_clipboard.content.id, request) return new_xblock, notices -def import_staged_content_for_library_sync(new_xblock: XBlock, lib_block: XBlock, request) -> StaticFileNotices: +def import_staged_content_for_library_sync(downstream_xblock: XBlock, lib_block: XBlock, request) -> StaticFileNotices: """ - Import a block (along with its children and any required static assets) from - the "staged" OLX in the user's clipboard. + Import the static assets from the library xblock to the downstream xblock + through staged content. Also updates the OLX references to point to the new + locations of those assets in the downstream course. Does not deal with permissions or REST stuff - do that before calling this. - 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 + Returns a summary of changes made to static files in the destination course. """ if not content_staging_api: raise RuntimeError("The required content_staging app is not installed") - library_sync = content_staging_api.save_xblock_to_user_library_sync(lib_block, request.user.id) - if not library_sync: + staged_content = content_staging_api.stage_xblock_temporarily(lib_block, request.user.id) + if not staged_content: # expired/error/loading return StaticFileNotices() store = modulestore() - with store.bulk_operations(new_xblock.scope_ids.usage_id.context_key): + with store.bulk_operations(downstream_xblock.context_key): # Now handle static files that need to go into Files & Uploads. - static_files = content_staging_api.get_staged_content_static_files(library_sync.content.id) - notices, substitutions = _import_files_into_course( - course_key=new_xblock.scope_ids.usage_id.context_key, - staged_content_id=library_sync.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 + # If the required files already exist, nothing will happen besides updating the olx. + try: + notices = _insert_static_files_into_downstream_xblock(downstream_xblock, staged_content.id, request) + finally: + staged_content.delete() return notices @@ -592,6 +588,9 @@ def _import_files_into_course( if result is True: new_files.append(file_data_obj.filename) substitutions.update(substitution_for_file) + elif substitution_for_file: + # substitutions need to be made because OLX references to these files need to be updated + substitutions.update(substitution_for_file) elif result is None: pass # This file already exists; no action needed. else: @@ -662,8 +661,8 @@ def _import_file_into_course( contentstore().save(content) 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, {} + # The file already exists and matches exactly, so no action is needed except substitutions + return None, {clipboard_file_path: f"static/{import_path}"} else: # There is a conflict with some other file that has the same name. return False, {} diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 8ebdc1c9f243..4e5e11eddb3e 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -25,17 +25,14 @@ StagedContentFileData, StagedContentStatus, UserClipboardData, - UserLibrarySyncData, ) from .models import ( UserClipboard as _UserClipboard, StagedContent as _StagedContent, StagedContentFile as _StagedContentFile, - UserLibrarySync as _UserLibrarySync, ) from .serializers import ( UserClipboardSerializer as _UserClipboardSerializer, - UserLibrarySyncSerializer as _UserLibrarySyncSerializer, ) from .tasks import delete_expired_clipboards @@ -168,25 +165,16 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int return _user_clipboard_model_to_data(clipboard) -def save_xblock_to_user_library_sync( - block: XBlock, user_id: int, version_num: int | None = None -) -> UserLibrarySyncData: +def stage_xblock_temporarily( + block: XBlock, user_id: int, purpose: str = LIBRARY_SYNC_PURPOSE, version_num: int | None = None, +) -> _StagedContent: """ - Save an XBlock's OLX for library sync. + "Stage" an XBlock by copying it (and its associated children + static assets) + into the content staging area. This XBlock can then be accessed and manipulated + using any of the staged content APIs, before being deleted. """ - staged_content = _save_xblock_to_staged_content(block, user_id, LIBRARY_SYNC_PURPOSE, version_num) - usage_key = block.usage_key - - # Create/update the library sync entry - (sync, _created) = _UserLibrarySync.objects.update_or_create( - user_id=user_id, - defaults={ - "content": staged_content, - "source_usage_key": usage_key, - }, - ) - - return _user_library_sync_model_to_data(sync) + staged_content = _save_xblock_to_staged_content(block, user_id, purpose, version_num) + return staged_content def get_user_clipboard(user_id: int, only_ready: bool = True) -> UserClipboardData | None: @@ -235,31 +223,6 @@ def get_user_clipboard_json(user_id: int, request: HttpRequest | None = None): return serializer.data -def get_user_library_sync_json(user_id: int, request: HttpRequest | None = None): - """ - Get the detailed status of the user's library sync, in exactly the same format - as returned from the - /api/content-staging/v1/library-sync/ - REST API endpoint. This version of the API is meant for "preloading" that - REST API endpoint so it can be embedded in a larger response sent to the - user's browser. - - (request is optional; including it will make the "olx_url" absolute instead - of relative.) - """ - try: - sync = _UserLibrarySync.objects.get(user_id=user_id) - except _UserLibrarySync.DoesNotExist: - # This user does not have any library sync content. - return {"content": None, "source_usage_key": "", "source_context_title": "", "source_edit_url": ""} - - serializer = _UserLibrarySyncSerializer( - _user_library_sync_model_to_data(sync), - context={"request": request}, - ) - return serializer.data - - def _staged_content_to_data(content: _StagedContent) -> StagedContentData: """ Convert a StagedContent model instance to an immutable data object. @@ -288,17 +251,6 @@ def _user_clipboard_model_to_data(clipboard: _UserClipboard) -> UserClipboardDat ) -def _user_library_sync_model_to_data(sync: _UserLibrarySync) -> UserLibrarySyncData: - """ - Convert a UserLibrarySync model instance to an immutable data object. - """ - return UserLibrarySyncData( - content=_staged_content_to_data(sync.content), - source_usage_key=sync.source_usage_key, - source_context_title=sync.get_source_context_title(), - ) - - def get_staged_content_olx(staged_content_id: int) -> str | None: """ Get the OLX (as a string) for the given StagedContent. diff --git a/openedx/core/djangoapps/content_staging/data.py b/openedx/core/djangoapps/content_staging/data.py index 4d535c9fcfcf..d095f2506b17 100644 --- a/openedx/core/djangoapps/content_staging/data.py +++ b/openedx/core/djangoapps/content_staging/data.py @@ -76,17 +76,3 @@ class UserClipboardData: def source_context_key(self) -> LearningContextKey: """ Get the context (course/library) that this was copied from """ return self.source_usage_key.context_key - - -@frozen -class UserLibrarySyncData: - """Read-only data model for User Library Sync data""" - - content: StagedContentData = field(validator=validators.instance_of(StagedContentData)) - source_usage_key: UsageKey = field(validator=validators.instance_of(UsageKey)) # type: ignore[type-abstract] - source_context_title: str - - @property - def source_context_key(self) -> LearningContextKey: - """Get the context (course/library) that this was copied from""" - return self.source_usage_key.context_key diff --git a/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py b/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py deleted file mode 100644 index 99ee67b18892..000000000000 --- a/openedx/core/djangoapps/content_staging/migrations/0006_userlibrarysync.py +++ /dev/null @@ -1,41 +0,0 @@ -# Generated by Django 4.2.17 on 2025-01-25 01:10 - -from django.conf import settings -from django.db import migrations, models -import django.db.models.deletion -import opaque_keys.edx.django.models - - -class Migration(migrations.Migration): - - dependencies = [ - ("auth", "0012_alter_user_first_name_max_length"), - ("content_staging", "0005_stagedcontent_version_num"), - ] - - operations = [ - migrations.CreateModel( - name="UserLibrarySync", - fields=[ - ( - "user", - models.OneToOneField( - on_delete=django.db.models.deletion.CASCADE, - primary_key=True, - serialize=False, - to=settings.AUTH_USER_MODEL, - ), - ), - ( - "source_usage_key", - opaque_keys.edx.django.models.UsageKeyField( - help_text="Original usage key/ID of the thing that is being synced.", max_length=255 - ), - ), - ( - "content", - models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="content_staging.stagedcontent"), - ), - ], - ), - ] diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 65e8c8326a44..307957cc213e 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -144,43 +144,3 @@ def save(self, *args, **kwargs): # Enforce checks on save: self.full_clean() return super().save(*args, **kwargs) - - -class UserLibrarySync(models.Model): - """ - Each user can trigger a sync from a library component to that component in a course. - This model is used to facilitate that and to ease tracking. - """ - - user = models.OneToOneField(User, on_delete=models.CASCADE, primary_key=True) - content = models.ForeignKey(StagedContent, on_delete=models.CASCADE) - source_usage_key = UsageKeyField( - max_length=255, - help_text=_("Original usage key/ID of the thing that is being synced."), - ) - - @property - def source_context_key(self) -> LearningContextKey: - """Get the context (library) that this was copied from""" - return self.source_usage_key.context_key - - def get_source_context_title(self) -> str: - """Get the title of the source context, if any""" - # Just return the ID as the name, since it can only be a library - return str(self.source_context_key) - - def clean(self): - """Check that this model is being used correctly.""" - # These could probably be replaced with constraints in Django 4.1+ - if self.user.id != self.content.user.id: - raise ValidationError("User ID mismatch.") - if self.content.purpose != LIBRARY_SYNC_PURPOSE: - raise ValidationError( - f"StagedContent.purpose must be '{CLIPBOARD_PURPOSE}' to use it as clipboard content." - ) - - def save(self, *args, **kwargs): - """Save this model instance""" - # Enforce checks on save: - self.full_clean() - return super().save(*args, **kwargs) diff --git a/openedx/core/djangoapps/content_staging/serializers.py b/openedx/core/djangoapps/content_staging/serializers.py index ca1847ca854c..fff9ff316e6f 100644 --- a/openedx/core/djangoapps/content_staging/serializers.py +++ b/openedx/core/djangoapps/content_staging/serializers.py @@ -77,14 +77,3 @@ class PostToClipboardSerializer(serializers.Serializer): user's clipboard. """ usage_key = serializers.CharField(help_text="Usage key to copy into the clipboard") - - -class UserLibrarySyncSerializer(serializers.Serializer): - """ - Serializer for the status of the user's library sync (a UserLibrarySyncData instance) - """ - - content = StagedContentSerializer(allow_null=True) - source_usage_key = serializers.CharField(allow_blank=True) - # The title of the course that the content came from originally, if relevant - source_context_title = serializers.CharField(allow_blank=True) From 0b3b62c13bd74b431c8030ebcf210a4d9b21dcc2 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Wed, 29 Jan 2025 09:27:46 -0300 Subject: [PATCH 4/7] fix: linting --- cms/djangoapps/contentstore/helpers.py | 5 +++-- openedx/core/djangoapps/content_staging/models.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 31d5a17d0708..8e78a6b93c28 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -262,7 +262,9 @@ class StaticFileNotices: error_files: list[str] = Factory(list) -def _insert_static_files_into_downstream_xblock(downstream_xblock: XBlock, staged_content_id: int, request) -> StaticFileNotices: +def _insert_static_files_into_downstream_xblock( + downstream_xblock: XBlock, staged_content_id: int, request +) -> StaticFileNotices: """ Gets static files from staged content, and inserts them into the downstream XBlock. """ @@ -274,7 +276,6 @@ def _insert_static_files_into_downstream_xblock(downstream_xblock: XBlock, stage usage_key=downstream_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. diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 307957cc213e..5e007bc4485a 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -14,7 +14,7 @@ from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none -from .data import CLIPBOARD_PURPOSE, LIBRARY_SYNC_PURPOSE, StagedContentStatus +from .data import CLIPBOARD_PURPOSE, StagedContentStatus log = logging.getLogger(__name__) From 0615a893aa52c5c18f9c2de5b65eadd9e52e6c27 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Wed, 29 Jan 2025 18:05:44 -0300 Subject: [PATCH 5/7] feat: pr suggestions and do not import when no assets --- cms/djangoapps/contentstore/helpers.py | 23 ++++++++++++------- .../rest_api/v2/views/downstreams.py | 4 ++-- .../core/djangoapps/content_staging/api.py | 2 +- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 8e78a6b93c28..8a3c9e75dccf 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -29,6 +29,8 @@ from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers import openedx.core.djangoapps.content_staging.api as content_staging_api import openedx.core.djangoapps.content_tagging.api as content_tagging_api +from openedx.core.lib.xblock_serializer.api import XBlockSerializer +from openedx.core.djangoapps.content_staging.data import LIBRARY_SYNC_PURPOSE from .utils import reverse_course_url, reverse_library_url, reverse_usage_url @@ -335,7 +337,7 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> return new_xblock, notices -def import_staged_content_for_library_sync(downstream_xblock: XBlock, lib_block: XBlock, request) -> StaticFileNotices: +def import_static_assets_for_library_sync(downstream_xblock: XBlock, lib_block: XBlock, request) -> StaticFileNotices: """ Import the static assets from the library xblock to the downstream xblock through staged content. Also updates the OLX references to point to the new @@ -346,21 +348,26 @@ def import_staged_content_for_library_sync(downstream_xblock: XBlock, lib_block: Returns a summary of changes made to static files in the destination course. """ + if not XBlockSerializer( + lib_block, + fetch_asset_data=True, + ).static_files: + return StaticFileNotices() if not content_staging_api: raise RuntimeError("The required content_staging app is not installed") - staged_content = content_staging_api.stage_xblock_temporarily(lib_block, request.user.id) + staged_content = content_staging_api.stage_xblock_temporarily(lib_block, request.user.id, LIBRARY_SYNC_PURPOSE) if not staged_content: # expired/error/loading return StaticFileNotices() store = modulestore() - with store.bulk_operations(downstream_xblock.context_key): - # Now handle static files that need to go into Files & Uploads. - # If the required files already exist, nothing will happen besides updating the olx. - try: + try: + with store.bulk_operations(downstream_xblock.context_key): + # Now handle static files that need to go into Files & Uploads. + # If the required files already exist, nothing will happen besides updating the olx. notices = _insert_static_files_into_downstream_xblock(downstream_xblock, staged_content.id, request) - finally: - staged_content.delete() + finally: + staged_content.delete() return notices diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index da92fb464788..46e67e87ea0c 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -73,7 +73,7 @@ UpstreamLink, UpstreamLinkException, NoUpstream, BadUpstream, BadDownstream, fetch_customizable_fields, sync_from_upstream, decline_sync, sever_upstream_link ) -from cms.djangoapps.contentstore.helpers import import_staged_content_for_library_sync +from cms.djangoapps.contentstore.helpers import import_static_assets_for_library_sync from common.djangoapps.student.auth import has_studio_write_access, has_studio_read_access from openedx.core.lib.api.view_utils import ( DeveloperErrorViewMixin, @@ -199,7 +199,7 @@ def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respons downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) try: upstream = sync_from_upstream(downstream, request.user) - static_file_notices = import_staged_content_for_library_sync(downstream, upstream, request) + static_file_notices = import_static_assets_for_library_sync(downstream, upstream, request) except UpstreamLinkException as exc: logger.exception( "Could not sync from upstream '%s' to downstream '%s'", diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 4e5e11eddb3e..709f474b3d07 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -166,7 +166,7 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int def stage_xblock_temporarily( - block: XBlock, user_id: int, purpose: str = LIBRARY_SYNC_PURPOSE, version_num: int | None = None, + block: XBlock, user_id: int, purpose: str, version_num: int | None = None, ) -> _StagedContent: """ "Stage" an XBlock by copying it (and its associated children + static assets) From f153884552921d9b4e1997caf381731f8f7fec03 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Wed, 29 Jan 2025 21:28:50 -0300 Subject: [PATCH 6/7] fix: optimize and replace missing function name --- cms/djangoapps/contentstore/helpers.py | 5 +---- .../contentstore/rest_api/v2/views/tests/test_downstreams.py | 2 +- .../contentstore/xblock_storage_handlers/view_handlers.py | 4 ++-- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 8a3c9e75dccf..65c264a8bdb2 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -348,10 +348,7 @@ def import_static_assets_for_library_sync(downstream_xblock: XBlock, lib_block: Returns a summary of changes made to static files in the destination course. """ - if not XBlockSerializer( - lib_block, - fetch_asset_data=True, - ).static_files: + if not lib_block.runtime.get_block_assets(lib_block, fetch_asset_data=False): return StaticFileNotices() if not content_staging_api: raise RuntimeError("The required content_staging app is not installed") diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index f0e3dfc8e1a4..31877b9153d5 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -248,7 +248,7 @@ def call_api(self, usage_key_string): @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) @patch.object(downstreams_views, "sync_from_upstream") - @patch.object(downstreams_views, "import_staged_content_for_library_sync", return_value=StaticFileNotices()) + @patch.object(downstreams_views, "import_static_assets_for_library_sync", return_value=StaticFileNotices()) def test_200(self, mock_sync_from_upstream, mock_import_staged_content): """ Does the happy path work? diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 8d10858b4e40..775f08e5fa2c 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -80,7 +80,7 @@ from ..helpers import ( get_parent_xblock, import_staged_content_from_user_clipboard, - import_staged_content_for_library_sync, + import_static_assets_for_library_sync, is_unit, xblock_embed_lms_url, xblock_lms_url, @@ -607,7 +607,7 @@ def _create_block(request): f"using provided library_content_key='{upstream_ref}'" ) return JsonResponse({"error": str(exc)}, status=400) - static_file_notices = import_staged_content_for_library_sync(created_block, lib_block, request) + static_file_notices = import_static_assets_for_library_sync(created_block, lib_block, request) modulestore().update_item(created_block, request.user.id) response["upstreamRef"] = upstream_ref response["static_file_notices"] = asdict(static_file_notices) From 88dcfa48bb8bcf6fcef8dd7a19b13aa85ee48d52 Mon Sep 17 00:00:00 2001 From: Daniel Valenzuela Date: Mon, 3 Feb 2025 18:03:00 -0300 Subject: [PATCH 7/7] fix: linting unused imports --- cms/djangoapps/contentstore/helpers.py | 1 - openedx/core/djangoapps/content_staging/api.py | 1 - 2 files changed, 2 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 65c264a8bdb2..1ceb2bc3c01d 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -29,7 +29,6 @@ from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers import openedx.core.djangoapps.content_staging.api as content_staging_api import openedx.core.djangoapps.content_tagging.api as content_tagging_api -from openedx.core.lib.xblock_serializer.api import XBlockSerializer from openedx.core.djangoapps.content_staging.data import LIBRARY_SYNC_PURPOSE from .utils import reverse_course_url, reverse_library_url, reverse_usage_url diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 709f474b3d07..acc920fd6cb4 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -20,7 +20,6 @@ from .data import ( CLIPBOARD_PURPOSE, - LIBRARY_SYNC_PURPOSE, StagedContentData, StagedContentFileData, StagedContentStatus,