Merge remote-tracking branch 'TylerRick/use_serialized_value_for_object_changes'

Conflicts:
	lib/paper_trail/has_paper_trail.rb

lib/paper_trail/has_paper_trail.rb

@@ -80,6 +80,44 @@ module PaperTrail
       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
@@ -167,15 +205,21 @@ module PaperTrail
             :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|
+          self.class.serialize_attribute_changes(changes) # Use serialized value for attributes when necessary
+        end
+      end
+
       def record_destroy
         if switched_on? and not new_record?
           version_class.create merge_metadata(:item_id   => self.id,
@@ -221,7 +265,9 @@ module PaperTrail
       end
 
       def object_to_string(object)
-        object.attributes.except(*self.class.paper_trail_options[:skip]).to_yaml
+        object.attributes.except(*self.class.paper_trail_options[:skip]).tap do |attributes|
+          self.class.serialize_attributes attributes
+        end.to_yaml
       end
 
       def changed_notably?

lib/paper_trail/version.rb

@@ -79,6 +79,7 @@ class Version < ActiveRecord::Base
           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 @@ class Version < ActiveRecord::Base
   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

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

test/dummy/db/migrate/20110208155312_set_up_test_tables.rb

@@ -80,6 +80,7 @@ class SetUpTestTables < ActiveRecord::Migration
 
     create_table :people, :force => true do |t|
       t.string :name
+      t.string :time_zone
     end
 
     create_table :songs, :force => true do |t|

test/unit/model_test.rb

@@ -916,6 +916,60 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
     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 }