feat(wiki): deleting an entry no longer destroys its history (#408) #477
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/408-lore-soft-delete"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The soft-delete half of #408, completing it. The restore half landed in #476.
The gap
lore_entry_versions.lore_entry_idis ON DELETE CASCADE, so a hard delete took the entry's entire edit history with it in the same statement. A GM who deleted what they believed was a duplicate — and realised afterwards it was the canonical entry with months of accumulated lore — had nothing left to recover from: not the entry, and not even the version list they could have rebuilt it from by hand.Entries now go to a trash (
deleted_at,deleted_by_id), stay recoverable for a grace period, and are removed for real bypurge_trashed_lore_entriesonce it elapses. Migratione3f4a5b6c7d9, verified up → down → up.Filtering reads is the actual work
Soft delete is only correct if every read path excludes trashed rows, and a missed one is a silent bug that surfaces much later as "the deleted entry is still in the search results". All 13
select(LoreEntry)sites are filtered explicitly rather than through a default loader criterion — greppable, and each one somewhere a reviewer can check:get_lore_entry(..., include_deleted=True)is the deliberate opt-in, with exactly two callers: restore, and the purge. There is a test per surface rather than one test for the flag, because that is the failure mode this design has.Two things checked rather than assumed:
LoreEntry.mention including model declarations and annotations. The real read surface is 13 selects, 16 joins and 6 relationships across 5 files — which is what made explicit filtering the right call rather than the default-loader approach I had initially leaned towards.Campaign.lore_entriesis declared but never read anywhere. I had written a comment justifying a filter on it before checking. It now says accurately that the relationship is unused, and that anyone who starts reading it must filter trashed entries themselves.The grace period is a floor, not a preference
get_trash_retention_daysclamps a stored value up to a 7-day minimum — the same treatmentget_audio_trash_retention_daysgets (#427) and for a related reason: months of edit history cannot be regenerated at any price, so the window is enforced in code rather than left to configuration. Defaults to 30 days, because realising you deleted the wrong entry usually happens the next time someone looks for it, which can be a session or two later.retention.trash_purge_atcomputes the boundary once, so the countdown the trash view shows and the moment the purge actually fires cannot drift apart — the same reasonpermanent_deletion_atexists.Endpoints
DELETEnow trashes (still 204 — the caller does not need to care which kind of gone it is), plusGET /lore-trashandPOST /lore/{id}/restore, both GM-only and audited.Verification
21 new tests. 11 mutations, all caught: reverting to a hard delete, dropping the filter from any read surface, a restore that does not clear the flag, a purge that ignores the grace or sweeps live rows, a purge that records nothing, and a retention floor that is only a default.
One test initially survived its mutation: the trash-list test seeded only a trashed entry, so "show the trash" and "show everything" returned the same thing and removing the filter changed nothing. It now seeds a live entry too and asserts it stays out.
Full backend suite 1588 passed;
ruff checkandruff format --checkclean.Next
#405 is the same convention applied to campaigns, plus its typed-confirmation and export-offer criteria.
🤖 Generated with Claude Code
`lore_entry_versions.lore_entry_id` is ON DELETE CASCADE, so a hard delete took the entry's entire edit history with it in the same statement. A GM who deleted what they believed was a duplicate — and realised afterwards it was the canonical entry with months of accumulated lore — had nothing left to recover from: not the entry, and not even the version list they could have rebuilt it from by hand. Entries now go to a trash (`deleted_at`, `deleted_by_id`), stay recoverable for a grace period, and are removed for real by `purge_trashed_lore_entries` once it elapses. Migration e3f4a5b6c7d9. Filtering reads is the actual work ---------------------------------- Soft delete is only correct if every read path excludes trashed rows, and a missed one is a silent bug that surfaces much later as "the deleted entry is still in the search results". All 13 `select(LoreEntry)` sites are filtered explicitly rather than through a default loader criterion — greppable, and each one is somewhere a reviewer can check: wiki list · search · bot proposal queue · the single-entry getter · the bot's /ask context · character-to-wiki matching · shelf card lookups · three draft generation flows · merge source proposals · campaign-wide lore load `get_lore_entry(..., include_deleted=True)` is the deliberate opt-in and has exactly two callers: restore, and the purge. There is a test per surface rather than one test for the flag, because that is the failure mode this design has. Two things I checked rather than assumed: - I had told the user this was "80 places across 20 files". That was a count of every `LoreEntry.` mention, including model declarations and annotations. The real read surface is 13 selects, 16 joins and 6 relationships across 5 files — which is what made explicit filtering the right call rather than a default loader criterion. - `Campaign.lore_entries` is declared but **never read anywhere**, so it has no filter. I had written a comment justifying one before checking; that comment now says accurately that it is unused and that anyone who starts reading it must filter it themselves. The grace period is a floor, not a preference --------------------------------------------- `get_trash_retention_days` clamps a stored value up to a 7-day minimum, the same treatment `get_audio_trash_retention_days` gets and for a related reason: months of edit history cannot be regenerated at any price, so the window is enforced in code rather than left to configuration. Defaults to 30 days — realising you deleted the wrong entry usually happens the next time someone looks for it, which can be a session or two later. `retention.trash_purge_at` computes the boundary once, so the countdown the trash view shows and the moment the purge fires cannot drift apart — the same reason `permanent_deletion_at` exists. Endpoints: DELETE now trashes (still 204 — the caller does not need to care which kind of gone), plus GET /lore-trash and POST /lore/{id}/restore, both GM-only and audited. Verification: 21 new tests, 11 mutations all caught — reverting to a hard delete, dropping the filter from any read surface, a restore that does not clear the flag, a purge that ignores the grace or sweeps live rows, a retention floor that is only a default. One test initially survived its mutation because it seeded only a trashed entry, so "show the trash" and "show everything" looked identical; it now seeds a live entry too. Migration verified up/down/up. Full backend suite 1588 passed; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>