From 0fb9d60ee65a4499538541adf740e8466f470bf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Jos=C3=A9=20Mata?= Date: Tue, 3 Feb 2026 15:45:25 +0100 Subject: [PATCH] Use `dependent: :purge_later` for ActiveRecord attachments (#882) * Use dependent: :purge_later for user profile_image cleanup This is a simpler alternative to PR #787's callback-based approach. Instead of adding a custom callback and method, we use Rails' built-in `dependent: :purge_later` option which is already used by FamilyExport and other models in the codebase. This single-line change ensures orphaned ActiveStorage attachments are automatically purged when a user is destroyed, without the overhead of querying all attachments manually. https://claude.ai/code/session_01Np3deHEAJqCBfz3aY7c3Tk * Add dependent: :purge_later to all ActiveStorage attachments Extends the attachment cleanup from PR #787 to cover ALL models with ActiveStorage attachments, not just User.profile_image. Models updated: - PdfImport.pdf_file - prevents orphaned PDF files from imports - Account.logo - prevents orphaned account logos - PlaidItem.logo, SimplefinItem.logo, SnaptradeItem.logo, CoinstatsItem.logo, CoinbaseItem.logo, LunchflowItem.logo, MercuryItem.logo, EnableBankingItem.logo - prevents orphaned provider logos This ensures that when a family is deleted (cascade from last user purge), all associated storage files are properly cleaned up via Rails' built-in dependent: :purge_later mechanism. https://claude.ai/code/session_01Np3deHEAJqCBfz3aY7c3Tk * Make sure `Provider` generator adds it * Fix tests --------- Co-authored-by: Claude --- app/models/account.rb | 2 +- app/models/coinbase_item.rb | 2 +- app/models/coinstats_item.rb | 2 +- app/models/enable_banking_item.rb | 2 +- app/models/lunchflow_item.rb | 2 +- app/models/mercury_item.rb | 2 +- app/models/pdf_import.rb | 2 +- app/models/plaid_item.rb | 2 +- app/models/simplefin_item.rb | 2 +- app/models/snaptrade_item.rb | 2 +- app/models/user.rb | 2 +- .../family/templates/item_model.rb.tt | 2 +- test/models/account_test.rb | 19 +++++++- test/models/pdf_import_test.rb | 17 +++++++ test/models/user_test.rb | 48 +++++++++++++++++++ 15 files changed, 95 insertions(+), 13 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index dd95023a2..d2a86eaf2 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -36,7 +36,7 @@ class Account < ApplicationRecord manual.where.not(status: :pending_deletion) } - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later delegated_type :accountable, types: Accountable::TYPES, dependent: :destroy delegate :subtype, to: :accountable, allow_nil: true diff --git a/app/models/coinbase_item.rb b/app/models/coinbase_item.rb index 667223a23..f67aeb753 100644 --- a/app/models/coinbase_item.rb +++ b/app/models/coinbase_item.rb @@ -24,7 +24,7 @@ class CoinbaseItem < ApplicationRecord validates :api_secret, presence: true belongs_to :family - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later has_many :coinbase_accounts, dependent: :destroy has_many :accounts, through: :coinbase_accounts diff --git a/app/models/coinstats_item.rb b/app/models/coinstats_item.rb index 72da54d1f..c2c9f51fd 100644 --- a/app/models/coinstats_item.rb +++ b/app/models/coinstats_item.rb @@ -22,7 +22,7 @@ class CoinstatsItem < ApplicationRecord validates :api_key, presence: true belongs_to :family - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later has_many :coinstats_accounts, dependent: :destroy has_many :accounts, through: :coinstats_accounts diff --git a/app/models/enable_banking_item.rb b/app/models/enable_banking_item.rb index 8e84cdb62..7de761bde 100644 --- a/app/models/enable_banking_item.rb +++ b/app/models/enable_banking_item.rb @@ -17,7 +17,7 @@ class EnableBankingItem < ApplicationRecord validates :client_certificate, presence: true, on: :create belongs_to :family - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later has_many :enable_banking_accounts, dependent: :destroy has_many :accounts, through: :enable_banking_accounts diff --git a/app/models/lunchflow_item.rb b/app/models/lunchflow_item.rb index 9f4f4cc7a..1165ba836 100644 --- a/app/models/lunchflow_item.rb +++ b/app/models/lunchflow_item.rb @@ -14,7 +14,7 @@ class LunchflowItem < ApplicationRecord validates :api_key, presence: true, on: :create belongs_to :family - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later has_many :lunchflow_accounts, dependent: :destroy has_many :accounts, through: :lunchflow_accounts diff --git a/app/models/mercury_item.rb b/app/models/mercury_item.rb index a6bcaeb99..2c781265a 100644 --- a/app/models/mercury_item.rb +++ b/app/models/mercury_item.rb @@ -21,7 +21,7 @@ class MercuryItem < ApplicationRecord validates :token, presence: true, on: :create belongs_to :family - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later has_many :mercury_accounts, dependent: :destroy has_many :accounts, through: :mercury_accounts diff --git a/app/models/pdf_import.rb b/app/models/pdf_import.rb index 0e0250462..90cb3379d 100644 --- a/app/models/pdf_import.rb +++ b/app/models/pdf_import.rb @@ -1,5 +1,5 @@ class PdfImport < Import - has_one_attached :pdf_file + has_one_attached :pdf_file, dependent: :purge_later validates :document_type, inclusion: { in: DOCUMENT_TYPES }, allow_nil: true diff --git a/app/models/plaid_item.rb b/app/models/plaid_item.rb index c7f325134..4ac59d0cf 100644 --- a/app/models/plaid_item.rb +++ b/app/models/plaid_item.rb @@ -17,7 +17,7 @@ class PlaidItem < ApplicationRecord before_destroy :remove_plaid_item belongs_to :family - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later has_many :plaid_accounts, dependent: :destroy has_many :legacy_accounts, through: :plaid_accounts, source: :account diff --git a/app/models/simplefin_item.rb b/app/models/simplefin_item.rb index 07039ad77..253b6098c 100644 --- a/app/models/simplefin_item.rb +++ b/app/models/simplefin_item.rb @@ -20,7 +20,7 @@ class SimplefinItem < ApplicationRecord before_destroy :remove_simplefin_item belongs_to :family - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later has_many :simplefin_accounts, dependent: :destroy has_many :legacy_accounts, through: :simplefin_accounts, source: :account diff --git a/app/models/snaptrade_item.rb b/app/models/snaptrade_item.rb index 365d6af3f..0c627d7d3 100644 --- a/app/models/snaptrade_item.rb +++ b/app/models/snaptrade_item.rb @@ -29,7 +29,7 @@ class SnaptradeItem < ApplicationRecord # via ensure_user_registered!, so we don't validate them on create belongs_to :family - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later has_many :snaptrade_accounts, dependent: :destroy has_many :linked_accounts, through: :snaptrade_accounts diff --git a/app/models/user.rb b/app/models/user.rb index 3ab9276f8..df8f9fc5e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -59,7 +59,7 @@ class User < ApplicationRecord User.exists? ? fallback_role : :super_admin end - has_one_attached :profile_image do |attachable| + has_one_attached :profile_image, dependent: :purge_later do |attachable| attachable.variant :thumbnail, resize_to_fill: [ 300, 300 ], convert: :webp, saver: { quality: 80 } attachable.variant :small, resize_to_fill: [ 72, 72 ], convert: :webp, saver: { quality: 80 }, preprocessed: true end diff --git a/lib/generators/provider/family/templates/item_model.rb.tt b/lib/generators/provider/family/templates/item_model.rb.tt index 9cfbf1997..9d4a8e437 100644 --- a/lib/generators/provider/family/templates/item_model.rb.tt +++ b/lib/generators/provider/family/templates/item_model.rb.tt @@ -27,7 +27,7 @@ class <%= class_name %>Item < ApplicationRecord <% end -%> belongs_to :family - has_one_attached :logo + has_one_attached :logo, dependent: :purge_later has_many :<%= file_name %>_accounts, dependent: :destroy has_many :accounts, through: :<%= file_name %>_accounts diff --git a/test/models/account_test.rb b/test/models/account_test.rb index 8a52192af..5a41f432e 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -1,7 +1,7 @@ require "test_helper" class AccountTest < ActiveSupport::TestCase - include SyncableInterfaceTest, EntriesTestHelper + include SyncableInterfaceTest, EntriesTestHelper, ActiveJob::TestHelper setup do @account = @syncable = accounts(:depository) @@ -155,4 +155,21 @@ class AccountTest < ActiveSupport::TestCase assert @account.taxable? assert_not @account.tax_advantaged? end + + test "destroying account purges attached logo" do + @account.logo.attach( + io: StringIO.new("fake-logo-content"), + filename: "logo.png", + content_type: "image/png" + ) + + attachment_id = @account.logo.id + assert ActiveStorage::Attachment.exists?(attachment_id) + + perform_enqueued_jobs do + @account.destroy! + end + + assert_not ActiveStorage::Attachment.exists?(attachment_id) + end end diff --git a/test/models/pdf_import_test.rb b/test/models/pdf_import_test.rb index a2491ee1a..37281b5cc 100644 --- a/test/models/pdf_import_test.rb +++ b/test/models/pdf_import_test.rb @@ -132,4 +132,21 @@ class PdfImportTest < ActiveSupport::TestCase assert_nil @import.account assert_not_includes @import.mapping_steps, Import::AccountMapping end + + test "destroying import purges attached pdf_file" do + @import.pdf_file.attach( + io: StringIO.new("fake-pdf-content"), + filename: "statement.pdf", + content_type: "application/pdf" + ) + + attachment_id = @import.pdf_file.id + assert ActiveStorage::Attachment.exists?(attachment_id) + + perform_enqueued_jobs do + @import.destroy! + end + + assert_not ActiveStorage::Attachment.exists?(attachment_id) + end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index a1c0c496e..53b49ed9a 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -1,10 +1,17 @@ require "test_helper" class UserTest < ActiveSupport::TestCase + include ActiveJob::TestHelper + def setup @user = users(:family_admin) end + def teardown + clear_enqueued_jobs + clear_performed_jobs + end + test "should be valid" do assert @user.valid?, @user.errors.full_messages.to_sentence end @@ -348,4 +355,45 @@ class UserTest < ActiveSupport::TestCase assert_equal :member, User.role_for_new_family_creator(fallback_role: :member) assert_equal "custom_role", User.role_for_new_family_creator(fallback_role: "custom_role") end + + # ActiveStorage attachment cleanup tests + test "purging a user removes attached profile image" do + user = users(:family_admin) + user.profile_image.attach( + io: StringIO.new("profile-image-data"), + filename: "profile.png", + content_type: "image/png" + ) + + attachment_id = user.profile_image.id + assert ActiveStorage::Attachment.exists?(attachment_id) + + perform_enqueued_jobs do + user.purge + end + + assert_not User.exists?(user.id) + assert_not ActiveStorage::Attachment.exists?(attachment_id) + end + + test "purging the last user cascades to remove family and its export attachments" do + family = Family.create!(name: "Solo Family", locale: "en", date_format: "%m-%d-%Y", currency: "USD") + user = User.create!(family: family, email: "solo@example.com", password: "password123") + export = family.family_exports.create! + export.export_file.attach( + io: StringIO.new("export-data"), + filename: "export.zip", + content_type: "application/zip" + ) + + export_attachment_id = export.export_file.id + assert ActiveStorage::Attachment.exists?(export_attachment_id) + + perform_enqueued_jobs do + user.purge + end + + assert_not Family.exists?(family.id) + assert_not ActiveStorage::Attachment.exists?(export_attachment_id) + end end