Merge pull request #16180 from rafaelfranca/rm-dirty

Improve Active Model Dirty API.

6 files changed, 79 insertions, 18 deletions

diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md
index 2565b24c97..555cd259d2 100644
--- a/activemodel/CHANGELOG.md
+++ b/activemodel/CHANGELOG.md
@@ -1,4 +1,16 @@
-*   Added `undo_changes` method to `ActiveModel::Dirty` API to restore all the
+*   Deprecate `reset_#{attribute}` in favor of `restore_#{attribute}`.
+
+    These methods may cause confusion with the `reset_changes` that behaves differently
+    of them.
+
+*   Deprecate `ActiveModel::Dirty#reset_changes` in favor of `#clear_changes_information`.
+
+    This method name is causing confusion with the `reset_#{attribute}`
+    methods. While `reset_name` set the value of the name attribute for the
+    previous value `reset_changes` only discard the changes and previous
+    changes.
+
+*   Added `restore_attributes` method to `ActiveModel::Dirty` API to restore all the
     changed values to the previous data.
 
     *Igor G.*
diff --git a/activemodel/lib/active_model/dirty.rb b/activemodel/lib/active_model/dirty.rb
index dc6088a39a..24214187af 100644
--- a/activemodel/lib/active_model/dirty.rb
+++ b/activemodel/lib/active_model/dirty.rb
@@ -15,9 +15,9 @@ module ActiveModel
   # * Call <tt>attr_name_will_change!</tt> before each change to the tracked
   #   attribute.
   # * Call <tt>changes_applied</tt> after the changes are persisted.
-  # * Call <tt>reset_changes</tt> when you want to reset the changes
+  # * Call <tt>clear_changes_information</tt> when you want to reset the changes
   #   information.
-  # * Call <tt>undo_changes</tt> when you want to restore previous data.
+  # * Call <tt>restore_attributes</tt> when you want to restore previous data.
   #
   # A minimal implementation could be:
   #
@@ -37,15 +37,18 @@ module ActiveModel
   #
   #     def save
   #       # do persistence work
+  #
   #       changes_applied
   #     end
   #
   #     def reload!
-  #       reset_changes
+  #       # get the values from the persistence layer
+  #
+  #       clear_changes_information
   #     end
   #
   #     def rollback!
-  #       undo_changes
+  #       restore_attributes
   #     end
   #   end
   #
@@ -113,6 +116,7 @@ module ActiveModel
     included do
       attribute_method_suffix '_changed?', '_change', '_will_change!', '_was'
       attribute_method_affix prefix: 'reset_', suffix: '!'
+      attribute_method_affix prefix: 'restore_', suffix: '!'
     end
 
     # Returns +true+ if any attribute have unsaved changes, +false+ otherwise.
@@ -184,15 +188,20 @@ module ActiveModel
         @changed_attributes = ActiveSupport::HashWithIndifferentAccess.new
       end
 
-      # Removes all dirty data: current changes and previous changes.
-      def reset_changes # :doc:
+      # Clear all dirty data: current changes and previous changes.
+      def clear_changes_information # :doc:
         @previously_changed = ActiveSupport::HashWithIndifferentAccess.new
         @changed_attributes = ActiveSupport::HashWithIndifferentAccess.new
       end
 
+      def reset_changes
+        ActiveSupport::Deprecation.warn "#reset_changes is deprecated and will be removed on Rails 5. Please use #clear_changes_information instead."
+        clear_changes_information
+      end
+
       # Restore all previous data.
-      def undo_changes # :doc:
-        changed_attributes.each_key { |attr| reset_attribute! attr }
+      def restore_attributes # :doc:
+        changed_attributes.each_key { |attr| restore_attribute! attr }
       end
 
       # Handle <tt>*_change</tt> for +method_missing+.
@@ -215,6 +224,13 @@ module ActiveModel
 
       # Handle <tt>reset_*!</tt> for +method_missing+.
       def reset_attribute!(attr)
+        ActiveSupport::Deprecation.warn "#reset_#{attr}! is deprecated and will be removed on Rails 5. Please use #restore_#{attr}! instead."
+
+        restore_attribute!(attr)
+      end
+
+      # Handle <tt>restore_*!</tt> for +method_missing+.
+      def restore_attribute!(attr)
         if attribute_changed?(attr)
           __send__("#{attr}=", changed_attributes[attr])
           changed_attributes.delete(attr)
diff --git a/activemodel/test/cases/dirty_test.rb b/activemodel/test/cases/dirty_test.rb
index 562fadbb85..6b8bb5216b 100644
--- a/activemodel/test/cases/dirty_test.rb
+++ b/activemodel/test/cases/dirty_test.rb
@@ -43,11 +43,15 @@ class DirtyTest < ActiveModel::TestCase
     end
 
     def reload
+      clear_changes_information
+    end
+
+    def deprecated_reload
       reset_changes
     end
 
     def rollback
-      undo_changes
+      restore_attributes
     end
   end
 
@@ -111,7 +115,7 @@ class DirtyTest < ActiveModel::TestCase
 
   test "resetting attribute" do
     @model.name = "Bob"
-    @model.reset_name!
+    @model.restore_name!
     assert_nil @model.name
     assert !@model.name_changed?
   end
@@ -181,7 +185,24 @@ class DirtyTest < ActiveModel::TestCase
     assert_equal ActiveSupport::HashWithIndifferentAccess.new, @model.changed_attributes
   end
 
-  test "undo_changes should restore all previous data" do
+  test "reset_changes is deprecated" do
+    @model.name = 'Dmitry'
+    @model.name_changed?
+    @model.save
+    @model.name = 'Bob'
+
+    assert_equal [nil, 'Dmitry'], @model.previous_changes['name']
+    assert_equal 'Dmitry', @model.changed_attributes['name']
+
+    assert_deprecated do
+      @model.deprecated_reload
+    end
+
+    assert_equal ActiveSupport::HashWithIndifferentAccess.new, @model.previous_changes
+    assert_equal ActiveSupport::HashWithIndifferentAccess.new, @model.changed_attributes
+  end
+
+  test "restore_attributes should restore all previous data" do
     @model.name = 'Dmitry'
     @model.color = 'Red'
     @model.save
diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb
index e1a86fd3aa..b58295a106 100644
--- a/activerecord/lib/active_record/attribute_methods/dirty.rb
+++ b/activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -34,7 +34,7 @@ module ActiveRecord
       # <tt>reload</tt> the record and clears changed attributes.
       def reload(*)
         super.tap do
-          reset_changes
+          clear_changes_information
         end
       end
 
@@ -64,7 +64,7 @@ module ActiveRecord
         store_original_raw_attributes
       end
 
-      def reset_changes
+      def clear_changes_information
         super
         original_raw_attributes.clear
       end
diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb
index ea73c561e9..69a7f25213 100644
--- a/activerecord/test/cases/dirty_test.rb
+++ b/activerecord/test/cases/dirty_test.rb
@@ -169,7 +169,19 @@ class DirtyTest < ActiveRecord::TestCase
     pirate = Pirate.create!(:catchphrase => 'Yar!')
     pirate.catchphrase = 'Ahoy!'
 
-    pirate.reset_catchphrase!
+    assert_deprecated do
+      pirate.reset_catchphrase!
+    end
+    assert_equal "Yar!", pirate.catchphrase
+    assert_equal Hash.new, pirate.changes
+    assert !pirate.catchphrase_changed?
+  end
+
+  def test_restore_attribute!
+    pirate = Pirate.create!(:catchphrase => 'Yar!')
+    pirate.catchphrase = 'Ahoy!'
+
+    pirate.restore_catchphrase!
     assert_equal "Yar!", pirate.catchphrase
     assert_equal Hash.new, pirate.changes
     assert !pirate.catchphrase_changed?
@@ -398,7 +410,7 @@ class DirtyTest < ActiveRecord::TestCase
   def test_dup_objects_should_not_copy_dirty_flag_from_creator
     pirate = Pirate.create!(:catchphrase => "shiver me timbers")
     pirate_dup = pirate.dup
-    pirate_dup.reset_catchphrase!
+    pirate_dup.restore_catchphrase!
     pirate.catchphrase = "I love Rum"
     assert pirate.catchphrase_changed?
     assert !pirate_dup.catchphrase_changed?
diff --git a/guides/source/active_model_basics.md b/guides/source/active_model_basics.md
index 1131a83c36..3eaeeff389 100644
--- a/guides/source/active_model_basics.md
+++ b/guides/source/active_model_basics.md
@@ -202,7 +202,7 @@ person.valid?                        # => raises ActiveModel::StrictValidationFa
 ### ActiveModel::Naming
 
 Naming adds a number of class methods which make the naming and routing
-easier to manage. The module defines the `model_name` class method which 
+easier to manage. The module defines the `model_name` class method which
 will define a number of accessors using some `ActiveSupport::Inflector` methods.
 
 ```ruby
@@ -220,4 +220,4 @@ Person.model_name.param_key           # => "person"
 Person.model_name.i18n_key            # => :person
 Person.model_name.route_key           # => "people"
 Person.model_name.singular_route_key  # => "person"
-```
\ No newline at end of file
+```