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

Add PaperTrail.config.association_reify_error_behaviour #1091

Conversation

Projects
None yet
2 participants
@westonganger
Copy link
Contributor

commented May 19, 2018

Fixes #1090

The added tests do pass when configured appropriately in the dummy app initializer before running the tests. However I am not sure exactly how to dynamically test this option so I have commented out the tests, please advise.

README.md Outdated
`PaperTrail::Reifiers::HasOne::FoundMoreThanOne` error)
1. Sometimes the has_one association will find more than one possible candidate and will raise a `PaperTrail::Reifiers::HasOne::FoundMoreThanOne` error. For example, see `spec/models/person_spec.rb`
- If you are not using STI, you may want to just assume the first result (of multiple) is the correct one and continue. Versions pre v8.1.2 and below did this without error or warning. To do so add the following line to your initializer: `PaperTrail.config.association_reify_error_behaviour = :warn`. Valid options are: `[:error, :warn, :ignore]`
- When using STI, even if you enable :warn you will likely still end up recieving an `ActiveRecord::AssociationTypeMismatch` error in which case your SOL

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck May 21, 2018

Member

Try to avoid idioms in documentation. "SOL" is an expression that only native English speakers will understand. Perhaps instead we should recommend the :error option for STI users?

This comment has been minimized.

Copy link
@westonganger

westonganger May 22, 2018

Author Contributor

Im not going to recommend the :error option to STI users in case they are also struggling with the non-STI version of this problem. Beside the :error option is already the default.

"PaperTrail.config.track_associations = true\n",
"PaperTrail.config.association_reify_error_behaviour = :error"
)
end

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck May 21, 2018

Member

This change seems unrelated to this PR. Please advise.

version = versions.first
version.logger&.warn(
format(FoundMoreThanOne::MESSAGE_FMT, base_class_name, versions.length)
)

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck May 21, 2018

Member

Nice idea to use version.logger.

I think I'd prefer:

version.logger&.warn(
  FoundMoreThanOne.new(base_class_name, versions.length).message
)

What do you think?

This comment has been minimized.

Copy link
@westonganger

westonganger May 22, 2018

Author Contributor

Meh. I'd prefer not to because that creates an extra error object for no reason.

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck May 23, 2018

Member

Meh. I'd prefer not to because that creates an extra error object for no reason.

The reason is to avoid duplicating the implementation of FoundMoreThanOne#message. By duplicating the implementation, if the message changes, you have to change it in two places.

.. that creates an extra error object ..

For me, the duplication of code seems worse than a few object allocations, but that's not based on any benchmark of course, just a gut instinct.

# )

# person.reload.versions.second.reify(has_one: true)
# end

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck May 21, 2018

Member

If you're having trouble writing this type of test, I'd be happy with testing load_version in isolation, even using mocks. I'd be happier with a test that used real database records, but for now an isolated test is OK with me.

@@ -45,7 +45,8 @@ def create_migration_file
def create_initializer
create_file(
"config/initializers/paper_trail.rb",
"PaperTrail.config.track_associations = #{!!options.with_associations?}\n"
"PaperTrail.config.track_associations = true\n",
"PaperTrail.config.association_reify_error_behaviour = :error"

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck May 23, 2018

Member

Nice thinking to include this in the generator

)
end
end
end

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck May 23, 2018

Member

I like how you put these specs in different files.

Can we put them in spec/paper_trail/association_error_behaviour though? Because, spec/models is intended to correlate to app/models.

# config.association_reify_error_behaviour = :warn
# expect(config.association_reify_error_behaviour).to eq(:warn)
# end
# end

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck May 23, 2018

Member

Given the other specs you've added, it looks like we can delete this commented-out spec?

@jaredbeck jaredbeck merged commit d056c7e into paper-trail-gem:master May 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jaredbeck

This comment has been minimized.

Copy link
Member

commented May 23, 2018

Thanks Weston, I'd like to tidy a few very minor things up (eg. changelog format, nothing major) and then I'll cut a release.

@jaredbeck

This comment has been minimized.

Copy link
Member

commented May 23, 2018

Released in 9.1.0

jkeck added a commit to jkeck/paper_trail that referenced this pull request May 30, 2018

Use with-associations option for the generated track_associations...
...initializer.

It appears that paper-trail-gem#1091 introduced this unintentionally by moving the initializer generator into a conditional in paper-trail-gem@f9a8a77#diff-385d0586e82e9b33429c177d016048af but when it was finally merged in under d056c7e that conditional was removed but the interpolated `with_associations?` option was never added back.

This means that is you use the default behavior of not using the with-associations option when running the install generator, you still get `PaperTrail.config.track_associations = true` in your generated initializer even though none of the other migration files, etc are generated so `Could not find table 'version_associations'` is raised when saving any model versioned through paper trail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.