-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Respect BNPL limits_per_currency (country, min, max) #9626
Conversation
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: +95 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
public static function get_stripe_minimum_amount( $currency ) { | ||
switch ( $currency ) { | ||
case 'AED': | ||
case 'MYR': | ||
case 'PLN': | ||
case 'RON': | ||
$minimum_amount = 200; | ||
break; | ||
case 'BGN': | ||
$minimum_amount = 100; | ||
break; | ||
case 'CZK': | ||
$minimum_amount = 1500; | ||
break; | ||
case 'DKK': | ||
$minimum_amount = 250; | ||
break; | ||
case 'GBP': | ||
$minimum_amount = 30; | ||
break; | ||
case 'HKD': | ||
$minimum_amount = 400; | ||
break; | ||
case 'HUF': | ||
$minimum_amount = 17500; | ||
break; | ||
case 'JPY': | ||
$minimum_amount = 5000; | ||
break; | ||
case 'MXN': | ||
case 'THB': | ||
$minimum_amount = 1000; | ||
break; | ||
case 'NOK': | ||
case 'SEK': | ||
$minimum_amount = 300; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned in the PR description, this was added to handle BNPL minimum amounts but is actually comprised of Stripe minimum amounts. It's was only used for that purpose and is safe to remove.
public static function get_cached_minimum_amount( $currency ) { | ||
$cached = get_transient( 'wcpay_minimum_amount_' . strtolower( $currency ) ); | ||
|
||
if ( (int) $cached ) { | ||
return (int) $cached; | ||
} elseif ( $fallback_to_local_list ) { | ||
return self::get_stripe_minimum_amount( $currency ); | ||
} | ||
|
||
return null; | ||
return (int) $cached ? (int) $cached : null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous comment, reverting this back to it's state prior to #9355.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in pretty good shape save for my note about reusing a nonce across ajax actions. I also left a few comments to try and get a things tweaked to match up with limits I see in Stripe's documentation.
includes/class-wc-payments.php
Outdated
@@ -1713,7 +1714,7 @@ public static function ajax_get_woopay_signature() { | |||
* Get cart total. | |||
*/ | |||
public static function ajax_get_cart_total() { | |||
check_ajax_referer( 'wcpay-get-cart-total', 'security' ); | |||
check_ajax_referer( 'wcpay-bnpl-nonce', 'security' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd rather see this method use a separate nonce than the BNPL method below. I don't think there's much of an "attack vector" here, but it's best practice for each ajax action to use their own nonce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough! Done in 613bb62.
'isCart' => is_cart(), | ||
'isCartBlock' => $is_cart_block, | ||
'cartTotal' => WC_Payments_Utils::prepare_amount( $cart_total, $currency_code ), | ||
'nonce' => wp_create_nonce( 'wcpay-bnpl-nonce' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted this in class-wc-payments-utils.php as well, but I think we should separate the nonces here.
includes/class-wc-payments-utils.php
Outdated
'max' => 1150000, | ||
], // Represents GBP 1 - 11,500 GBP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be 5,000 GBP.
'max' => 1150000, | |
], // Represents GBP 1 - 11,500 GBP. | |
'max' => 500000, | |
], // Represents GBP 1 - 5,000 GBP. |
includes/class-wc-payments-utils.php
Outdated
'max' => 400000, | ||
], // Represents EUR 1 - 4,000 EUR. | ||
Country_Code::FRANCE => [ | ||
'min' => 3500, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like France should also be 100
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for going through all of these again. Very much appreciated.
includes/class-wc-payments-utils.php
Outdated
Country_Code::FRANCE => [ | ||
'min' => 3500, | ||
'max' => 400000, | ||
], // Represents EUR 35 - 4000 EUR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick to keep the formatting consistent (and adjust for the lower limit).
], // Represents EUR 35 - 4000 EUR. | |
], // Represents EUR 1 - 4,000 EUR. |
Thanks @brettshumaker. All of the changes have been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for handling those last few bits! This was working and testing as described for me.
} | ||
} | ||
} | ||
|
||
$enabled_upe_payment_methods = $this->gateway->get_payment_method_ids_enabled_at_checkout(); | ||
$enabled_upe_payment_methods = $this->gateway->get_upe_enabled_payment_method_ids(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just ensures we're getting all of the enabled BNPL payment methods. get_payment_method_ids_enabled_at_checkout()
is a filtered list for checkout. This means, if you visit a PDP while you have products in the cart, you might not see the correct BNPL methods in the PMME. For example, with a $5 product in the cart, a $60 PDP won't display Affirm ($50 minimum) because it's been filtered out based on cart totals. Demo here, second example from the bottom. This change solves that.
Hey @brettshumaker 👋 - I added one last commit (3eae337) after your previous approval, if you don't mind taking a quick look. |
Fixes #9456
Changes proposed in this Pull Request
As described in #9456, when there is no BNPL offer on PDPs, the PMME is displaying a skeleton loader which resolves to an empty space. This PR solves that by predicting whether the PMME will render any payment methods. If not, the PMME is hidden, with no skeleton displayed and no dead space below the product price. An earlier PR attempted to solve this for minimum price limits only, but rather than the BNPL minimums, the overall Stripe charge limits were used, which differ from BNPL minimums. To solve this, I've aggregated all
$limits_per_currency
arrays into a function that is used to determine whether any offers will be available. The result is surfaced on the front end via thewcpayStripeSiteMessaging.isBnplAvailable
.This also ensures that the PMME visibility is toggled when product quantity or variation changes result in price changes that affect payment method visibility. This done listening for changes to quantity or variation, then sending a request to get the BNPL availability.
Testing instructions
No BNPL offers
Quantity Updates
Further tests can be perform using the conditions in the unit test here.
Before
After
npm run changelog
to add a changelog file, choosepatch
to 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