-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Switch v2 libraries to Learning Core data models #34066
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
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
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. |
kdmccormick
left a comment
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
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. |
|
Sandbox deployment failed 💥 |
|
Sandbox deployment failed 💥 |
|
Sandbox deployment failed 💥 |
bradenmacdonald
left a comment
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! |
| ) | ||
| xml_node = etree.fromstring(content.text) | ||
| block_type = usage_key.block_type | ||
| keys = ScopeIds(self.user_id, block_type, None, usage_key) |
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.
Hey @ormsbee , is setting def_id=None here the long-term intention?
Context: I'm on a quest to type-annotate XBlock, and I'm looking for the correct annotation for def_id. Currently I have:
DefinitionId: t.TypeAlias = DefinitionKey | UsageKey | ObjectId | LocalId | strand that handles that ModulestoreRuntimes. But in order to handle the LearningCoreRuntime, looks like I'll need to add | None to that. Happy to do that, just want to check your instincts before proceeding.
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.
Yeah, it's a long term intention. I think that the def_id as it exists today is an unhelpful leak of Split's internal storage details, and that it's more trouble than it's worth. I'd like to eventually kill it off entirely.

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_uuidfield, 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_uuidfield 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:
apimodule calls instead.TODO in follow-up PRs:
Bugs I'm seeing (not sure if I introduced them or not):
display_nameon 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