Support static assets when copy/pasting between courses and libraries

[ormsbee]

Background

Enhance copy-paste between Courses and Libraries so that it brings along static assets.

The biggest challenge is dealing with the mismatch between how Libraries store assets (per-Component) and how Courses store assets (global Files and Uploads space). To bridge this, we're going to kludge a component-local namespace in Files and Uploads by making use of the obscure feature that you can create folders there at an API level, even if no such UI exists.

What's in this PR?

Assets work when copy-pasting between library components.

Assets work when copy-pasting from a library to a course, with the convention being to put that file in a subdirectory of the form: components/{block_type}/{block_id}/file

Example:

Note that this path does not currently show up in the UI for Files and Uploads:

Assets work when copy-pasting from a course to a library

Top level assets are put into a static folder in the Component, per Learning Core conventions:

Limitations

Roundtrips don't work properly. There's no normalized form, so directories will start nesting if you copy from library and paste into course, then copy the pasted thing and paste back into library, etc. This was deemed acceptable for Sumac.

Low level stuff

• XBlockSerializerForLearningCore has been removed, with the url_name stripping functionality added as an optional param to XBlockSerializer (the other stuff was for children and "vertical" -> "unit" conversion, neither of which are relevant now.
• url_name is now stripped out of anything added to the clipboard, so that we don't end up writing it in block.xml when it is redundant (and would be stripped out with the next write anyway).
• @kdmccormick: You very recently changed set_library_block_olx to return the new ComponentVersion.version_num, and I'm changing it again to return the whole ComponentVersion because I needed to grab other fields off of it. I fixed the code/tests that I saw, and I only mention this in case you started getting weird errors later while rebasing.

[bradenmacdonald]

@ormsbee

@kdmccormick: You very recently changed set_library_block_olx to return the new ComponentVersion.version_num, and I'm changing it again to return the whole ComponentVersion because I needed to grab other fields off of it.

That was me :p I don't think it will affect any in-flight work.

[ormsbee]

That was me :p I don't think it will affect any in-flight work.

Oh, weird. I thought I remembered it as part of one of his PRs. Ah well, in any case, thanks folks. 😄

[bradenmacdonald]

Roundtrips don't work properly. There's no normalized form, so directories will start nesting if you copy from library and paste into course, then copy the pasted thing and paste back into library, etc. This was deemed acceptable for Sumac.

When copying from a course and pasting into a library, would it not be fairly simple to detect paths of the pattern components/something/something/name and flatten those out back to static/name (provided static/name doesn't also exist)? Or even just strip out any directories in the static asset paths?

[ormsbee]

@bradenmacdonald:

When copying from a course and pasting into a library, would it not be fairly simple to detect paths of the pattern components/something/something/name and flatten those out back to static/name (provided static/name doesn't also exist)? Or even just strip out any directories in the static asset paths?

It is, but there are some weird edge cases that I didn't want to play whack-a-mole with in the absence of having a normalized form. For instance:

1. It's legal to use an AssetKey that points to the asset in the subdirectory, or the actual directory written out, but the conversion isn't 1:1 because when going to AssetKey you substitute "_" for "/".
2. The frontend editor derives its "/static/path/to/file" from the AssetKey that it's been transformed into (for preview purposes), but that transformation only goes one way, so it's translating back to "/static/path_to_file"–which actually works because it uses AssetKey to do the lookup on the backend.
3. I'm not sure what the right behavior should be for copy-pasting those things with namespaced assets between the same course.

We need to pass a little more metadata about the asset to the frontend to do this properly, and I just didn't want to try to get that sane by tomorrow. The current setup just gets us to a place where it's no weirder than putting it into that subdirectory manually.

[bradenmacdonald]

@ormsbee Given all that, I'm almost wondering if we should namespace the library-imported assets as e.g. block_asset_(type)_(id)_(filename) and leave everything flat, instead of bothering with the subfolders at all? Then we avoid the assetkey ambiguities. (Or use something like . instead of _ if necessary, if _ is still problematic.)

[ormsbee]

I guess I'm betting that if we can at least store them in a separated space, we'll have enough metadata information to rationalize this better in the future.

[bradenmacdonald]

OK, but is a pseudo-subfolder any more of a more "separated space" than a unique prefix like block_asset_(type)_(id)_? I guess yes, because authors can't create folders in the UI? And because the filenames appear nicer in "Files & Uploads"?

[ormsbee]

Yeah, it's not great, but I think it's slightly better.

[kdmccormick]

@ormsbee Just in case it's confusing you too, this warning:

pylintrc:1:0: W0012: Unknown option value for '--disable', expected a valid pylint message and got 'broad-exception-raised' (unknown-option-value)

showed up for me, and it disappeared as soon as I resolved the other pylint violations. You should be able to ignore it.

[kdmccormick]

@ormsbee when I edit this pasted-in library component:

the editor renders the altext instead of the image:

could that be this PR, or a frontend-app-authoring issue?

[ormsbee]

@kdmccormick: it should be fixed once openedx/frontend-app-authoring#1403 merges

[kdmccormick]

I was able to create a collision (on the AssetKey level, I believe) by uploading a file to a course named something like components_html_html1_little-library.png. It had the effect of clobbering away the component-level little-library.png, but then html1 seamlessly used the new course-level uploaded asset. In the course export, the component-level asset was gone, in favor of the new course-level asset.

Honestly, that's the best behavior I could have hoped for, and I doubt that anyone will name a file like that, so I'm not worried about fixing it for MVP, but I do think it's one of those things that we should go back and try to address later.

[kdmccormick]

Tested and verified. Code looks good. Thank you, as always, for the helpful comments.

[ormsbee]

I was able to create a collision (on the AssetKey level, I believe) by uploading a file to a course named something like components_html_html1_little-library.png. It had the effect of clobbering away the component-level little-library.png, but then html1 seamlessly used the course-level uploaded asset. Honestly, that's the best behavior I could have hoped for, and I doubt that anyone will name a file like that, so I'm not worried about fixing it for MVP, but I do think it's one of those things that we should go back and try to address later.

Yeah, I mentioned this in a slack thread a couple of weeks ago, and I think the conclusion was that we could live with it for now.

[kdmccormick]

I'm giving this one a rebase @ormsbee

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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