[FC-0009] Copy/Paste associated static assets #32346

bradenmacdonald · 2023-06-02T01:32:54Z

Description

This implements openedx/modular-learning#16 so that users can copy and paste components between different courses, with static assets coming along "automatically" as needed.

Supporting information

See openedx/modular-learning#16

This also includes a small fix for openedx/modular-learning#66

Testing instructions

Prequisite: enable the new copy-paste feature:

Go to http://studio.local.overhang.io:8001/admin/waffle/flag/ (or devstack/sandbox equivalent)
Add a new flag, contentstore.enable_copy_paste_feature and enable it for Everyone

Instructions:

You need two courses.
In course A, create some components that reference static assets using the /static/blah.ext format. The files should be added to course A's "Files & Uploads". Ideally:
- A Text/HTML component that embeds an image or links to a PDF.
- A drag and drop component with a custom image.
- A video with a custom transcript file uploaded via its settings.
- A Problem component that depends on python_lib.zip but doesn't directly reference it (example from MIT; though note that the sample course contains two symlinks which must be replaced with hard links before it can be imported, if you want to import their sample course).
Copy these components and paste them into Course B.
Verify that the static assets now appear in Course B's "Files & Uploads" and that the components work as before.
Also test what happens if Course B already has an existing file with the same name but different contents (a warning should be shown).
This also includes a fix for When copy-pasting a Text (HTML) block, the "Raw Editor" setting is reset modular-learning#66 - please set an HTML block to "raw" editor then copy and paste it, and ensure the raw editor is saved.

~~Known issue: in the destination course's "Files & Uploads" page, no thumbnail images will be shown for automatically-copied assets. I'm not sure why but can investigate that in a future PR.~~ Working now!

Deadline

None

TODOs

Implement copying of static assets from "Files & Uploads"
Implement copying of static assets from the per-XBlock filesystem (e.g. video transcripts)
Implement pluggable filter to control what gets copied
Implement filter to avoid copying very large assets
Save static assets into the new course on paste
Display a notification to the user about new files or conflicting files
~~Display the list of assets in the paste preview~~ (excluded from UX design for now)
Implement copying of python_lib.zip or at least store its hash
Add more test cases

Private-ref: MNG-3723

openedx-webhooks · 2023-06-02T01:32:59Z

Thanks for the pull request, @bradenmacdonald!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

bradenmacdonald · 2023-06-02T01:36:05Z

@Agrendalath @ormsbee FYI, this is still WIP but you can have a look at how it's shaping up if you want.

When copying an XBlock, you can see its static assets staged in the django admin, but I haven't implemented pasting yet.

Agrendalath · 2023-06-19T15:45:11Z

@bradenmacdonald, I'll review this tomorrow.

Agrendalath

@bradenmacdonald, great work! I left some comments about edge cases, but other than that, it works as expected.

cms/djangoapps/contentstore/helpers.py

openedx/core/djangoapps/content_staging/views.py

openedx/core/lib/xblock_serializer/utils.py

openedx/core/djangoapps/content_staging/views.py

bradenmacdonald · 2023-06-20T21:43:22Z

@Agrendalath Thank you for the really excellent review. I've addressed all your comments.

Agrendalath

👍

I tested this: copying various XBlocks works correctly in different cases (missing files, missing data, large files, duplicates, file conflicts, transcripts, etc.)
I read through the code
I checked for accessibility issues: n/a
Includes documentation: n/a
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Agrendalath · 2023-06-21T07:54:38Z

openedx/core/djangoapps/content_staging/views.py

+                    # and then either inform the user or find another way to import the file (e.g. if the file still
+                    # exists in the "Files & Uploads" contentstore of the source course, based on source_key_str).
+                    data_file=ContentFile(content=f.data, name=f.filename) if f.data else None,
+                    source_key_str=str(source_key) if source_key else "",


Suggested change

source_key_str=str(source_key) if source_key else "",

source_key_str=str(f.source_key) if f.source_key else "",

Agrendalath · 2023-06-21T07:55:41Z

openedx/core/djangoapps/content_staging/views.py

        # Enqueue a (potentially slow) task to delete the old staged content
        try:
            delete_expired_clipboards.delay(expired_ids)
        except Exception as err:  # pylint: disable=broad-except
            log.exception(f"Unable to enqueue cleanup task for StagedContents: {','.join(str(x) for x in expired_ids)}")
        # Return the response:
        return Response(serializer.data)
+
+    def _save_static_assets_to_clipboard(self, static_files, usage_key, staged_content):


Suggested change

def _save_static_assets_to_clipboard(self, static_files, usage_key, staged_content):

@staticmethod

def _save_static_assets_to_clipboard(

static_files: list[StaticFile], usage_key: UsageKey, staged_content: StagedContent

):

Just a suggestion. We'll need from openedx.core.lib.xblock_serializer.api import serialize_xblock_to_olx, StaticFile to get this working.

Agrendalath · 2023-06-21T07:58:44Z

openedx/core/djangoapps/content_staging/views.py

+                    source_key_str=str(source_key) if source_key else "",
+                    md5_hash=f.md5_hash or "",
+                )
+            except Exception as err:  # pylint: disable=broad-except


Tiny nit: we're not using the err variable in any of these except clauses in this file (you can grep by "as err").

Huh, I thought pylint would have warned about that. Thanks.

Agrendalath · 2023-06-21T09:17:19Z

cms/djangoapps/contentstore/helpers.py

+        if data is None:
+            raise NotFoundError(file_data_obj.source_key)
+        content = StaticContent(new_key, name=filename, content_type=content_type, data=data)
+        contentstore().save(content)


Suggested change

contentstore().save(content)

thumbnail_content, thumbnail_location = contentstore().generate_thumbnail(content)

if thumbnail_content is not None:

content.thumbnail_location = thumbnail_location

contentstore().save(content)

Optional. Currently, the thumbnails for copied files are missing.

Agrendalath · 2023-06-21T09:21:23Z

openedx/core/djangoapps/content_staging/views.py

+            if content:
+                md5_hash = hashlib.md5(f.data).hexdigest()
+                # This asset came from the XBlock's filesystem, e.g. a video block's transcript file
+                source_key = usage_key
+            elif source_key:
+                sc = contentstore().find(source_key)
+                md5_hash = sc.content_digest
+                content = sc.data
+            else:
+                md5_hash = ""  # Unknown


Suggested change

if content:

md5_hash = hashlib.md5(f.data).hexdigest()

# This asset came from the XBlock's filesystem, e.g. a video block's transcript file

source_key = usage_key

elif source_key:

sc = contentstore().find(source_key)

md5_hash = sc.content_digest

content = sc.data

else:

md5_hash = "" # Unknown

md5_hash = ""

if content:

md5_hash = hashlib.md5(f.data).hexdigest()

# This asset came from the XBlock's filesystem, e.g. a video block's transcript file

source_key = usage_key

# Check if the resource exists. It can be absent if an XBlock contains an invalid link.

elif source_key and (sc := AssetManager.find(source_key, throw_on_not_found=False)):

md5_hash = sc.content_digest

content = sc.data

To get this snippet working, we need to replace from xmodule.contentstore.django import contentstore with from xmodule.assetstore.assetmgr import AssetManager.

This fixes the scenario when /static/absent_file is present in the XBlock. Otherwise, the copy operation is interrupted.

Agrendalath · 2023-06-21T09:32:08Z

@ormsbee, this is ready for your review.

ormsbee

Just comments/questions–nothing to block merge.

openedx/core/djangoapps/content_staging/data.py

ormsbee · 2023-06-25T15:41:07Z

cms/djangoapps/contentstore/helpers.py

+        # However, currently the only known case for that is video block's transcript files, and those will
+        # automatically be "carried over" to the new XBlock even in a different course because the video ID is the same,
+        # and VAL will thus make the transcript available.


Does this work with VideoBlocks that don't use edx-val, i.e. those with .srt files that were manually uploaded? The VideoBlock has its own weird logic to do those file mappings, right?

FWIW, I'm okay with merging this even if it doesn't work quite right for non-pipeline video, but I want to make sure I'm understanding things correctly.

Ah, yes. The video block's .srt files are copied into the clipboard in any case, since the logic was already in the serializer, but when I was testing things I couldn't find any use case to actually make them available at paste time. So I just added this comment and left it at that for now. I guess that if VAL is disabled, then this would be useful. Do you think that's something important to support in the future?

I do think so, yeah. Many folks won't use the video pipeline because they prefer to manage their videos through other services, and the new VideoBlock work is going to preserve support for different sources.

But like I said, not a blocker.

openedx/modular-learning#71

ormsbee · 2023-06-26T14:14:02Z

openedx/core/djangoapps/content_staging/filters.py

+    This API is considered BETA. Once it is stable, this definition should be moved into openedx_filters.
+    """
+
+    filter_type = "org.openedx.content_authoring.staged_content.static_filter_source.v1"


Not blocking, but I'm curious why this was done as a filter, as opposed to some new optional method on XBlocks?

Mostly I wanted this to be very flexible and configurable at any level - the instance level in particular. If some XBlock has a need to associate a particular static file with itself, it can do so using a filter, but so can an instance administrator if there is some usage pattern on their instance that requires static files for, say, HTML XBlocks, which the normal method of detection doesn't pick up. I consider this kind of an experimental approach though as it says in the docstring. If it doesn't see any usage we can remove the filter entirely.

On second thought, now that I'm not using filters for python_lib.zip I think I'll do a follow-up PR to just remove this filter. YAGNI. openedx/modular-learning#70

ormsbee · 2023-06-26T14:16:12Z

openedx/core/djangoapps/content_staging/filters.py

+        """
+        Filter the list of file_datas to remove any large files
+        """
+        limit = 10 * 1024 * 1024


Is there a specific set of files we're expecting to get filtered at this point?

No, I just wanted to avoid performance issues if someone copies an XBlock that links to a huge file that's in Files & Uploads. Don't want to block the copy from completion while huge files are uploaded to temp storage that may not even be needed.

So in such a case (copying something that references a huge file), now we'll just skip that file, and intra-course copy-paste will still work for, and for cross-course copy-paste the user can copy the large asset manually.

Co-authored-by: David Ormsbee <dave@axim.org>

openedx-webhooks · 2023-06-27T19:06:47Z

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2023-06-28T13:54:44Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-06-28T15:13:51Z

2U Release Notice: This PR has been deployed to the edX production environment.

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core committer labels Jun 2, 2023

bradenmacdonald marked this pull request as draft June 2, 2023 01:34

bradenmacdonald added the FC Relates to an Axim Funded Contribution project label Jun 2, 2023

bradenmacdonald force-pushed the copy-asset-dependencies branch 4 times, most recently from 84644d3 to c4ce948 Compare June 8, 2023 02:36

bradenmacdonald added 6 commits June 15, 2023 17:50

refactor: improve typing of StaticFile named tuple

dd4213c

feat: copy static asset files into the clipboard

7505855

feat: save the source key in StagedContentFile

5f75a83

feat: paste static assets

ee6e698

feat: show notification in studio about pasted assets

ed91565

fix: HTML XBlocks would lose the editor="raw" setting when copy-pasted.

5c2ad99

bradenmacdonald force-pushed the copy-asset-dependencies branch from 0734670 to 3b87039 Compare June 16, 2023 01:05

feat: copy python_lib.zip to the clipboard when it seems to be in use

63d152d

bradenmacdonald force-pushed the copy-asset-dependencies branch from 3b87039 to 63d152d Compare June 16, 2023 01:30

feat: add tests for copying static assets

780383a

bradenmacdonald marked this pull request as ready for review June 18, 2023 20:31

bradenmacdonald requested review from Agrendalath and ormsbee June 18, 2023 20:31

bradenmacdonald mentioned this pull request Jun 18, 2023

When copy-pasting a Text (HTML) block, the "Raw Editor" setting is reset openedx/modular-learning#66

Closed

Agrendalath requested changes Jun 20, 2023

View reviewed changes

fix: address review comments

bb30799

bradenmacdonald force-pushed the copy-asset-dependencies branch from b0c5dab to bb30799 Compare June 20, 2023 22:08

Agrendalath approved these changes Jun 21, 2023

View reviewed changes

fix: address further review comments

c7774fe

ormsbee reviewed Jun 26, 2023

View reviewed changes

docs: address review comment

07700f1

Co-authored-by: David Ormsbee <dave@axim.org>

bradenmacdonald merged commit 12a8d99 into openedx:master Jun 27, 2023

bradenmacdonald deleted the copy-asset-dependencies branch June 27, 2023 19:06

This was referenced Jun 27, 2023

Remove the filter from copy-paste static assets openedx/modular-learning#70

Closed

Copy subtitles for video blocks when VAL is not used openedx/modular-learning#71

Closed

kenclary mentioned this pull request Jun 29, 2023

feat: basic get/post endpoint for v2 xblocks. TNL-10873 #32600

Merged

bradenmacdonald mentioned this pull request Jul 6, 2023

[FC-0009] Remove unneeded filter from static asset copy/paste code #32660

Merged

kenclary mentioned this pull request Aug 4, 2023

fix: fix mako runtime bug with studio_view. TNL-10912 #32912

Merged

PKulkoRaccoonGang mentioned this pull request Mar 10, 2024

feat: [FC-0044] Course unit - Copy/paste functionality openedx/frontend-app-authoring#884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FC-0009] Copy/Paste associated static assets #32346

[FC-0009] Copy/Paste associated static assets #32346

bradenmacdonald commented Jun 2, 2023 •

edited

Loading

openedx-webhooks commented Jun 2, 2023 •

edited

Loading

bradenmacdonald commented Jun 2, 2023

Agrendalath commented Jun 19, 2023

Agrendalath left a comment

bradenmacdonald commented Jun 20, 2023

Agrendalath left a comment •

edited

Loading

Agrendalath Jun 21, 2023

Agrendalath Jun 21, 2023

Agrendalath Jun 21, 2023 •

edited

Loading

bradenmacdonald Jun 21, 2023

Agrendalath Jun 21, 2023

Agrendalath Jun 21, 2023 •

edited

Loading

Agrendalath commented Jun 21, 2023

ormsbee left a comment

ormsbee Jun 25, 2023

ormsbee Jun 25, 2023

bradenmacdonald Jun 26, 2023

ormsbee Jun 27, 2023

bradenmacdonald Jun 27, 2023

ormsbee Jun 26, 2023

bradenmacdonald Jun 26, 2023

bradenmacdonald Jun 27, 2023 •

edited

Loading

ormsbee Jun 26, 2023

bradenmacdonald Jun 26, 2023

openedx-webhooks commented Jun 27, 2023

edx-pipeline-bot commented Jun 28, 2023

edx-pipeline-bot commented Jun 28, 2023

	source_key_str=str(source_key) if source_key else "",
	source_key_str=str(f.source_key) if f.source_key else "",

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

[FC-0009] Copy/Paste associated static assets #32346

[FC-0009] Copy/Paste associated static assets #32346

Conversation

bradenmacdonald commented Jun 2, 2023 • edited Loading

Description

Supporting information

Testing instructions

Deadline

TODOs

openedx-webhooks commented Jun 2, 2023 • edited Loading

bradenmacdonald commented Jun 2, 2023

Agrendalath commented Jun 19, 2023

Agrendalath left a comment

Choose a reason for hiding this comment

bradenmacdonald commented Jun 20, 2023

Agrendalath left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Agrendalath Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Agrendalath Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Agrendalath commented Jun 21, 2023

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openedx-webhooks commented Jun 27, 2023

edx-pipeline-bot commented Jun 28, 2023

edx-pipeline-bot commented Jun 28, 2023

bradenmacdonald commented Jun 2, 2023 •

edited

Loading

openedx-webhooks commented Jun 2, 2023 •

edited

Loading

Agrendalath left a comment •

edited

Loading

Agrendalath Jun 21, 2023 •

edited

Loading

Agrendalath Jun 21, 2023 •

edited

Loading

bradenmacdonald Jun 27, 2023 •

edited

Loading