Post-processing

[ssnickolay]

• [x] Introduce after_persist feature (currently, supports only ActiveRecord adapter)
• [x] Introduce Operation as a result of cloning
• [x] Clean up resolvers, declarations and adapters layers
• [x] Rename test models to use a more meaningful domain model
• [x] Update Rubocop to 0.61
• [x] Update bundler to 2.0
• [x] Drop supporting for Ruby 2.2

---

Dept:

• Pass calculated object over all post-processing blocks to control process (ex: validation case)
• Use Operation for Sequel adapter
• Update Docs
• Update Changelog

[palkan]

after_clone very closest to finalize IMO

hm, don't think so; finalize is a part of cloning process, after_clone clearly states that it's the next step.

Or something similar to to fix (because it is the main goal)

Not sure. That's the way we use it, but not the only one.

Maybe, after_save? It makes sense since we're calling it after saving a record.

Hm, your key point is execution after_clone is the default behavior, am I right? The same question - is wrapping with a transaction is default behavior for you?

Hm, maybe, it's becoming too opinionated.

Let's consider the possible scenarios (we currently taking into account Fountain case, which is not the only one):

• I want to build a clone of a record and do something with it myself – I don't expect Clowne to persist the clone record for me, neither doing anything else except building a fresh record:

clone_result = MyCloner.call record
# we can add a general API (the same as `to_model` for sequel)
# for this case
clone_record = clone_result.to_record

• I want to build a clone of a record and persist it immediately:

clone_result = MyCloner.call record
clone_record = clone_result.persist!
# which is roughly the same as (for ActiveRecord)
clone_record = clone_result.to_record.tap(&:save!)

• I want to build a clone of a record, perform some transformations with the clone without persisting it – that's the one scenario we missed: we can do some post processing even without persisting records (probably, we can do in our Fountain case too: resolve unsaved associations before calling save and make autosave do the job for us).

Now I think we need different types of callbacks. Our cloning lifecycle is the following:

clone record (the current `Cloner#call` behaviour) 
 |__ do something with the clone record (unsaved) (`after_clone` callback?)
 |__ persist record (`save`)
 |__ do something after saved (`after_persist` callback?)

In this case, I would suggest calling after_clone callbacks by default when calling Cloner#cloner (I don't even think we need an option to skip this, that should be a part of callback logic itself).

And after_persist callback is called when we call CloneResult#persist.

What about transactions? I see two options:

• we don't care about transactions and do not wrap anything (thus after_persist is called after save transaction)
• we do care about transactions but provide one more callback to be called outside of transaction (e.g. after_save is within transaction and after_persist is outside); the problem with this approach is that we couldn't be sure that after_persist is called outside of all transactions in general (we can do this for ActiveRecord only though).

[ssnickolay]

clone record (the current `Cloner#call` behaviour) 
 |__ do something with the clone record (unsaved) (`after_clone` callback?)
 |__ persist record (`save`)
 |__ do something after saved (`after_persist` callback?)

So, we need two different callbacks: after_clone and after_persist. Make sense for me (both names and count of callback).

In this case, I would suggest calling after_clone callbacks by default when calling Cloner#cloner (I don't even think we need an option to skip this, that should be a part of callback logic itself).

Good for me :+1:

What about transactions? I see two options: ....

That's why I want to move all transaction handling to the user side. Just because in another way it could lead to several edge cases (especially with non AR adapters) which we cannot see for this moment. At least for the first implementation. But I understand your thoughts and transactions out the box can be useful...Maybe in future, we will add default transaction via kwargs or via other Operation interface but it not so clear for now, IMO.

TL;DR our plan:

1. Rename post_processing callback to after_persist
2. Add Operation#persist!(after_persist: true) and Operation#to_record as we discussed above. Without transaction.
3. Add issue for after_clone callback (good for http://cultofmartians.com ;) )
4. Complete checkboxes in describing (README etc)

Wdyt?

[palkan]

LGTM.

Only one question:

Add Operation#persist!(after_persist: true)

Do we need this after_persist modifier? What is the case?

[ssnickolay]

Do we need this after_persist modifier? What is the case? ... And after_persist callback is called when we call CloneResult#persist.

I think we should provide a possibility to save the record without after_persist callbacks.

I see two options for this:

1. Use kwargs and persist!(after_safe: true) == persist! # after_safe: true - by default
2. Provide 3 different methods:
   • save! # just save without after_persist callbacks
   • run_after_persist! # run only callbacks
   • persist! # the same as save! && run_after_persist!

IMO the best decision to combinate both options:

class Operation
  def persist!(after_safe: true)
   to_record.save!
   run_after_persist! if after_safe
  end

  def run_after_persist!
     raise unless to_record.persisted?
     # calls calbacks
  end
end

So user can use what he want:

operation.persist! 
# the same as
operation.to_record.save! && operation.run_after_persist!

operation.persist!(after_persist: false)
# the same as
operation.to_record.save!

[palkan]

I think we should provide a possibility to save the record without after_persist callbacks.

It sounds like calling ActiveRecord#save! without calling callbacks; if there is a callback, it means something, one can not just skip it.

Btw, do we pass params to callbacks? We could used them to control whether we want to run the callback or not:

after_persist do |origin, record, skip_something: false|
  next if skip_something
  # ...
end

This way we move responsibility of calling callbacks to the cloner class itself.

[ssnickolay]

This way we move responsibility of calling callbacks to the cloner class itself.

Accepted, sounds reasonable 👍