Add deferrable FK utility, test case, and migration

[bjester]

Summary

• Adds utility for updating existing FK constraints, that are immediate by default, to have a deferrable flag
  • This corrects an issue where databases that were created prior to Django 3.1 did not set the deferrable flag, and after that, Django enabled FK checks by default. Django relies on deferrable FK constraints for its cascade deletion because it executes deletions out of order (as shown in the issue).
  • The utility works for both a Django migration and manual invocation, such that Kolibri can include it as a version upgrade migration.
• A Django migration was added to invoke the utility explicitly for morango models automatically
• A test verifies the proof-of-concept of how the utility operates, including its preservation of data

Creation of this utility was started in Kolibri, when it was realized other apps (like Morango) may need this. Since Morango is a standalone package with respect to Kolibri, the utility was added here, made to automatically handle morango models, and to allow Kolibri to leverage the utility in its own way.

Reviewer guidance

• Are the tests sufficient?
• Is there test coverage lacking?

I ran Kolibri 0.16.2 to generate an old database with the issue, along with some data (classes and users). With these changes integrated into Kolibri, I then ran both via a Kolibri upgrade on top of the the 0.16.2 home, to verify that it updated the FK constraints. All data was preserved.

I can provide the 0.16.2 home upon request.

Before (0.16.2)

sqlite> .schema kolibriauth_collection
CREATE TABLE IF NOT EXISTS "kolibriauth_collection" (
  "id" char(32) NOT NULL PRIMARY KEY, 
  "_morango_dirty_bit" bool NOT NULL,
   "_morango_source_id" varchar(96) NOT NULL, 
  "_morango_partition" varchar(128) NOT NULL, 
  "name" varchar(100) NOT NULL, 
  "kind" varchar(20) NOT NULL, 
  "dataset_id" char(32) NOT NULL REFERENCES "kolibriauth_facilitydataset" ("id"), 
  "parent_id" char(32) NULL REFERENCES "kolibriauth_collection" ("id")
);

After (pending 0.19.5)

sqlite> .schema kolibriauth_collection
CREATE TABLE IF NOT EXISTS "kolibriauth_collection" (
  "id" char(32) NOT NULL PRIMARY KEY, 
  "_morango_dirty_bit" bool NOT NULL, 
  "_morango_source_id" varchar(96) NOT NULL, 
  "_morango_partition" varchar(128) NOT NULL, 
  "dataset_id" char(32) NOT NULL REFERENCES "kolibriauth_facilitydataset" ("id") DEFERRABLE INITIALLY DEFERRED,
  "name" varchar(100) NOT NULL, 
  "kind" varchar(20) NOT NULL, 
  "parent_id" char(32) NULL REFERENCES "kolibriauth_collection" ("id") DEFERRABLE INITIALLY DEFERRED
);

Issues addressed

Supports resolution of learningequality/kolibri#14884

AI Usage

Creation of this utility was started in Kolibri with AI. It generated the bulk of the utility and the test, then it was brought over to Morango, and a human refactored the utility to make it flexible for both Morango and Kolibri, and to isolate looping over applications.

[rtibblesbot]

Solid implementation — checking sqlite_master, calling _remake_table, and no-oping on non-SQLite backends is clean. CI's only failure is the contributor gate, not test failures.

Findings are suggestions and nitpicks. The parameter-naming issue has a real impact on Kolibri callers; the rest are commentary or test hygiene.

• suggestion: *_app_labels params compare against app_config.name, not Django's label — silent mismatch for Kolibri callers
• suggestion: _remake_table private-API dependency should be documented
• suggestion: mixed immediate/deferrable FK edge case — note as known limitation
• suggestion: atomic=True in run() vs atomic=False in test — worth a clarifying comment
• nitpick: test_iter_apps__all asserts full ordered list; RunPython.noop reverse is the right call

---

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran a phased review pipeline over the pull request diff:

• Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
• Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
• Specialized frontend/backend review passes applied framework-specific lenses where those files changed
• For UI changes: manual QA and an accessibility audit against a live dev server, when available
• Checked CI status and linked issue acceptance criteria
• Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence

[rtibbles]

Code and test changes make sense to me.

[rtibbles]

I had never heard of this function before this week, now I have reviewed two PRs that contain it!

https://github.com/learningequality/kolibri/pull/14881/changes#diff-ca76c44748846b391bdb2cebc1b735de22710e703b7c79cb50d8bf6f2b70de14R30

[rtibblesbot]

All 6 prior findings resolved. Two new suggestions, one coverage gap.

CI is failing on contributor-gate due to an expired App token — infrastructure, not test failure.

• suggestion (inline): deferrable_foreign_keys.py:125 — DatabaseError caught and not re-raised; with atomic=False on the migration, Django still records it as applied even if individual table rebuilds silently fail. If the intent is best-effort, document the contract explicitly and consider logging which tables were skipped so operators can identify them.
• suggestion (inline): test_deferrable_foreign_keys.py:70 — test_iter_apps__all hardcodes the full INSTALLED_APPS list; any change to test settings breaks this test for unrelated reasons. Prefer [app.name for app in global_apps.get_app_configs()] as the expected value.
• suggestion: The three test_iter_apps__* tests only use apps where label == name ('morango', 'facility_profile'), so the app_config.label vs app_config.name fix has no regression coverage. A one-liner would lock it in:

  def test_iter_apps__include_by_label(self):
      # 'auth' label != 'django.contrib.auth' name
      op = MakeForeignKeysDeferrable(include_app_labels=['auth'])
      self.assertEqual([app.name for app in op._iter_apps(global_apps)], ['django.contrib.auth'])

Prior-finding status

RESOLVED — morango/deferrable_foreign_keys.py — app_config.name vs app_config.label mismatch
RESOLVED — morango/deferrable_foreign_keys.py — _remake_table private-API documentation
RESOLVED — morango/deferrable_foreign_keys.py — mixed immediate/deferrable FK edge case
RESOLVED — morango/deferrable_foreign_keys.py — atomic=True in run() vs atomic=False in test
WITHDRAWN — tests/testapp/tests/test_deferrable_foreign_keys.py:70 — ordered list assertion (withdrawn prior round)
RESOLVED — tests/testapp/tests/test_deferrable_foreign_keys.py — string-formatting table name without quote_name

---

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

• Retrieved prior bot reviews via the GitHub API
• Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
• Only raised NEW findings for newly introduced code
• Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
• Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence

[rtibblesbot]

suggestion: DatabaseError is caught here but not re-raised. With atomic=False on the migration, there is no wrapping transaction to roll back, so Django will mark the migration as applied even if some table rebuilds silently failed — and the idempotency check at the top means those tables will never be retried. If the intent is best-effort (rebuild as many tables as possible rather than aborting on first failure), document that contract explicitly and emit a summary log at the end listing any tables that failed, so operators can identify and repair them manually. If full atomicity is desired, remove the try/except and let the exception propagate.

[rtibblesbot]

suggestion: This assertion hardcodes the full ordered INSTALLED_APPS list. Any addition, removal, or reordering of apps in the test settings will break this test for reasons unrelated to _iter_apps. Derive the expected value dynamically instead:

expected = [app.name for app in global_apps.get_app_configs()]
self.assertEqual([app.name for app in op._iter_apps(global_apps)], expected)