-
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
Draft
ormsbee
wants to merge
10
commits into
openedx:master
Choose a base branch
from
ormsbee:upgrade-to-refactored-lc
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+120
−21
Draft
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
45b17f8
temp: consolidate learning core app install to api call
ormsbee d9af181
refactor: rewrite some history to pretend the learning core models we…
ormsbee 188774d
temp: remove migration history rewriting, since we're keeping the old…
ormsbee 8402e5c
temp: move all dependencies to assume oel_authoring
ormsbee cddb3a0
temp: revert temp: move all dependencies to assume oel_authoring
ormsbee 811126d
temp: one-to-one field must be updated
ormsbee 219c5c5
fix: prevent migration ordering inconsistencies
ormsbee a85bec5
refactor: change oel_authoring references to be openedx_content
ormsbee 5e4e574
temp: adjust requirements to pull in my branch for testing
ormsbee 81adb60
temp: try to install from branch, again
ormsbee File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
31 changes: 31 additions & 0 deletions
31
cms/djangoapps/contentstore/migrations/0015_alter_componentlink_upstream_block_and_more.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # Generated by Django 5.2.10 on 2026-01-25 21:52 | ||
|
|
||
| import django.db.models.deletion | ||
| from django.db import migrations, models | ||
| from django.db.migrations.operations.special import SeparateDatabaseAndState | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('contentstore', '0014_remove_componentlink_downstream_is_modified_and_more'), | ||
| ('openedx_content', '0001_initial'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| SeparateDatabaseAndState( | ||
| database_operations=[], | ||
| state_operations=[ | ||
| migrations.AlterField( | ||
| model_name='componentlink', | ||
| name='upstream_block', | ||
| field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='links', to='openedx_content.component'), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='containerlink', | ||
| name='upstream_container', | ||
| field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='links', to='openedx_content.container'), | ||
| ), | ||
| ] | ||
| ), | ||
| ] |
46 changes: 46 additions & 0 deletions
46
...re_migrator/migrations/0007_alter_modulestoreblockmigration_change_log_record_and_more.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # Generated by Django 5.2.10 on 2026-01-25 21:52 | ||
|
|
||
| import django.db.models.deletion | ||
| from django.db import migrations, models | ||
| from django.db.migrations.operations.special import SeparateDatabaseAndState | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('modulestore_migrator', '0006_alter_modulestoreblocksource_forwarded_and_more'), | ||
| ('openedx_content', '0001_initial'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| SeparateDatabaseAndState( | ||
| database_operations=[], | ||
| state_operations=[ | ||
| migrations.AlterField( | ||
| model_name='modulestoreblockmigration', | ||
| name='change_log_record', | ||
| field=models.OneToOneField(null=True, on_delete=django.db.models.deletion.SET_NULL, to='openedx_content.draftchangelogrecord'), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='modulestoreblockmigration', | ||
| name='target', | ||
| field=models.ForeignKey(blank=True, help_text='The target entity of this block migration, set to null if it fails to migrate', null=True, on_delete=django.db.models.deletion.CASCADE, to='openedx_content.publishableentity'), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='modulestoremigration', | ||
| name='change_log', | ||
| field=models.ForeignKey(help_text='Changelog entry in the target learning package which records this migration', null=True, on_delete=django.db.models.deletion.SET_NULL, to='openedx_content.draftchangelog'), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='modulestoremigration', | ||
| name='target', | ||
| field=models.ForeignKey(help_text='Content will be imported into this library', on_delete=django.db.models.deletion.CASCADE, to='openedx_content.learningpackage'), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='modulestoremigration', | ||
| name='target_collection', | ||
| field=models.ForeignKey(blank=True, help_text='Optional - Collection (within the target library) into which imported content will be grouped', null=True, on_delete=django.db.models.deletion.SET_NULL, to='openedx_content.collection'), | ||
| ), | ||
| ] | ||
| ), | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 changes: 26 additions & 0 deletions
26
...ore/djangoapps/content_libraries/migrations/0012_alter_contentlibrary_learning_package.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # Generated by Django 5.2.9 on 2026-01-25 19:44 | ||
|
|
||
| import django.db.models.deletion | ||
| from django.db import migrations, models | ||
| from django.db.migrations.operations.special import SeparateDatabaseAndState | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('content_libraries', '0011_remove_contentlibrary_bundle_uuid_and_more'), | ||
| ('openedx_content', '0001_initial'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| SeparateDatabaseAndState( | ||
| database_operations=[], | ||
| state_operations=[ | ||
| migrations.AlterField( | ||
| model_name='contentlibrary', | ||
| name='learning_package', | ||
| field=models.OneToOneField(default=None, null=True, on_delete=django.db.models.deletion.RESTRICT, to='openedx_content.learningpackage'), | ||
| ), | ||
| ] | ||
| ), | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Uh oh!
There was an error while loading. Please reload this page.
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:
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.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.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.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:
migration0001_initial` requires that all `backcompat.*` migrations except the last ones removing model state are run.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.backcompat.*migrations that remove model field state will listopenedx_contentapp's0002_rename_tables_to_openedx_contentas a dependency.A further complication is that
openedx_learningwill often run its migrations without edx-platform present (e.g. for CI or standalone dev purposes), so we can't force0002_rename_tables_to_openedx_contentin theopenedx_contentapp 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 theapps.pyinitialization for theopenedx_contentapp.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_APPSfor 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.
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.
Ah, right. Thanks. I'm still trying to understand this a bit more so I'll ask questions on the ADR PR.