From 64c9438bdb4838865fe4ab2261540d4d061b9832 Mon Sep 17 00:00:00 2001 From: Brendon Muir Date: Mon, 13 Mar 2017 19:09:22 +1300 Subject: [PATCH] Don't update a child destroyed via relation (#261) --- Gemfile | 1 + gemfiles/rails_3_2.gemfile | 1 + gemfiles/rails_4_1.gemfile | 1 + gemfiles/rails_4_2.gemfile | 1 + gemfiles/rails_5_0.gemfile | 1 + gemfiles/rails_5_1.gemfile | 1 + .../active_record/acts/callback_definer.rb | 4 +- .../acts/scope_method_definer.rb | 14 ++++ test/helper.rb | 1 + test/test_no_update_for_extra_classes.rb | 2 +- test/test_no_update_for_scope_destruction.rb | 72 +++++++++++++++++++ 11 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 test/test_no_update_for_scope_destruction.rb diff --git a/Gemfile b/Gemfile index 60bc3683..75b18056 100644 --- a/Gemfile +++ b/Gemfile @@ -12,6 +12,7 @@ group :test do gem "minitest", "~> 5.0" gem "test_after_commit", "~> 0.4.2" gem "timecop" + gem "mocha" end group :sqlite do diff --git a/gemfiles/rails_3_2.gemfile b/gemfiles/rails_3_2.gemfile index d7b2eca6..749eb3c2 100644 --- a/gemfiles/rails_3_2.gemfile +++ b/gemfiles/rails_3_2.gemfile @@ -12,6 +12,7 @@ group :test do gem "minitest", "~> 5.0" gem "test_after_commit", "~> 0.4.2" gem "timecop" + gem "mocha" gem "after_commit_exception_notification" end diff --git a/gemfiles/rails_4_1.gemfile b/gemfiles/rails_4_1.gemfile index c298083b..ccbe81ff 100644 --- a/gemfiles/rails_4_1.gemfile +++ b/gemfiles/rails_4_1.gemfile @@ -12,6 +12,7 @@ group :test do gem "minitest", "~> 5.0" gem "test_after_commit", "~> 0.4.2" gem "timecop" + gem "mocha" gem "after_commit_exception_notification" end diff --git a/gemfiles/rails_4_2.gemfile b/gemfiles/rails_4_2.gemfile index f7aa54e3..54d36f98 100644 --- a/gemfiles/rails_4_2.gemfile +++ b/gemfiles/rails_4_2.gemfile @@ -12,6 +12,7 @@ group :test do gem "minitest", "~> 5.0" gem "test_after_commit", "~> 0.4.2" gem "timecop" + gem "mocha" end group :sqlite do diff --git a/gemfiles/rails_5_0.gemfile b/gemfiles/rails_5_0.gemfile index ada7e34a..a37ef825 100644 --- a/gemfiles/rails_5_0.gemfile +++ b/gemfiles/rails_5_0.gemfile @@ -12,6 +12,7 @@ group :test do gem "minitest", "~> 5.0" gem "test_after_commit", "~> 0.4.2" gem "timecop" + gem "mocha" end group :sqlite do diff --git a/gemfiles/rails_5_1.gemfile b/gemfiles/rails_5_1.gemfile index 9ae6c8da..1d12adf0 100644 --- a/gemfiles/rails_5_1.gemfile +++ b/gemfiles/rails_5_1.gemfile @@ -12,6 +12,7 @@ group :test do gem "minitest", "~> 5.0" gem "test_after_commit", "~> 0.4.2" gem "timecop" + gem "mocha" end group :sqlite do diff --git a/lib/acts_as_list/active_record/acts/callback_definer.rb b/lib/acts_as_list/active_record/acts/callback_definer.rb index 8a143df1..84918c06 100644 --- a/lib/acts_as_list/active_record/acts/callback_definer.rb +++ b/lib/acts_as_list/active_record/acts/callback_definer.rb @@ -3,8 +3,8 @@ def self.call(caller_class, add_new_at) caller_class.class_eval do before_validation :check_top_position, unless: :act_as_list_no_update? - before_destroy :lock! - after_destroy :decrement_positions_on_lower_items, unless: :act_as_list_no_update? + before_destroy :lock!, unless: "destroyed_via_scope? || act_as_list_no_update?" + after_destroy :decrement_positions_on_lower_items, unless: "destroyed_via_scope? || act_as_list_no_update?" before_update :check_scope, unless: :act_as_list_no_update? after_update :update_positions, unless: :act_as_list_no_update? diff --git a/lib/acts_as_list/active_record/acts/scope_method_definer.rb b/lib/acts_as_list/active_record/acts/scope_method_definer.rb index 14dbc3ce..5643f89c 100644 --- a/lib/acts_as_list/active_record/acts/scope_method_definer.rb +++ b/lib/acts_as_list/active_record/acts/scope_method_definer.rb @@ -17,6 +17,11 @@ def self.call(caller_class, scope) define_method :scope_changed? do changed.include?(scope_name.to_s) end + + define_method :destroyed_via_scope? do + return false if ActiveRecord::VERSION::MAJOR < 4 + scope == (destroyed_by_association && destroyed_by_association.foreign_key.to_sym) + end elsif scope.is_a?(Array) define_method :scope_condition do scope.inject({}) do |hash, column| @@ -27,6 +32,11 @@ def self.call(caller_class, scope) define_method :scope_changed? do (scope_condition.keys & changed.map(&:to_sym)).any? end + + define_method :destroyed_via_scope? do + return false if ActiveRecord::VERSION::MAJOR < 4 + scope_condition.keys.include? (destroyed_by_association && destroyed_by_association.foreign_key.to_sym) + end else define_method :scope_condition do eval "%{#{scope}}" @@ -35,6 +45,10 @@ def self.call(caller_class, scope) define_method :scope_changed? do false end + + define_method :destroyed_via_scope? do + false + end end self.scope :in_list, lambda { where("#{quoted_position_column_with_table_name} IS NOT NULL") } diff --git a/test/helper.rb b/test/helper.rb index 0d4614c9..15689ad1 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -11,6 +11,7 @@ end require "active_record" require "minitest/autorun" +require "mocha/mini_test" require "#{File.dirname(__FILE__)}/../init" if defined?(ActiveRecord::VERSION) && diff --git a/test/test_no_update_for_extra_classes.rb b/test/test_no_update_for_extra_classes.rb index 973fca54..0525a78c 100644 --- a/test/test_no_update_for_extra_classes.rb +++ b/test/test_no_update_for_extra_classes.rb @@ -33,7 +33,7 @@ def setup end ActiveRecord::Base.connection.schema_cache.clear! - [TodoList, Item, TodoItemAttachment].each(&:reset_column_information) + [TodoList, TodoItem, TodoItemAttachment].each(&:reset_column_information) super end diff --git a/test/test_no_update_for_scope_destruction.rb b/test/test_no_update_for_scope_destruction.rb new file mode 100644 index 00000000..f360b996 --- /dev/null +++ b/test/test_no_update_for_scope_destruction.rb @@ -0,0 +1,72 @@ +require 'helper' + +class DestructionTodoList < ActiveRecord::Base + has_many :destruction_todo_items, dependent: :destroy + has_many :destruction_tada_items, dependent: :destroy +end + +class DestructionTodoItem < ActiveRecord::Base + belongs_to :destruction_todo_list + acts_as_list scope: :destruction_todo_list +end + +class DestructionTadaItem < ActiveRecord::Base + belongs_to :destruction_todo_list + acts_as_list scope: [:destruction_todo_list_id, :enabled] +end + +class NoUpdateForScopeDestructionTestCase < Minitest::Test + def setup + ActiveRecord::Base.connection.create_table :destruction_todo_lists do |t| + end + + ActiveRecord::Base.connection.create_table :destruction_todo_items do |t| + t.column :position, :integer + t.column :destruction_todo_list_id, :integer + end + + ActiveRecord::Base.connection.create_table :destruction_tada_items do |t| + t.column :position, :integer + t.column :destruction_todo_list_id, :integer + t.column :enabled, :boolean + end + + ActiveRecord::Base.connection.schema_cache.clear! + [DestructionTodoList, DestructionTodoItem, DestructionTadaItem].each(&:reset_column_information) + super + end + + def teardown + teardown_db + super + end + + class NoUpdateForScopeDestructionTest < NoUpdateForScopeDestructionTestCase + def setup + super + @list = DestructionTodoList.create! + + @todo_item_1 = DestructionTodoItem.create! position: 1, destruction_todo_list_id: @list.id + @tada_item_1 = DestructionTadaItem.create! position: 1, destruction_todo_list_id: @list.id, enabled: true + end + + def test_no_update_children_when_parent_destroyed + if ActiveRecord::VERSION::MAJOR < 4 + DestructionTodoItem.any_instance.expects(:decrement_positions_on_lower_items).once + DestructionTadaItem.any_instance.expects(:decrement_positions_on_lower_items).once + else + DestructionTodoItem.any_instance.expects(:decrement_positions_on_lower_items).never + DestructionTadaItem.any_instance.expects(:decrement_positions_on_lower_items).never + end + assert @list.destroy + end + + def test_update_children_when_sibling_destroyed + @todo_item_1.expects(:decrement_positions_on_lower_items).once + @tada_item_1.expects(:decrement_positions_on_lower_items).once + assert @todo_item_1.destroy + assert @tada_item_1.destroy + end + + end +end \ No newline at end of file