An openedx_catalog app with a representation for CourseRuns [FC-0117]#479
An openedx_catalog app with a representation for CourseRuns [FC-0117]#479bradenmacdonald wants to merge 23 commits intomainfrom
Conversation
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by 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. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
I don't know if we'll force it to be stable, but we'll probably want a |
17b313f to
8177a58
Compare
87f845c to
ccebc9d
Compare
88e2f82 to
bf0fbd6
Compare
Historical note: Apparently there was an old PR to add an opaque key for catalog courses, but it never merged: openedx/opaque-keys#87 Seems like they were called "aggregate courses" then. |
8d56dc0 to
d67189d
Compare
ac60e0e to
b83e3f7
Compare
kdmccormick
left a comment
There was a problem hiding this comment.
Partial review as I'm signing off for tonight, but overall really solid. I appreciate the comments and validation a lot. My only concerns so far are some superficial naming stuff.
| ("course-v1:...") and never expose the integer primary key of this model. | ||
|
|
||
| Note: throughout the system, we often abbreviate "course run" as just | ||
| "course", so the two concepts are usually interchangeable. The _set_ of runs |
There was a problem hiding this comment.
| "course", so the two concepts are usually interchangeable. The _set_ of runs | |
| "course", so the two terms are usually interchangeable. The _set_ of runs |
| null=False, | ||
| related_name="runs", | ||
| ) | ||
| run = case_sensitive_char_field( |
There was a problem hiding this comment.
since we have org_code and course_code, why not run_code?
as an aside, I like the word "code" for this short sluggy things that combine together to make opaquekeys. We've called it several other things in other places ("slug", "url_name", "id", "key") but I'd been keen to retcon them all into "code" -- library_code, collection_code, block_code, container_code, etc.
| # When this course run was first created. We don't track "modified" as this model should be basically immutable, | ||
| # and the only field that may ever change is "display_name"; we also don't want people to think that the "modified" | ||
| # time reflects when edits were last made to the course. |
| # '+' is a bad separator because it can mean " " in URLs. | ||
| # '-', '.', and '_' cannot be used since they're allowed in the org code | ||
| # So for now we use ':', and in the future we may make the whole slug customizable. | ||
| return f"{self.org_code}:{self.course_code}" |
There was a problem hiding this comment.
whether or not to make it an OpaqueKey key... great question 😛
My take on OpaqueKeys is that their implementation is away more complicated than it needs to be, buuuuut, they are still better than parsing everything by hand all over the place, which is what will naturally happen over time whenever we have a key-like thing floating around.
As a compromise, what about returning a string that we could retroactively turn into an opaquekey if we see the need? All it would need is a prefix, like:
| return f"{self.org_code}:{self.course_code}" | |
| return f"catalog-course:{self.org_code}:{self.course_code}" |
| help_text=_("The internal database ID for this course. Should not be exposed to users nor in APIs."), | ||
| editable=False, | ||
| ) | ||
| course_id = CourseKeyField( |
There was a problem hiding this comment.
| course_id = CourseKeyField( | |
| course_key = CourseKeyField( |
Could we call this course_key? *_id in Django usually a foreign key, and I think it's unfortunate we've used course_id in so many places. I'd love to standardize on *_key for anything that's an OpaqueKey instance.
| # Enforce that the course ID must end with "+run" where "run" is an exact match for the "run" field. | ||
| # This check may be removed or changed in the future if our course ID format ever changes | ||
| models.CheckConstraint( | ||
| # Note: EndsWith() on SQLite is always case-insensitive, so we code the constraint like this: | ||
| condition=Exact(Right("course_id", Length("run") + 1), Concat(models.Value("+"), "run")), | ||
| name="oex_catalog_courserun_courseid_run_match_exactly", | ||
| violation_error_message=_("The CourseRun 'run' field should match the run in the course_id key."), | ||
| ), |
There was a problem hiding this comment.
I have a sense that we'll eventually want to relax this in order to allow sites more flexibility on how they market their course runs, but I agree with adding the constraint for now and seeing how it plays out.
There was a problem hiding this comment.
Turns out that I already had to relax it a bit, because it was breaking on CCX keys like ccx-v1:org+code+run+ccx@1 which don't end with +run. In fact, I realized CCX keys break a few different assumptions here - they also violate the constraint that (org, code, run) is unique per course run, because all CCX variants of a run have the same base course ID.
4dd8ad7 to
43f7b7c
Compare
An implementation of #469.
Related platform PR: openedx/openedx-platform#38023
Notes
edx-organizations(which also requires Pillow for thelogofield)opaque-keys, which was already an indirect dependency but not used directlydisplay_name, and catalog courses do not have names nor exist in the core platform at all. I'm proposing we add adisplay_namefield to the new coreCatalogCoursemodel to support various use cases, including the proposed new studio home page. See the code for how this can be backfilled and how runs can always override the name for each run.CatalogCourse/CourseRunobjects; just a minimal API that platform code can use to keep them in sync withCourseOverview.course_overviewsapp that syncs data from modulestore ->CourseOverview->openedx_catalog. It would be more robust and future-friendly to instead sync data directly fromSplitModulestoreCourseIndex->openedx_catalog, but it's harder to get information likedisplay_nameandlanguagein the latter case as that's not available in theSplitModulestoreCourseIndextable. It could be retrieved from Mongo though.CourseOverviewbased on thecourse_publishedsignal, any test cases in platform that want to use these models have to be sure to enable that signal, which is disabled for test by default.Architecture Diagram
See ARCHITECTURE.md.
Questions
org_codeandcourse_codegood terms to use? Should I call the latternumberinstead, like other parts of the code do? Should I call the org partorg_short_name?url_codefor each CatalogCourse useful? Do we want to make it editable now, or in the future?Can be addressed later:
CourseRunaSoftDeletableModelto support course deletion without data loss? (Maybe we can't now, because soft-deleting it in that one table wouldn't affect the other tables that the system actually references. But in the future we could add this.)openedx_contentcatalog_visibility,visible_to_staff_only, andcourse_visibilitywhich all have different effects and different enum values, and also need to support "use system default")OrganizationCourse