From adf8b0243acb772c38cf4b56fd8c032349f61c1e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 9 Sep 2026 17:25:29 -0700 Subject: [PATCH] Remove a book's revision history when the book is destroyed 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. --- app/models/edit.rb | 11 ++++++- app/models/leaf/editable.rb | 2 +- lib/rails_ext/action_text_markdown.rb | 2 +- test/models/book_test.rb | 44 +++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 3 deletions(-) diff --git a/app/models/edit.rb b/app/models/edit.rb index 57bccf91..bdf64da9 100644 --- a/app/models/edit.rb +++ b/app/models/edit.rb @@ -1,9 +1,11 @@ class Edit < ApplicationRecord belongs_to :leaf - delegated_type :leafable, types: Leafable::TYPES, dependent: :destroy + delegated_type :leafable, types: Leafable::TYPES enum :action, %w[ revision trash ].index_by(&:itself) + after_destroy :destroy_superseded_leafable, if: :revision? + scope :sorted, -> { order(created_at: :desc) } scope :before, ->(edit) { where("created_at < ?", edit.created_at) } scope :after, ->(edit) { where("created_at > ?", edit.created_at) } @@ -15,4 +17,11 @@ def previous def next leaf.edits.after(self).first end + + private + # A revision holds the leafable it superseded, which nothing else references. A trash + # edit shares the leaf's current leafable, and that one is the leaf's to destroy. + def destroy_superseded_leafable + leafable&.destroy + end end diff --git a/app/models/leaf/editable.rb b/app/models/leaf/editable.rb index d00f2a69..96987bcc 100644 --- a/app/models/leaf/editable.rb +++ b/app/models/leaf/editable.rb @@ -4,7 +4,7 @@ module Leaf::Editable MINIMUM_TIME_BETWEEN_VERSIONS = 10.minutes included do - has_many :edits, dependent: :delete_all + has_many :edits, dependent: :destroy after_update :record_moved_to_trash, if: :was_trashed? end diff --git a/lib/rails_ext/action_text_markdown.rb b/lib/rails_ext/action_text_markdown.rb index afc0b2b5..a202d69c 100644 --- a/lib/rails_ext/action_text_markdown.rb +++ b/lib/rails_ext/action_text_markdown.rb @@ -36,7 +36,7 @@ module ActionText::Markdown::Uploads extend ActiveSupport::Concern included do - has_many_attached :uploads, dependent: :destroy + has_many_attached :uploads end end diff --git a/test/models/book_test.rb b/test/models/book_test.rb index be0e37d2..de2b8cb1 100644 --- a/test/models/book_test.rb +++ b/test/models/book_test.rb @@ -1,6 +1,8 @@ require "test_helper" class BookTest < ActiveSupport::TestCase + include ActiveJob::TestHelper + test "slug is generated from title" do book = Book.create!(title: "Hello, World!") assert_equal "hello-world", book.slug @@ -40,4 +42,46 @@ class BookTest < ActiveSupport::TestCase test "markable returns empty string for book with no leaves" do assert_equal "", books(:manual).markable end + + test "destroying a book removes superseded revisions along with their content and files" do + page = leaves(:welcome_page).page + page.body.uploads.attach io: file_fixture("pixel.bmp").open, filename: "pixel.bmp", content_type: "image/bmp" + upload_blob = page.body.uploads.blobs.sole + leaves(:welcome_page).edit leafable_params: { body: "Revised body" } + + picture = leaves(:reading_picture).picture + image_blob = picture.image.blob + leaves(:reading_picture).edit leafable_params: { caption: "Revised caption" } + + assert_equal page, leaves(:welcome_page).edits.revision.sole.leafable + assert_equal picture, leaves(:reading_picture).edits.revision.sole.leafable + + perform_enqueued_jobs do + books(:handbook).destroy + end + + assert_not Edit.exists?(leaf_id: leaves(:welcome_page, :reading_picture).map(&:id)) + assert_not Page.exists?(page.id) + assert_not ActionText::Markdown.exists?(record: page) + assert_not Picture.exists?(picture.id) + assert_not ActiveStorage::Attachment.exists?(blob: [ upload_blob, image_blob ]) + assert_not ActiveStorage::Blob.exists?(upload_blob.id) + assert_not ActiveStorage::Blob.exists?(image_blob.id) + assert_not ActiveStorage::Blob.service.exist?(upload_blob.key) + assert_not ActiveStorage::Blob.service.exist?(image_blob.key) + end + + test "destroying a book removes the content a trashed leaf shares with its trash edit" do + page = leaves(:welcome_page).page + leaves(:welcome_page).trashed! + + assert_equal page, leaves(:welcome_page).edits.trash.sole.leafable + + books(:handbook).destroy + + assert_not Leaf.exists?(leaves(:welcome_page).id) + assert_not Edit.exists?(leaf_id: leaves(:welcome_page).id) + assert_not Page.exists?(page.id) + assert_not ActionText::Markdown.exists?(record: page) + end end