From a4cc88d0851343ac16e2294c06c5a4101189c410 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 20 Jan 2014 21:57:47 -0200 Subject: Extract all attribute changed work to its own method This will make easier to hook on this feature to customize the behavior --- activerecord/lib/active_record/attribute_methods/dirty.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index 19e81abba5..f7065d183f 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -43,6 +43,13 @@ module ActiveRecord def write_attribute(attr, value) attr = attr.to_s + save_changed_attribute(attr, value) + + # Carry on. + super(attr, value) + end + + def save_changed_attribute(attr, value) # The attribute already has an unsaved change. if attribute_changed?(attr) old = changed_attributes[attr] @@ -51,9 +58,6 @@ module ActiveRecord old = clone_attribute_value(:read_attribute, attr) changed_attributes[attr] = old if _field_changed?(attr, old, value) end - - # Carry on. - super(attr, value) end def update_record(*) -- cgit v1.2.3 From a57a2bcf4a2c29519d553277e4439790ca443cc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 20 Jan 2014 21:59:20 -0200 Subject: Make enum feature work with dirty methods To make this possible we have to override the save_changed_attribute hook. --- activerecord/CHANGELOG.md | 18 +++++++++++++++ activerecord/lib/active_record/enum.rb | 19 +++++++++++++++- activerecord/test/cases/enum_test.rb | 40 ++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 170818cd1c..9de076961b 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,21 @@ +* Make enum fields work as expected with the `ActiveModel::Dirty` API. + + Before this change, using the dirty API would have surprising results: + + conversation = Conversation.new + conversation.status = :active + conversation.status = :archived + conversation.status_was # => 0 + + After this change, the same code would result in: + + conversation = Conversation.new + conversation.status = :active + conversation.status = :archived + conversation.status_was # => "active" + + *Rafael Mendonça França* + * Ensure `second` through `fifth` methods act like the `first` finder. The famous ordinal Array instance methods defined in ActiveSupport diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index 3deb2d65f8..06e87cf854 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -63,6 +63,12 @@ module ActiveRecord # # Conversation.where("status <> ?", Conversation.statuses[:archived]) module Enum + DEFINED_ENUMS = [] # :nodoc: + + def enum_attribute?(attr_name) # :nodoc: + DEFINED_ENUMS.include?(attr_name.to_sym) + end + def enum(definitions) klass = self definitions.each do |name, values| @@ -70,6 +76,8 @@ module ActiveRecord enum_values = ActiveSupport::HashWithIndifferentAccess.new name = name.to_sym + DEFINED_ENUMS.unshift name + # def self.statuses statuses end klass.singleton_class.send(:define_method, name.to_s.pluralize) { enum_values } @@ -114,7 +122,16 @@ module ActiveRecord private def _enum_methods_module @_enum_methods_module ||= begin - mod = Module.new + mod = Module.new do + def save_changed_attribute(attr_name, value) + if self.class.enum_attribute?(attr_name) + old = clone_attribute_value(:read_attribute, attr_name) + changed_attributes[attr_name] = self.class.public_send(attr_name.pluralize).key old + else + super + end + end + end include mod mod end diff --git a/activerecord/test/cases/enum_test.rb b/activerecord/test/cases/enum_test.rb index 1f98801e93..0fe7dfe4ea 100644 --- a/activerecord/test/cases/enum_test.rb +++ b/activerecord/test/cases/enum_test.rb @@ -51,6 +51,46 @@ class EnumTest < ActiveRecord::TestCase assert @book.written? end + test "enum changed attributes" do + old_status = @book.status + @book.status = :published + assert_equal old_status, @book.changed_attributes[:status] + end + + test "enum changes" do + old_status = @book.status + @book.status = :published + assert_equal [old_status, 'published'], @book.changes[:status] + end + + test "enum attribute was" do + old_status = @book.status + @book.status = :published + assert_equal old_status, @book.attribute_was(:status) + end + + test "enum attribute changed" do + @book.status = :published + assert @book.attribute_changed?(:status) + end + + test "enum attribute changed to" do + @book.status = :published + assert @book.attribute_changed?(:status, to: 'published') + end + + test "enum attribute changed from" do + old_status = @book.status + @book.status = :published + assert @book.attribute_changed?(:status, from: old_status) + end + + test "enum attribute changed from old status to new status" do + old_status = @book.status + @book.status = :published + assert @book.attribute_changed?(:status, from: old_status, to: 'published') + end + test "assign non existing value raises an error" do e = assert_raises(ArgumentError) do @book.status = :unknown -- cgit v1.2.3 From a0520fceff9148ebfbb2e09745ba1416bceef2bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 20 Jan 2014 23:10:48 -0200 Subject: Add more tests for the dirty feature for enums --- .../lib/active_record/attribute_methods/dirty.rb | 1 - activerecord/lib/active_record/enum.rb | 26 +++++++++++++----- activerecord/test/cases/enum_test.rb | 32 ++++++++++++++++++++++ activerecord/test/models/book.rb | 1 + activerecord/test/schema/schema.rb | 1 + 5 files changed, 53 insertions(+), 8 deletions(-) diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index f7065d183f..68168bb729 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -45,7 +45,6 @@ module ActiveRecord save_changed_attribute(attr, value) - # Carry on. super(attr, value) end diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index 06e87cf854..0990a4a871 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -123,14 +123,26 @@ module ActiveRecord def _enum_methods_module @_enum_methods_module ||= begin mod = Module.new do - def save_changed_attribute(attr_name, value) - if self.class.enum_attribute?(attr_name) - old = clone_attribute_value(:read_attribute, attr_name) - changed_attributes[attr_name] = self.class.public_send(attr_name.pluralize).key old - else - super + private + def save_changed_attribute(attr_name, value) + if self.class.enum_attribute?(attr_name) + if attribute_changed?(attr_name) + old = changed_attributes[attr_name] + + if self.class.public_send(attr_name.pluralize)[old] == value + changed_attributes.delete(attr_name) + end + else + old = clone_attribute_value(:read_attribute, attr_name) + + if old != value + changed_attributes[attr_name] = self.class.public_send(attr_name.pluralize).key old + end + end + else + super + end end - end end include mod mod diff --git a/activerecord/test/cases/enum_test.rb b/activerecord/test/cases/enum_test.rb index 0fe7dfe4ea..9fbfebcca2 100644 --- a/activerecord/test/cases/enum_test.rb +++ b/activerecord/test/cases/enum_test.rb @@ -91,6 +91,38 @@ class EnumTest < ActiveRecord::TestCase assert @book.attribute_changed?(:status, from: old_status, to: 'published') end + test "enum didn't change" do + old_status = @book.status + @book.status = old_status + assert_not @book.attribute_changed?(:status) + end + + test "persist changes that are dirty" do + old_status = @book.status + @book.status = :published + assert @book.attribute_changed?(:status) + @book.status = :written + assert @book.attribute_changed?(:status) + end + + test "reverted changes that are not dirty" do + old_status = @book.status + @book.status = :published + assert @book.attribute_changed?(:status) + @book.status = old_status + assert_not @book.attribute_changed?(:status) + end + + test "reverted changes are not dirty going from nil to value and back" do + book = Book.create!(nullable_status: nil) + + book.nullable_status = :married + assert book.attribute_changed?(:nullable_status) + + book.nullable_status = nil + assert_not book.attribute_changed?(:nullable_status) + end + test "assign non existing value raises an error" do e = assert_raises(ArgumentError) do @book.status = :unknown diff --git a/activerecord/test/models/book.rb b/activerecord/test/models/book.rb index 4cb2c7606b..2170018068 100644 --- a/activerecord/test/models/book.rb +++ b/activerecord/test/models/book.rb @@ -9,6 +9,7 @@ class Book < ActiveRecord::Base enum status: [:proposed, :written, :published] enum read_status: {unread: 0, reading: 2, read: 3} + enum nullable_status: [:single, :married] def published! super diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 9a7d918a25..99a53434f6 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -97,6 +97,7 @@ ActiveRecord::Schema.define do t.column :name, :string t.column :status, :integer, default: 0 t.column :read_status, :integer, default: 0 + t.column :nullable_status, :integer end create_table :booleans, force: true do |t| -- cgit v1.2.3 From 5620e62f3f1920e70199a870a0a3c4feaedb9f98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 21 Jan 2014 20:32:52 -0200 Subject: Store the enum values in the DEFINED_ENUM constant This will make simpler to compare if the values changed in the save_changed_attribute method. --- activerecord/lib/active_record/enum.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index 0990a4a871..77a5fe9ae0 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -63,10 +63,10 @@ module ActiveRecord # # Conversation.where("status <> ?", Conversation.statuses[:archived]) module Enum - DEFINED_ENUMS = [] # :nodoc: + DEFINED_ENUMS = {} # :nodoc: - def enum_attribute?(attr_name) # :nodoc: - DEFINED_ENUMS.include?(attr_name.to_sym) + def enum_mapping_for(attr_name) # :nodoc: + DEFINED_ENUMS[attr_name.to_sym] end def enum(definitions) @@ -76,8 +76,6 @@ module ActiveRecord enum_values = ActiveSupport::HashWithIndifferentAccess.new name = name.to_sym - DEFINED_ENUMS.unshift name - # def self.statuses statuses end klass.singleton_class.send(:define_method, name.to_s.pluralize) { enum_values } @@ -115,6 +113,8 @@ module ActiveRecord # def active!() update! status: :active end define_method("#{value}!") { update! name => value } end + + DEFINED_ENUMS[name] = enum_values end end end @@ -125,18 +125,18 @@ module ActiveRecord mod = Module.new do private def save_changed_attribute(attr_name, value) - if self.class.enum_attribute?(attr_name) + if (mapping = self.class.enum_mapping_for(attr_name)) if attribute_changed?(attr_name) old = changed_attributes[attr_name] - if self.class.public_send(attr_name.pluralize)[old] == value + if mapping[old] == value changed_attributes.delete(attr_name) end else old = clone_attribute_value(:read_attribute, attr_name) if old != value - changed_attributes[attr_name] = self.class.public_send(attr_name.pluralize).key old + changed_attributes[attr_name] = mapping.key old end end else -- cgit v1.2.3 From 55f6c8c908fea2609cbc8503f8d87460fd1b16b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 21 Jan 2014 20:35:21 -0200 Subject: Use string as keys --- activerecord/lib/active_record/enum.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index 77a5fe9ae0..53dde5e564 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -66,7 +66,7 @@ module ActiveRecord DEFINED_ENUMS = {} # :nodoc: def enum_mapping_for(attr_name) # :nodoc: - DEFINED_ENUMS[attr_name.to_sym] + DEFINED_ENUMS[attr_name.to_s] end def enum(definitions) @@ -114,7 +114,7 @@ module ActiveRecord define_method("#{value}!") { update! name => value } end - DEFINED_ENUMS[name] = enum_values + DEFINED_ENUMS[name.to_s] = enum_values end end end -- cgit v1.2.3