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

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

Merged
merged 1 commit into from
May 31, 2016

Conversation

md5
Copy link
Contributor

@md5 md5 commented May 17, 2016

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

cc/ @owenr

send "#{recording_order}_destroy",
:record_destroy,
:if => :save_version?
if recording_order.to_s == 'after' and
Copy link
Contributor Author

@md5 md5 May 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

This fix should probably be made on master as well.

Please do, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

md5 commented May 17, 2016

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
Copy link
Member

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

@md5
Copy link
Contributor Author

md5 commented May 17, 2016

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

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
Copy link
Contributor Author

md5 commented May 17, 2016

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

@owenr
Copy link
Contributor

owenr commented May 18, 2016

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

@md5
Copy link
Contributor Author

md5 commented May 18, 2016 via email

@jaredbeck
Copy link
Member

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
Copy link
Contributor Author

md5 commented May 18, 2016

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

@@ -29,7 +29,7 @@
it 'should default to after destroy' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/after/before/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jaredbeck
Copy link
Member

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

@md5
Copy link
Contributor Author

md5 commented May 18, 2016

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

@jaredbeck
Copy link
Member

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

@md5
Copy link
Contributor Author

md5 commented May 31, 2016

@jaredbeck Rebased

@jaredbeck
Copy link
Member

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 md5 changed the title Change paper_trail_on_destroy default to 'before' Warn if paper_trail_on_destroy(:after) is combined with ActiveRecord belongs_to_required_by_default May 31, 2016
@md5
Copy link
Contributor Author

md5 commented May 31, 2016

@jaredbeck Updated to just be a warning

@jaredbeck jaredbeck merged commit 1359b52 into paper-trail-gem:4.1-stable May 31, 2016
@jaredbeck
Copy link
Member

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

@md5 md5 deleted the backport-683 branch June 1, 2016 00:08
@md5
Copy link
Contributor Author

md5 commented Jun 1, 2016

No worries

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.

3 participants