Remove a book's revision history when the book is destroyed - #489
Conversation
Destroying a book cascades to its leaves, but Leaf#edits was declared dependent: :delete_all, which deletes the edit rows with a raw DELETE and never runs the edits' own callbacks. The Page or Picture each revision had superseded stayed behind with its markdown record, its Active Storage attachments and their blobs: deleting a private book left its earlier drafts and images in the database and on disk. Leaf#edits now destroys, and Edit destroys the leafable it superseded. That is only ever a revision's leafable: a trash edit shares the leaf's current leafable, and destroying that out from under the leaf broke the leaf's own destroy, so the unconditional dependent: :destroy on Edit#leafable moves to a callback that runs for revisions alone. Markdown uploads also drop their dependent: :destroy. Active Storage only purges a blob for dependent: :purge_later, so even with the cascade in place a destroyed book's embedded images stayed in storage. Nothing destroys a markdown record except destroying its page, and nothing destroys a page except destroying its book, so the default is right and the old value's only effect was to orphan blobs.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approval recommended
The deletion cascade correctly distinguishes owned revision content from shared trash content and is covered by focused regression tests.
Pull request overview
This PR fixes book deletion so revision content and associated Active Storage files are fully removed without prematurely destroying content shared by trash edits.
Changes:
- Destroys
Editrecords through callbacks when a leaf is removed. - Destroys superseded leafables only for revision edits.
- Restores default
purge_laterbehavior for Markdown uploads and adds deletion regression tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
app/models/edit.rb |
Adds revision-specific cleanup of superseded leafables. |
app/models/leaf/editable.rb |
Runs edit destruction callbacks during leaf deletion. |
lib/rails_ext/action_text_markdown.rb |
Uses default deferred blob purging for Markdown uploads. |
test/models/book_test.rb |
Covers revision, trash, attachment, blob, and file cleanup. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Destroying a book cascades to its leaves, but
Leaf has_many :edits, dependent: :delete_alldeleted the edit rows with a rawDELETE, so thedependent: :destroyonEdit#leafablenever ran. The Page or Picture each revision had superseded stayed behind with its ActionText markdown record, its Active Storage attachments and their blobs. Deleting a private book left its earlier drafts and images in the database and on disk; #486 stopped those orphans from being served, this removes them.Fix
Leaf#editsisdependent: :destroy, so each edit runs its callbacks.Editdestroys its leafable only for revisions. A revision holds the superseded copy, which nothing else references. A trash edit shares the leaf's current leafable, and destroying that out from under the leaf broke the leaf's own destroy (Leaf::Searchable'safter_destroy_commitreads the leafable). The unconditionaldependent: :destroyonEdit#leafablewas never sound for trash edits;delete_alljust meant it never fired.uploadsuse Active Storage's defaultdependent: :purge_later.:destroyis not an Active Storage option: only:purge_laterpurges a blob, so even with the cascade fixed a destroyed book's embedded images stayed in storage. Nothing destroys a markdown record except destroying its page, and nothing destroys a page except destroying its book (directly, or through a revision), so the old value's only observable effect was orphaning blobs on book delete.Purging is
_later, matchingBook#cover; the Procfile's resque-pool worker runs the purge jobs.Tests
test/models/book_test.rb:mainat the superseded-page assertion, and still red at the upload-blob assertion with only the cascade fixedFull suite green (222 runs); rubocop, brakeman and importmap audit clean.