-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @DanielVZ96! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
@ormsbee @bradenmacdonald here's the fix for static assets not being synced from libraries v2 to a library/bank component. it took me some time to figure out a clean way to reuse staged content. my only gripe is that my implementation almost unnecessarily creates a user library sync record. but it's there because all the code in content staging expects a model that relates a component to multiple staged contents. I think it could be ok because in the future it can enable other metadata for the library sync process. I'm currently working on linting and testing but the overall idea is implemented in case you want to review it. |
77c8103
to
3355545
Compare
3355545
to
f12ff12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some suggestions for simplifying this. I'm worried that making this code too specific to "library sync" is going to be really confusing for people coming later and trying to figure out where the library sync code is when we have the upstream/downstream code in edx-platform, this "library sync" code for assets only, and openedx/openedx-learning#269 for tracking links.
I'm also not sure that using StagedContent is worth the extra time for copying all the files and creating the staged content. Is the algorithm for finding static asset references in staged content and copying those referenced files into "Files & Uploads" really more complex than all the changes that are in this PR?
ecdc8dc
to
5a9b68f
Compare
# The file already exists and matches exactly, so no action is needed except substitutions | ||
return None, {clipboard_file_path: f"static/{import_path}"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those changes. I think this is looking much simpler and nicer!
I do have one more request, though it can come in a separate PR if you want: because creating StagedContent is potentially slow, I think that as an optimization, we should skip the whole [stage xblock -> insert static assets] workflow entirely if the upstream component doesn't have any associated static assets.
FYI @ormsbee and @kdmccormick on this new use of Staged Content / modification of upstream-downstream syncing.
if not XBlockSerializer( | ||
lib_block, | ||
fetch_asset_data=True, | ||
).static_files: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Thanks for addressing all my concerns so quickly @DanielVZ96 :) I will test this tomorrow. |
Description
Handles static asset sync when creating or refreshing library content and problem bank components to a unit.
This is done through refactoring the logic already in place in the staged content app which implemented generic logic for the clipboard feature.
Supporting information
Testing instructions
create a text library component
upload an image either in the advanced tab or directly in the text editor
if uploaded in the advanced tab, add it to the component html
Publish the component
Now in the cms add a new library v2 component referencing the text component above.
Assert it renders the image.
Repeat the steps above with the problem bank component
Deadline
ASAP
Other information