-
Notifications
You must be signed in to change notification settings - Fork 216
Add hook to allow for custom webhook post-processing #4466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
📈 PHP Unit Code Coverage Report
|
Co-authored-by: Diego Curbelo <diego@curbelo.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to implement a unified get_webook_order(...)
or get_order_from_webhook(...)
in a new PR, so meanwhile I'm approving this one.
@daledupreez should I go ahead and merge this? Since I see you are working on the other PR, and Diego approved this one (and I tested it myself when developing it). |
@wjrosa, if you don't mind waiting, can I review it one more time on Monday and make sure I am up to speed on the current state? That should also make it easier for me to update my follow-up PR once this change is merged. |
@daledupreez sure! No worries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wjrosa, I ended up noticing some subtle but important things that I addressed directly, so I think I'd like your eyes on this before we ship:
- I centralised the logic to trigger the action in a helper function, as we were only wrapping it in a
try/catch
in one location, and we were catchingException
instead ofThrowable
, and we had slightly different logging as a result. Everything is now consistent. - I added explicit initialisation of
$this->resolved_order
tonull
, and documented that the argument may benull
when the webhook is not related to an order. That is really important for developers to be aware of.
I have some other comments that I think are less critical, but are worth a look.
Co-authored-by: daledupreez <dale.du.preez@automattic.com>
Co-authored-by: daledupreez <dale.du.preez@automattic.com>
…ocommerce/woocommerce-gateway-stripe into add/hook-for-inbound-webhooks
Thanks for the review, @malithsen! @daledupreez should we merge this now? |
Towards STRIPE-564
Changes proposed in this Pull Request:
This PR proposes the addition of a new
wc_stripe_webhook_received
action that is triggered just after we process an inbound webhook from Stripe. This will make it easier for merchants and developers to include more details in WooCommerce based on the data from Stripe. The initial use case for this is to include more data for charges to allow for easier fraud detection while processing orders.Testing instructions
Changelog entry
Changelog Entry Comment
Comment
Post merge