Fix: BNPL payment methods should work when available in the Pay For Order page #9670
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: +159 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
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. |
danielmx-dev
changed the title
danielmx-dev
requested review from
a team and
frosso
and removed request for
a team
danielmx-dev
commented
Nov 5, 2024
Comment on lines
+739
to
+740
| // Total is 100 USD, which is above both payment methods (Affirm and AfterPay) minimums. | ||
| WC()->cart->add_to_cart( WC_Helper_Product::create_simple_product()->get_id(), 10 ); |
There was a problem hiding this comment.
The amount could be defined a bit more "dynamically" (in case the limits change in the future) but I didn't want to overcomplicate things, lmk what do you think.
frosso
approved these changes
Nov 6, 2024
| if ( ! did_action( '__wcpay_customer_data_localized' ) ) { | ||
| wp_localize_script( 'wcpay-upe-checkout', 'wcpayCustomerData', $prepared_customer_data ); | ||
| } | ||
| do_action( '__wcpay_customer_data_localized' ); |
There was a problem hiding this comment.
I like the way you're leveraging do_action/did_action to check whether a function had already been executed.
| $currency = get_woocommerce_currency(); | ||
| $order = null; | ||
| if ( is_wc_endpoint_url( 'order-pay' ) ) { | ||
| $order = wc_get_order( absint( get_query_var( 'order-pay' ) ) ); |
There was a problem hiding this comment.
(minor) WooCommerce Core does a similar check on the currency amount (although we're not leveraging it because of the multi-currency setup): https://github.com/woocommerce/woocommerce/blob/7eabbe4ecd7dd1cff6a36055c8a07c3bdf64abb1/plugins/woocommerce/includes/abstracts/abstract-wc-payment-gateway.php#L290-L308
However, I noticed that they first do:
$order_id = absint( get_query_var( 'order-pay' ) )And then:
if ( 0 < $order_id ) {
// proceeds with getting the order
}However, we're not doing the check on the $order_id, here.
I'm not sure if you considered it - but wanted to raise it anyways. I'm guessing that if we call wc_get_order() with '' (empty string - the default value returned by get_query_var(), we might get the order with id 0. Is that something we should be concerned about?
I don't think we'd need to be concerned about it, but wanted to double-check with you.
There was a problem hiding this comment.
I followed the same approach we already use to retrieve the order in the Pay for Order flow:
woocommerce-payments/includes/class-wc-payments-checkout.php
Lines 264 to 268 in c061a08
I also checked the source code for wc_get_order and it should return false in case we sent either 0 or ''.
https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/includes/class-wc-order-factory.php#L28-L32
https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/includes/class-wc-order-factory.php#L210-L221
| $currency = $order->get_currency(); | ||
| } else { | ||
| $currency = get_woocommerce_currency(); | ||
| } | ||
| // If the currency limits are not defined, we allow the PM for now (gateway has similar validation for limits). | ||
| // Additionally, we don't engage with limits verification in no-checkout context (cart is not available or empty). |
There was a problem hiding this comment.
Should we be concerned with (or change) this comment, since maybe the pay-for-order could be considered a "no-checkout context"? 👀
I wonder if, instead, we should perform a check like this:
$currency = get_woocommerce_currency();
if ( $order ) {
$currency = $order->get_currency();
}
// If the currency limits are not defined, we allow the PM for now (gateway has similar validation for limits).
$total = null;
if ( $order ) {
$total = $order->get_total();
} elseif ( isset( WC()->cart ) ) {
$total = WC()->cart->get_total( '' );
}
if ( isset( $this->limits_per_currency[ $currency ] ) && ! empty( $total ) ) {
$amount = WC_Payments_Utils::prepare_amount( $total, $currency );| @@ -119,6 +119,18 @@ export const getUpeSettings = () => { | |||
| }; | |||
| } | |||
|
|
|||
| if ( window.wcpayCustomerData ) { | |||
There was a problem hiding this comment.
It took me a bit to find the other area where we check for window.wcpayCustomerData and pass such data to Stripe, and to identify if there are any opportunities for improvement - but I think this is the only way, good find!
danielmx-dev
commented
Nov 6, 2024
|
@frosso I pushed some changes to improve the compatibility with BNPL payment methods and their requirements. I also updated the test instructions to include regression testing for other payment methods/flows. |
frosso
approved these changes
Nov 6, 2024
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 #8254
Changes proposed in this Pull Request
wcpayCustomerDataglobal variable is properly rendered. A couple issues related to this were fixed:payment_fields()code was running before thewcpay-upe-checkoutscript was registered, so whenwp_localize_scriptran, nothing was added to the page. This problem was solved for thewcpay_upe_configvariable by adding a callback towp_footerbut the same fix wasn't applied towcpayCustomerData. This PR should make both work consistently.is_enabled_at_checkoutwas updated so it checks the order data instead of the cart data to determine availabilityTesting instructions
Before (using develop)
BNPL payment methods showed up but nothing was rendered when selected. (Verify for all BNPL methods)
Clicking Pay for Order results in an error.
After (Using this branch)
The contents of the BNPL methods is properly rendered, customer information is prefilled based on the data filled for the order.
Submitting the order should result in a successful payment. Test this for all BNPL methods (Klarna, Afterpay/Clearpay, and Affirm).
Testing payment method availability
In WP Admin, update the Billing information of the pending order to another country.
If you reload the Pay for Order page, the BNPL methods should no longer show up.
In WP Admin, restore the billing country to US but change the line items so the total is less than 50 USD.
Reload the pay for order page, Affirm should not be displayed:
Regression Testing
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