When enforcing version_limit, order by created_at

Fixes #940

CHANGELOG.md

@@ -26,6 +26,9 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/).
   Fix error calling private method in rails 4.0
 - [#938](https://github.com/airblade/paper_trail/pull/938) - Fix bug where
   non-standard foreign key names broke belongs_to associations
+- [#940](https://github.com/airblade/paper_trail/pull/940) - When destroying
+  versions to stay under version_limit, don't rely on the database to
+  implicitly return the versions in the right order
 
 ## 6.0.2 (2016-12-13)
 

lib/paper_trail/version_concern.rb

@@ -312,7 +312,8 @@ module PaperTrail
     def enforce_version_limit!
       limit = PaperTrail.config.version_limit
       return unless limit.is_a? Numeric
-      previous_versions = sibling_versions.not_creates
+      previous_versions = sibling_versions.not_creates.
+        order(self.class.timestamp_sort_order("asc"))
       return unless previous_versions.size > limit
       excess_versions = previous_versions - previous_versions.last(limit)
       excess_versions.map(&:destroy)

spec/paper_trail/version_limit_spec.rb

@@ -0,0 +1,58 @@
+require "rails_helper"
+
+module PaperTrail
+  RSpec.describe Cleaner, versioning: true do
+    before do
+      @last_limit = PaperTrail.config.version_limit
+    end
+    after do
+      PaperTrail.config.version_limit = @last_limit
+    end
+
+    it "cleans up old versions" do
+      PaperTrail.config.version_limit = 10
+      widget = Widget.create
+
+      100.times do |i|
+        widget.update_attributes(name: "Name #{i}")
+        expect(Widget.find(widget.id).versions.count).to be <= 11
+        # 11 versions = 10 updates + 1 create.
+      end
+    end
+
+    it "deletes oldest versions, when the database returns them in a different order" do
+      epoch = DateTime.new(2017, 1, 1)
+
+      widget = Timecop.freeze(epoch) { Widget.create }
+
+      # Sometimes a database will returns records in a different order than
+      # they were inserted. That's hard to get the database to do, so we'll
+      # just create them out-of-order:
+      (1..5).to_a.shuffle.each do |n|
+        Timecop.freeze(epoch + n.hours) do
+          PaperTrail::Version.create!(
+            item: widget,
+            event: "update",
+            object: { "id" => widget.id, "name" => "Name #{n}" }.to_yaml
+          )
+        end
+      end
+      expect(Widget.find(widget.id).versions.count).to eq(6) # 1 create + 5 updates
+
+      # Now, we've recreated the scenario where we can accidentally clean up
+      # the wrong versions. Re-enable the version_limit, and trigger the
+      # clean-up:
+      PaperTrail.config.version_limit = 3
+      widget.versions.last.send(:enforce_version_limit!)
+
+      # Verify that we have fewer versions:
+      expect(widget.reload.versions.count).to eq(4) # 1 create + 4 updates
+
+      # Exclude the create, because the create will return nil for `#reify`.
+      last_names = widget.versions.not_creates.map(&:reify).map(&:name)
+      expect(last_names).to eq(["Name 3", "Name 4", "Name 5"])
+      # No matter what order the version records are returned it, we should
+      # always keep the most-recent changes.
+    end
+  end
+end