Check intent attached to order before processing payment #5346
Conversation
… and UPE classes.
|
Size Change: +112 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
- Update the main gateway to try reusing the attached intent_id - Change from create_and_confirm_intention to create_or_update_intention_with_confirmation: Support a new parameter $intent_id, add one more test. - Update to re-use the valid attached intent_id for UPE_Payment_Gateway
- Update from handlePreviousOrderPaid to handleDuplicatePayments - Add redirection for wcpay_upe_previous_successful_intent
This reverts commit 2df0db4.
htdat
force-pushed
the
add/check-intent-attached-to-order-before-payment
branch
from
8706dda to
08b841d
Compare
|
FYI. I am still looking into this E2E test failure, which seems consistent:
UpdateThis is debug.log from the E2E run above.
This error happens when a customer is trying to change a subscription (not order, its post type is With a subscription, we're saving a setup intent (starting with I fixed it in this commit by checking intent ID prefix before processing further. f3f7b5e |
|
Thanks for the awesome feedback! @RadoslavGeorgiev - I've addressed your comments, and accepted all of them. This PR is ready for review again. Regarding:
Good question but I would have mentioned earlier. In UPE gateway, For the main gateway, in this attach method is also already close to the API call in update_order_status already. The only remaining attach method in process_payment_for_order of the main gateway can be moved earlier, right after create_and_confirm_intention. However, reviewing all the complex code between the current position and the potential earlier position, I think it's acceptable to keep it in the current position. Otherwise, there are some pitfalls:
|
This E2E test failure is similar to a recent scheduled E2E run for the |
This E2E test failure is similar to a recent scheduled E2E run for the Update: Discussion regarding this: p1672840161807159-slack-C04D1QU76PK |
There was a problem hiding this comment.
The code looks and works great 🥳 🚢
I've tested:
- UPE & non-UPE
- Blocks vs classic
- 3DS vs non-3DS. When testing with a 3DS card, I let it fail and then switched to 4242 to make sure the original payment method remains.
In many cases, we still need to call this attach method in the same position, then that's double database update calls.
I think it's still worth simply storing the intent ID as early as possible. $order->update_meta_data() does not trigger a database update call by itself, that only happens when $order->save() or $order->save_meta_data() is called. Those would be called before marking the order as failed anyway, so adding a single update method call after the API call would make a lot of sense without any practical overhead. I'll leave that up to you though, the PR is approved anyway :)
htdat
changed the title
Fixes #5187
Changes proposed in this Pull Request
Testing instructions
Prep
npm run watchto build JS files.Code Snippetsor directly to the bottom of filewoocommerce-payments.phpin the root directory of this project.woocommerce-payments.phphttps://gist.github.com/htdat/09eef1ed0cbeec50a5354ca8da38d623Start
order-receivedpage of this order with this parameterwcpay_previous_successful_intent=yes, e.g.: http://localhost:8082/checkout/order-received/zyz/?key=wc_order_1b9oTbcXtWH0l&wcpay_previous_successful_intent=yesScreenshot:
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge