Warn if paper_trail_on_destroy(:after) is combined with ActiveRecord belongs_to_required_by_default

[md5]

Based on 18d35ef (see #683, #682)

cc/ @owenr

[md5]

Note, I changed this from recording_order == 'after' to recording_order.to_s == 'after' to ensure that the warning is still issued when the :after symbol is used. This fix should probably be made on master as well.

[jaredbeck]

Good idea!

This fix should probably be made on master as well.

Please do, thanks!

[md5]

I opened #813

I added a spec to test that the warning is actually being issued and it's not passing for some reason. We can discuss on the PR.

[md5]

Incidentally, I'm not sure I agree with the fix in #683. I probably would have set required: false on the belongs_to and added a conditional validates_presence_of :item, unless: -> { event == 'destroy' } (or some such).

[jaredbeck]

Hmm, it looks like CI didn't run, maybe because this PR isn't against the master branch?

[md5]

@jaredbeck I'm not sure. The only setting I'm familiar with in Travis that affects whether a branch builds is the "Build only if .travis.yml is present" setting.

[jaredbeck]

I can't find anything in their docs, so I reached out to Travis support. If you want to get CC'd on it, send me your email. Mine's [email protected].

[md5]

@jaredbeck If you don't mind I can wait for a report back.

[owenr]

Re: changing to optional relationship, see discussion in #682.
Maybe CI didn't run because this was cherry-picked?

[md5]

Cherry picked commits get a new SHA, so Travis should build it based on the PR. As for changing the relationship to required, I don't see much discussion in #682, other than @jaredbeck saying he'd like it to be required (which is not the case in Rails 4, as I'm sure you both know)

[jaredbeck]

I got a response back from Travis CI:

Thanks for reaching out and sorry for this issue.

I had a look and it seems GitHub returned a 404 when trying to get the payload for the commit on this pull request. This is an edge case that is not handled well on our side at the moment. Sometimes it's caused by a recent change to permissions of one members/collaborators of your repository or organization. Hence, do you remember changing the permissions recently?

Can you try pushing a new commit (it could be an empty one) on the PR to see if things are back on their feet?

Thank you for your understanding and please keep us posted on the status of this issue. We will follow-up upon your reply.

Please try pushing a new commit, Mike.

[md5]

@jaredbeck I did a no-op --amend and pushed a new commit. Looks like Travis is running now.

[jaredbeck]

s/after/before/

[md5]

Not sure how I missed that while pulling in the cherry-picked commit... Fixed in 65c3ce4

[md5]

And I pushed the version comparison fix I made in #813 as a new amended commit: cd4c667

[jaredbeck]

Do you understand this Bundler could not find compatible versions for gem "activesupport" error? I don't. o_O

[md5]

@jaredbeck I'm not sure what the deal is with that Bundler failure though.

[jaredbeck]

Mike, please rebase. The dependency resolution issue should be fixed by 29786c3

[md5]

@jaredbeck Rebased

[jaredbeck]

Mike, I just realized this is a breaking change. We're trying to follow semver, so we can't have a breaking change in 4.2. Would you mind if we just add the warning message, and do not change the default order? Sorry for realizing this so late, I guess I was focused on getting the travis build to pass.

[md5]

@jaredbeck Updated to just be a warning

[jaredbeck]

Thanks Mike! Sorry for all the grief with the Travis build.

[md5]

No worries