UPE: intent caching tweaks #5653
Closed
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
|
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
kalessil
commented
Mar 1, 2023
kalessil
commented
Mar 1, 2023
dmvrtx
reviewed
Mar 1, 2023
htdat
reviewed
Mar 2, 2023
…attic/woocommerce-payments into fix/5310-usage-of-processed-intent
kalessil
commented
Mar 2, 2023
kalessil
commented
Mar 2, 2023
kalessil
commented
Mar 2, 2023
|
A new batch of changes, and it makes sense to describe the current version. Three major decisions:
Tradeoffs:
This means it should be possible to open two pages and complete the order on one page; voila, the second open one uses a processed intent. Changes preventing duplicate orders/charges will likely cover this, but I suggest keeping the introduced verifications for now - I don't have better ideas on how to sync cached intent ID. |
|
I need inputs from more folks:
|
anu-rock
reviewed
Mar 8, 2023
| if ( $intent_data ) { | ||
| // Extract the intent and ensure it's not bound to an order yet nor being processed. | ||
| list( $cart_hash, $intent_id, $client_secret ) = explode( '-', $intent_data, 3 ); | ||
| $order = ( new WC_Payments_DB() )->order_from_intent_id( $intent_id ); |
There was a problem hiding this comment.
Can we instead get the instance of WC_Payments_DB from DI, like in webhook service?
| // The invoked method already handles order status changes on success and partially on failure. | ||
| // But, not all exceptions are triggering order status changes, so we have to clear intent cache. | ||
| // The intent caching is UPE-specific, so we have to keep it in the UPE-gateways only. | ||
| self::remove_upe_payment_intent_from_session(); |
There was a problem hiding this comment.
Suggested change
| self::remove_upe_payment_intent_from_session(); | |
| self::remove_upe_payment_intent_from_session(); | |
| throw $e; |
Do we not need to rethrow the exception here as well? Otherwise if the parent gateway class throws an exception, for whatever reason, won't we just catch it and then continue on to return sucess instead of failure?
|
I have removed Helix from the review-queue due to Subscriptions being passed off to Quark. I have asked them to review. |
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 #5310
Changes proposed in this Pull Request
Testing instructions
To be done
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