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

Destroy callback default changed to :before #683

Merged
merged 1 commit into from
Jan 15, 2016

Conversation

owenr
Copy link
Contributor

@owenr owenr commented Dec 31, 2015

To allow after_destroy versioning with Rails 5; fixes #682

@jaredbeck
Copy link
Member

There may be a better alternative, see discussion in #682.

@jaredbeck
Copy link
Member

Also, if we were to proceed with this PR, it would need a test, please.

@jaredbeck
Copy link
Member

Also, if we were to proceed with this PR, it would need a test, please.

Actually, we don't have tests set up for rails 5 yet, do we? I'll see if I can do that.

@owenr
Copy link
Contributor Author

owenr commented Jan 5, 2016

I've tried setting up a Rails 5 env but it looks like it might be a bit premature (dependencies force us to also support Sinatra 2.0.0 which is alpha, the rails-controller-testing gem does not do what it says on the tin, and some deprecation warnings are unavoidable without dropping support for older versions of Ruby/Rails). You can see my progress at GitHub and Travis.

@jaredbeck
Copy link
Member

I've tried setting up a Rails 5 env but it looks like it might be a bit premature ..

I'm actually working on this right now :) I'm trying out appraisal. If you want to email me (jared at jaredbeck dot com) we can hop on skype or something.

@jaredbeck jaredbeck changed the title Explicitly make PaperTrail::Version item relationship optional Destroy callback default changed to :before Jan 14, 2016
@jaredbeck
Copy link
Member

This is looking good. Can you squash your commits, please?

@owenr
Copy link
Contributor Author

owenr commented Jan 15, 2016

All done - please re-review.

@@ -5,7 +5,9 @@ module VersionConcern
extend ::ActiveSupport::Concern

included do
belongs_to :item, :polymorphic => true
opts = {:polymorphic => true}
opts[:required] = false if ActiveRecord::Base.respond_to? :belongs_to_required_by_default
Copy link
Member

Choose a reason for hiding this comment

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

Do we need both this change (required: false) and the change to the default recording_order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't - I was trying to revert that. Let me try again.

Also warn if paper_trail_on_destroy(:after) is combined with
ActiveRecord belongs_to_required_by_default
jaredbeck added a commit that referenced this pull request Jan 15, 2016
Destroy callback default changed to :before
@jaredbeck jaredbeck merged commit 80bbb26 into paper-trail-gem:master Jan 15, 2016
@jaredbeck
Copy link
Member

Merged, thanks Owen!

@batter
Copy link
Collaborator

batter commented Jan 15, 2016

👍

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