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

Enable static assets in Learning Core XBlock Runtime #35660

Merged
merged 2 commits into from
Oct 17, 2024
Merged

Enable static assets in Learning Core XBlock Runtime #35660

merged 2 commits into from
Oct 17, 2024

Conversation

ormsbee
Copy link
Contributor

@ormsbee ormsbee commented Oct 16, 2024 •
edited
Loading

Description

This enable the viewing of static assets in the Learning Core XBlock Runtime. It is version aware, so
calling the XBlock iframe with different versions will properly reflect the right assets for that
version of the Component.

Implements #35659

It does not currently support adding assets, and that must still be done via the add_assets_to_component
management command.

Copy-paste functionality will be a separate PR.

bradenmacdonald
bradenmacdonald reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Seems to be working well with the test content I have :)

openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py
Comment on lines +344 to +351
block_version = block._runtime_requested_version # pylint: disable=protected-access
if block_version == LatestVersion.DRAFT:
component_version = component.versioning.draft
elif block_version == LatestVersion.PUBLISHED:
component_version = component.versioning.published
else:
component_version = component.versioning.version_num(block_version)

return component_version
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it makes sense to combine this with the similar logic in get_block()?

edx-platform/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py

Lines 183 to 193 in ecbedbc

if self.authored_data_mode == AuthoredDataMode.STRICTLY_PUBLISHED and version != LatestVersion.PUBLISHED:
raise ValidationError("This runtime only allows accessing the published version of components")
if version == LatestVersion.DRAFT:
component_version = component.versioning.draft
elif version == LatestVersion.PUBLISHED:
component_version = component.versioning.published
else:
assert isinstance(version, int)
component_version = component.versioning.version_num(version)
if component_version is None:
raise NoSuchUsage(usage_key)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm kind of on the fence about it. When I took a brief sketch at extracting out the common part here, the helper naming and indirection just seemed to add more confusion than it was worth. I don't feel strongly about it, but I'm inclined to just leave the bit of redundancy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with me!

openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py Outdated Show resolved Hide resolved
openedx/core/lib/xblock_serializer/block_serializer.py Outdated Show resolved Hide resolved
bradenmacdonald
bradenmacdonald approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Looks great! I tested viewing assets in the new library Preview UI in the authoring MFE and it's all working as expected.

Screenshot

@ormsbee ormsbee force-pushed the library-asset-versioning branch 2 times, most recently from 2ba2a77 to 2e8d733 Compare October 17, 2024 16:24
@ormsbee
feat: versioned asset support for Learning Core XBlock runtime
eb3dfe2 
Add support for displaying static assets in the Learing Core XBlock
runtime via "/static/asset-name" style substitutions in the OLX. This is
currently used for new Content Library components.

Static asset display is version-aware, so viewing older versions of the
XBlock content via the embed view will show the appropriate assets for
that version.
@ormsbee
test: add data.py to acceptable isolated app imports
95c4e69 
Per OEP-49, both api.py and data.py are allowed to be imported into
other apps:

https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0049-django-app-patterns.html#api-py
@ormsbee ormsbee force-pushed the library-asset-versioning branch from f2cad1c to 95c4e69 Compare October 17, 2024 17:20
@ormsbee ormsbee merged commit 23c4276 into openedx:master Oct 17, 2024
49 checks passed
@ormsbee ormsbee deleted the library-asset-versioning branch October 17, 2024 17:58
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

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

@bradenmacdonald bradenmacdonald bradenmacdonald approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Libraries Overhaul
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ormsbee @edx-pipeline-bot @bradenmacdonald