From 85733fe0d007d1780e42f97940230e118fc46364 Mon Sep 17 00:00:00 2001 From: Owen Rodda Date: Mon, 11 Jan 2016 22:07:57 -0500 Subject: [PATCH] Update PaperTrail and test suite for ActiveRecord 5 compatibility --- .rubocop_todo.yml | 1 + .travis.yml | 2 -- Appraisals | 3 ++ gemfiles/ar5.gemfile | 2 ++ lib/paper_trail/record_history.rb | 2 +- lib/paper_trail/reifier.rb | 16 +++++++++- spec/models/gadget_spec.rb | 2 +- spec/models/not_on_update_spec.rb | 4 ++- spec/models/widget_spec.rb | 12 +++++-- spec/rails_helper.rb | 1 + spec/requests/articles_spec.rb | 4 +-- spec/spec_helper.rb | 8 +++++ test/dummy/config/application.rb | 3 ++ test/functional/controller_test.rb | 20 ++++++------ .../functional/enabled_for_controller_test.rb | 4 +-- test/test_helper.rb | 31 +++++++++++++++---- test/unit/associations_test.rb | 6 +++- 17 files changed, 91 insertions(+), 30 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 4205c7921..fb23883f2 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -68,6 +68,7 @@ Style/AccessorMethodName: Style/Alias: Exclude: - 'lib/paper_trail/has_paper_trail.rb' + - 'lib/paper_trail/reifier.rb' # Offense count: 2 # Cop supports --auto-correct. diff --git a/.travis.yml b/.travis.yml index a52a081f3..a65f6c893 100644 --- a/.travis.yml +++ b/.travis.yml @@ -30,8 +30,6 @@ gemfile: matrix: fast_finish: true - allow_failures: - - gemfile: gemfiles/ar5.gemfile exclude: - gemfile: gemfiles/ar5.gemfile rvm: 1.9.3 diff --git a/Appraisals b/Appraisals index 8acc3080c..1c532a8bd 100644 --- a/Appraisals +++ b/Appraisals @@ -35,4 +35,7 @@ appraise "ar5" do gem "rspec-expectations", github: "rspec/rspec-expectations" gem "rspec-mocks", github: "rspec/rspec-mocks" gem "rspec-support", github: "rspec/rspec-support" + gem 'rails-controller-testing' + # Sinatra stable conflicts with AR5's rack dependency + gem 'sinatra', github: 'sinatra/sinatra', branch: '2.2.0-alpha' end diff --git a/gemfiles/ar5.gemfile b/gemfiles/ar5.gemfile index 9b2fed950..0731fa663 100644 --- a/gemfiles/ar5.gemfile +++ b/gemfiles/ar5.gemfile @@ -11,5 +11,7 @@ gem "rspec-core", :github => "rspec/rspec-core" gem "rspec-expectations", :github => "rspec/rspec-expectations" gem "rspec-mocks", :github => "rspec/rspec-mocks" gem "rspec-support", :github => "rspec/rspec-support" +gem "rails-controller-testing" +gem "sinatra", :github => "sinatra/sinatra", :branch => "2.2.0-alpha" gemspec :path => "../" diff --git a/lib/paper_trail/record_history.rb b/lib/paper_trail/record_history.rb index c9b953b6a..c3e6a2255 100644 --- a/lib/paper_trail/record_history.rb +++ b/lib/paper_trail/record_history.rb @@ -16,7 +16,7 @@ def initialize(versions, version_class) # Returns ordinal position of `version` in `sequence`. # @api private def index(version) - sequence.index(version) + sequence.to_a.index(version) end private diff --git a/lib/paper_trail/reifier.rb b/lib/paper_trail/reifier.rb index 137d9f682..15dbe0578 100644 --- a/lib/paper_trail/reifier.rb +++ b/lib/paper_trail/reifier.rb @@ -154,7 +154,9 @@ def reify_has_ones(transaction_id, model, options = {}) else child = version.reify(options.merge(:has_many => false, :has_one => false)) model.appear_as_new_record do - model.send "#{assoc.name}=", child + without_persisting(child) do + model.send "#{assoc.name}=", child + end end end end @@ -265,6 +267,18 @@ def versions_by_id(klass, version_id_subquery) where("id IN (#{version_id_subquery})"). inject({}) { |acc, v| acc.merge!(v.item_id => v) } end + + # Temporarily suppress #save so we can reassociate with the reified + # master of a has_one relationship. Since ActiveRecord 5 the related + # object is saved when it is assigned to the association. ActiveRecord + # 5 also happens to be the first version that provides #suppress. + def without_persisting(record) + if record.class.respond_to? :suppress + record.class.suppress { yield } + else + yield + end + end end end end diff --git a/spec/models/gadget_spec.rb b/spec/models/gadget_spec.rb index 7782e9baf..e8055b723 100644 --- a/spec/models/gadget_spec.rb +++ b/spec/models/gadget_spec.rb @@ -15,7 +15,7 @@ end it "should still generate a version when only the `updated_at` attribute is updated" do - expect { gadget.update_attribute(:updated_at, Time.now) }.to change{gadget.versions.size}.by(1) + expect { gadget.update_attribute(:updated_at, Time.now + 1) }.to change{gadget.versions.size}.by(1) end end diff --git a/spec/models/not_on_update_spec.rb b/spec/models/not_on_update_spec.rb index 7b57c0ded..01f22160a 100644 --- a/spec/models/not_on_update_spec.rb +++ b/spec/models/not_on_update_spec.rb @@ -12,7 +12,9 @@ it "increments the `:updated_at` timestamp" do before = record.updated_at - record.touch_with_version + Timecop.travel(1) do + record.touch_with_version + end expect(record.updated_at).to be > before end end diff --git a/spec/models/widget_spec.rb b/spec/models/widget_spec.rb index 453df9f90..078de28d2 100644 --- a/spec/models/widget_spec.rb +++ b/spec/models/widget_spec.rb @@ -43,7 +43,9 @@ subject { widget.versions.last.reify } it "should reset the value for the timestamp attrs for update so that value gets updated properly" do - expect { subject.save! }.to change(subject, :updated_at) + Timecop.travel(5) do + expect { subject.save! }.to change(subject, :updated_at) + end end end end @@ -253,13 +255,17 @@ it "creates a version" do count = widget.versions.size - widget.touch_with_version + Timecop.travel(1) do + widget.touch_with_version + end expect(widget.versions.size).to eq(count + 1) end it "increments the `:updated_at` timestamp" do time_was = widget.updated_at - widget.touch_with_version + Timecop.travel(1) do + widget.touch_with_version + end expect(widget.updated_at).to be > time_was end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index f44d4cc3d..358e0f3e3 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -12,6 +12,7 @@ require 'paper_trail/frameworks/rspec' require 'shoulda/matchers' require 'ffaker' +require 'timecop' # prevent Test::Unit's AutoRunner from executing during RSpec's rake task Test::Unit.run = true if defined?(Test::Unit) && Test::Unit.respond_to?(:run=) diff --git a/spec/requests/articles_spec.rb b/spec/requests/articles_spec.rb index 70b61eaae..a39fc847f 100644 --- a/spec/requests/articles_spec.rb +++ b/spec/requests/articles_spec.rb @@ -8,7 +8,7 @@ it "should not create a version" do expect(PaperTrail).to be_enabled_for_controller - expect { post(articles_path, valid_params) }.to_not change(PaperTrail::Version, :count) + expect { post articles_path, params_wrapper(valid_params) }.to_not change(PaperTrail::Version, :count) end it "should not leak the state of the `PaperTrail.enabled_for_controller?` into the next test" do @@ -21,7 +21,7 @@ context "`current_user` method returns a `String`" do it "should set that value as the `whodunnit`" do - expect { post articles_path, valid_params }.to change(PaperTrail::Version, :count).by(1) + expect { post articles_path, params_wrapper(valid_params) }.to change(PaperTrail::Version, :count).by(1) expect(article.title).to eq('Doh') expect(article.versions.last.whodunnit).to eq('foobar') end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 0dca3a47b..f6523fa0f 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -102,3 +102,11 @@ Kernel.srand config.seed =end end + +def params_wrapper(args) + if defined?(::Rails) && Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new('5.0.0.beta1') + { params: args } + else + args + end +end \ No newline at end of file diff --git a/test/dummy/config/application.rb b/test/dummy/config/application.rb index 0b38150a5..20973f848 100644 --- a/test/dummy/config/application.rb +++ b/test/dummy/config/application.rb @@ -69,6 +69,9 @@ class Application < Rails::Application if v >= Gem::Version.new("4.2") && v < Gem::Version.new("5.0.0.beta1") config.active_record.raise_in_transactional_callbacks = true end + if v >= Gem::Version.new("5.0.0.beta1") + config.active_record.time_zone_aware_types = [:datetime] + end end # Set test order for Test::Unit if possible diff --git a/test/functional/controller_test.rb b/test/functional/controller_test.rb index 8eead7321..800114e23 100644 --- a/test/functional/controller_test.rb +++ b/test/functional/controller_test.rb @@ -15,32 +15,32 @@ class ControllerTest < ActionController::TestCase test 'disable on create' do @request.env['HTTP_USER_AGENT'] = 'Disable User-Agent' - post :create, :widget => { :name => 'Flugel' } + post :create, params_wrapper({ widget: { name: 'Flugel' } }) assert_equal 0, assigns(:widget).versions.length end test 'disable on update' do @request.env['HTTP_USER_AGENT'] = 'Disable User-Agent' - post :create, :widget => { :name => 'Flugel' } + post :create, params_wrapper({ widget: { name: 'Flugel' } }) w = assigns(:widget) assert_equal 0, w.versions.length - put :update, :id => w.id, :widget => { :name => 'Bugle' } + put :update, params_wrapper({ id: w.id, widget: { name: 'Bugle' } }) widget = assigns(:widget) assert_equal 0, widget.versions.length end test 'disable on destroy' do @request.env['HTTP_USER_AGENT'] = 'Disable User-Agent' - post :create, :widget => { :name => 'Flugel' } + post :create, params_wrapper({ widget: { name: 'Flugel' } }) w = assigns(:widget) assert_equal 0, w.versions.length - delete :destroy, :id => w.id + delete :destroy, params_wrapper({ id: w.id }) widget = assigns(:widget) assert_equal 0, PaperTrail::Version.with_item_keys('Widget', w.id).size end test 'create' do - post :create, :widget => { :name => 'Flugel' } + post :create, params_wrapper({ widget: { name: 'Flugel' } }) widget = assigns(:widget) assert_equal 1, widget.versions.length assert_equal 153, widget.versions.last.whodunnit.to_i @@ -51,7 +51,7 @@ class ControllerTest < ActionController::TestCase test 'update' do w = Widget.create :name => 'Duvel' assert_equal 1, w.versions.length - put :update, :id => w.id, :widget => { :name => 'Bugle' } + put :update, params_wrapper({ id: w.id, widget: { name: 'Bugle' } }) widget = assigns(:widget) assert_equal 2, widget.versions.length assert_equal 153, widget.versions.last.whodunnit.to_i @@ -62,7 +62,7 @@ class ControllerTest < ActionController::TestCase test 'destroy' do w = Widget.create :name => 'Roundel' assert_equal 1, w.versions.length - delete :destroy, :id => w.id + delete :destroy, params_wrapper({ id: w.id }) widget = assigns(:widget) assert_equal 2, widget.versions.length assert_equal '127.0.0.1', widget.versions.last.ip @@ -72,7 +72,7 @@ class ControllerTest < ActionController::TestCase test "controller metadata methods should get evaluated if paper trail is enabled for controller" do @request.env['HTTP_USER_AGENT'] = 'User-Agent' - post :create, :widget => { :name => 'Flugel' } + post :create, params_wrapper({ widget: { name: 'Flugel' } }) assert PaperTrail.enabled_for_controller? assert_equal 153, PaperTrail.whodunnit assert PaperTrail.controller_info.present? @@ -82,7 +82,7 @@ class ControllerTest < ActionController::TestCase test "controller metadata methods should not get evaluated if paper trail is disabled for controller" do @request.env['HTTP_USER_AGENT'] = 'Disable User-Agent' - post :create, :widget => { :name => 'Flugel' } + post :create, params_wrapper({ widget: { name: 'Flugel' } }) assert_equal 0, assigns(:widget).versions.length assert !PaperTrail.enabled_for_controller? assert PaperTrail.whodunnit.nil? diff --git a/test/functional/enabled_for_controller_test.rb b/test/functional/enabled_for_controller_test.rb index e8a6eb2a2..928002b9e 100644 --- a/test/functional/enabled_for_controller_test.rb +++ b/test/functional/enabled_for_controller_test.rb @@ -6,7 +6,7 @@ class EnabledForControllerTest < ActionController::TestCase context "`PaperTrail.enabled? == true`" do should 'enabled_for_controller?.should == true' do assert PaperTrail.enabled? - post :create, :article => { :title => 'Doh', :content => FFaker::Lorem.sentence } + post :create, params_wrapper({ article: { title: 'Doh', content: FFaker::Lorem.sentence } }) assert_not_nil assigns(:article) assert PaperTrail.enabled_for_controller? assert_equal 1, assigns(:article).versions.length @@ -18,7 +18,7 @@ class EnabledForControllerTest < ActionController::TestCase should 'enabled_for_controller?.should == false' do assert !PaperTrail.enabled? - post :create, :article => { :title => 'Doh', :content => FFaker::Lorem.sentence } + post :create, params_wrapper({ article: { title: 'Doh', content: FFaker::Lorem.sentence } }) assert !PaperTrail.enabled_for_controller? assert_equal 0, assigns(:article).versions.length end diff --git a/test/test_helper.rb b/test/test_helper.rb index 193ccf71b..ae637b680 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -16,6 +16,17 @@ def using_mysql? end require File.expand_path("../dummy/config/environment.rb", __FILE__) +if Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new('5.0.0.beta1') + ActiveSupport.on_load(:action_controller) do + ActionController::TestCase.send(:include, Rails::Controller::Testing::TestProcess) + ActionController::TestCase.send(:include, Rails::Controller::Testing::TemplateAssertions) + + ActionDispatch::IntegrationTest.send(:include, Rails::Controller::Testing::TemplateAssertions) + ActionDispatch::IntegrationTest.send(:include, Rails::Controller::Testing::Integration) + + ActionView::TestCase.send(:include, Rails::Controller::Testing::TemplateAssertions) + end +end require "rails/test_help" require 'shoulda' require 'ffaker' @@ -89,6 +100,19 @@ def change_schema reset_version_class_column_info! end +def params_wrapper(args) + if defined?(::Rails) && Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new('5.0.0.beta1') + { params: args } + else + args + end +end + +def reset_version_class_column_info! + PaperTrail::Version.connection.schema_cache.clear! + PaperTrail::Version.reset_column_information +end + def restore_schema ActiveRecord::Migration.verbose = false ActiveRecord::Schema.define do @@ -97,9 +121,4 @@ def restore_schema end ActiveRecord::Migration.verbose = true reset_version_class_column_info! -end - -def reset_version_class_column_info! - PaperTrail::Version.connection.schema_cache.clear! - PaperTrail::Version.reset_column_information -end +end \ No newline at end of file diff --git a/test/unit/associations_test.rb b/test/unit/associations_test.rb index ea1c4ead2..5fb2e73ab 100644 --- a/test/unit/associations_test.rb +++ b/test/unit/associations_test.rb @@ -19,7 +19,11 @@ class AssociationsTest < ActiveSupport::TestCase # These would have been done in test_helper.rb if using_mysql? is true unless using_mysql? - self.use_transactional_fixtures = false + if self.respond_to? :use_transactional_tests= + self.use_transactional_tests = false + else + self.use_transactional_fixtures = false + end setup { DatabaseCleaner.start } end