feat: Use openedx_catalog app, backfill its CourseRuns [FC-0117]#38023
feat: Use openedx_catalog app, backfill its CourseRuns [FC-0117]#38023bradenmacdonald wants to merge 16 commits intoopenedx:masterfrom
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. |
|
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. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where 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. |
c17ed68 to
08950bf
Compare
21f3493 to
c569920
Compare
2e97a3f to
be726d3
Compare
.../core/djangoapps/content/course_overviews/migrations/0030_backfill_new_catalog_courseruns.py
Outdated
Show resolved
Hide resolved
| from openedx_catalog import api as catalog_api | ||
| from openedx_catalog.models_api import CatalogCourse, CourseRun |
There was a problem hiding this comment.
I'm following our existing convention here, but it seems a tiny bit annoying to import some things from models_api and everything else from api when it's all part of the public API.
There was a problem hiding this comment.
yeah I'd be up to revisiting the models_api vs api conversation soon, but for this PR I'd vote keep it like this
4cc7ab6 to
b1a10b7
Compare
b1a10b7 to
5d2ec4b
Compare
| CourseRun = apps.get_model("openedx_catalog", "CourseRun") | ||
|
|
||
| created_catalog_course_ids: set[int] = set() | ||
| all_course_runs = CourseIndex.objects.filter(base_store="mongodb", library_version="").order_by("-pk") |
There was a problem hiding this comment.
Lobotomized Old Mongo courses still exist for the purposes of displaying in the Learner Dashboard, reporting enrollments, etc. That's unlikely to change because there are grades and credentials associated with that user data.
I think we should derive the list of course runs only from CourseOverview.
There was a problem hiding this comment.
(I mean, I guess we could do it from the mixed modulestore interface, but I assume you wanted to use the model because it's fast.)
There was a problem hiding this comment.
Yeah, I'll change it to just use CourseOverview.
There was a problem hiding this comment.
Updated.
Lobotomized Old Mongo courses still exist for the purposes of displaying in the Learner Dashboard, reporting enrollments, etc.
We really need to fix our devstack to include useful default courses including something like this, if we're expected to support it indefinitely.
ormsbee
left a comment
There was a problem hiding this comment.
Aside from the for-testing-purposes github requirements thing, LGTM. Thank you!
| NORMALIZE_LANGUAGE_CODES = { | ||
| "zh-hans": "zh-cn", | ||
| "zh-hant": "zh-hk", | ||
| "ca@valencia": "ca-es-valencia", |
| raise ValueError( | ||
| f'The organization short code "{org_code}" exists in modulestore ({course_key}) but ' | ||
| "not the Organizations table, and auto-creating organizations is disabled. You can resolve this by " | ||
| "creating the Organization manually (e.g. from the Django admin) or turning on auto-creation. " | ||
| "You can set active=False to prevent this Organization from being used other than for historical data. " | ||
| ) from exc |
There was a problem hiding this comment.
This is a great error message, thank you.
Description
This is the openedx-platform companion PR to openedx/openedx-core#479 . Please see that PR for more information.