Skip to content

Commit

Permalink
Fix create callback called on destroy (rubysherpas#513)
Browse files Browse the repository at this point in the history
* update Rails

* add specs for broken case

* apply fix

Co-authored-by: Djilani Kebaili <djilani.kebaili@epfl.ch>
  • Loading branch information
2 people authored and arun-zc committed Feb 6, 2024
1 parent 0932853 commit edf7d0e
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 3 deletions.
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ if RUBY_ENGINE == 'rbx'
end
end

rails = ENV['RAILS'] || '~> 5.2.0'
rails = ENV['RAILS'] || '~> 6.0.4'

if rails == 'edge'
gem 'rails', github: 'rails/rails'
Expand Down
4 changes: 2 additions & 2 deletions lib/paranoia.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def restore(id_or_ids, opts = {})
end

def paranoia_destroy
transaction do
with_transaction_returning_status do
run_callbacks(:destroy) do
@_disable_counter_cache = deleted?
result = paranoia_delete
Expand Down Expand Up @@ -143,7 +143,7 @@ def paranoia_destroyed?
alias :deleted? :paranoia_destroyed?

def really_destroy!
transaction do
with_transaction_returning_status do
run_callbacks(:real_destroy) do
@_disable_counter_cache = paranoia_destroyed?
dependent_reflections = self.class.reflections.select do |name, reflection|
Expand Down
42 changes: 42 additions & 0 deletions test/paranoia_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def setup!
'featureful_models' => 'deleted_at DATETIME, name VARCHAR(32)',
'plain_models' => 'deleted_at DATETIME',
'callback_models' => 'deleted_at DATETIME',
'after_commit_callback_models' => 'deleted_at DATETIME',
'fail_callback_models' => 'deleted_at DATETIME',
'related_models' => 'parent_model_id INTEGER, parent_model_with_counter_cache_column_id INTEGER, deleted_at DATETIME',
'asplode_models' => 'parent_model_id INTEGER, deleted_at DATETIME',
Expand Down Expand Up @@ -158,6 +159,20 @@ def test_destroy_behavior_for_freshly_loaded_plain_models_callbacks
assert model.instance_variable_get(:@after_commit_callback_called)
end

def test_destroy_behavior_for_freshly_saved_models_after_commit_callbacks
model = AfterCommitCallbackModel.create!

assert_equal 1, model.after_create_commit_called_times
assert_equal 0, model.after_destroy_commit_called_times

# clear the counters, but do not reload from DB
model.remove_called_variables

model.destroy
assert_equal 0, model.after_create_commit_called_times
assert_equal 1, model.after_destroy_commit_called_times
end

def test_delete_behavior_for_plain_models_callbacks
model = CallbackModel.new
model.save
Expand Down Expand Up @@ -1196,6 +1211,33 @@ def remove_called_variables
end
end

class AfterCommitCallbackModel < ActiveRecord::Base
acts_as_paranoid

after_commit :increment_after_create_commit_called_times, on: :create
after_commit :increment_after_destroy_commit_called_times, on: :destroy

def increment_after_create_commit_called_times
@after_create_commit_called_times = after_create_commit_called_times + 1
end

def increment_after_destroy_commit_called_times
@after_destroy_commit_called_times = after_destroy_commit_called_times + 1
end

def after_create_commit_called_times
@after_create_commit_called_times || 0
end

def after_destroy_commit_called_times
@after_destroy_commit_called_times || 0
end

def remove_called_variables
instance_variables.each {|name| (name.to_s.end_with?('_called_times')) ? remove_instance_variable(name) : nil}
end
end

class ParentModel < ActiveRecord::Base
acts_as_paranoid
has_many :paranoid_models, dependent: :destroy
Expand Down

0 comments on commit edf7d0e

Please sign in to comment.