add 'limit' option to has_paper_trail to override global PaperTrail.config.version_limit setting

[enrico]

…PaperTrail.config.version_limit value on a per-model basis.

Thank you for your contribution!

Check the following boxes:

• Wrote good commit messages.
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new
  code introduces user-observable changes.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.

[jaredbeck]

has_paper_trail limit: 2

Hi Enrico, I like this proposed API. I feel like this idea has been discussed in the past, but don't have time to go looking for it right now.

[enrico]

Jared, didn't know about the closed pr #915 .
I looked at the conversation and found out this comment by you:

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.

Looks like the solution could be to use item_subtype, but I read in the documents that it's supposed to be optional in the migrations... (yet the tests rely on it, since it's part of the dummy app).

I made some changes to make it work using the item_subtype attribute (by using constantize and and avoiding instantiating item). All tests work (including the one I had to change the code with .reify(dup: true)

    def version_limit_for_item
      klass = (respond_to?(:item_subtype) && item_subtype ? item_subtype : item_type).constantize
      if klass.respond_to?(:paper_trail_options) && klass.paper_trail_options.key?(:limit)
        return klass.paper_trail_options[:limit]
      end
      PaperTrail.config.version_limit
    end

[enrico]

Jared,

so my conclusion is that this feature (per-class limit in the has_paper_trail block) works correctly for non-STI classes but fails with STI classes when the item_subtype column is missing.

The code as of now uses respond_to?(:item_subtype) to cover the optionality of the column, but could be simplified if the column is actually always there.

Questions:

1. Is there any particular reason why the column should not be part of the default migration for paper trail 10? ( of course, this would be my favorite option :)

2. If we still want to leave item_subtype optional, could we still ship this and document the requirement of item_subtype? We could also display an error when we detect the limit key in the paper_trail_options hash and the item_subtype column is missing...

Thanks!
Enrico.

[jaredbeck]

Hi Enrico, I haven't had time to review this PR (I haven't even read all the comments) but I'll try to quickly answer your latest questions.

Is there any particular reason why the column [item_subtype] should not be part of the default migration for paper trail 10?

It was determined that most people do not need item_subtype. Also, by making it optional, we made it easier for people to upgrade to PT 10.

If we still want to leave item_subtype optional, could we still ship this and document the requirement of item_subtype?

Yes, in section 4.b.1 please.

We could also display an error when we detect the limit key in the paper_trail_options hash and the item_subtype column is missing...

In theory, errors like this can be very helpful (better than docs) and I'd encourage you to add one. In practice they can be hard to implement because it can't be checked until we're certain that ActiveRecord will have loaded the schema. Generally speaking, the later you perform the check, the better.

[enrico]

I think I'm done. please check when you get a chance.

[jaredbeck]

Hi Enrico, I've done my first review. I've pushed my suggestions. Please review them. Summary:

• when item_subtype is absent, raise instead of warning
• copywriting
  • changelog entries should be concise
  • focus new docs in section 2.e.1
• faster tests (create fewer records)

[enrico]

Suggested change

	if PaperTrail::Version.column_names.include?("item_subtype")
	if self.class.item_subtype_column_present?

I think this is a must, now that you've added the class method to check for the item_subtype...

[enrico]

Jared, your proposed changes look a lot cleaner than what I came up with (and I have to embarrassedly admit that in this process I learned about &.)!.

Please let me know about my suggestion above. If you agree, I can make the change and squish all commits.

[jaredbeck]

Jared, your proposed changes look a lot cleaner ..

Thanks! 😊

.. I learned about &. ..

Did you know that Matz calls it the "lonely operator" because it looks like a person sitting and looking at a stone? 😀

Please let me know about my suggestion above.

Good catch, patched by 81f760b

If you agree, I can make the change and squish all commits.

You don't have to, I can squash when I merge this PR. You'll still be the author of the resulting commit. I'm just waiting for CI to pass.