Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AM::Dirty less dirty to plugin into AR or other library #10816

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions activemodel/CHANGELOG.md
@@ -1,3 +1,9 @@
* Added new API methods `reset_changes` and `changed_applied` to AM::Dirty
that control changes state. Previsously you needed to update internal
instance variables, but now API methods are available.

*Bogdan Gusiev*

* Fix regression in has_secure_password. When a password is set, but a
confirmation is an empty string, it would incorrectly save.

Expand Down
26 changes: 16 additions & 10 deletions activemodel/lib/active_model/dirty.rb
Expand Up @@ -14,13 +14,7 @@ module ActiveModel
# track.
# * Call <tt>attr_name_will_change!</tt> before each change to the tracked
# attribute.
#
# If you wish to also track previous changes on save or update, you need to
# add:
#
# @previously_changed = changes
#
# inside of your save or update method.
# * Call <tt>changes_applied</tt> after the changes are persisted.
#
# A minimal implementation could be:
#
Expand All @@ -39,8 +33,8 @@ module ActiveModel
# end
#
# def save
# @previously_changed = changes
# @changed_attributes.clear
# # do persistence work
# changes_applied
# end
# end
#
Expand Down Expand Up @@ -129,7 +123,7 @@ def changes
# person.save
# person.previous_changes # => {"name" => ["bob", "robert"]}
def previous_changes
@previously_changed
@previously_changed ||= {}
end

# Returns a hash of the attributes with unsaved changes indicating their original
Expand All @@ -144,6 +138,18 @@ def changed_attributes

private

# Removes current changes and makes them accessible through +previous_changes+.
def changes_applied
@previously_changed = changes
@changed_attributes = {}
end

# Removes all dirty data: current changes and previous changes
def reset_changes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to put the reset_changes in the class documentation or users will not know it exists.

@previously_changed = {}
@changed_attributes = {}
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be better to just clear the hashes in these methods instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this is right idea. You can assign this hashes to some variable in your code and you might not expect it to change by some internals of activemodel.


# Handle <tt>*_changed?</tt> for +method_missing+.
def attribute_changed?(attr)
changed_attributes.include?(attr)
Expand Down
3 changes: 1 addition & 2 deletions activemodel/test/cases/dirty_test.rb
Expand Up @@ -29,8 +29,7 @@ def color=(val)
end

def save
@previously_changed = changes
@changed_attributes.clear
changes_applied
end
end

Expand Down
15 changes: 6 additions & 9 deletions activerecord/lib/active_record/attribute_methods/dirty.rb
Expand Up @@ -30,25 +30,22 @@ def self.partial_updates; partial_writes; end
# Attempts to +save+ the record and clears changed attributes if successful.
def save(*)
if status = super
@previously_changed = changes
@changed_attributes.clear
changes_applied
end
status
end

# Attempts to <tt>save!</tt> the record and clears changed attributes if successful.
def save!(*)
super.tap do
@previously_changed = changes
@changed_attributes.clear
changes_applied
end
end

# <tt>reload</tt> the record and clears changed attributes.
def reload(*)
super.tap do
@previously_changed.clear
@changed_attributes.clear
reset_changes
end
end

Expand All @@ -59,11 +56,11 @@ def write_attribute(attr, value)

# The attribute already has an unsaved change.
if attribute_changed?(attr)
old = @changed_attributes[attr]
@changed_attributes.delete(attr) unless _field_changed?(attr, old, value)
old = changed_attributes[attr]
changed_attributes.delete(attr) unless _field_changed?(attr, old, value)
else
old = clone_attribute_value(:read_attribute, attr)
@changed_attributes[attr] = old if _field_changed?(attr, old, value)
changed_attributes[attr] = old if _field_changed?(attr, old, value)
end

# Carry on.
Expand Down
4 changes: 1 addition & 3 deletions activerecord/lib/active_record/core.rb
Expand Up @@ -438,8 +438,6 @@ def init_internals
@aggregation_cache = {}
@association_cache = {}
@attributes_cache = {}
@previously_changed = {}
@changed_attributes = {}
@readonly = false
@destroyed = false
@marked_for_destruction = false
Expand All @@ -456,7 +454,7 @@ def init_changed_attributes
# optimistic locking) won't get written unless they get marked as changed
self.class.columns.each do |c|
attr, orig_value = c.name, c.default
@changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value, @attributes[attr])
changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value, @attributes[attr])
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/persistence.rb
Expand Up @@ -433,7 +433,7 @@ def touch(name = nil)

changes[self.class.locking_column] = increment_lock if locking_enabled?

@changed_attributes.except!(*changes.keys)
changed_attributes.except!(*changes.keys)
primary_key = self.class.primary_key
self.class.unscoped.where(primary_key => self[primary_key]).update_all(changes) == 1
end
Expand Down
2 changes: 1 addition & 1 deletion guides/source/active_model_basics.md
Expand Up @@ -120,8 +120,8 @@ class Person
end

def save
@previously_changed = changes
# do save work...
changes_applied
end
end
```
Expand Down