Avoid WooPay duplicate charges when order cannot be completely processed #7968
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cesarcosta99
requested review from
a team and
lovo-h
and removed request for
a team
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.27 MB ℹ️ View Unchanged
|
lovo-h
approved these changes
Jan 4, 2024
There was a problem hiding this comment.
Code changes look good and work as intended. I left some comments but they were mainly suggestions.
These changes fix the "prevent the customer from being charged twice", as requested in
2199-gh-Automattic/woopay. But it may leave the customer believing that the order did not go through.
With this fix, we prevent the customer from being charged twice for the same order in WooPay. However, since the customer might believe that the order did not go through, there's a chance that the shopper might attempt to checkout through the merchant site.
I noticed we intend to create an issue (2199-gh-Automattic/woopay#issuecomment-1866910080) to fill the remaining gap.
Testing Instructions
- ✅ Test: reproduce the issue.
- ✅ Test: the fix prevents duplicate charges.
- ✅ Fixes Automattic/woopay#2199.
Comment on lines
+105
to
+111
| $intent_meta_order_id_raw = $intent->get_metadata()['order_id'] ?? ''; | ||
| $intent_meta_order_id = is_numeric( $intent_meta_order_id_raw ) ? intval( $intent_meta_order_id_raw ) : 0; | ||
| $intent_meta_order_number_raw = $intent->get_metadata()['order_number'] ?? ''; | ||
| $intent_meta_order_number = is_numeric( $intent_meta_order_number_raw ) ? intval( $intent_meta_order_number_raw ) : 0; | ||
| $paid_on_woopay = filter_var( $intent->get_metadata()['paid_on_woopay'] ?? false, FILTER_VALIDATE_BOOLEAN ); | ||
| $is_woopay_order = $order->get_id() === $intent_meta_order_number; | ||
| if ( ! ( $paid_on_woopay && $is_woopay_order ) && $intent_meta_order_id !== $order->get_id() ) { |
There was a problem hiding this comment.
Nit: No changes are necessary but what do you think about renaming the variables as follows:
Suggested change
| $intent_meta_order_id_raw = $intent->get_metadata()['order_id'] ?? ''; | |
| $intent_meta_order_id = is_numeric( $intent_meta_order_id_raw ) ? intval( $intent_meta_order_id_raw ) : 0; | |
| $intent_meta_order_number_raw = $intent->get_metadata()['order_number'] ?? ''; | |
| $intent_meta_order_number = is_numeric( $intent_meta_order_number_raw ) ? intval( $intent_meta_order_number_raw ) : 0; | |
| $paid_on_woopay = filter_var( $intent->get_metadata()['paid_on_woopay'] ?? false, FILTER_VALIDATE_BOOLEAN ); | |
| $is_woopay_order = $order->get_id() === $intent_meta_order_number; | |
| if ( ! ( $paid_on_woopay && $is_woopay_order ) && $intent_meta_order_id !== $order->get_id() ) { | |
| $intent_meta_order_id_raw = $intent->get_metadata()['order_id'] ?? ''; | |
| $intent_meta_merchant_order_id = is_numeric( $intent_meta_order_id_raw ) ? intval( $intent_meta_order_id_raw ) : 0; | |
| $intent_meta_order_number_raw = $intent->get_metadata()['order_number'] ?? ''; | |
| $intent_meta_woopay_order_id = is_numeric( $intent_meta_order_number_raw ) ? intval( $intent_meta_order_number_raw ) : 0; | |
| $paid_on_woopay = filter_var( $intent->get_metadata()['paid_on_woopay'] ?? false, FILTER_VALIDATE_BOOLEAN ); | |
| $is_woopay_order = $order->get_id() === $intent_meta_woopay_order_id; | |
| $is_merchant_order = $intent_meta_merchant_order_id === $order->get_id(); | |
| if ( ! ( $paid_on_woopay && $is_woopay_order ) && ! $is_merchant_order ) { |
There was a problem hiding this comment.
I don't think this renaming is beneficial because depending on the context we're analyzing the intent order ID may belong to WooPay order instead of the merchant's, and that may cause confusion. I only named is_woopay_order like that because it seems the order_number was created especially to distinguish WooPay orders and merchant orders.
| Type: fix | ||
| Comment: Fix WooPay duplicate charges. | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: Remove the additional empty line.
There was a problem hiding this comment.
This file is automatically generated and will be removed in the release process, so that's fine 🙂
|
I'm going ahead and adding this to the merge queue. The e2e test check failure is unrelated to this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes https://github.com/Automattic/woopay/issues/2199
Changes proposed in this Pull Request
Adds a check for WooPay orders in the duplicate payment prevention service. This check is needed because the WooPay order IDs doesn't match the order ID in the context, but the order number instead.
See https://github.com/Automattic/woopay/issues/2199 and https://github.com/Automattic/woopay/pull/2408 for more context.
Testing instructions
Pre-requisite
You will need to apply these changes directly to WooPay in
docker/wordpress/wp-content/plugins/woocommerce-payments/includes/class-duplicate-payment-prevention-service.php. Building the plugin is not an alternative because the WCPay version on WooPay is different and that might break things.Tests
Run the same tests described in https://github.com/Automattic/woopay/pull/2408, including the pre-requisite.
The only difference is in the last three steps of the
Test: the fix prevent duplicate charges, which should beFailed.Failedand with just a single charge.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