Remove the redundant network requests in capture_charge
#6408
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.42 MB ℹ️ View Unchanged
|
This has allows us to further reduce a network call by removing the update intention API request before the capture step.
anu-rock
changed the title
capture_chargecapture_charge
dpaun1985
reviewed
Sep 18, 2023
|
Tracing steps back: IIRC (should be double-checked toward the PR which added it initially), getting intent, which we are dropping here, was introduced to pull original meta-data created by mobile apps via Stripe SDK directly. And extending the metas to WooPayments goodies. So both mobile/WooPayments can live easy lives and extend metas as needed. This change might work if Strpe merges meta with old ones. But the documentation doesn't clarify if this merge/override handling of metadata data happens there. So, the questions:
|
|
I'm glad you brought up this point. Based on my testing, metadata merge works the same way as before.
Merging of metadata happens in our code rather than Stripe: The
I haven't tested with the mobile app, but I believe my testing instructions cover this, especially this point: "Also verify on the intent details page that the metadata contains reader_ID along with order-related key-value pairs." P.S. I have added a prerequisite in testing instructions above for Server#3937. |
|
@anu-rock: oki-doki. But the order meta-data, including many WooPayments internal fields - I'm not sure if we shall expose those blindly in intent and if the mobile app creates intents with the same set of data. I'd cross-check an intent meta created by the mobile app vs complimentary order meta-data. |
|
Okay ignore my previous comment - we did use order meta previously, so no big change there. |
kalessil
approved these changes
Sep 21, 2023
|
@kalessil Awesome, thanks 🙌 |
|
@kalessil Just an FYI - I tested using the mobile app just to be double sure. All worked as expected (including metadata). I've updated the testing instructions accordingly 🙌 |
This means reverting the Payment_Type constant fix I did earlier. In fact, that's also how it's used in other places (eg. in Payment_Information).
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 #6358
Changes proposed in this Pull Request
By removing the redundant network requests, we aim to make capturing an intent faster.
In
capture_charge:Testing instructions
Prerequisite: Server#3937
Do this first with
trunkand then with this branch. Compare the response time in both case: it should be less with this branch.Note
On @anu-rock's MacBook Pro 16" with M2 Pro: Before: 9 secs, After: 6 secs.
Create a new order in WooCommerce frontend with "Cash on delivery" as the payment method.
Create a payment intent in Stripe Dashboard. Go to: Your test account → Payments → Create payment → Manual payment. Select the "capture funds later" option.
On the intent details page, scroll down to the "Metadata" section. Create a new key
order_idwith value from the order created in the previous step. Create another keyreader_IDwith value "wisepad".Make a POST request to
/wp-json/wc/v3/payments/orders/<order_id>/capture_terminal_payment.Verify that the payment succeeds.
Also verify on the intent details page that the metadata contains
reader_IDalong with order-related key-value pairs.BONUS: Testing with mobile app
Use this guide (PdfdoF-D-p2) to test with the Woo mobile app and verify that:
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