Repository navigation
fix: match openedx-platform's SIMPLE_HISTORY_DATE_INDEX in test settings - #853
Conversation
|
Thanks for the pull request, @ufedaseyeuconsultant! 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. |
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
This comment is a collaboration between Claude and me.
@ufedaseyeuconsultant Thanks for tracking this down. Since the point of this PR is getting openedx/openedx-platform#39199 green, there are a few more things to fold in before it's ready, plus one change on the platform PR itself.
1. Migration number conflict. #852 merged this morning and added 0008_competency_mastery_status, 0009, and 0010, and its 0008 also depends on 0007_alter_criterion_override_help_text. That means this PR's 0008 forks the migration graph, and Django will refuse to migrate with a "Conflicting migrations detected" error once both are on main. GitHub still shows this as mergeable because the filenames are different. Could you rebase on main, delete this PR's 0008, and rerun makemigrations so it comes out as 0011 depending on 0010_studentcompetencystatus? Heads up that #854 also adds a 0011 (and 0012), so whichever of the two PRs merges second will need to renumber.
2. Lint Python Imports failure on #39199. src/openedx_learning/applets/cbe/models/criteria.py:18 does from openedx_catalog.models import CourseRun. openedx-platform lists both openedx_catalog and openedx_learning as isolated apps in its import-linter config, which only allows other code to import from an app's api, models_api, data, and tests modules. openedx_catalog/models_api.py already re-exports CourseRun for exactly this purpose (as a foreign key target), so changing that line to from openedx_catalog.models_api import CourseRun should clear it. The foreign key still points at the same model, so this doesn't need a migration.
3. Compile requirements failure on #39199 (that PR, not this one). That check regenerates requirements/edx/base.txt and requirements/edx/development.txt and fails because the regenerated files don't match what's committed. Once this PR merges and semantic-release cuts the new patch version, #39199 will need to bump to that version instead of 1.6.0 anyway, so rerunning make upgrade-package package=openedx-core and committing the regenerated files should take care of both. I haven't confirmed exactly why the files differ today, so let me know if it still fails after a clean regenerate.
@ormsbee Could you add this one to your review queue alongside #854? It's blocking QA on #758, which was also part of our Oct 2 goal. I also wanted to check one thing with you: with this change, openedx-core's committed migrations only stay in sync if SIMPLE_HISTORY_DATE_INDEX in its test settings matches whatever openedx-platform sets, because django-simple-history bakes that setting into the history_date index at migration time. Should openedx-core's test settings mirror openedx-platform's for this case?
6477f59 to
cd51ceb
Compare
|
@mgwozdz-unicon done |
ormsbee
left a comment
There was a problem hiding this comment.
Thanks for tracking this down, and for the writeup. The diagnosis checks out: django-simple-history reads SIMPLE_HISTORY_DATE_INDEX while it's building each historical model class (models.py#L568 and #L594-L606 in 3.13.0), so whatever that setting is when someone runs makemigrations is what ends up in the migration file. Claude ran the CBE tests and makemigrations --check on this branch for me, and both are clean under test_settings.
I also wanted to check one thing with you: with this change, openedx-core's committed migrations only stay in sync if SIMPLE_HISTORY_DATE_INDEX in its test settings matches whatever openedx-platform sets, because django-simple-history bakes that setting into the history_date index at migration time. Should openedx-core's test settings mirror openedx-platform's for this case?
@mgwozdz-unicon: Yes. Any setting that feeds into model state needs to match openedx-platform, since that's where the migrations we ship actually run. It's a frustrating bit of coupling, but it shows up in other repos too (e.g. edx-organizations). 😞
Only one blocking item: Please set it in projects/dev.py as well (inline). That's the settings module manage.py defaults to, and running makemigrations under it on this branch puts the index right back. I would definitely run into this personally.
I also belatedly realized that the migrations will have to be renumbered, since I just merged #854.
Thank you.
Claude drafted the first pass of this review and ran the CBE tests and makemigrations --check for me against test_settings, projects.dev, and plain main.
| # must set it to whatever its actual consumer uses, or installing those migrations into that | ||
| # consumer's project drifts out of sync with its live model state. openedx-platform (this | ||
| # library's primary consumer) sets this to False in openedx/envs/common.py. | ||
| SIMPLE_HISTORY_DATE_INDEX = False |
There was a problem hiding this comment.
Please add this to projects/dev.py too, since that's what manage.py uses when DJANGO_SETTINGS_MODULE isn't set. Claude ran makemigrations --check --dry-run on this branch under projects.dev, and it generates a 0012 that re-adds the index on all three tables (plus one for organizations). A one-line comment there pointing back at this one is enough–no need to repeat the explanation.
| # django-simple-history bakes this setting's value into the db_index it generates for every | ||
| # HistoricalRecords() model's history_date field, so a library that ships committed migrations | ||
| # must set it to whatever its actual consumer uses, or installing those migrations into that | ||
| # consumer's project drifts out of sync with its live model state. openedx-platform (this | ||
| # library's primary consumer) sets this to False in openedx/envs/common.py. | ||
| SIMPLE_HISTORY_DATE_INDEX = False |
There was a problem hiding this comment.
Nit: This explains the mechanism, but not why openedx-platform made the choice, which is the first thing the next person is going to ask when they wonder whether it can be flipped back. Maybe something like?
| # django-simple-history bakes this setting's value into the db_index it generates for every | |
| # HistoricalRecords() model's history_date field, so a library that ships committed migrations | |
| # must set it to whatever its actual consumer uses, or installing those migrations into that | |
| # consumer's project drifts out of sync with its live model state. openedx-platform (this | |
| # library's primary consumer) sets this to False in openedx/envs/common.py. | |
| SIMPLE_HISTORY_DATE_INDEX = False | |
| # django-simple-history bakes this setting into the history_date field of every | |
| # HistoricalRecords() model, and therefore into any migration we generate for one. | |
| # openedx-platform has had this set to False since its 2023 django-simple-history | |
| # upgrade (openedx/openedx-platform#32880), to avoid adding an index to every | |
| # historical table in the platform at once. Our migrations run there, so this has | |
| # to match. | |
| SIMPLE_HISTORY_DATE_INDEX = False |
| from simple_history.models import HistoricalRecords | ||
|
|
||
| from openedx_catalog.models import CourseRun | ||
| from openedx_catalog.models_api import CourseRun |
There was a problem hiding this comment.
Nit: This is the right fix, but it bothers me a little that openedx-platform's import-linter had to be the one to catch it. Our .importlinter only layers the top-level packages and says nothing about which modules of openedx_catalog the layers above may reach into, which is the whole reason models_api.py exists. We should add the equivalent of openedx-platform's isolated_apps contract (pyproject.toml, the one with allowed_modules = ["api", "models_api", "data", "tests"]) on the openedx-core side, so the next one of these fails here instead.
Not required to merge, but I would appreciate it if you could add it here or to a follow-up PR.
There was a problem hiding this comment.
Thinking on this more, the contract stuff should definitely be a follow-up PR, so we unblock the upgrade as soon as possible. Thank you.
There was a problem hiding this comment.
Will be changed in the follow-up PR
django-simple-history bakes the SIMPLE_HISTORY_DATE_INDEX setting's value into the db_index it generates for every HistoricalRecords() model's history_date field. This library's test settings never set it, so its committed migrations recorded db_index=True (the django-simple-history default) for CompetencyCriteriaGroup, CompetencyCriterion, and CompetencyRuleProfile's historical tables. openedx-platform, the primary consumer these models are built for, sets SIMPLE_HISTORY_DATE_INDEX = False globally in openedx/envs/common.py, so installing this library there makes the live model state disagree with the shipped migrations: Django detects a missing migration for history_date on every one of these three tables the moment openedx-platform's own test suite loads them, failing its MigrationTests.test_migrations_are_in_sync check. Sets the same value in test_settings.py so this library's own migrations match what its actual consumer produces, and adds the migration that `makemigrations` generates as a result. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rebasing onto upstream/main (not this fork's lagging origin/main) put this branch's own 0008 migration side by side with upstream's unrelated 0008_competency_mastery_status, 0009, and 0010 (all added after this branch forked), forking the migration graph since both chains depend on 0007. Deletes this branch's old 0008 and regenerates it via makemigrations so it lands as 0011, depending on 0010_studentcompetencystatus; its operations are otherwise identical to the deleted file. Also switches criteria.py's CourseRun import from openedx_catalog.models to openedx_catalog.models_api. openedx-platform's own import-linter config treats openedx_catalog as an isolated app, reachable only through its api/models_api/data/tests modules, so the direct .models import fails that check once openedx-platform installs this library, even though this repo's own, more permissive layering contract never caught it. models_api already re-exports CourseRun for exactly this kind of foreign-key reference; the FK still targets the same model, so no migration is needed for this part. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
manage.py defaults to projects.dev when DJANGO_SETTINGS_MODULE isn't set, and running makemigrations under it (the path a contributor is most likely to hit locally) regenerated the history_date index on all three CompetencyCriteria* historical tables, undoing this fix. projects/dev.py now sets the same value, pointing back at test_settings.py's comment instead of repeating it. Also replaces that comment with one that explains why openedx-platform chose False, not just the baking mechanism: it's been set since openedx-platform's 2023 django-simple-history upgrade (openedx/openedx-platform#32880), to avoid adding an index to every historical table in the platform at once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rebased onto upstream/main, which merged two more migrations (0011_studentcompetencycriterionstatus, 0012_studentcompetencycriteriagroupstatus) since this branch's migration was last renumbered, forking the graph again. Deletes the old 0011 and regenerates via makemigrations, landing as 0013 depending on 0012_studentcompetencycriteriagroupstatus; the field operations are otherwise identical to the deleted file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cd51ceb to
f42e6b2
Compare
|
@ormsbee Migration updated, attr added |
Rebased onto upstream/main, which merged openedx#853 since this branch was last renumbered, adding its own 0013 migration depending on the same 0012_studentcompetencycriteriagroupstatus this branch's migration also depended on. Deletes the old 0013 and regenerates via makemigrations, landing as 0014; the field operations are otherwise identical to the deleted file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drops the commit hash and the "copied back and forth" sentence from the module docstring (nothing copies this file back to openedx-platform, so that process doesn't exist), removes the two method docstrings now that the file is closer to openedx-platform's original, and cuts the list() workaround's annotations and three-line comment down to one line (the annotations did nothing: mypy skips the untyped check() method's body, and pylint passes without them either way). Also shortens the comment above the new contract in .importlinter: drops the clause about how the bad import happened, since that history belongs in openedx#853 and openedx#860, not in the config. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Description
Fixes a migration/model drift that only surfaces once this library's models are installed into
openedx-platform: everyHistoricalRecords()table (CompetencyCriteriaGroup,CompetencyCriterion,CompetencyRuleProfile) shipped a migration recordinghistory_date = models.DateTimeField(db_index=True), butdjango-simple-historyderives that field'sdb_indexfrom theSIMPLE_HISTORY_DATE_INDEXDjango setting at the time the historical modelclass is built (
"history_date": models.DateTimeField(db_index=self._date_indexing is True),confirmed identical in
django-simple-history3.12.0 and 3.13.0 source, so this is not alibrary-version issue). This library's own
test_settings.pynever set that setting, so itsmigrations were generated against the
django-simple-historydefault (indexed).openedx-platformsets
SIMPLE_HISTORY_DATE_INDEX = Falseglobally inopenedx/envs/common.py, so the moment thesemodels load there, the live model state (
db_index=False) disagrees with what this library's ownmigrations recorded, and
openedx-platform'sMigrationTests.test_migrations_are_in_syncfailswith a pending
AlterFieldonhistory_datefor all three historical tables.Sets
SIMPLE_HISTORY_DATE_INDEX = Falsein this library's owntest_settings.py, matchingopenedx-platform's setting (the primary consumer these models are built for), and adds themigration
makemigrationsgenerates as a result, dropping the now-mismatched index onhistory_datefor all three affected tables.This change impacts the Developer and Operator roles (a migration-correctness fix with no
behavioral effect on authoring or learning functionality). It has no UI effect.
Supporting information
openedx-platformPR to bump this library to 1.6.0,which failed CI on this exact
test_migrations_are_in_synccheck once 1.6.0 was installedthere.
Testing instructions
DJANGO_SETTINGS_MODULE=test_settings pytest --no-cov— full suite passes (914 passed, 1skipped locally).
DJANGO_SETTINGS_MODULE=test_settings python manage.py makemigrations --check --dry-runandthe same under
mysql_test_settings— both report "No changes detected".openedx-platformcheckout, add a temporary
[tool.uv.sources]override pointingopenedx-coreat this branch(
{ path = "<path-to-this-checkout>", editable = true }),uv lock,uv sync --group testing --frozen --python 3.12(match CI's Python version; a newer local default can pick aninterpreter
xmlsechas no prebuilt wheel for, an unrelated build failure), then:the temporary
pyproject.toml/uv.lockchanges afterward (git checkout --); they're a localverification aid, not meant to be committed there.
Deadline
Unblocks an in-progress
openedx-platformPR bumping this library to 1.6.0 for QA(openedx/openedx-platform#39199)Other information
value here is whatever
SIMPLE_HISTORY_DATE_INDEXopenedx-platformsets — if that settingvalue ever changes there, this library's migrations would need a matching follow-up. No code
dependency on this PR, though.
consumer), not an architectural decision.
python-semantic-releasedriven by conventional commit prefixes; the
fix:commit here triggers an automatic patchrelease once merged.
SIMPLE_HISTORY_DATE_INDEX = False(i.e. relies ondjango-simple-history's own default) willnow see the inverse drift.
django-simple-historyhas no per-model override forhistory_datespecifically (
no_db_indexonly affects fields copied from the tracked model, not thissynthetic one), so there is no way to make this field setting-independent via its current public
API. Pinning this library's own test settings to its actual primary consumer's choice is the
available option here, not a complete fix for every possible consumer.
AlterFielddropping an index (DROP INDEXunder the hood onmost backends). Reversible, no data loss, no backfill required.
🤖 Generated with help of Claude Code