Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid using private API of PaperTrail #3059

Merged
merged 2 commits into from
Sep 18, 2018

Conversation

jaredbeck
Copy link
Contributor

@jaredbeck jaredbeck commented Aug 27, 2018

  1. version_class_name is private API. Please use paper_trail_options instead.
  2. Fixes deprecation warning re: PaperTrail.whodunnit

`version_class_name` is private API. Please use `paper_trail_options` instead.
@jaredbeck jaredbeck changed the title PaperTrail options Avoid using private API of PaperTrail Aug 27, 2018
@jaredbeck
Copy link
Contributor Author

jaredbeck commented Aug 27, 2018

version_class_name is private API. Please use paper_trail_options instead.

The options that go into paper_trail_options are well-documented and so unlikely to change without warning. (Basically, everything you pass to has_paper_trail goes straight into paper_trail_options)

version_class_name however is utterly private API never documented or intended for anyone else to use.

@jaredbeck jaredbeck force-pushed the paper_trail_options branch from 448e639 to f427dc3 Compare August 27, 2018 04:55
@jaredbeck
Copy link
Contributor Author

I also had some ideas about how to improve the test coverage for the PT adapter, but it'll require schema changes, and activerecord is happy on travis but mongoid is complaining and it's 0100 here and I just want to go to bed. 😴

@jaredbeck jaredbeck force-pushed the paper_trail_options branch from f427dc3 to a216c74 Compare August 27, 2018 16:24
@jaredbeck
Copy link
Contributor Author

jaredbeck commented Aug 27, 2018

Please restart job https://travis-ci.org/sferik/rails_admin/jobs/421174193 - suspected intermittent failure unrelated to this PR - all other jobs passing

@mshibuya mshibuya merged commit 6d08234 into railsadminteam:master Sep 18, 2018
@mshibuya
Copy link
Member

Great work, thanks!

@jaredbeck jaredbeck deleted the paper_trail_options branch September 19, 2018 01:12
@rdunlop
Copy link

rdunlop commented Sep 27, 2018

This change is causing issues for me, when I try to view the History of a model which does Not have papertrail, it fails with

NoMethodError (undefined method `paper_trail_options' for #<Class:0x00007f80f6086500>

I've been able to resolve this by reverting this change.

@jaredbeck
Copy link
Contributor Author

Dang, sorry Robin. So rails_admin will use the RailsAdmin::Extensions::PaperTrail::AuditingAdapter even when you set config.audit_with :something_else?

@rdunlop
Copy link

rdunlop commented Sep 27, 2018

@jaredbeck I haven't had the opportunity to fully understand what's going on here, it could very well be a mis-configuration on my app...it's had many versions of rails_admin/pundit/papertrail over the years...and I just try to keep updating when possible...and then I run into breakages.

Here's what I have right now:

RailsAdmin.config do |config|
....
  config.authorize_with :pundit
  # If you want to track changes on your models:
  # config.audit_with :history, 'User'

  # Or with a PaperTrail: (you need to install it first)
  config.audit_with :paper_trail, 'User'
...
end

And then I fixed my current problem by reverting your fix:

# Fix issue where it tries to load paper_trail_options for a model which has none
# this reverts https://github.com/sferik/rails_admin/pull/3059/files
module RailsAdmin
  module Extensions
    module PaperTrail
      class AuditingAdapter
        def version_class_for(model)
          model_name = model.name
          klass = model_name.constantize.try(:version_class_name).try(:constantize)
          klass || @version_class
        end
      end
    end
  end
end

I also have a monkeypatch for rails_admin in order to change the pundit behavior, which could be involved (https://github.com/sudosu/rails_admin_pundit/issues/12#issuecomment-328357027)...but I don't think it should be. (I apologize that I haven't created a minimum reproduction scenario).

Note: the pertinent gems in my system right now are:
pundit (2.0.0)
rails_admin (1.4.2)
paper_trail (10.0.1)
(I do not have rails_admin_pundit or other paper_trail related gems installed).

I also tried it with paper_trail 9.2.0, same behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants