fix(webapp): refuse to approve a trashed lore proposal (#630) #631
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/630-approve-trashed-proposal"
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?
Follow-up to #629, where this was found and deliberately left out because it is a write path rather than a count.
The defect
POST /campaigns/{id}/lore/{entry_id}/approveis one of the few proposal paths that does not funnel throughlore_service.get_lore_entry, which appliesdeleted_at IS NULLfor everyone else.approve_lore_proposalclearsproposed_by_botbut leavesdeleted_atset, so approving a trashed proposal produced an entry that was neither a proposal nor a visible entry: in the trash, counting down to_purge_trashed_lore_entries_async, with the GM shown a success response and nothing to see anywhere.Silent in both directions — no error, and nothing appears in the wiki. Reachable from a stale tab, a double submit or a replayed request; the listing filters correctly, so the id only has to be slightly out of date.
The fix
One predicate on the lookup, so it answers 404 like every other read path does for a trashed entry. The entry stays trashed and still a proposal, which matters: restoring it puts it back in the review queue to be approved properly, rather than restoring an entry the GM never knowingly approved.
I chose the 404 over the "undelete and approve" reading the issue offered as an alternative. Approving something you deleted is far more likely to be a stale request than a deliberate act, and the restore flow already exists for when it is deliberate.
The siblings the issue asked about
DELETE /lore/{entry_id}already goes throughget_lore_entry, so a trashed proposal 404s there today. No change needed.LoreRelationshiphas nodeleted_atcolumn at all; relationships are hard-deleted. There is no equivalent gap.Tests
Both added to
test_lore_soft_delete.py, whose header says there is deliberately a test per surface rather than one test for the flag — approve was a surface it was missing:proposed_by_bot) — confirmed to fail against the pre-fix source;Backend suite:
3432 passed, 13 skipped.ruff checkandruff format --checkclean under CI's pinned 0.4.4.Closes #630
🤖 Generated with Claude Code
The approve endpoint is one of the few proposal paths that does not funnel through `lore_service.get_lore_entry`, which applies `deleted_at IS NULL` for everyone else. `approve_lore_proposal` clears `proposed_by_bot` but not `deleted_at`, so approving a trashed proposal produced an entry that was neither a proposal nor a visible entry: sitting in the trash, counting down to `_purge_trashed_lore_entries_async`, with the GM shown a success response and nothing to see anywhere. Reachable from a stale tab, a double submit or a replayed request — the listing filters correctly, so the id only has to be slightly out of date — and silent in both directions when it happened. Now a 404, matching the answer every other read path gives for a trashed entry, and the entry stays trashed *and* still a proposal, so restoring it puts it back in the review queue to be approved properly. Checked while in there: the reject path (`DELETE /lore/{entry_id}`) already goes through `get_lore_entry` and is correct, and `LoreRelationship` has no `deleted_at` at all, so the relationship-proposal approve path has no equivalent gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>