diff --git a/lib/paper_trail/has_paper_trail.rb b/lib/paper_trail/has_paper_trail.rb index 95fb28f4c..3926b0a30 100644 --- a/lib/paper_trail/has_paper_trail.rb +++ b/lib/paper_trail/has_paper_trail.rb @@ -88,6 +88,42 @@ def paper_trail_off def paper_trail_on self.paper_trail_enabled_for_model = true end + + # Used for Version#object attribute + def serialize_attributes(attributes) + serialized_attributes.each do |key, coder| + if attributes.key?(key) + attributes[key] = coder.dump(attributes[key]) + end + end + end + def unserialize_attributes(attributes) + serialized_attributes.each do |key, coder| + if attributes.key?(key) + attributes[key] = coder.load(attributes[key]) + end + end + end + + # Used for Version#object_changes attribute + def serialize_attribute_changes(changes) + serialized_attributes.each do |key, coder| + if changes.key?(key) + old_value, new_value = changes[key] + changes[key] = [coder.dump(old_value), + coder.dump(new_value)] + end + end + end + def unserialize_attribute_changes(changes) + serialized_attributes.each do |key, coder| + if changes.key?(key) + old_value, new_value = changes[key] + changes[key] = [coder.load(old_value), + coder.load(new_value)] + end + end + end end # Wrap the following methods in a module so we can include them only in the @@ -175,15 +211,22 @@ def record_update :whodunnit => PaperTrail.whodunnit } if version_class.column_names.include? 'object_changes' - # The double negative (reject, !include?) preserves the hash structure of self.changes. - data[:object_changes] = self.changes.reject do |key, value| - !notably_changed.include?(key) - end.to_yaml + data[:object_changes] = changes_for_paper_trail.to_yaml end send(self.class.versions_association_name).build merge_metadata(data) end end + def changes_for_paper_trail + # The double negative (reject, !include?) preserves the hash structure of self.changes. + self.changes.reject do |key, value| + !notably_changed.include?(key) + end.tap do |changes| + # Use serialized value for attributes that are serialized + self.class.serialize_attribute_changes(changes) + end + end + def record_destroy if switched_on? and not new_record? version_class.create merge_metadata(:item_id => self.id, @@ -229,7 +272,9 @@ def item_before_change end def object_to_string(object) - object.attributes.except(*self.class.skip).to_yaml + object.attributes.except(*self.class.skip).tap do |attributes| + self.class.serialize_attributes attributes + end.to_yaml end def changed_notably? diff --git a/lib/paper_trail/version.rb b/lib/paper_trail/version.rb index 7e4b4cc34..7f991b297 100644 --- a/lib/paper_trail/version.rb +++ b/lib/paper_trail/version.rb @@ -79,6 +79,7 @@ def reify(options = {}) model = klass.new end + model.class.unserialize_attributes attrs attrs.each do |k, v| if model.respond_to?("#{k}=") model.send :write_attribute, k.to_sym, v @@ -103,7 +104,9 @@ def reify(options = {}) def changeset if self.class.column_names.include? 'object_changes' if changes = object_changes - HashWithIndifferentAccess[YAML::load(changes)] + HashWithIndifferentAccess[YAML::load(changes)].tap do |changes| + item_type.constantize.unserialize_attribute_changes(changes) + end else {} end diff --git a/test/dummy/app/models/person.rb b/test/dummy/app/models/person.rb index ab77d1c44..7a173093e 100644 --- a/test/dummy/app/models/person.rb +++ b/test/dummy/app/models/person.rb @@ -2,4 +2,27 @@ class Person < ActiveRecord::Base has_many :authorships, :dependent => :destroy has_many :books, :through => :authorships has_paper_trail + + # Convert strings to TimeZone objects when assigned + def time_zone=(value) + if value.is_a? ActiveSupport::TimeZone + super + else + zone = ::Time.find_zone(value) # nil if can't find time zone + super zone + end + end + + # Store TimeZone objects as strings when serialized to database + class TimeZoneSerializer + def dump(zone) + zone.try(:name) + end + + def load(value) + ::Time.find_zone!(value) rescue nil + end + end + + serialize :time_zone, TimeZoneSerializer.new end diff --git a/test/dummy/db/migrate/20110208155312_set_up_test_tables.rb b/test/dummy/db/migrate/20110208155312_set_up_test_tables.rb index 0381e68b8..9eade1de1 100644 --- a/test/dummy/db/migrate/20110208155312_set_up_test_tables.rb +++ b/test/dummy/db/migrate/20110208155312_set_up_test_tables.rb @@ -80,6 +80,7 @@ def self.up create_table :people, :force => true do |t| t.string :name + t.string "time_zone" end create_table :songs, :force => true do |t| diff --git a/test/unit/model_test.rb b/test/unit/model_test.rb index 485005eb9..20160a0c8 100644 --- a/test/unit/model_test.rb +++ b/test/unit/model_test.rb @@ -906,6 +906,60 @@ def without(&block) end end + context 'When an attribute has a custom serializer' do + setup { @person = Person.create(:time_zone => "Samoa") } + + should "be an instance of ActiveSupport::TimeZone" do + assert_equal ActiveSupport::TimeZone, @person.time_zone.class + end + + context 'when that attribute is updated' do + setup do + @attribute_value_before_change = @person.instance_variable_get(:@attributes)['time_zone'] + @person.assign_attributes({ :time_zone => 'Pacific Time (US & Canada)' }) + @changes_before_save = @person.changes.dup + @person.save! + end + + # Tests for serialization: + # Before the serialized attributes fix, the object/object_changes value that was stored was ridiculously long (58723). + should 'version.object should not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do + assert @person.versions.last.object. length < 105, "object length was #{@person.versions.last.object .length}" + end + should 'version.object_changes should not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do + assert @person.versions.last.object_changes.length < 105, "object_changes length was #{@person.versions.last.object_changes.length}" + end + # But now it stores the short, serialized value. + should 'version.object attribute should have stored the value returned by the attribute serializer' do + as_stored_in_version = HashWithIndifferentAccess[YAML::load(@person.versions.last.object)] + assert_equal 'Samoa', as_stored_in_version[:time_zone] + assert_equal @attribute_value_before_change.serialized_value, as_stored_in_version[:time_zone] + end + should 'version.object_changes attribute should have stored the value returned by the attribute serializer' do + as_stored_in_version = HashWithIndifferentAccess[YAML::load(@person.versions.last.object_changes)] + assert_equal ['Samoa', 'Pacific Time (US & Canada)'], as_stored_in_version[:time_zone] + assert_equal @person.instance_variable_get(:@attributes)['time_zone'].serialized_value, as_stored_in_version[:time_zone].last + end + + # Tests for unserialization: + should 'version.reify should convert the attribute value back to its original, unserialized value' do + assert_equal @attribute_value_before_change.unserialized_value, @person.versions.last.reify.time_zone + end + should 'version.changeset should convert the attribute value back to its original, unserialized value' do + assert_equal @person.instance_variable_get(:@attributes)['time_zone'].unserialized_value, @person.versions.last.changeset[:time_zone].last + end + should "record.changes (before save) returns the original, unserialized values" do + assert_equal [ActiveSupport::TimeZone, ActiveSupport::TimeZone], @changes_before_save[:time_zone].map(&:class) + end + should 'version.changeset should be the same as record.changes was before the save' do + assert_equal @changes_before_save, @person.versions.last.changeset + assert_equal [ActiveSupport::TimeZone, ActiveSupport::TimeZone], @person.versions.last.changeset[:time_zone].map(&:class) + end + + end + end + + context 'A new model instance which uses a custom Version class' do setup { @post = Post.new }