Allow WooPay to request session data from merchant #8268
Merged
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: +506 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
|
Adding @ricardo as a reviewer given the round robin assignment from the WooPay PR. |
ricardo
requested changes
Mar 12, 2024
There was a problem hiding this comment.
Dropped the main comment in the WooPay repo, but the approach looks good to me. Nice work!
Only left pretty minor questions on this PR.
All tests have passed:
✅ With third party cookies disabled, there's a request to woopay/?cache_checkout_key that redirects to the merchant checkout if the user is logged out.
✅ With third party cookies disabled, there's a request to woopay/?cache_checkout_key that redirects to the WooPay checkout if the user is logged in.
✅ Existing flows work as expected.
✅ Tested both in blocks and shortcode cart.
✅ Tested with a user logged in to the merchant store.
✅ Tested in sandbox with Jetpack blog auth.
tests/e2e/specs/wcpay/merchant/merchant-orders-full-refund.spec.js
Outdated
Show resolved
Hide resolved
|
@ricardo I have now updated the implementation to use an encrypted and signed session data passed through the redirect URL to WooPay instead of storing a temporary session on WooPay. This was based on my discussion with Systems and their recommendation that the storage could be unreliable in this use case. I have updated https://github.com/Automattic/woopay/pull/2529 as well to handle this implementation and so they should be tested again together. Sorry for the back and forth on this PR, but this should be the last reworking of the implementation now that we've had discussions more broadly. |
ricardo
requested changes
Mar 21, 2024
client/checkout/woopay/direct-checkout/woopay-direct-checkout.js
Outdated
Show resolved
Hide resolved
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.
Changes proposed in this Pull Request
This PR is to address a performance optimization in the WooPay Direct Checkout feature. This PR is one half of the fix for https://github.com/Automattic/woopay/issues/2480. This PR should be tested in tandem with https://github.com/Automattic/woopay/pull/2529.
The overall feature is to send a minimum amount of session data to WooPay to start, then let WooPay request the rest of the session data only if it is needed (i.e. the shopper is already logged into WooPay). If the shopper is not logged into WooPay the full session data should not be requested and the shopper should be taken to the merchant checkout page. More details about the feature and this optimization can be found in https://github.com/Automattic/woopay/issues/2480.
To accomplish that goal, this PR adds the following:
Pass the minimum session data to the connect page and receive a redirect URL.The points of failure I focused on in this PR:
In all cases when there is a failure there, we should default to taking the shopper to the merchant checkout page.
Notes
One key thing to note is the need to unset the
preloaded_requestson the full session data response. This is unfortunate as it reduces some of the performance benefits. The reason this had to be unset is that the preloaded requests userest_preload_api_requestwhich just replicates a REST request but with no headers, which means noCart-Token. This leads the cart not being found and instead returning an empty cart object breaking things on WooPay. I experimented with other ways to request the cart data doing something like this:But ran into the problem that the initial requests had been fired and so WooCommerce wasn't trying to load the cart again since it already loaded it on the first requests in
get_init_session_requestand it generally seemed "too late" to update the current session to use the Cart Token. Given the time constraints I opted to leave the unset in for now and revisit in a separate issue, but if we find a good solution I'd love to add it to this PR!Testing instructions
All tests should start with WooPay enabled, the
_wcpay_feature_woopay_direct_checkoutoption set to 1 (enabled), and an item in the cart.Proceed to Checkoutbutton should take the shopper to the merchant checkout. If third party cookies are disabled, there will be a request towoopay/?checkout_redirect=1&blog_id=...that results in a 302 redirect to the merchant checkout. In this case nothing should ever get stored on WooPay for the session.Proceed to Checkoutbutton should take the shopper to the WooPay checkout. If third party cookies are disabled, there will be a request towoopay/?checkout_redirect=1&blog_id=...that results in a 302 redirect to WooPay with aplatform_checkout_key.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