-
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
Switch v2 libraries to Learning Core data models #34066
Switch v2 libraries to Learning Core data models #34066
Conversation
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. |
f8adde1
to
31a939e
Compare
c00003f
to
57f4c62
Compare
|
||
libraries = [ | ||
# TODO: Do we really need these fields for the library listing view? |
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.
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.
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? |
@kdmccormick, @bradenmacdonald: The |
Sandbox deployment successful. Sandbox LMS is available at https://pr-34066-2432b1.sandboxes.opencraft.hosting Instance Tutor config : https://axim-sandbox-build-logs-4626178240.nyc3.digitaloceanspaces.com/pr-34066-2432b1-51564656-6156678534-tutor-config.yml |
@kdmccormick, @bradenmacdonald: The re-worked |
Removed the create-sandbox label so that the sandbox gets destroyed. Will remake it with a more backwards compatible migration later. |
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 just reviewed learning_core_runtime.py. Two suggestions, and one thing I think is actually broken (the get_learning_context_impl all).
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
@kdmccormick, @bradenmacdonald: |
@kdmccormick: incorporated your feedback on |
|
409fdc6
to
eb76b50
Compare
force-pushed a rebase onto the latest master |
@kdmccormick, @bradenmacdonald: Everything here should be reviewable now. |
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. |
@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. |
Sandbox deployment failed 💥 |
Sandbox deployment failed 💥 |
Sandbox deployment failed 💥 |
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.
Those changes look great @kdmccormick 👏🏻
Sandbox deployment failed 💥 |
Sandbox deployment failed 💥 |
OK, I haven't the faintest idea why this PR's sandbox is failing migrations, but I did confirm that:
|
Sandbox deployment failed 💥 |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
🎉 @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! |
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. TheContentLibraries.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:
api
module calls instead.TODO in follow-up PRs:
Bugs I'm seeing (not sure if I introduced them or not):
display_name
on null. But the value is written correctly into the database and shows up correctly when reloading.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
:PR sandbox
Settings
Tutor requirements