Find existing refunds by refund ID when processing webhook #2581

tc33 · 2023-03-22T14:48:25Z

Changes proposed in this Pull Request:

When handling a refund webhook, this attempts to lookup any existing refund by the refund ID first, before falling back to the current approach using the charge ID. As described in #2580, this will allow other extensions to decouple refunds from the original order the charge is against. The current approach leads to duplicate refunds on the WooCommerce side in these situations.

In general it seems to be more intuitive and more reliable to lookup the refund directly by the refund ID like this, rather than going via the order with the same charge ID.

Testing instructions

The basic behaviour to be tested:

Refunds initiated from WooCommerce result in webhook, which bails before creating a new refund, due to the presence of the existing refund
Refunds initiated from Stripe admin result in webhook, which creates a new refund within WooCommerce

In addition, to test the scenario I'm hoping to support:

When two orders are given the same charge ID (for testing this can just be done directly in the database), refunds initiated from one order in WooCommerce do not add a duplicate refund on the other order

The basic behaviour should not be changed so existing e2e tests for refunds should continue to be relevant. As far as I can tell there's nothing to specifically test for duplicate refunds though.

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

Added testing instructions to the Release Testing Instructions wiki page (or does not apply)

tc33 · 2023-03-22T14:55:50Z

@reykjalin PR as discussed on the Woo Marketplace Slack

asumaran · 2023-04-14T20:15:43Z

includes/class-wc-stripe-helper.php

+	public static function get_order_by_refund_id( $refund_id ) {
+		global $wpdb;
+
+		if ( class_exists( 'Automattic\WooCommerce\Utilities\OrderUtil' ) && OrderUtil::custom_orders_table_usage_is_enabled() ) {


asumaran

Looks good. I haven't tested with Split Orders. I just checked the changes don't break our extension.

Tested the following scenarios:

I only left a non-blocking comment. Please update the PR with the latest develop.

asumaran · 2023-06-02T19:35:21Z

includes/class-wc-stripe-helper.php

+			);
+			$order_id = current( $orders ) ? current( $orders )->get_id() : false;
+		} else {
+			$order_id = $wpdb->get_var( $wpdb->prepare( "SELECT DISTINCT ID FROM $wpdb->posts as posts LEFT JOIN $wpdb->postmeta as meta ON posts.ID = meta.post_id WHERE meta.meta_value = %s AND meta.meta_key = %s", $refund_id, '_stripe_refund_id' ) );


wc_get_orders() seems to work here even with HPOS enabled. I assume you reused it from get_order_by_charge_id(). It might be better to use wc_get_orders() only if it works in both cases.

If you want to test yourself, to enable HPOS you can go to WooCommerce -> Settings -> Advanced -> "Enable the high performance order storage feature"

With HPOS disabled, unfortunately the meta_query is just ignored by wc_get_orders().

I did check this and I thought you were right for a moment, because it seems to return the correct order. But, if you test with the limit removed you can see actually all orders are being returned in date order, and naturally in testing it was normally the most recent order being tested with.

This implementation is consistent with get_order_by_intent_id() and get_order_by_setup_intent_id(), which would presumably have the same problem without the custom queries. get_order_by_charge_id() is a little different because it doesn't need a meta query.

I see, it's always returning the last order (refund). Which is a coincidence because the last refund is not necessarily the one we are handling on the incoming Webhook.

asumaran · 2023-06-02T19:50:21Z

changelog.txt

@@ -1,6 +1,7 @@
 *** Changelog ***

 = x.x.x - 2023-xx-xx =
+* Fix - Lookup existing refunds by refund ID when processing webhooks.


Please update your branch with develop, this will likely go in 7.5.0

tc33 · 2023-06-15T17:38:46Z

Sorry @asumaran for the hold-up. When testing with HPOS I was seeing some odd behaviour. I've now realised that the _stripe_refund_id meta data is not getting saved at all when HPOS is enabled. That looks like a bug, so I'll raise it as a separate issue, I don't think it needs to hold this up any further.

As an aside, I can confirm this PR resolves the issue in Split Orders.

asumaran

LGTM. Thanks for your contribution @tc33!

I've now realised that the _stripe_refund_id meta data is not getting saved at all when HPOS is enabled. That looks like a bug, so I'll raise it as a separate issue

Great! Please add more details to replicate the issue.

…ce#2581)

danielvonmitschke · 2023-10-04T11:48:04Z

As I'm looking through this code (to add these changes to an old version of this plugin) I am wondering if the whole _stripe_refund_id handling is correct.
From what I can see in the code we are only saving one _stripe_refund_id per order.
If I make another refund the previous refund id is overwritten and therefore the get_order_by_refund_id() method cannot work correctly.

Find existing refunds by refund ID when processing webhook

78500ac

asumaran reviewed Apr 14, 2023

View reviewed changes

asumaran reviewed Jun 2, 2023

View reviewed changes

tc33 force-pushed the fix/2580-refund-webhook-id branch from ca3866c to 78500ac Compare June 15, 2023 17:05

Merge 'develop' into fix/2580-refund-webhook-id

d96dfea

Merge branch 'develop' into fix/2580-refund-webhook-id

e0e589f

asumaran approved these changes Jun 16, 2023

View reviewed changes

asumaran merged commit b07d999 into woocommerce:develop Jun 16, 2023

asumaran self-assigned this Jun 16, 2023

stoyan0v pushed a commit to SiteGround/woocommerce-gateway-stripe that referenced this pull request Oct 3, 2023

Find existing refunds by refund ID when processing webhook (woocommer…

8b949c6

…ce#2581)

danielvonmitschke mentioned this pull request Oct 5, 2023

Double refunds still happening (triggered through web hook) #2724

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Find existing refunds by refund ID when processing webhook #2581

Find existing refunds by refund ID when processing webhook #2581

tc33 commented Mar 22, 2023

tc33 commented Mar 22, 2023

asumaran Apr 14, 2023

asumaran left a comment •

edited

Loading

asumaran Jun 2, 2023

asumaran Jun 2, 2023

tc33 Jun 15, 2023 •

edited

Loading

asumaran Jun 16, 2023

asumaran Jun 2, 2023

tc33 commented Jun 15, 2023 •

edited

Loading

asumaran left a comment

danielvonmitschke commented Oct 4, 2023

Find existing refunds by refund ID when processing webhook #2581

Find existing refunds by refund ID when processing webhook #2581

Conversation

tc33 commented Mar 22, 2023

Changes proposed in this Pull Request:

Testing instructions

tc33 commented Mar 22, 2023

asumaran Apr 14, 2023

Choose a reason for hiding this comment

asumaran left a comment • edited Loading

Choose a reason for hiding this comment

asumaran Jun 2, 2023

Choose a reason for hiding this comment

asumaran Jun 2, 2023

Choose a reason for hiding this comment

tc33 Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

asumaran Jun 16, 2023

Choose a reason for hiding this comment

asumaran Jun 2, 2023

Choose a reason for hiding this comment

tc33 commented Jun 15, 2023 • edited Loading

asumaran left a comment

Choose a reason for hiding this comment

danielvonmitschke commented Oct 4, 2023

asumaran left a comment •

edited

Loading

tc33 Jun 15, 2023 •

edited

Loading

tc33 commented Jun 15, 2023 •

edited

Loading