-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Switch to use refactored openedx_content app
#37924
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
base: master
Are you sure you want to change the base?
Conversation
ormsbee
commented
Jan 21, 2026
- temp: consolidate learning core app install to api call
- refactor: rewrite some history to pretend the learning core models were always in oel_authoring
…re always in oel_authoring
This reverts commit 5706f208877a4ff3d250314e8e9d3aa0051eeee7.
c1d35e0 to
219c5c5
Compare
openedx_content app
|
@kdmccormick, @bradenmacdonald: This should work with the new |
| # The openedx_learning apps require contentstore, modulestore_migrator, | ||
| # content.search, and content_staging to be in INSTALLED_APPS. If they are | ||
| # not here and LMS migrations are run before CMS migrations, it will cause | ||
| # errors (certain openedx_learning apps ) | ||
| *openedx_learning_apps_to_install(), | ||
| 'cms.djangoapps.contentstore', | ||
| 'cms.djangoapps.modulestore_migrator', | ||
| 'openedx.core.djangoapps.content.search', | ||
| 'openedx.core.djangoapps.content_staging', |
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.
Can you expand on this? First, adding these CMS apps to LMS seems like a big change and I'd prefer to avoid it for now. Second, how do the "openedx_learning apps require contentstore" ? It doesn't make sense to me that Learning Core has any dependencies on Platform, and I thought we very explicitly don't want that ever.
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 messy. I go over this in more detail in the revised ADR (relevant part pasted below):
The ordering of these migrations is important, and existing edx-platform migrations should remain unchanged. This is important to make sure that we do not introduce ordering inconsistencies for existing installations that are upgrading.
Therefore, the migrations will happen in the following order:
- All
backcompat.*apps migrations except for the final ones that delete model state. This takes us up to where migrations would already be before we make any changes. - The
openedx_contentapp's0001_intialmigration that adds model state without changing the database. At this point, model state exists for the same models in all the oldbackcompat.*apps as well as the newopenedx_contentapp. - edx-platform apps that had foreign keys to old
backcompat.*apps models will need to be switched to point to the newopenedx_contentapp models. This will likewise be done without a database change, because they're still pointing to the same tables and columns. - Now that edx-platform references have been updated, we can delete the model state from the old
backcompat.*apps and rename the underlying tables (in either order).
The tricky part is to make sure that the old backcompat.* apps models still exist when the edx-platform migrations to move over the references runs. This is problematic because the edx-platform migrations can only specify that they run after the new openedx_content models are created. They cannot specify that they run before the old backcompat models are dropped.
So in order to enforce this ordering, we do the following:
- The``openedx_content
migration0001_initial` requires that all `backcompat.*` migrations except the last ones removing model state are run. - The
openedx_contentmigration0002_rename_tables_to_openedx_contentmigration requires that the edx-platform migrations changing references over run. This is important anyway, because we want to make sure those reference changes happen before we change any table names. - The final
backcompat.*migrations that remove model field state will listopenedx_contentapp's0002_rename_tables_to_openedx_contentas a dependency.
A further complication is that openedx_learning will often run its migrations without edx-platform present (e.g. for CI or standalone dev purposes), so we can't force 0002_rename_tables_to_openedx_content in the openedx_content app to have references to edx-platform migrations. To get around this, we dynamically inject those migration dependencies only if we detect those edx-platform apps exist in the currently loaded Django project. This injection happens in the apps.py initialization for the openedx_content app.
The final complication is that we want these migration dependencies to be the same regardless of whether you're running edx-platform migrations with the LMS or CMS (Studio) settings, or we run the risk of getting into an inconsistent state and dropping the old models before all the edx-platform apps can run their migrations to move their references. To do this, we have to make sure that the edx-platform apps that reference Learning Core models are present in the INSTALLED_APPS for both configurations.
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.
Honestly, this has convinced me even more that we need to unify everything into one INSTALLED_APPS configuration across CMS and LMS, though I'm not about to attempt that bit of yak shaving just yet.