Supply correct payment method instance to process_redirect_payment #6170
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
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.1 MB ℹ️ View Unchanged
|
mdmoore
changed the title
6 tasks
mdmoore
requested review from
a team and
timur27
and removed request for
a team
…ub.com/Automattic/woocommerce-payments into fix/5854-use-correct-payment-method-id
FangedParakeet
approved these changes
May 10, 2023
There was a problem hiding this comment.
Tested with a few different LPMs and all the metadata appears to be lining up now: transaction details look correct, with the correct order details and payment method details, and I was unable to produce the well-known and dreaded "application_fee_amount" error message.
Changes LGTM. It's worth noting that for now, the parent function UPE_Payment_Gateway::set_payment_method_title_for_order is still required for some payment flows with the split-UPE, but when the deferred intent UPE reaches fully operational parity, we will need to remove it.
But that'll be a story for another day. For today, let's 
.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Unfortunately your order cannot be processed as the originating bank/merchant has declined your transaction. Please attempt your purchase again. Quickteller Business Payment Gateway |
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 #5854, Fixes #5781
Changes proposed in this Pull Request
The crux of the the problem here is an early return in
process_redirect_payment. It's this early return that prevents the UPE payment intent from being removed from the session, causing the issue described in #5781.UPE_Split_Payment_GatewayextendsUPE_Payment_Gatewayand overridesget_selected_payment_methodto redefine the logic with a simple check against$this->stripe_id. The problem is that$this->stripe_idis alwayscarddue to hooks in the parent class resetting the stripe id for it's own uses. So redirect payment methods such asgiropayare always compared tocardand we returnfalse. That's fine as that's whatUPE_Payment_Gatewayneeds andUPE_Split_Payment_Gatewaydoesn't need thestripe_idat this point in the execution. We really only need an instance of the payment method which we can get from theupe_payment_method_mapviaWC_Payments::get_payment_method_by_id().Testing
The goal is to test that
process_redirect_paymentfully completes. This would mean thatremove_upe_intent_from_session()runs as well, which covers testing for both #5854 and #5781. Testing with Xdebug breakpoints has been problematic because the hooks that initiate this process fire multiple times, making it confusing as to whats actually occurring. I've found it best to just useLogger::log()to ensure the process completes.Logger::log( 'process_redirect_payment has completed' )wp_woocommerce_sessions table. This will ensure noupe_payment_intentis present yet.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