Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: static assets in problem bank and library content block #36173

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 74 additions & 23 deletions cms/djangoapps/contentstore/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -262,6 +264,37 @@ 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
Expand Down Expand Up @@ -299,31 +332,46 @@ 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_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
locations of those assets in the downstream course.

Does not deal with permissions or REST stuff - do that before calling this.

Returns a summary of changes made to static files in the destination
course.
"""
if not XBlockSerializer(
bradenmacdonald marked this conversation as resolved.
Show resolved Hide resolved
lib_block,
fetch_asset_data=True,
).static_files:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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):
# This block doesn't have any static asset files for us to worry about.

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, LIBRARY_SYNC_PURPOSE)
if not staged_content:
# expired/error/loading
return StaticFileNotices()

store = modulestore()
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()

return notices


def _fetch_and_set_upstream_link(
copied_from_block: str,
copied_from_version_num: int,
Expand Down Expand Up @@ -548,6 +596,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:
Expand Down Expand Up @@ -618,8 +669,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}"}
Comment on lines +668 to +669
Copy link
Contributor Author

Choose a reason for hiding this comment

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

library sync was failing when updating downstream blocks with the following button
image

else:
# There is a conflict with some other file that has the same name.
return False, {}
Expand Down
10 changes: 8 additions & 2 deletions cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@
"ready_to_sync": Boolean
}
"""

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
Expand All @@ -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_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,
Expand Down Expand Up @@ -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_static_assets_for_library_sync(downstream, upstream, request)
except UpstreamLinkException as exc:
logger.exception(
"Could not sync from upstream '%s' to downstream '%s'",
Expand All @@ -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:
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -247,14 +248,16 @@ 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?
"""
self.client.login(username="superuser", password="password")
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):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -598,16 +599,18 @@ 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(
f"Could not sync to new block at '{created_block.usage_key}' "
f"using provided library_content_key='{upstream_ref}'"
)
return JsonResponse({"error": str(exc)}, status=400)
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["upstreamRef"] = upstream_ref
response["static_file_notices"] = asdict(static_file_notices)

return JsonResponse(response)

Expand Down
3 changes: 2 additions & 1 deletion cms/lib/xblock/upstream_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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:
Expand Down
Loading
Loading