From 202ee99c43d2090463dc774d2a6f6c388f3f67ed Mon Sep 17 00:00:00 2001 From: Ben Atkins Date: Tue, 1 Apr 2014 13:38:53 -0400 Subject: [PATCH] Attempt to sort based on primary key in scope methods on VersionConcern if primary key is an integer. This makes some of the changes to these scope methods that came from 6a4aba2 more flexible, in that the user can choose to compare timestamps if desired, but it defaults to comparing and sorting, via the primary key (if it is an integer). If the primary key is not an integer, it still defaults to using the PaperTrail.timestamp_field. This is my proposed fix for #314, and I also believe it should fix #317. It seems that that this issue is usually encountered when testing PaperTrail with MySQL (presumably due to lack of microsecond timestamp support). --- lib/paper_trail/has_paper_trail.rb | 2 +- lib/paper_trail/version_concern.rb | 33 ++++++++++++++++++++++++------ test/unit/version_test.rb | 16 ++++----------- 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/lib/paper_trail/has_paper_trail.rb b/lib/paper_trail/has_paper_trail.rb index 052d37dee..296c2434f 100644 --- a/lib/paper_trail/has_paper_trail.rb +++ b/lib/paper_trail/has_paper_trail.rb @@ -177,7 +177,7 @@ def originator def version_at(timestamp, reify_options={}) # Because a version stores how its object looked *before* the change, # we need to look for the first version created *after* the timestamp. - v = send(self.class.versions_association_name).subsequent(timestamp).first + v = send(self.class.versions_association_name).subsequent(timestamp, true).first v ? v.reify(reify_options) : self end diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index d5a62e05e..f45bb1e8a 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -33,22 +33,43 @@ def not_creates where 'event <> ?', 'create' end - # These methods accept a timestamp or a version and returns other versions that come before or after - def subsequent(obj) + # Expects `obj` to be an instance of `PaperTrail::Version` by default, but can accept a timestamp if + # `timestamp_arg` receives `true` + def subsequent(obj, timestamp_arg = false) + if timestamp_arg != true && self.primary_key_is_int? + return where("#{table_name}.#{self.primary_key} > ?", obj).order("#{table_name}.#{self.primary_key} ASC") + end + obj = obj.send(PaperTrail.timestamp_field) if obj.is_a?(self) where("#{table_name}.#{PaperTrail.timestamp_field} > ?", obj). - order("#{table_name}.#{PaperTrail.timestamp_field} ASC") + order(self.timestamp_sort_order) end - def preceding(obj) + def preceding(obj, timestamp_arg = false) + if timestamp_arg != true && self.primary_key_is_int? + return where("#{table_name}.#{self.primary_key} < ?", obj).order("#{table_name}.#{self.primary_key} DESC") + end + obj = obj.send(PaperTrail.timestamp_field) if obj.is_a?(self) where("#{table_name}.#{PaperTrail.timestamp_field} < ?", obj). - order("#{table_name}.#{PaperTrail.timestamp_field} DESC") + order(self.timestamp_sort_order('DESC')) end + def between(start_time, end_time) where("#{table_name}.#{PaperTrail.timestamp_field} > ? AND #{table_name}.#{PaperTrail.timestamp_field} < ?", - start_time, end_time).order("#{table_name}.#{PaperTrail.timestamp_field} ASC") + start_time, end_time).order(self.timestamp_sort_order) + end + + # defaults to using the primary key as the secondary sort order if possible + def timestamp_sort_order(order = 'ASC') + self.primary_key_is_int? ? + "#{table_name}.#{PaperTrail.timestamp_field} #{order}, #{table_name}.#{self.primary_key} #{order}" : + "#{table_name}.#{PaperTrail.timestamp_field} #{order}" + end + + def primary_key_is_int? + @primary_key_is_int ||= columns_hash[primary_key].type == :integer end # Returns whether the `object` column is using the `json` type supported by PostgreSQL diff --git a/test/unit/version_test.rb b/test/unit/version_test.rb index b92c6a190..8022d9d3f 100644 --- a/test/unit/version_test.rb +++ b/test/unit/version_test.rb @@ -66,9 +66,9 @@ class PaperTrail::VersionTest < ActiveSupport::TestCase context "receiving a TimeStamp" do should "return all versions that were created before the Timestamp; descendingly by order of the `PaperTrail.timestamp_field`" do - value = PaperTrail::Version.subsequent(1.hour.ago) + value = PaperTrail::Version.subsequent(1.hour.ago, true) assert_equal value, @animal.versions.to_a - assert_not_nil value.to_sql.match(/ORDER BY versions.created_at ASC\z/) + assert_not_nil value.to_sql.match(/ORDER BY versions.created_at ASC/) end end @@ -76,10 +76,6 @@ class PaperTrail::VersionTest < ActiveSupport::TestCase should "grab the Timestamp from the version and use that as the value" do value = PaperTrail::Version.subsequent(@animal.versions.first) assert_equal value, @animal.versions.to_a.tap { |assoc| assoc.shift } - # This asssertion can't pass in Ruby18 because the `strftime` method doesn't accept the %6 (milliseconds) command - if RUBY_VERSION.to_f >= 1.9 and not defined?(JRUBY_VERSION) - assert_not_nil value.to_sql.match(/WHERE \(versions.created_at > '#{@animal.versions.first.send(PaperTrail.timestamp_field).strftime("%F %T.%6N")}'\)/) - end end end end @@ -89,9 +85,9 @@ class PaperTrail::VersionTest < ActiveSupport::TestCase context "receiving a TimeStamp" do should "return all versions that were created before the Timestamp; descendingly by order of the `PaperTrail.timestamp_field`" do - value = PaperTrail::Version.preceding(Time.now) + value = PaperTrail::Version.preceding(Time.now, true) assert_equal value, @animal.versions.reverse - assert_not_nil value.to_sql.match(/ORDER BY versions.created_at DESC\z/) + assert_not_nil value.to_sql.match(/ORDER BY versions.created_at DESC/) end end @@ -99,10 +95,6 @@ class PaperTrail::VersionTest < ActiveSupport::TestCase should "grab the Timestamp from the version and use that as the value" do value = PaperTrail::Version.preceding(@animal.versions.last) assert_equal value, @animal.versions.to_a.tap { |assoc| assoc.pop }.reverse - # This asssertion can't pass in Ruby18 because the `strftime` method doesn't accept the %6 (milliseconds) command - if RUBY_VERSION.to_f >= 1.9 and not defined?(JRUBY_VERSION) - assert_not_nil value.to_sql.match(/WHERE \(versions.created_at < '#{@animal.versions.last.send(PaperTrail.timestamp_field).strftime("%F %T.%6N")}'\)/) - end end end end