Enable control over the order of AR callbacks

[Ninigi]

Ok, this is embarrassing, but it was actually easier for me to delete the fork and redo my changes than trying to get a grip on how to rework all those commits in this PR ... I left the PR as a reference, but I guess all you need to review is this.

[Ninigi]

and messed it up again... sigh It's too late today and this is becoming quite frustrating... Guess I'll have to look into it tomorrow once again.

[jaredbeck]

sigh It's too late today and this is becoming quite frustrating

:( take a break. When you come back, you'll get it 👍

[Ninigi]

OK I think this should be reviewable now... I left everything as it was wherever possible, so the changes should be obvious now.
Was good that I looked into it once again though, because there were some non-critical misstakes I removed before reopening this PR.

On a side-note, there was a case when a single test would fail on Travis-CI. I looked through the code and just couldn't find what could have caused it (was an "undefined method for nil-class" error), when I couldn't find anything I just reran the build and it passed without me changing anything. Any ideas where that came from?

[jaredbeck]

Thank you for "minimizing" this PR by moving that other refactoring to a separate PR.

A few things I like:

• It makes callbacks more explicit and thus easier to understand.
• It gives the user control over the order of callbacks, making integrating with other libraries easier.
• It doesn't appear to break backwards compatibility.

A few things I don't like:

• The introduction of state, specifically the new @_set_up instance variable. Is there a way we can do this without state?
• The paper_trail_after_destroy and paper_trail_before_destroy methods. I don't want a Cambrian explosion of methods for every conceivable combination of options.
• The handling of options. I think I'd prefer if has_paper_trail remain mandatory, and to require that it come before the callbacks. This may help to eliminate the proposed @_set_up state.

[Ninigi]

Thanks for the feedback (I was really looking forward to this)

The introduction of state, specifically the new @_set_up instance variable. Is there a way we can do this without state?

To be honest, I did not really like this either, I did it to work around the problem of breaking the original has_paper_trail way of doing it

The paper_trail_after_destroy and paper_trail_before_destroy methods. I don't want a Cambrian explosion of methods for every conceivable combination of options.

No problem, I just added this to have a more explicit way of adding the callback instead of adding another "do this + option". If you don't like it, scratch it :)

The handling of options. I think I'd prefer if has_paper_trail remain mandatory, and to require that it come before the callbacks. This may help to eliminate the proposed @_set_up state.

Love and hate for this

Hate: I tried to do it without adding a state, but I found myself with either clattering the models with reduntant stuff, or missing some of the important setups.
Love: I chose the easiest way to do this, but I already planed on doing a revision and refine it, even if you had just accepted this PR, so it is nice to get the "go for a better implemantion, because we basically like it". Just asuming "has_paper_trail" to be mandatory makes everything a lot easier too, I guess I did not see the forest because of all the trees here ( = german proverb saying "I did not see the obvious, because I was involved with the detail").

Since I felt like a complete idiot with the last PR and the way it went (it seems everyone but me knows how to easily sqash commits the way he wants), for the changes you want, should I do a new PR, update this one with a commit or do something else?

*What your suggestions would solve: *

• No more caveates with callback(options)
• No confussion of how to use PaperTrail

Problems your suggestions imply:

• has_paper_trail(options) options could have impact on the callbacks I have not thought about yet - think all the ways has_paper_trail(options) and paper_trail_destroy etc can be combined. I am willing to provide tests for combinations I can think about, but you know how this usually ends ;)
• would you allow things like:

class Thingy < ActiveRecord::Base
  has_paper_trail :ignore => [:specific_thing]
  paper_trail_create :ignore => :another_thing
  paper_trail_update
end

or should callbacks not take any options at all? If they should, should they apply to the event they track?

class Thingy < ActiveRecord::Base
  has_paper_trail
  paper_trail_create :ignore => :thing
  paper_trail_update :ignore => :some_other_thing
end
# This creates the first version, should it ever ignore anything, if set in the options?
thingy = Thingy.create(:thing => "A Swallow", :some_other_thing > "original other thing")
# This creates the update version... No discussion about what we can or can not skip, because it should be up to the consumer.
thingy.update_attributes(:some_other_thing => "rambazamba")

# It leads to some confusion
thingy.versions.first.thing # should it say "A Swallow" or nil?
thingy.versions.last.some_other_thing # should it say "rambazamba" or "original other thing"

[jaredbeck]

.. should callbacks not take any options at all?

Good question. Yes, I think the easiest thing for now would be to keep all of the options where they are, passed to has_paper_trail. The only option passed to a callback would be the :before or :after passed to paper_trail_destroy.

.. should I do a new PR, update this one with a commit or do something else?

Doesn't matter, whatever is easiest for you.

[Ninigi]

OK, I am having a hard time trying to keep the traditional way of using has_paper_trail and the "call it explicitely" way... No matter how you look at it, there is no way around introducing some kind of "hack" to not break older versions.
Right now I am looking into a way to delete the set callbacks if callback-methods are used, but it is really hard. Give me some time for this, but I don't know if you would like my solution.

[jaredbeck]

Right now I am looking into a way to delete the set callbacks if callback-methods are used, but it is really hard.

Yeah, that sounds tough. What about something like:

has_paper_trail on: []
paper_trail_create
paper_trail_update
paper_trail_destroy

I'm assuming that passing on: [] will prevent has_paper_trail from installing any callbacks.

[jaredbeck]

Also, what do you think about naming the methods:

paper_trail_on_create
paper_trail_on_update
paper_trail_on_destroy

Adding the word "on" makes it sound more like a callback, and it's not much more typing.

[Ninigi]

Also, what do you think about naming the methods:

paper_trail_on_create
paper_trail_on_update
paper_trail_on_destroy

I like that. Actually my very first shot at this was with methods named "record_create" etc until I realised there methods with those names already in use internally ;)

has_paper_trail on: []
paper_trail_create
paper_trail_update
paper_trail_destroy

I'm assuming that passing on: [] will prevent has_paper_trail from installing any callbacks.

Yes it would, but it would make the usage a bit awkward, don't you think? I will put a little more time into trying to find a way to make the traditional has_paper_trail work with this. If I can't make it work, I'll do it with the empty :on option and maybe tackle this problem in another iteration.

Thanks for your feedback.

[Ninigi]

Ok, so the way I did it will set all callbacks on has_paper_trail and then clear the callback_chain of those callbacks when you first call paper_trail_on_*.
I looked into the ActiveSupport code for this, they use the CallbackChain#delete method internally to do exactyl what I was trying to do with it and been doing so as long as the CallbackChain has been there, so it should be backwards compatible.
The methods I wrote for that could be removed if you decided to use has_paper_trail together with one of the paper_trail_on_* methods (meaning using has_paper_trail alone would not be allowed). They will have to stay if using has_paper_trail alone should set up all callbacks.

The way it would work now is

class ModelTrackingCreateEvent < ActiveRecord::Base
  # Will track create only
  has_paper_trail :ignore => ['something'], ...
  paper_trail_on_create # no options possible
end

class ModelTrackingAllEvents
  has_paper_trail :ignore => ['something'], ...
end

[jaredbeck]

Yeah, that sounds tough. What about something like:

has_paper_trail on: []
paper_trail_create
paper_trail_update
paper_trail_destroy

it would make the usage a bit awkward, don't you think?
.. the way I did it will set all callbacks on has_paper_trail and then clear the callback_chain of those callbacks when you first call paper_trail_on_*.

I don't like the complexity that this introduces. I'm OK with a slightly harder-to-use API if it makes the library a lot easier to maintain.

[jaredbeck]

I'd rather keep the contents of Callbacks here in ClassMethods. I generally agree with Steve Klabnik's article "Mixins: A refactoring anti-pattern".

[Ninigi]

All right, I'll just admit defeat on the callback-chain thing ;) I have a way of keeping the library maintainable in mind, but I feel like I am trying to force this instead of accepting it is an unnecessary addition and just do it the easiest way, like you suggested.

I moved the methods back into lib/has_paper_trail.rb. I disagree that having all methods in a file adds to maintainability or readability if it inflates it to 500 lines of code and more, but I think you are right that those methods belong into the ClassMethods module.

[jaredbeck]

• Should read paper_trail_on_update.
• PR number in link should be 614, right?

[jaredbeck]

Looks good. Thanks Fabian! This will be a big help to people who need to control the order of callbacks.