Add a credit_card_created hook

[jordan-brough]

Bonobos has some specific code we want to invoke when a new credit card is created and added to an order. And the Solidus community has talked about trying to provide specific, focused configuration hooks for customization, so this seems like a good opportunity for that.

I'm envisioning Solidus invoking something like:

Spree::Config.credit_card_created.try!(:call, credit_card, payment) (where payment will be present if the card was created in the context of a payment).

Does that seem reasonable? Does anyone have preferences on any particular details of the above?

We want to avoid using after_create because:

1. We're trying to avoid after_create &etc in general
2. In this particular situation a CreditCard after_create doesn't have access to any associated payments, due to the way our polymorphic relationship is currently working. And time spent fixing that seems like time spent in the wrong direction
3. We need this hook to fire after everything else in the associated update_cart call finishes executing.

More context:
What we actually want to do is mark the payment.order's ship address as a 'verified address' for that credit card whenever a credit card is newly created and added to an order.

[braidn]

@jordan-brough I know I am new here (want to start adding to Solidus) but, what are your opinions of a publisher model, much like Wisper ? Pretty sure it would satisfy your avoidance of after_create. What yah think?

[jordan-brough]

@braidn thanks for the suggestion. I haven't looked at Wisper so I'm not sure if it would make sense for Solidus. But I think the core question in this PR is independent of something like Wisper -- the question being "does credit_card_created make sense as a hook"? (whether it's implemented via Wisper or otherwise).

I think considering something like Wisper could be worthwhile (I don't know) but probably as a separate issue.

[jordan-brough]

@gmacdougall @jhawthorn @cbrunsdon @magnusvk @athal7 any thoughts on this?

[cbrunsdon]

@jordan-brough I'm not a big fan of attempting these hooks with the .try!

I guess as a background I have a few questions:

• What are all the places credit cards are currently created?
• Who is (and when are they) going to call that callback? (which is related to the one above)
• Is this ran only after a credit card is persisted to the database? Should it run after a transaction is rolled back?

Excellent time to have a conversation though, I can see many people wanting to run code when credit cards are created.

[BenMorganIO]

I'm going to be 👎 on Whisper mainly because its going to be an added dependency as well as one more ruby library to learn. I also have no understanding of the subscribe/publish model.

[braidn]

IMHO, if we are asking about the need of the hook? In my world which is with both request/response checkouts and SPA like checkouts, then no. I wouldn't see enough use of this hook. Although, I totally could see this use in the community.

What about the old module#prepend around https://github.com/solidusio/solidus/blob/master/core/app/models/spree/order_updater.rb#L158 ?

A check could exist if the order payment is in the state where you want the credit_card_created to fire or just call super? Still not convinced though a hook like this could be widely used...

[jordan-brough]

@cbrunsdon

What are all the places credit cards are currently created?
Who is (and when are they) going to call that callback? (which is related to the one above)

Great questions. Here's what I've been able to find:

• Api::OrdersController#update (via OrderContents#update_cart)
• Api::OrdersController#create (via Core::Importer::Order.import)
• Api::PaymentsController#create (via order.payments.build)
• Api::CheckoutsController#update (via Order#update_from_params)
• Admin::OrdersController#update (via OrderContents#update_cart)
• Admin::PaymentsController#create (via order.payments.build)
• frontend/CheckoutController#update (via Order#update_from_params)
• frontend/OrdersController#update (via OrderContents#update_cart)

I think it would be great if we could get them all using something in common.

But, let's assume that we could figure that stuff out, and find a good caller for this callback. Do you all agree that some form of a credit_card_created(credit_card, payment) hook would be a good way to allow Bonobos to implement our ship-address-verification without having to monkey patch Solidus?

I don't want to spend too much energy figuring out the details unless people are on board with the basic idea of the hook itself.

Is this ran only after a credit card is persisted to the database? Should it run after a transaction is rolled back?

I think I'd say only after everything persisted and not run if it fails. That lowers the surface area of the hook (e.g. it can't stop an order from saving successfully, etc so it's usage is limited) which is a good thing imo.

Thoughts?

[jordan-brough]

@braidn

if we are asking about the need of the hook

Bonobos has a requirement for some sort of a hook to avoid monkey patching and our requirement seems reasonable to me. So I'm mostly asking whether this seems like a good way to provide such a hook, or if people have other ideas.

[jordan-brough]

@BenMorganIO

I'm going to be 👎 on Whisper

Yeah, let's leave that level of implementation detail out of this discussion for now at least.

[athal7]

For what it's worth I think a pub/sub approach similar to whisper internal to solidus could have some code organization benefits, but could encourage worsening of implicit behavior rather than encouraging explicit behavior.

@jordan-brough for me an ideal state would be that way have a gateway interface for adding a credit card (whether it be to an order or optionally just to a user) where, post-success, we kick off a delayed job through ActiveJob (to prevent failures from giving a failed the credit card addition response) that does 'credit card address verification', which can be a no-op for default payment methods, but happens to do something in the payment method you care about.

[braidn]

Yes, pub/sub does nothing to encourage explicit behavior, it's actually an explicit role back in my mind. Here is a Wisper like/extremely lite implementation that has been used for personal projects.

https://gist.github.com/braidn/baacaea0527b722d25cb

[jordan-brough]

Per the chat w/ the Solidus core team today I'm going to dive into the following and see how plausible it looks:

• Refactor the 8 endpoints above to use some common base
• Add some way for applications/extensions to hook into "credit card created" (including any payments it was added to) when the credit card is created through that common base.

I'll open a PR if I'm able to get something worth looking at.