Switching many of the scopes and methods on the Version class that de…

…al with ordering to use the timestamp_field for ordering instead of the primary key to keep consistent with the change that was made for #235

lib/paper_trail/has_paper_trail.rb

@@ -151,7 +151,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).following(timestamp).first
+        v = send(self.class.versions_association_name).subsequent(timestamp).first
         v ? v.reify(reify_options) : self
       end
 

lib/paper_trail/version.rb

@@ -26,23 +26,19 @@ def self.not_creates
       where 'event <> ?', 'create'
     end
 
-    scope :subsequent, lambda { |version|
-      where("#{self.primary_key} > ?", version).order("#{self.primary_key} ASC")
+    # These methods accept a timestamp or a version and returns other versions that come before or after
+    scope :subsequent, lambda { |obj|
+      obj = obj.send(PaperTrail.timestamp_field) if obj.is_a?(self)
+      where("#{PaperTrail.timestamp_field} > ?", obj).order("#{PaperTrail.timestamp_field} ASC")
     }
-
-    scope :preceding, lambda { |version|
-      where("#{self.primary_key} < ?", version).order("#{self.primary_key} DESC")
-    }
-
-    scope :following, lambda { |timestamp|
-      # TODO: is this :order necessary, considering its presence on the has_many :versions association?
-      where("#{PaperTrail.timestamp_field} > ?", timestamp).
-        order("#{PaperTrail.timestamp_field} ASC, #{self.primary_key} ASC")
+    scope :preceding, lambda { |obj|
+      obj = obj.send(PaperTrail.timestamp_field) if obj.is_a?(self)
+      where("#{PaperTrail.timestamp_field} < ?", obj).order("#{PaperTrail.timestamp_field} DESC")
     }
 
     scope :between, lambda { |start_time, end_time|
       where("#{PaperTrail.timestamp_field} > ? AND #{PaperTrail.timestamp_field} < ?", start_time, end_time).
-        order("#{PaperTrail.timestamp_field} ASC, #{self.primary_key} ASC")
+        order("#{PaperTrail.timestamp_field} ASC")
     }
 
     # Restore the item from this version.
@@ -145,8 +141,8 @@ def previous
     end
 
     def index
-      id_column = self.class.primary_key.to_sym
-      sibling_versions.select(id_column).order("#{id_column} ASC").map(&id_column).index(self.send(id_column))
+      @index ||= sibling_versions.select([PaperTrail.timestamp_field, self.class.primary_key.to_sym]).
+        order("#{PaperTrail.timestamp_field} ASC").index(self)
     end
 
     private

test/unit/version_test.rb

@@ -48,10 +48,50 @@ class PaperTrail::VersionTest < ActiveSupport::TestCase
       assert PaperTrail::Version.not_creates.present?
     }
 
-    should "return all items except create events" do
+    should "return all versions except create events" do
       PaperTrail::Version.not_creates.each do |version|
         assert_not_equal "create", version.event
       end
     end
   end
+
+  context "PaperTrail::Version.subsequent" do
+    setup { 2.times { @animal.update_attributes(:name => Faker::Lorem.word) } }
+
+    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)
+        assert_equal value, @animal.versions.to_a
+        assert_not_nil value.to_sql.match(/ORDER BY created_at ASC\z/)
+      end
+    end
+
+    context "receiving a `PaperTrail::Version`" do
+      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 }
+        assert_not_nil value.to_sql.match(/WHERE \(created_at > '#{@animal.versions.first.send(PaperTrail.timestamp_field).strftime("%F %T.%6N")}'\)/)
+      end
+    end
+  end
+
+  context "PaperTrail::Version.preceding" do
+    setup { 2.times { @animal.update_attributes(:name => Faker::Lorem.word) } }
+
+    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)
+        assert_equal value, @animal.versions.reverse
+        assert_not_nil value.to_sql.match(/ORDER BY created_at DESC\z/)
+      end
+    end
+
+    context "receiving a `PaperTrail::Version`" do
+      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
+        assert_not_nil value.to_sql.match(/WHERE \(created_at < '#{@animal.versions.last.send(PaperTrail.timestamp_field).strftime("%F %T.%6N")}'\)/)
+      end
+    end
+  end
 end