Skip to content

Commit

Permalink
Promote clear_attribute_change as attribute methods
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kamipo committed Jun 3, 2020
1 parent 35fe9bc commit 6f1bf2a
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 3 deletions.
1 change: 1 addition & 0 deletions activemodel/lib/active_model/dirty.rb
Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/persistence.rb
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/timestamp.rb
Expand Up @@ -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
Expand Down
20 changes: 20 additions & 0 deletions activerecord/test/cases/dirty_test.rb
Expand Up @@ -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

Expand Down
11 changes: 11 additions & 0 deletions activerecord/test/cases/persistence_test.rb
Expand Up @@ -86,13 +86,24 @@ 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

accounts(:signals37).increment(:credit_limit).increment!(:credit_limit)
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
Expand Down

0 comments on commit 6f1bf2a

Please sign in to comment.