search

Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
176 stars 69 forks source link

Fix Duplicate Charges: Delay webhook processing, query scheduled before failure #5188

Open htdat opened 1 year ago

htdat commented 1 year ago

Description

We should delay webhook processing on the client.

Acceptance criteria

  1. Instead of processing the webhook event payment_intent.succeeded immediately, store it in a similar fashion to the webhook reliability service, and schedule it to be processed in 1 or 2 minutes.

  2. Still, in combination with retries upon exceptions, we should be able to use the data from webhooks, in case they arrive in the meantime. This means that storage/scheduling should be done in a way that allows us to query scheduled events by either:

    • (1) order ID (without the necessity for a server call),
    • OR (2) from order meta (which requires a step to save the event ID to order meta before scheduling the event).

  3. Within the catch block of WC_Payment_Gateway_WCPay::process_payment , if there is a stored intent (carried by the webhook), we’ll use it to proceed with the other post-payment steps in the gateway. Because of the retries we would have done before that, the webhook should be already accessible this way, given that webhooks are delivered normally, and without issues.

Designs

Testing instructions

  1. Go to '…'
  2. Click on '…'
  3. Scroll down to '…'
  4. Observe expected behaviour

Dev notes

Important consideration: If we can, we should do this based on merchant-specific flags. This way, if we notice any issues, we’ll be able to toggle this delay remotely.

Additional context

paJDYF-6wC-p2#comment-15189 p1669286462288109/1669219023.651449-slack-C04CH2FJ8Q0

htdat commented 1 year ago

We should keep in mind regarding https://github.com/Automattic/woocommerce-payments/issues/5279 before/while working on this issue. If wcpay_webhook_process_event is refactored to process multiple webhooks at once, it's more complicated for us to delay the webhook processing.

p1671441724757289-slack-C04CH2FJ8Q0

htdat commented 1 year ago

It's likely that we will need to introduce a new way to store webhook data. The current approach using transients appears not reliable across different types of hostings, caching, etc. p1671520152621799-slack-C0208C3BXHP - https://github.com/Automattic/woocommerce-payments/issues/5374

Also, in the Gamma planning call, we removed this issue from Epic Prevent Duplicate Charge issue 48-gh-automattic/woocommerce-payments-meta due to the storage change above.

RadoslavGeorgiev commented 1 year ago

The estimate here is being changed from 1 to 3 SPs. Even though https://github.com/Automattic/woocommerce-payments/pull/5407 contains plenty of changes already, it still needs:

  1. To be brought up to date.
  2. Tests are missing and/or broken.
  3. Needs polishing.
naman03malhotra commented 1 year ago

Hey Rado, I think it would be good to mention why we switched to this approach for future context also linking PR that Zvonimir worked on.

zmaglica commented 1 year ago

Hey Rado, I think it would be good to mention why we switched to this approach for future context also linking PR that Zvonimir worked on.

I will link P2 comment or post about decisions we took, why we made them and provide all the details.