From cad0b7d91dbc5abf4b0d7fdbcf2d0620557a3b0f Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Sat, 9 Jun 2018 17:30:00 +0900 Subject: Fix `touch` option to behave consistently with `Persistence#touch` method `touch` option was added to `increment!` (#27660) and `update_counters` (#26995). But that option behaves inconsistently with `Persistence#touch` method. If `touch` option is passed attribute names, it won't update update_at/on attributes unlike `Persistence#touch` method. Due to changed from `Persistence#touch` to `increment!` with `touch` option, #31405 has a regression that `counter_cache` with `touch` option which is passed attribute names won't update update_at/on attributes. I think that the inconsistency is not intended. To get back consistency, ensure that `touch` option updates update_at/on attributes. --- activerecord/test/cases/counter_cache_test.rb | 12 +++---- activerecord/test/cases/locking_test.rb | 24 ++++++++----- activerecord/test/cases/persistence_test.rb | 52 +++++++++++++++++++++------ 3 files changed, 63 insertions(+), 25 deletions(-) (limited to 'activerecord/test/cases') diff --git a/activerecord/test/cases/counter_cache_test.rb b/activerecord/test/cases/counter_cache_test.rb index e0948f90ac..99d286dc52 100644 --- a/activerecord/test/cases/counter_cache_test.rb +++ b/activerecord/test/cases/counter_cache_test.rb @@ -280,38 +280,38 @@ class CounterCacheTest < ActiveRecord::TestCase end test "update counters with touch: :written_on" do - assert_touching @topic, :written_on do + assert_touching @topic, :updated_at, :written_on do Topic.update_counters(@topic.id, replies_count: -1, touch: :written_on) end end test "update multiple counters with touch: :written_on" do - assert_touching @topic, :written_on do + assert_touching @topic, :updated_at, :written_on do Topic.update_counters(@topic.id, replies_count: 2, unique_replies_count: 2, touch: :written_on) end end test "reset counters with touch: :written_on" do - assert_touching @topic, :written_on do + assert_touching @topic, :updated_at, :written_on do Topic.reset_counters(@topic.id, :replies, touch: :written_on) end end test "reset multiple counters with touch: :written_on" do - assert_touching @topic, :written_on do + assert_touching @topic, :updated_at, :written_on do Topic.update_counters(@topic.id, replies_count: 1, unique_replies_count: 1) Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: :written_on) end end test "increment counters with touch: :written_on" do - assert_touching @topic, :written_on do + assert_touching @topic, :updated_at, :written_on do Topic.increment_counter(:replies_count, @topic.id, touch: :written_on) end end test "decrement counters with touch: :written_on" do - assert_touching @topic, :written_on do + assert_touching @topic, :updated_at, :written_on do Topic.decrement_counter(:replies_count, @topic.id, touch: :written_on) end end diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index 8513edb0ab..33bd74e114 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -445,32 +445,38 @@ class OptimisticLockingTest < ActiveRecord::TestCase assert_equal 0, car.wheels_count assert_equal 0, car.lock_version - previously_car_updated_at = car.updated_at - travel(2.second) do + previously_updated_at = car.updated_at + previously_wheels_owned_at = car.wheels_owned_at + travel(1.second) do Wheel.create!(wheelable: car) end assert_equal 1, car.reload.wheels_count - assert_not_equal previously_car_updated_at, car.updated_at assert_equal 1, car.lock_version + assert_operator previously_updated_at, :<, car.updated_at + assert_operator previously_wheels_owned_at, :<, car.wheels_owned_at - previously_car_updated_at = car.updated_at - travel(1.day) do + previously_updated_at = car.updated_at + previously_wheels_owned_at = car.wheels_owned_at + travel(2.second) do car.wheels.first.update(size: 42) end assert_equal 1, car.reload.wheels_count - assert_not_equal previously_car_updated_at, car.updated_at assert_equal 2, car.lock_version + assert_operator previously_updated_at, :<, car.updated_at + assert_operator previously_wheels_owned_at, :<, car.wheels_owned_at - previously_car_updated_at = car.updated_at - travel(2.second) do + previously_updated_at = car.updated_at + previously_wheels_owned_at = car.wheels_owned_at + travel(3.second) do car.wheels.first.destroy! end assert_equal 0, car.reload.wheels_count - assert_not_equal previously_car_updated_at, car.updated_at assert_equal 3, car.lock_version + assert_operator previously_updated_at, :<, car.updated_at + assert_operator previously_wheels_owned_at, :<, car.wheels_owned_at end def test_polymorphic_destroy_with_dependencies_and_lock_version diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb index 4c332e30aa..7348a22dd3 100644 --- a/activerecord/test/cases/persistence_test.rb +++ b/activerecord/test/cases/persistence_test.rb @@ -206,12 +206,28 @@ class PersistenceTest < ActiveRecord::TestCase assert_equal initial_credit + 2, a1.reload.credit_limit end - def test_increment_updates_timestamps + def test_increment_with_touch_updates_timestamps topic = topics(:first) - topic.update_columns(updated_at: 5.minutes.ago) - previous_updated_at = topic.updated_at - topic.increment!(:replies_count, touch: true) - assert_operator previous_updated_at, :<, topic.reload.updated_at + assert_equal 1, topic.replies_count + previously_updated_at = topic.updated_at + travel(1.second) do + topic.increment!(:replies_count, touch: true) + end + assert_equal 2, topic.reload.replies_count + assert_operator previously_updated_at, :<, topic.updated_at + end + + def test_increment_with_touch_an_attribute_updates_timestamps + topic = topics(:first) + assert_equal 1, topic.replies_count + previously_updated_at = topic.updated_at + previously_written_on = topic.written_on + travel(1.second) do + topic.increment!(:replies_count, touch: :written_on) + end + assert_equal 2, topic.reload.replies_count + assert_operator previously_updated_at, :<, topic.updated_at + assert_operator previously_written_on, :<, topic.written_on end def test_destroy_all @@ -333,12 +349,28 @@ class PersistenceTest < ActiveRecord::TestCase assert_equal 41, accounts(:signals37, :reload).credit_limit end - def test_decrement_updates_timestamps + def test_decrement_with_touch_updates_timestamps topic = topics(:first) - topic.update_columns(updated_at: 5.minutes.ago) - previous_updated_at = topic.updated_at - topic.decrement!(:replies_count, touch: true) - assert_operator previous_updated_at, :<, topic.reload.updated_at + assert_equal 1, topic.replies_count + previously_updated_at = topic.updated_at + travel(1.second) do + topic.decrement!(:replies_count, touch: true) + end + assert_equal 0, topic.reload.replies_count + assert_operator previously_updated_at, :<, topic.updated_at + end + + def test_decrement_with_touch_an_attribute_updates_timestamps + topic = topics(:first) + assert_equal 1, topic.replies_count + previously_updated_at = topic.updated_at + previously_written_on = topic.written_on + travel(1.second) do + topic.decrement!(:replies_count, touch: :written_on) + end + assert_equal 0, topic.reload.replies_count + assert_operator previously_updated_at, :<, topic.updated_at + assert_operator previously_written_on, :<, topic.written_on end def test_create -- cgit v1.2.3