Add a credit_card_created hook about solidus HOT 12 CLOSED

@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?

from solidus.

@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.

from solidus.

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

from solidus.

@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.

from solidus.

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.

from solidus.

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...

from solidus.

@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?

from solidus.

@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.

from solidus.

@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.

from solidus.

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.

from solidus.

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

from solidus.

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.

from solidus.