Skip to content
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

Correctly handle IPP failed orders #3689

Merged
merged 6 commits into from
Jan 3, 2025
Merged

Conversation

wjrosa
Copy link
Contributor

@wjrosa wjrosa commented Jan 2, 2025

Fixes #3631

Changes proposed in this Pull Request:

When we perform In-Person Payments and the payment fails, Stripe sends a payment_intent.payment_failed webhook event (as expected), but the body is a bit different: we may not have the signature property. Because of it, our webhook handler class fails to retrieve the associated order, skipping the whole failure processing code.

This PR fixes the issue by adding two more ways of extracting the order ID from the webhook event body: from the order_id property inside the metadata property, and the charges array.

Testing instructions

  • Create public-facing test store (JN website recommended)
  • Connect your test Stripe account. It must have completed the onboarding process
  • Setup webhook calls
  • Create a test product
  • Follow this guide to set up an Android emulator with the Woo app and IPP-enabled. Point it to your store
  • Create the new order following the above and try to collect payments using the simulated card reader. Add any custom amounts that can trigger failures
  • Confirm that the order is marked as failed and a note is added:
Stripe SCA authentication failed. Reason: Your card was declined. You can call your bank for details. In testmode, using a physical test card with designated amount ending values produce specific decline responses. See https://stripe.com/docs/terminal/references/testing#physical-test-cards for details. Order status changed from Pending payment to Failed.

  • Covered with tests (or have a good reason not to test in description ☝️)
  • Added changelog entry in both changelog.txt and readme.txt (or does not apply)
  • Tested on mobile (or does not apply)

Post merge

cc @Mayisha @staskus

@wjrosa wjrosa self-assigned this Jan 2, 2025
@wjrosa wjrosa marked this pull request as ready for review January 2, 2025 15:03
@wjrosa wjrosa requested review from a team and annemirasol and removed request for a team January 2, 2025 15:03
@staskus
Copy link

staskus commented Jan 3, 2025

Thank you for addressing this, @wjrosa!

✅ I tested this branch on my store and Woo iOS and I can confirm that after a failed payment the order gets set to failed and the order note is added. 👍


Additionally, I tested repeated payment failures: tapping 'Try Collecting Again' which reuses the payment intent. We had problems with the same payment intent on WooPayments but it works well with Stripe Gateway. Mind, I had to modify the app to set the order back to pending before repeating a failed payment, otherwise the notes and other actions don't get made.

This is a known semi-challenge that we're currently discussing within the team pdfdoF-66D-p2 and with WooPayments folks. It's outside the scope of this PR and we can make improvements to this later.

Do you have any thoughts on this as well from Stripe Gateway's perspective, mainly:

  • Processing failed card_present payment orders the same way as pending orders.
  • Getting around $order->update_status( 'failed'..) limitation that only triggers WooCommerce failure actions when the status changes, and doesn't trigger when we call update_status( 'failed'..) on an already failed order?

I can create a GitHub issue and request if we settle on the solution on the WooPayments side.

@wjrosa
Copy link
Contributor Author

wjrosa commented Jan 3, 2025

Thank you for addressing this, @wjrosa!

✅ I tested this branch on my store and Woo iOS and I can confirm that after a failed payment the order gets set to failed and the order note is added. 👍

Additionally, I tested repeated payment failures: tapping 'Try Collecting Again' which reuses the payment intent. We had problems with the same payment intent on WooPayments but it works well with Stripe Gateway. Mind, I had to modify the app to set the order back to pending before repeating a failed payment, otherwise the notes and other actions don't get made.

This is a known semi-challenge that we're currently discussing within the team pdfdoF-66D-p2 and with WooPayments folks. It's outside the scope of this PR and we can make improvements to this later.

Do you have any thoughts on this as well from Stripe Gateway's perspective, mainly:

  • Processing failed card_present payment orders the same way as pending orders.
  • Getting around $order->update_status( 'failed'..) limitation that only triggers WooCommerce failure actions when the status changes, and doesn't trigger when we call update_status( 'failed'..) on an already failed order?

I can create a GitHub issue and request if we settle on the solution on the WooPayments side.

Thank you so much for reviewing and testing this, @staskus !

On the extension code, we have this block preventing different statuses from being handled by this method:

		if ( ! $order->has_status(
			apply_filters(
				'wc_stripe_allowed_payment_processing_statuses',
				[ 'pending', 'failed' ],
				$order
			)
		) ) {
			return;
		}

So, at least in theory, we should be able to process failed payments with no additional changes required.

  • Getting around $order->update_status( 'failed'..) limitation that only triggers WooCommerce failure actions when the status changes, and doesn't trigger when we call update_status( 'failed'..) on an already failed order?

Here, all I can think of is:
1 - Set the status to pending first, like was discussed on the issue you shared
2 - Force the trigger manually only for this specific case

I can create a GitHub issue and request if we settle on the solution on the WooPayments side.

That would be great 🙏

Copy link
Contributor

@annemirasol annemirasol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM! And thanks for adding unit tests ✨

I tested a few failures -- can see the order is marked as failed and the order notes contain the specific reason. 🚢

@wjrosa wjrosa merged commit cb16c6a into develop Jan 3, 2025
34 of 35 checks passed
@wjrosa wjrosa deleted the fix/correctly-handle-ipp-orders branch January 3, 2025 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Correctly handle failed card present payments
3 participants