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鈥檒l 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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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

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
1 participant