openedx
feat: Upstream Sync with Content Library Blocks
Changes:
- Expose
version_numfrom ContentLibraryXBlockMetadata - Create a new UpstreamSyncMixin which is added to all CMS blocks, including:
- New fields
upstream,upstream_version,upstream_overridden, andupstream_settings - An
upstream_linkXBlock GET/PUT handler for getting/setting a block's link to a library block - An
update_from_upstreamXBlock handler for pulling in library update
- New fields
- TEMPORARY: Display upstream version info in the block title, for debugging
Demo PR: https://github.com/openedx/frontend-app-course-authoring/pull/1202 ADR: https://github.com/kdmccormick/edx-platform/blob/kdmccormick/upstream-proto/docs/decisions/0020-upstream-block.rst
@bradenmacdonald So I've got this draft version of upstream sync working, implemented with XBlock fields and handlers. Using the XBlock fields and handlers for this was nice because it gave me persistence, URL routing, and OLX serialization for free.
But now I'm wondering if this is bad approach because we'll need to support Units and Subsections in libraries next, and we've agreed that those structural blocks should stop being XBlocks eventually.
The alternative I see is to:
- use Django models to store upstream links, upstream defaults, and upstream overrides,
- define REST/Python APIs for managing them, and
- handle OLX serialization as special case
Thoughts?
use Django models to store upstream links, upstream defaults, and upstream overrides,
Okay, I started prototyping this, and it's easier said than done. Particularly, if you imagine a model like this, there's a question of whether we FK to entities or entityverisons:
class UpstreamDownstreamLink(models.Model):
"""
Link a piece of content (the downstream) to a source for updates (the upstream).
"""
# Pretty straightforward -- FK to a particular upstream version, and then update the
# version whenever sync happens.
upstream = models.ForeignKey(PublishableEntityVersion)
# Which one of these is right?? Neither?
# This one would apply to all versions of the downstream component, so it would play weird
# draft/publish, and would fail if we ever added an "undo" button to studio.
downstream = models.OneToOneField(PublishableEntity)
# This seems more correct, but would require updating every time a new version of the downstream
# component is created. Perhaps this would work fine as a cache, but it seems wrong as the authoritative
# data source.
downstream = models.OneToOneField(PublishableEntityVersion)
It seems to me, then, that we do want to persist upstream metadata using XBlocks fields, so that version management works as we expect. Still, I'm open to feedback on whether we should use XBlock handlers vs. build a separate REST API for getting/settings links and syncing from upstream.
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 I think it's fine to implement this with fields and handlers for now; as you said, it gives you a lot for free. It even gives us most of the Unit and Subsection functionality for free, I think. Yes, we want to implement those differently in the future but we aren't going to be able to make that change anytime soon, and I think we can worry about how library sync works in that case after we have the new design for how that stuff works. Right now it's mostly an unknown.
e.g. I have an idea for an "outline" object that encodes the course hierarchy and library content and much more, and if we implemented it that way, we'd have to re-implement even your django model based prototype. Or previously I had proposed this compositor architecture which is different yet again. So I think it's not worth planning for that too much until we've got an ADR and buy-in about what the future direction is for the non-XBlock way to represent hierarchy.
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
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
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
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
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
I think that it'll be important for LearningPackages to have independent lifecycles. Foreign keys across LearningPackage boundaries are fine, but they should have on_delete=models.SET_NULL, and behave properly when the originating LearningPackage is deleted for whatever reason.
So for instance, if we are storing "Problem C1 in my course is based on version 4 of Problem L1 in a library that lives in another LearningPackage," we need to store "version 4" in a data model that's associated with the course, and not rely on always being able to follow the foreign key to get the upstream version number.
# Which one of these is right?? Neither? # This one would apply to all versions of the downstream component, so it would play weird # draft/publish, and would fail if we ever added an "undo" button to studio. downstream = models.OneToOneField(PublishableEntity) # This seems more correct, but would require updating every time a new version of the downstream # component is created. Perhaps this would work fine as a cache, but it seems wrong as the authoritative # data source. downstream = models.OneToOneField(PublishableEntityVersion)
I think downstream is the PublishableEntityVersion. Some points in its favor:
- It will do the right thing when it comes to draft/publish and reverts, as you point out.
- It's plausible that a publishable entity will switch upstreams later in its life, e.g. if a library is removed and another one is re-added. Tracking each version separately makes that easier.
- We can model this like we would model other metadata that we would attach to a
PublishableEntityVersion, in an established pattern that will be easier to cleanup.
To that last point, if we think of "upstream" as just another piece of extension-point metadata around the PublishableEntityVersion, then the model could look something like this:
class VersionUpstream(models.Model):
version = models.OneToOneField(PublishableEntityVersion, on_delete=models.CASCADE, primary_key=True)
upstream_version = models.ForeignKey(PublishableEntityVersion, on_delete=models.SET_NULL)
upstream_key = key_field()
upstream_version_num = models.PositiveIntegerField()
Of course, all of that is in the scenario where downstream is in a Learning Core data model, which is not the case today. I think that for any model in the short term, we would:
- Have a similar model that just has a UsageKey for downstream (so no versioning).
- Store the source of truth for upstream links in the ModuleStore, and;
- Update the model whenever that ModuleStore entry changes.
So that model really only exists to answer the question, "What needs updates?"
Thanks @ormsbee . Agreed all around, particularly on:
Store the source of truth for upstream links in the ModuleStore ... So that model really only exists to answer the question, "What needs updates?"
My latest commits get rid of the Django models. I figure we can add those as we get closer to needing them. What do you think (overall) of the current implementation?
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
What do you think (overall) of the current implementation?
Overall, I think the approach is good–especially given where we are today. My only review comments are on lower-level details.
Hi @kdmccormick! :smiley:
I'm working on the frontend to implement this:
- https://github.com/openedx/frontend-app-authoring/issues/1337
I'm a bit lost with the naming here, so I would like to confirm: When we add a library component to a course, we first need to copy the component to the course (creating a "downstream" block) and then link it to the block from the library ("upstream¨), right?
Do we already have a REST endpoint that does this?
I only found the PUT /api/v2/contentstore/downstreams/{usage_key_string}, which seems to do just the link step.
I'm a bit lost with the naming here, so I would like to confirm: When we add a library component to a course, we first need to copy the component to the course (creating a "downstream" block) and then link it to the block from the library ("upstream¨), right?
@rpenido That's right. The source block (in the library) is the upstream, and the linked copy (in the course) is the "downstream". For testing purposes, if you have this branch checked out, copying a block from a library and pasting it into a course should do the same thing (create the linked downstream copy) that we want to do via the other "Add library content to a course" workflow that you're working on. Although last time I tried this branch, it wasn't working due to a missing migration.
Thanks @ormsbee . I'll address all your concerns Monday, but I'm going to leave the nits for a followup PR.
@bradenmacdonald this is ready for review
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.
2U Release Notice: This PR has been deployed to the edX production environment.
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.
2U Release Notice: This PR has been deployed to the edX production environment.