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

Use with-associations option for the generated track_associations... #1098

Merged
merged 1 commit into from
May 30, 2018

Conversation

jkeck
Copy link
Contributor

@jkeck jkeck commented May 30, 2018

...initializer.

It appears that #1091 introduced this unintentionally by moving the initializer generator into a conditional in 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 if 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.

...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.
@jaredbeck
Copy link
Member

Hi Jessie, we are in the process of extracting the experimental association tracking feature into a separate gem, https://github.com/westonganger/paper_trail-association_tracking

Please open your PR there, thanks.

@jaredbeck jaredbeck closed this May 30, 2018
@jkeck
Copy link
Contributor Author

jkeck commented May 30, 2018

@jaredbeck between 9.0.2 and 9.1.0 the install generator for paper-trail started generating a configuration that forces you to erroneously opt into the experimental association tracking. I don't want association tracking, however 9.1.0 now forces you into association tracking when running the default generator without any options by introducing the bug that I fix in this PR.

I shouldn't open up a PR there, because paper-trail itself broke its own install generator by defaulting to generating PaperTrail.config.track_associations = true instead of PaperTrail.config.track_associations = false regardless of what options you pass when running generate paper_trail:install. It's not even possible to run the install generator and get PaperTrail.config.track_associations = false in the initializer (which is the way we want our setup to be, considering this experimental feature is not ready for production as stated in your documentation).

Our project, which uses paper_trail, has to provide a subsequent generator to revert this erroneous initializer generation to not break our entire app by simply switching from 9.0.2 to 9.1.0 (while still using the same generator task as we were in 9.0.2)

While forcing the generator to always be PaperTrail.config.track_associations = true in a minor version might be considered backwards compatible to you, from the perspective of an rails engine that depends on paper_trail, this most certainly was not backwards compatible to us.

It seems like if you were to extract that feature out into a gem, it would be great to allow your consumers to opt-into that experimental behavior instead of being forced into it (as is the case in 9.1.0).

@jaredbeck
Copy link
Member

Jessie, thanks for the clarification.

I will soon (hopefully in a few days) be merging the remove_association_tracking branch into master here. When I do that, the line of code you are trying to fix will no longer exist. The line of code you are trying to fix now lives in the https://github.com/westonganger/paper_trail-association_tracking repo.

That said, I guess there's some advantage to merging what you have here and cutting a 9.1.1 release. I'll do that.

BTW, the plan is for the remove_association_tracking branch to become 9.2.0.

It seems like if you were to extract that feature out into a gem, it would be great to allow your consumers to opt-into that experimental behavior ...

That's the plan. For now, PT will have a runtime dependency on PT-AT, but eventually you'll have to opt in by putting PT-AT in your Gemfile.

@jaredbeck jaredbeck reopened this May 30, 2018
@jaredbeck jaredbeck merged commit 7510adb into paper-trail-gem:master May 30, 2018
@jkeck
Copy link
Contributor Author

jkeck commented May 30, 2018

Thank you @jaredbeck! Much appreciated.

@jkeck jkeck deleted the patch-1 branch May 31, 2018 00:55
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.

2 participants