-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: model for upstream-downstream links #269
Conversation
Thanks for the pull request, @navinkarkera! This repository is currently maintained by @axim-engineering. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
aa3e463
to
87df63e
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.
👍 I left a few nits/questions about some field types and naming conventions, but this is working well :)
- I tested this using the test instructions on feat: upstream-downstream entity linking edx-platform#36111
- I read through the code
-
I checked for accessibility issues by using my keyboard to navigateN/A - Includes documentation -- code comments for modules and classes
- User-facing strings are extracted for translation -- mostly, but I've noted where the model
verbose_name
fields need marking too.
eb706d7
to
89d47ca
Compare
@ormsbee Pinging you here if you want to take a look 👀 |
89d47ca
to
b6010cb
Compare
@ChrisChV: I'll review this tomorrow (Thursday). Thank you. |
""" | ||
This represents link between any two publishable entities or link between publishable entity and a course | ||
xblock. It helps in tracking relationship between xblocks imported from libraries and used in different courses. | ||
""" |
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.
Question: Is the fact that this is living in openedx-learning distorting the design and making it harder than it needs to be? Because if this was in edx-platform, we'd be using UsageKey fields, CourseKeyFields, and using a join to CourseOverview for the context title, right?
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.
Except for usagekey and coursekey fields, there won't be much difference. All details about the xblock can be fetched from meilisearch index from the frontend, see this wip code section as CourseOverview
can only help us with course name, while we need block name, description, type, location etc.
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.
Why are we storing the "context title" anyways?
I am also wondering if edx-platform is a better place for this. Because Learning Core generally deals with Components, PublishableEntities, and LearningPackages, whereas these ____stream_usage_key
and ____stream_context_key
fields are more platform-specific workarounds for the fact that courses are not yet using Learning Core.
usage_key
and context_key
don't yet appear in this codebase, for example.
If we do want to keep this approach though, would it be crazy to have a LegacyXBlock(PublishableEntityMixin)
that wraps a UsageKey
field?
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.
In fact, I realize now the concepts of "upstream/downstream syncing" don't yet appear in Learning Core at all anyways. So it seems a bit odd if we have some APIs for tracking the links here, but no sync code, and the edx-platform codebase has APIs for syncing only.
We need to decide if this is a useful primitive that we want people to use to build things with, or if this is a layer on top of learning core that's more platform specific. There's a future world where everything is in Learning Core, and this model can be much simpler - ForeignKey(PublishableEntity) on both sides. I am wondering if we should wait until then before moving it into Learning Core. But i don't really have a strong opinion on this.
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.
Why are we storing the "context title" anyways?
Displaying list of courses using the library block in library authoring page. We can however remove it and fetch the title (and other details if required) via meilisearch index.
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.
There's a future world where everything is in Learning Core, and this model can be much simpler - ForeignKey(PublishableEntity) on both sides. I am wondering if we should wait until then before moving it into Learning Core.
This seems like a good option, we will be in a much better position then.
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 agree with @bradenmacdonald on this. Apologies if I was the one that suggested it be here in the first place, but after seeing how it needs to be implemented, I think it's better in edx-platform for now.
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.
@ormsbee @bradenmacdonald Done, moved this to edx-platform. We can close this PR now.
Adds models and api's for Publishable entity links.
See openedx/edx-platform#36111 for more information and testing instructions.
Related to openedx/modular-learning#243