From 6f1bf2a3ddbc0d355dd2ea979df2b20ea80f727d Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Thu, 4 Jun 2020 03:40:52 +0900
Subject: [PATCH] Promote `clear_attribute_change` as attribute methods

For now, `increment` with aliased attribute does work, but `increment!`
with aliased attribute does not work, due to `clear_attribute_change` is
not aware of attribute aliases.

We sometimes partially updates specific attributes in dirties, at that
time it relies on `clear_attribute_change` to clear partially updated
attribute dirties. If `clear_attribute_change` is not attribute method
unlike others, we need to resolve attribute aliases manually only for
`clear_attribute_change`, it is a little inconvinient for me.

From another point of view, we have `restore_attributes`,
`restore_attribute!`, `clear_attribute_changes`, and
`clear_attribute_change`. Despite almost similar features
`restore_attribute!` is an attribute method but `clear_attribute_change`
is not.

Given the above, I'd like to promote `clear_attribute_change` as
attribute methods to fix issues caused by the inconsisteny.
---
 activemodel/lib/active_model/dirty.rb         |  1 +
 activerecord/lib/active_record/persistence.rb |  4 ++--
 activerecord/lib/active_record/timestamp.rb   |  2 +-
 activerecord/test/cases/dirty_test.rb         | 20 +++++++++++++++++++
 activerecord/test/cases/persistence_test.rb   | 11 ++++++++++
 5 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/activemodel/lib/active_model/dirty.rb b/activemodel/lib/active_model/dirty.rb
index 1f67bc6555b2a..a193a28e6a150 100644
--- a/activemodel/lib/active_model/dirty.rb
+++ b/activemodel/lib/active_model/dirty.rb
@@ -126,6 +126,7 @@ module Dirty
       attribute_method_suffix "_changed?", "_change", "_will_change!", "_was"
       attribute_method_suffix "_previously_changed?", "_previous_change", "_previously_was"
       attribute_method_affix prefix: "restore_", suffix: "!"
+      attribute_method_affix prefix: "clear_", suffix: "_change"
     end
 
     def initialize_dup(other) # :nodoc:
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index d4779ac4c6a53..c4d7e9a7da0ed 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -707,9 +707,9 @@ def increment(attribute, by = 1)
     # Returns +self+.
     def increment!(attribute, by = 1, touch: nil)
       increment(attribute, by)
-      change = public_send(attribute) - (attribute_in_database(attribute.to_s) || 0)
+      change = public_send(attribute) - (public_send(:"#{attribute}_in_database") || 0)
       self.class.update_counters(id, attribute => change, touch: touch)
-      clear_attribute_change(attribute) # eww
+      public_send(:"clear_#{attribute}_change")
       self
     end
 
diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb
index c9e6ef06b7953..83bb20357c6fc 100644
--- a/activerecord/lib/active_record/timestamp.rb
+++ b/activerecord/lib/active_record/timestamp.rb
@@ -159,7 +159,7 @@ def max_updated_column_timestamp
     def clear_timestamp_attributes
       all_timestamp_attributes_in_model.each do |attribute_name|
         self[attribute_name] = nil
-        clear_attribute_changes([attribute_name])
+        clear_attribute_change(attribute_name)
       end
     end
   end
diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb
index 406565f34ccf6..1688f9ca7cf6d 100644
--- a/activerecord/test/cases/dirty_test.rb
+++ b/activerecord/test/cases/dirty_test.rb
@@ -154,12 +154,32 @@ def test_restore_attribute!
     pirate = Pirate.create!(catchphrase: "Yar!")
     pirate.catchphrase = "Ahoy!"
 
+    assert_equal "Ahoy!", pirate.catchphrase
+    assert_equal ["Yar!", "Ahoy!"], pirate.catchphrase_change
+
     pirate.restore_catchphrase!
+
+    assert_nil pirate.catchphrase_change
     assert_equal "Yar!", pirate.catchphrase
     assert_equal Hash.new, pirate.changes
     assert_not_predicate pirate, :catchphrase_changed?
   end
 
+  def test_clear_attribute_change
+    pirate = Pirate.create!(catchphrase: "Yar!")
+    pirate.catchphrase = "Ahoy!"
+
+    assert_equal "Ahoy!", pirate.catchphrase
+    assert_equal ["Yar!", "Ahoy!"], pirate.catchphrase_change
+
+    pirate.clear_catchphrase_change
+
+    assert_nil pirate.catchphrase_change
+    assert_equal "Ahoy!", pirate.catchphrase
+    assert_equal Hash.new, pirate.changes
+    assert_not_predicate pirate, :catchphrase_changed?
+  end
+
   def test_nullable_number_not_marked_as_changed_if_new_value_is_blank
     pirate = Pirate.new
 
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
index 7b7aa7e9b7860..e1fd9854a5f67 100644
--- a/activerecord/test/cases/persistence_test.rb
+++ b/activerecord/test/cases/persistence_test.rb
@@ -86,6 +86,7 @@ def test_delete_all
 
   def test_increment_attribute
     assert_equal 50, accounts(:signals37).credit_limit
+
     accounts(:signals37).increment! :credit_limit
     assert_equal 51, accounts(:signals37, :reload).credit_limit
 
@@ -93,6 +94,16 @@ def test_increment_attribute
     assert_equal 53, accounts(:signals37, :reload).credit_limit
   end
 
+  def test_increment_aliased_attribute
+    assert_equal 50, accounts(:signals37).available_credit
+
+    accounts(:signals37).increment!(:available_credit)
+    assert_equal 51, accounts(:signals37, :reload).available_credit
+
+    accounts(:signals37).increment(:available_credit).increment!(:available_credit)
+    assert_equal 53, accounts(:signals37, :reload).available_credit
+  end
+
   def test_increment_nil_attribute
     assert_nil topics(:first).parent_id
     topics(:first).increment! :parent_id