Add support for per-model version_limit

[whatthewhat]

This is the same feature as suggested in #781, but implemented as a has_paper_trail option:

class Widget < ActiveRecord::Base
  has_paper_trail :version_limit => 10
end

One thing I was not 100% sure about when implementing the change is whether it's safe to call item in the after_create callback. I'm not really familiar with the codebase so it's hard to tell if there is a scenario where the item is not loaded and this will result in an extra DB query.. please advise :)

If that does create an issue the code could be changed to something like item_type.constantize.paper_trail_options[:version_limit]

[whatthewhat]

Just realised I was only running the Test::Unit suite locally, will take a look at rspec failures shortly. Also, should I have added the new test to the rspec suite?

Update

Right, so fetching paper_trail_options from the record itself was obviously the wrong solution (does not work for deleted records), but doing item_type.constantize.paper_trail_options also has an issue - it does not work with AST models..

What would you say is the recommended way to access paper_trail_options from the Version model?

[batter]

but doing item_type.constantize.paper_trail_options also has an issue - it does not work with AST models..

What does AST stand for? Are you referring to single table inheritance?

What would you say is the recommended way to access paper_trail_options from the Version model?

PaperTrail declares a class_attribute for paper_trail_options when has_paper_trail is declared on a model class. In ActiveSupport, the class_attribute declaration creates a class-level attribute whose value is inherited by subclasses and instances but can be overwritten at those levels while not overriding the class level. So I'm inclined to agree with your proposal that your best bet is to do something like:

(item && item.class || item_type.constantize).paper_trail_options

If that doesn't work due to STI concerns, and you have a separate model class, perhaps you could try something like this?

reflect_on_association(:item).class_name.constantize

Can you please elaborate on what your primary concerns are?

[whatthewhat]

What does AST stand for? Are you referring to single table inheritance?

Yep, I meant STI of course, sorry :)

@batter I think the solution you suggested should cover all the cases, just amended the commit, thanks for your help!

Some of the builds are still failing on this test, though. I'm not sure what is causing the failure and am unable to reproduce it locally at the moment.. have you seen something similar in the past? At first I thought it might be a test ordering issue, but it does not seem like the rspec suite runs tests in random order..
Somehow calling item in that after_create callback affects how reify works in that deleted versions test, I was unable to properly track down what happens yet..

[jaredbeck]

Somehow calling item in that after_create callback affects how reify works in that deleted versions test ..

Yeah. It might be because enforce_version_limit! is an after_create callback and calling item goes to the db and loads the record, changing the "persistence status" of that record somehow.

I can tell you for sure that item_type.constantize is no good, it is not compatible with STI. Consider the case of the Vehicle and the Car in our test suite. The car is the STI child, and is versioned (calls has_paper_trail). The Vechicle is the STI parent and is not versioned. Because AR stores the string "Vehicle" in versions.item_type, we cannot use item_type.constantize (because Vehicle.paper_trail_options would raise a NoMethodError).

This is a tricky one. It seems you can't call item during an after_create, but without calling item how are you going to get at the options? I don't know.

[whatthewhat]

Spent some time reading paper_trail and activerecord source code trying to figure this out - no luck, unfortunately :(

CI only seems to fail for MySQL, I'll try to set it up locally at some point to see if that helps clearing things up..

[jaredbeck]

Are you still working on this, Mikhail? Do you want to keep this PR open? Thanks.

[whatthewhat]

I would still very much like to see this feature in paper_trail :) I'll be on vacation at the end of the month and will likely have time to dig deeper into the issue.

Just in case I fail, would you be open to adding this feature without STI support (but with a readme note and a good error message)?

[jaredbeck]

Just in case I fail, would you be open to adding this feature without STI support (but with a readme note and a good error message)?

No, thanks. I'd rather not have the feature, if it can't support STI.

[jaredbeck]

Closing due to inactivity, but we'll be happy to re-open if you make progress, Mikhail.