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.

Sign up for GitHub

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

Jump to bottom

XBlock asset support for new libraries experience (WIP) #35557

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

XBlock asset support for new libraries experience (WIP) #35557

wants to merge 6 commits into from

Conversation

ormsbee
Copy link
Contributor

@ormsbee ormsbee commented Sep 27, 2024 •
edited
Loading

A very hacky pass at getting Learning Core backed static assets to show up for libraries.

Things I need to look into:

  • Showing different versions of the content, since it's rendering on LMS and we're not using Studio vs. LMS to decide which version to get. (Would be nice to get any version.)
  • Current static-replace convention is to strip the leading static/ piece, but that might be useful.
  • To what extent do we really need AssetKeys in new libraries?
  • I had to comment out XBlockSerializer's scan-content-for-static-asset-references functionality, because it had some assumptions about course content. Will probably create an optional function pointer argument so that Learning Core can provide their own implementation for this.

This depends on:

Also, in order for this to work at all, I had to comment out:

https://github.com/openedx/frontend-app-authoring/blob/c80483c0533a10e46c59e9bd28e02a7650720a61/src/editors/sharedComponents/TinyMceWidget/hooks.js#L103-L116

That frontend code was auto-replacing static asset references with links that would show up in the WYSIWG editor (by casting them to asset keys), and then changing them back into the original static references when saving. Unfortunately, the code doesn't understand v2 libraries and generates broken keys.

@bradenmacdonald
Copy link
Contributor

@ormsbee I don't think we need to be using the LMS to render the previews today. I think it would actually make more sense to change to use Studio. The Authoring MFE really shouldn't be using the LMS APIs at all. In fact, I'd even like to find a way for the "preview published version" functionality (that we don't have yet) to use some CMS URL.

bradenmacdonald
bradenmacdonald reviewed Sep 28, 2024
View reviewed changes
openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py
Comment on lines +332 to +333
TODO: Like get_block, we currently assume that we're using the Draft
version. This should be a runtime parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably know this, but here in apps.py is where we can set runtime parameters like "studio should default to DRAFT" and "LMS should default to PUBLISHED".

It would be very cool if we could later override this default using a context manager when needed, but I don't know if that's a safe way to implement such an override.

Copy link
Contributor Author

@ormsbee ormsbee Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is it sufficient to do Draft vs. Published, or should we be able to do any stored version (for the sake of seeing what changed)? (I was assuming we'd eventually need the latter.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Being able to see any stored version would be great, so I think we should do that. But more often I'll just want to specify "latest draft" or "latest published" so I hope we can have both options.

@bradenmacdonald
Copy link
Contributor

My test instructions:

  1. Get the code and restart tutor:
cd tutor
git fetch origin pull/1124/head:ormsbee_private_media
git checkout ormsbee_private_media
cd ..
cd openedx-learning
git fetch origin pull/234/head:ormsbee_asset_integration
git checkout ormsbee_asset_integration
cd ..
cd edx-platform
git fetch origin pull/35557/head:ormsbee_new_assets_hacky
git checkout ormsbee_new_assets_hacky
tutor dev stop
tutor config save
tutor dev start cms lms mfe
  1. Check out this PR for the authoring MFE (git fetch origin pull/1346/head:olx_editor && git checkout olx_editor)
  2. Go into a v2 library in the MFE and create a Text component. Use the OLX editor to give it some HTML like this: 
    <html display_name="Text with Image"><![CDATA[
  <p>Here is a picture:</p>
  <img alt="Picture" src="/static/example.jpg" />
]]></html>
  3. Go to http://studio.local.openedx.io:8001/admin/oel_components/componentversion/ - the new HTML component version should be at the top of the list.
  4. Find an image and put it into the openedx-learning folder. Call it example.jpg
  5. Run this command to add the image to the ComponentVersion - substitute the correct ID of the library containing your component as well as the "Component" ID obtained from the ComponentVersion. 
    tutor dev exec cms python manage.py cms add_assets_to_component lib:OpenCraftX:ALPHA xblock.v1:html:103e1e17-a459-4a56-8919-06f0c69392a3 static/example.jpg:/mnt/openedx-learning/example.jpg
  6. Refresh the view in the course authoring MFE:

Screenshot showing it works

It works!!

This was referenced Sep 29, 2024
Open
Open
bradenmacdonald
bradenmacdonald reviewed Sep 29, 2024
View reviewed changes
openedx/core/djangoapps/content_libraries/api.py
'component_version_uuid': component_version.uuid,
'asset_path': cvc.key,
}
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please make these absolute URLs?

Screenshot of code diff required to do so

site_root_url = get_xblock_app_config().get_site_root_url()

@ormsbee
Copy link
Contributor Author

ormsbee commented Sep 30, 2024

This PR now also relies on openedx/frontend-app-authoring#1349 (disables static asset substitution in the editor, and switches the preview embed window to point to Studio instead of LMS).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradenmacdonald bradenmacdonald bradenmacdonald left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ormsbee @bradenmacdonald