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

Do not allow multiple has_paper_trail definitions for models #1479

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

fatkodima
Copy link
Contributor

Fixes #1478.

There were 3 variants on how to proceed:

  1. ignore multiple has_paper_trail calls - this can be confusing, for example
class Parent < ApplicationRecord
  has_paper_trail on: [:create]
end

class Child < Parent
  has_paper_trail on: [:update]
end

For which events we should create version records? 🤷 Same without STI (just when we include some module and have the has_paper_trail called on the class) or just has_paper_trail defined on the ApplicationRecord and then in the concrete class too.

  1. detect if we use STI and do something smart about it (as suggested in the issue) - this won't work, because when we detect that we use STI, the callbacks are already defined on the parent model and we can't "undefine" them
  2. raise if we have multiple calls - implemented in this PR. Looks the most sane to me, but this is a breaking change

@jaredbeck jaredbeck merged commit 82b73c9 into paper-trail-gem:master Sep 14, 2024
7 checks passed
@jaredbeck
Copy link
Member

[option 3] raise if we have multiple calls - implemented in this PR. Looks the most sane to me, but this is a breaking change

I agree. This is the only option that I'm interested in maintaining.

Thanks for the contribution.

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.

Calling has_paper_trail on STI models causes 2 inserts to versions
2 participants