mirror of
https://github.com/we-promise/sure
synced 2026-04-25 17:15:07 +02:00
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user