Support belongs_to_required_by_default by joelmichael · Pull Request #985 · paper-trail-gem/paper_trail

joelmichael · 2017-08-24T19:58:12Z

We're trying to upgrade our app to use the Rails 5.1 defaults, but we ran into a problem with PaperTrail. Specifically, when we try to soft delete certain records (i.e., mark them as deleted without actually deleting the row from the DB), and then try to make a change to the soft-deleted record, it raises an error saying the versions are invalid. This is because of the belongs_to :item on VersionConcern, which makes the association required with the new defaults. To stay compatible with the old way and allow deleting and soft deleting of records, we should make this association optional.

An unrelated commit adds the rails-controller-testing gem to the Gemfile. I tried adding it as a development dependency in the gemspec, but couldn't get it to work. Without this gem, a lot of the tests fail for me.

joelmichael · 2017-08-24T21:07:27Z

Test failures are due to the CI using older versions of the gems (Rails 4.2), due to there being no Gemfile.lock.

jaredbeck · 2017-08-24T21:50:54Z

Hi Joel, thanks for the contribution!

Re: The rails-controller-testing gem: Why do we need it? We haven't needed it before. I just did a build a week ago and it passed: https://travis-ci.org/airblade/paper_trail/builds/266018448
Re: belongs_to(optional:) I have not used optional before and I'll have to do some research. Does it just test that item.present? or does it query the database? The docs are a bit unclear, they just say:

:optional
When set to true, the association will not have its presence validated.

joelmichael · 2017-08-24T21:54:03Z

@jaredbeck The gem is needed for Rails 5 backwards compatibility with the controller tests, specifically the assigns feature which was removed.

The optional: true is required by Rails 5 as well, because Rails 5 makes required: true the default on all belongs_to associations.

This isn't showing up in CI because, it seems to me, Travis is including the Rails 4.2 gems instead of Rails 5.0 or 5.1.

jaredbeck · 2017-08-24T22:42:19Z

@@ -1,2 +1,3 @@
 source "https://rubygems.org"
 gemspec
+gem "rails-controller-testing"


Re: The rails-controller-testing gem: Why do we need it? We haven't needed it before. ...

Oops, I was wrong about this, we already are using the rails-controller-testing gem. Please see our Appraisals file. No changes to our Gemfile are necessary.

joelmichael · 2017-08-25T00:02:30Z

Sorry about my confusion. I removed the commit that edited the Gemfile and added some version conditionals where necessary. Seems to pass in all versions for me locally, so I suspect it'll pass on CI soon.

joelmichael · 2017-08-25T00:13:33Z

I updated the commit message and renamed this PR for improved clarity.

joelmichael · 2017-08-25T00:19:02Z

Note that it now prints out this warning message in AR 5.0 and 5.1 appraisal specs:

DEPRECATION WARNING: paper_trail.on_destroy(:after) is incompatible with ActiveRecord's
belongs_to_required_by_default and has no effect. Please use :before
or disable belongs_to_required_by_default.
 (called from <class:AfterDestroyModifier> at /Users/joel/Code/paper_trail/spec/dummy_app/app/models/callback_modifier.rb:25)

Concern re: Gemfile has been addressed

jaredbeck · 2017-08-25T04:46:23Z

Joel, thanks for addressing my concerns re: the rails-controller-testing gem. I know the test suite is hard to run (because we test against so many rails' and dbs) and I hope you found the contributing guide helpful. Please open a new PR if you have any suggestions for the contributing guide.

Now, let me see if I understand the changes that remain in this PR.

We're trying to upgrade our app to use the Rails 5.1 defaults, but we ran into a problem with PaperTrail.
Specifically, when we try to soft delete certain records (i.e., mark them as deleted without actually deleting the row from the DB), and then try to make a change to the soft-deleted record, it raises an error saying the versions are invalid.

Do you get a RecordInvalid error? Are you referring to the following change, which happened in rails 5.0?

belongs_to will now trigger a validation error by default if the association is not present. You can turn this off on a per-association basis with optional: true. Also deprecate required option in favor of optional for belongs_to. (Pull Request)
http://guides.rubyonrails.org/5_0_release_notes.html

Earlier, I asked:

I have not used optional before and I'll have to do some research. Does it [optional: false] just test that item.present? or does it query the database?

Did this question get answered?

.. when we try to soft delete certain records (i.e., mark them as deleted without actually deleting the row from the DB), and then try to make a change to the soft-deleted record, it raises an error saying the versions are invalid.

Please include a test that demonstrates this issue. Use an existing test model in spec/dummy_app if at all possible. If not, it's ok to create a new one.

To stay compatible with the old way and allow deleting and soft deleting of records, we should make this association optional.

Based only on the release notes I quoted above, I agree that adding optional: true improves compatibility. I think we're on the right track.

Please add an entry to the changelog, under Unreleased -> Fixed.

Do we need to add optional: true to as many of our test models as you have done? I'd rather change as few tests as possible.

Note that it now prints out this warning message in AR 5.0 and 5.1 appraisal specs:
DEPRECATION WARNING: paper_trail.on_destroy(:after) is incompatible with ActiveRecord's
belongs_to_required_by_default and has no effect. Please use :before
or disable belongs_to_required_by_default. ..

Well that's a problem. We don't want our test suite printing any deprecation warnings. First, why is the warning printed? Clearly, because of the changes to spec/dummy_app/config/application.rb. Now, what is the warning telling us? Our specs are using on_destory(:after), I assume. So, what options do we have? Change those specs to use :before? That seems easiest, but then we lose test coverage of :after. WDYT?

Thanks again for your work on this.

jaredbeck · 2017-08-25T04:48:57Z

Please include a test that demonstrates this issue.

On second thought, is it possible that many tests demonstrate this issue once your change to spec/dummy_app/config/application.rb is applied?

jaredbeck

Various concerns given in comment 324822869: #985 (comment)

joelmichael · 2017-08-25T22:06:47Z

I know the test suite is hard to run (because we test against so many rails' and dbs) and I hope you found the contributing guide helpful.

It wasn't too hard after I figured out how to use appraisal. I didn't look at the guide until you mentioned it just now; it seems fairly straight-forward to me.

Do you get a RecordInvalid error? Are you referring to the following change, which happened in rails 5.0?

Yes, I get that error, and I'm referring to that change.

Did this question get answered?

I didn't answer it. According to the API documentation: "When set to true, the association will not have its presence validated."

On second thought, is it possible that many tests demonstrate this issue once your change to spec/dummy_app/config/application.rb is applied?

Yes, that is the case. It's why I changed the models I did, so that the tests would pass. If you remove any of those changes, there will be test failures. That's the only reason I didn't add a new test, but I'm not totally against making a new one anyway. Just not sure if it's necessary.

Well that's a problem. We don't want our test suite printing any deprecation warnings. First, why is the warning printed?

It's printed because we now have this configuration option enabled. I'm not really sure why the :after function is incompatible with it. One possible solution is to check for the configuration option prior to defining the :after functionality in the specs, and also before testing it. This way, PaperTrail could continue to support this functionality for older ActiveRecord versions, or for Rails 5.x users who do not have this configuration option enabled.

Thanks again for your work on this.

I am happy to do it, and it's in my interest, as this is acting as a blocker for enabling this default in our code. Thank you for your interest in this PR.

joelmichael · 2017-08-29T22:11:22Z

I've made some updates to this PR. To avoid the deprecation warning, it will now check the version and config setting prior to defining and testing the :after callback. I also added a description of the fix to the Changelog.

jaredbeck

Hi Joel, this is quite close, just a few nit-picks left, thanks.

jaredbeck · 2017-08-30T03:00:19Z

@@ -1,3 +1,4 @@
+


Please remove blank line

jaredbeck · 2017-08-30T03:01:19Z

 ### Fixed

- None
+- The "item" association will no longer be required when belongs_to_required_by_default is enabled.


Please follow the convention of this file and put in a link to the PR, so people can find out more about the change if they have questions.

jaredbeck · 2017-08-30T03:10:31Z

      end
-      if v >= Gem::Version.new("5.0.0.beta1")
+      if v >= Gem::Version.new("5.0.0.beta1") && v < Gem::Version.new("5.1")
+        config.active_record.belongs_to_required_by_default = true


Why not load_defaults("5.0")?

Apparently this command was added in Rails 5.1.

jaredbeck · 2017-08-30T03:14:56Z

+        config.active_record.time_zone_aware_types = [:datetime]
+      end
+      if v >= Gem::Version.new("5.1")
+        config.load_defaults 5.1


This is bordering on annoyingly pedantic, I'm sure, but we should not use a Float here. Float#to_s is not predictable. Please use string "5.1". Thanks.

5.1.to_s # => "5.1" 5.99999999999999999999999999999999999999999999999999999.to_s => "6.0"

I know, I know, it's not relevant, how likely are we to hit a floating point issue in a rails version number? But, I can't help it. :)

Since Rails 5.0, belongs_to_required_by_default has been the official ActiveRecord default. Add the configuration lines necessary to enable this default in both 5.0 and 5.1. Add "optional: true" where necessary to fix spec failures caused by this change. Add version-checking conditionals where necessary. Update the Changelog appropriately.

joelmichael · 2017-08-30T21:04:07Z

I believe I have corrected all of the reported issues. Please let me know if anything else is necessary. Thanks!

jaredbeck · 2017-08-31T01:05:39Z

I believe I have corrected all of the reported issues. Please let me know if anything else is necessary. Thanks!

Looks good, thanks Joel.

jaredbeck previously requested changes Aug 24, 2017

View reviewed changes

joelmichael changed the title ~~Make PaperTrail compatible with Rails 5.1 defaults~~ Support belongs_to_required_by_default Aug 25, 2017

jaredbeck requested changes Aug 25, 2017

View reviewed changes

jaredbeck requested changes Aug 30, 2017

View reviewed changes

jaredbeck reviewed Aug 30, 2017

View reviewed changes

jaredbeck approved these changes Aug 31, 2017

View reviewed changes

jaredbeck merged commit 935999d into paper-trail-gem:master Aug 31, 2017

Conversation

joelmichael commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelmichael commented Aug 24, 2017

Uh oh!

jaredbeck commented Aug 24, 2017

Uh oh!

joelmichael commented Aug 24, 2017

Uh oh!

jaredbeck Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

joelmichael commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelmichael commented Aug 25, 2017

Uh oh!

joelmichael commented Aug 25, 2017

Uh oh!

jaredbeck commented Aug 25, 2017

Uh oh!

jaredbeck commented Aug 25, 2017

Uh oh!

jaredbeck left a comment

Choose a reason for hiding this comment

Uh oh!

joelmichael commented Aug 25, 2017

Uh oh!

joelmichael commented Aug 29, 2017

Uh oh!

jaredbeck left a comment

Choose a reason for hiding this comment

Uh oh!

jaredbeck Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

jaredbeck Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

jaredbeck Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

joelmichael Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

jaredbeck Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

joelmichael commented Aug 30, 2017

Uh oh!

jaredbeck commented Aug 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joelmichael commented Aug 24, 2017 •

edited

Loading

joelmichael commented Aug 25, 2017 •

edited

Loading