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.

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

Switch v2 libraries to Learning Core data models #34066

Merged

Conversation

ormsbee
Copy link
Contributor

@ormsbee ormsbee commented Jan 16, 2024

Description & Supporting Information

This moves the Content Libraries V2 backend from Blockstore over to Learning Core. For high-level overview and rationale of this move, see

Supersedes:

Requires:

A note on backwards incompatibility

On the Blockstore DEPR, we did not hear that anyone is relying on Blockstore-backed V2 Content Libraries in production. So, we are applying this port in-place without any migration path from Blockstore to Learning Core. If you were using V2 Content Libraries in production: your libraries will stop working. However, we are not yet dropping the the ContentLibraries.bundle_uuid field, so it is still possible to manually migrate Blockstore data over to Learning Core. If you're in the situation, reach out to us. The ContentLibraries.bundle_uuid field will be dropped in Sumac.

Status

The biggest functional gap here is static asset support. I'm gong to try to merge this once I have tests fixed, even if it means we take a hit in terms of a regression in some functionality like static asset support–with the understanding that we're going to keep iterating to make this ready for Redwood.

TODO in this PR:

  • Convert remaining Learning Core model manipulation to use api module calls instead.
  • Squash the new migrations into one.
  • So much test removal/cleanup/rewriting.

TODO in follow-up PRs:

Bugs I'm seeing (not sure if I introduced them or not):

  • There's sometimes a JS error when a new block is created, about not being able to set display_name on null. But the value is written correctly into the database and shows up correctly when reloading.
  • The ProblemBlock editor seems to revert to code editing after the initial creation–did I change the OLX serialization?

FYI @kdmccormick, @bradenmacdonald

Testing

This PR's sandbox can be compared against:

If you want to run this on your devstack, you must put the following in your cms/envs/private.py:

from .devstack import FEATURES

FEATURES['ENABLE_LIBRARY_AUTHORING_MICROFRONTEND'] = True

PR sandbox

Settings

PLUGINS:
    - mfe
    - grove
    - s3
    - library-authoring-mfe

Tutor requirements

git+https://github.com/overhangio/tutor.git@nightly
git+https://github.com/overhangio/tutor-mfe.git@nightly
git+https://gitlab.com/opencraft/dev/tutor-contrib-grove.git@main
git+https://github.com/hastexo/tutor-contrib-s3.git@main
git+https://github.com/openedx/openedx-tutor-plugins@main#subdirectory=plugins/tutor-contrib-library-authoring-mfe

@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 6, 2024

Okay, I've updated to the changes in openedx/openedx-learning#149, and it seems to be working locally. In the morning, I'll squash what I have here, do a rebase to what's on master today, and then see what explodes in terms of conflicts.

@ormsbee ormsbee force-pushed the learning-core-libs-readwrite-runtime branch from f8adde1 to 31a939e Compare February 6, 2024 15:40
@ormsbee ormsbee added the content libraries misc Libraries Overhaul tech work not captured in the stories label Feb 8, 2024
@kdmccormick kdmccormick added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Feb 9, 2024
@ormsbee ormsbee force-pushed the learning-core-libs-readwrite-runtime branch from c00003f to 57f4c62 Compare February 9, 2024 16:39

libraries = [
# TODO: Do we really need these fields for the library listing view?
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I would say that this is definitely "nice to have" in the UI (indicate which libraries have unfinished changes, so you can find them and finish your work), but definitely not a requirement for MVP.

But there are other ways to work around this from a UX perspective - for example, trying to go for something more like Google Docs, where changes are encouraged to be published immediately and if you close a library page / XBlock editor without publishing your changes to any given XBlock it will prompt you "are you sure you want to leave without publishing your changes?". In that scenario, unpublished changes are rare and we don't need to highlight them as much.

openedx/core/djangoapps/content_libraries/api.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/content_libraries/api.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/content_libraries/api.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/content_libraries/api.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/xblock/api.py Outdated Show resolved Hide resolved
@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 11, 2024

There's one major outstanding bug that I'm seeing, and that is that it looks like the Learning Core XBlock runtime is serializing the default values into the metadata dict that comes across the fields REST API call that the editor uses.

Looking back, the XBlock runtime for Learning Core is something I copy-pasta'd from Blockstore, with some things slashed out. There are parts of the original blockstore runtime that I don't really understand as well as I should. @bradenmacdonald: May I grab an hour of your time on Monday to walk through the Blockstore XBlock runtime and FieldData?

@ormsbee ormsbee marked this pull request as ready for review February 13, 2024 02:23
@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 13, 2024

@kdmccormick, @bradenmacdonald: The openedx/core/djangoapps/content_libraries/views.py module is ready for review.

@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 14, 2024

@kdmccormick, @bradenmacdonald: The re-worked openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py is ready for review.

@ormsbee ormsbee removed the create-sandbox open-craft-grove should create a sandbox environment from this PR label Feb 14, 2024
@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 14, 2024

Removed the create-sandbox label so that the sandbox gets destroyed. Will remake it with a more backwards compatible migration later.

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

I just reviewed learning_core_runtime.py. Two suggestions, and one thing I think is actually broken (the get_learning_context_impl all).

@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 14, 2024

@kdmccormick, @bradenmacdonald: openedx/core/djangoapps/content_libraries/api.py is ready for review.

@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 14, 2024

@kdmccormick: incorporated your feedback on learning_core_runtime.py

@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 14, 2024

xblock/api.py is ready for review.

db_keyword_overrides.yml Outdated Show resolved Hide resolved
@ormsbee ormsbee force-pushed the learning-core-libs-readwrite-runtime branch from 409fdc6 to eb76b50 Compare February 14, 2024 21:46
@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 14, 2024

force-pushed a rebase onto the latest master

@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 15, 2024

@kdmccormick, @bradenmacdonald: Everything here should be reviewable now.

@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 15, 2024

The last actual issue I'm aware of is a set of unit tests that fails in CI but not locally. It involves signals and mocks, so I don't know if there's something weird going on there. These same tests were working earlier.

@kdmccormick
Copy link
Member

@bradenmacdonald All comments are addressed. Here are all the changes I've made (plus some noise from the merge commits): ormsbee/edx-platform@0f36be2...learning-core-libs-readwrite-runtime . Can you give those a look? In the meantime, I'll do some smoke testing.

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

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.

Those changes look great @kdmccormick 👏🏻

openedx/core/djangoapps/content_libraries/api.py Outdated Show resolved Hide resolved
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@kdmccormick
Copy link
Member

OK, I haven't the faintest idea why this PR's sandbox is failing migrations, but I did confirm that:

  • migrations applied fine on tutor local
  • migrations applied fine on this PR's sandbox, which was originally created using master, and then was updated with this PR's code.
    • The Blockstore-authored libraries render as title-less cards which spin when you click on them. I think that's completely reasonable for now. If, in the future, we want to degrade more gracefully when there's no backing learning_package for a content_library, then we could do that.
      image

@kdmccormick kdmccormick removed the create-sandbox open-craft-grove should create a sandbox environment from this PR label Feb 22, 2024
@kdmccormick kdmccormick enabled auto-merge (squash) February 22, 2024 16:17
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@kdmccormick kdmccormick merged commit 86f1e5e into openedx:master Feb 22, 2024
46 checks passed
@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.

@ormsbee
Copy link
Contributor Author

ormsbee commented Feb 27, 2024

🎉 @kdmccormick, @bradenmacdonald: Thank you for getting this over the line! It feels so nice to finally have the learning core foundations in place under libraries!

@ormsbee ormsbee deleted the learning-core-libs-readwrite-runtime branch June 4, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content libraries misc Libraries Overhaul tech work not captured in the stories
Projects
Development

Successfully merging this pull request may close these issues.

5 participants