Repair data that violates this project's own field contracts - #522
Merged
Conversation
A full-database validation sweep (every row through full_clean()) found 31
rows whose values were never valid against the models as declared here.
Django enforces choices and blank= in its validation layer, which the write
paths that produced these rows did not run, so the database accepted them.
purchasing/0009: deletes 12 purchase-order lines that hold nothing at all
(blank description, qty 1, no cost, nothing received, no job, no Xero id, no
item codes, no raw payload, price_tbc unset); describes the 5 blank-
description lines that DO carry data, one of which has 2 units received at
$119.50 against a job; and maps the single status 'void' -> 'deleted'.
quoting/0004: unsets 13 mapped_metal_type values that were never valid
choices ('unspecified' x10, 'steel' x2, 'tungsten' x1) and clears their
parser_version so the parser re-derives them, skipping any row an operator
has hand-validated.
Verified by replaying both migrations against a restore of production inside
a transaction: 12/5/1/13 rows affected, leaving zero blank descriptions,
zero 'void' statuses and zero out-of-enum metal types.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013wDoeUfmKL3AnXzpYToyPk
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds two irreversible Django data migrations. The purchasing migration repairs empty lines, blank descriptions, and invalid ChangesPurchasing data repair
Quoting data repair
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A full-database validation sweep — every row of every model through
full_clean()— found 31 rows whose values were never valid against the models as declared in this repository. Django enforceschoicesandblank=in its validation layer, which the write paths that produced these rows never ran, so the database accepted them. They are invisible until something edits them.Found while preparing the v2 rewrite's data migration, but nothing here is a v2 requirement: every row below is a pre-existing defect against v1's own contracts.
purchasing/0009void→deleted(voidhas never been a choice)The deletion predicate is deliberately conservative — blank description and qty 1 and no unit cost and nothing received and no job and no Xero line id and no item codes and no metal/alloy and no dimensions/specifics/location and no raw import payload and
price_tbcunset. Any single populated column keeps the row.That matters: an initial cut of "all 17 blank-description lines are junk" would have deleted PO-0040, which has 2 units received at $119.50 allocated to a job, and PO-0027, which someone had flagged
price_tbc. Those keep their rows and gain a marker description instead.Verified before writing that
Stock.source_purchase_order_lineis the only inbound FK and that none of the deleted lines is referenced by one.quoting/0004Unsets 13
mapped_metal_typevalues that were never valid choices —unspecified×10,steel×2,tungsten×1 — and clears theirparser_versionso the parser re-derives them on its next run.They are unset rather than remapped by hand, because guessing would fabricate data:
unspecifiedmeans the parser had no answer, a wire brush described assteelis not a steel product, and a tungsten TIG electrode has no home in the enum. Rows an operator has hand-validated are excluded — their decision outranks both the parser and this migration. All 13 are currently unvalidated, so the guard is a no-op today; it exists because this database keeps taking writes until cutover.Verification
Both migrations were replayed against a restore of production inside a transaction, then rolled back:
Both are irreversible by design: reverse cannot tell a row this migration described from one that was always described, nor resurrect a deleted row, so reverse is a no-op rather than a wrong restore (house pattern:
purchasing/0007_text_unset_is_null).🤖 Generated with Claude Code
https://claude.ai/code/session_013wDoeUfmKL3AnXzpYToyPk
Summary by CodeRabbit