Set Callbacks explicitely instead of using has_papertrail

[Ninigi]

Refering to this 3rd-party issue I wrote some methods to set callbacks:

  paper_trail_update
  paper_trail_create
  paper_trail_destroy # can take arguments :before or :after, defaults to :after if no argument given

paper_trail_destroy(:after/:before) can be aliased as paper_trail_after_destroy/paper_trail_before_destroy.

There is a caveat when passing options:

  paper_trail_update :ignore => [:things]
  paper_trail_create :ignore => [:other_things]

will NOT result in paper_trail ignoring changes to :things and :other_things, but only :things, since the "initialization" will only be triggered once.

Also when using has_paper_trail will override the explicitely set callbacks:

  paper_trail_update :ignore => [:things]
  has_paper_trail

will result in all events to be tracked and the ignore option will not be set at all...

Not sure how to fix those issues yet, because I did not want to get rid of has_paper_trail and/or break working functions...

[jaredbeck]

Wow, this is great! I haven't reviewed your implementation yet, but I think this is a great idea, giving users more control over their ActiveRecord callbacks. Specifically, being able to control the order in which callbacks happen will solve a whole class of issues, especially integration with other libraries that have their own callbacks.

As you mentioned, this has been discussed before:

• Let library consumers use custom before_filter ordering #301
• Papertrail does not delete properly when combined with Paranoia #593 (comment)
• Control the order of set_paper_trail_whodunnit callback #556

Precedent in other libraries

Other popular ruby libraries give users control over callbacks, so there is a precedent for this. For example load_and_authorize_resource in CanCan[Can], or protect_from_forgery in ActionController.

Other libraries go so far as to only provide methods, and require users to set up callbacks manually. For example verify_authorized in Pundit.

Next steps for this PR

This is a big change, so it's going to take Ben and I a while to review. In the meantime, a few housekeeping requests:

• Please add a short changelog entry
• Use git rebase to squash the "fixed typo" and "merged master" commits

[Ninigi]

Oh, I didn't think this would strike you as a big change, so I was a little sloppy about some things... For example I moved the InstanceMethod module to its own file, because I felt pretty bugged by that huge has_paper_trail.rb file. I should have moved that into another PR now that I think about it.
I left all those commits in the PR too, because I did not think it would be a big deal again, but I'll do it right away ^^

But really don't expect too much of this, I just rewrote the has_paper_trail method to use the paper_trail_update(etc) methods etc and added an option to the destroy method... The other methods have been moved, but not changed, but it would not be hard to implement the option for them too.
So I basically just split the has_paper_trail method and made the method an alias for using the callback methods one after the other.

Could you give me some kind of guideline how to change the changelog? I didn't change it because I was under the impression that's what you guys want to do.

[jaredbeck]

I moved the InstanceMethod module to its own file, because I felt pretty bugged by that huge has_paper_trail.rb file. I should have moved that into another PR now that I think about it.

Yeah, if you could split that refactoring into a second PR, that would really help use review this one more quickly. Thanks!

[jaredbeck]

Could you give me some kind of guideline how to change the changelog?

Just follow the conventions you see in there. Further reading: http://keepachangelog.com/

[Ninigi]

ok... I have no idea how to do that. Should I just revert those changes, make another branche and make another PR? Or can I use some git magic? I really don't know...

[jaredbeck]

I have no idea how to do that

No idea how to do what? Split the refactoring into a second PR?

[Ninigi]

ok, give me another day :) I have been sick for a few days, but I think I will be able to tackle this tomorrow.

[Ninigi]

ok, I've squashed the commits, I am not really sure if it's worth it to revert the 'moved instance methods to its own file' commit and move it to another PR now that I've thought about it tbh, but of course I'll still do it if you prefer.
I was thinking about adding a whole bunch of tests though, to make sure options work with callback-methods, but I'd like to hear from you before I start to put more work into something that will get denied in the end :)

[jaredbeck]

I am not really sure if it's worth it to revert the 'moved instance methods to its own file' commit and move it to another PR now that I've thought about it tbh, but of course I'll still do it if you prefer.

Please do, thanks. We have very limited time to review PRs/issues, and the smaller this PR is, the easier it will be for us to review. Thanks.

[jaredbeck]

Please remove any unnecessary refactoring, rebase on master, and squash everything into a single commit. After you've done that, we'll be able to review this PR. Thanks!

[jaredbeck]

Fabian, we can close this PR, right? It is superseded by #614