-
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
Ensure WCPay JS loaded on Checkout page for zero order totals #5423
Conversation
that when order is updated via ajax from Checkout pages, payment form loads.
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
Working only for non-UPE payment method, need to fix for UPE and test for block checkout and other checkout combinations. |
the cart is updated via AJAX on checkout page.
and not in blocks checkout page as the latter already loads required scripts even when cart is free.
Fixed for UPE and blocks too. Ready for review @Automattic/gamma |
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.
Tested this locally. The fix works for Classic checkout and Blocks checkout, but it isn't working for UPE.
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.
Changes LGTM. Works on UPE, Classic checkout, and Blocks checkout.
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.
While reviewing this PR #5652, I did have a look at this issue too. All of these inline comments are more ideas, they're not meant to update/fix the code right away.
@@ -629,6 +629,12 @@ public function register_scripts() { | |||
); | |||
|
|||
wp_set_script_translations( 'WCPAY_CHECKOUT', 'woocommerce-payments' ); | |||
|
|||
if ( ! WC()->cart->needs_payment() && is_checkout() && ! has_block( 'woocommerce/checkout' ) ) { | |||
WC_Payments::get_gateway()->tokenization_script(); |
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.
We may just directly $this->tokenization_script()
as it's basically the same one but without calling an outside function to get another inside function.
@@ -629,6 +629,12 @@ public function register_scripts() { | |||
); | |||
|
|||
wp_set_script_translations( 'WCPAY_CHECKOUT', 'woocommerce-payments' ); | |||
|
|||
if ( ! WC()->cart->needs_payment() && is_checkout() && ! has_block( 'woocommerce/checkout' ) ) { |
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.
Nitpick: moving is_checkout()
as the first check as it can help terminate this code for most cases, which results in a little bit faster processing in general.
if ( ! WC()->cart->needs_payment() && is_checkout() && ! has_block( 'woocommerce/checkout' ) ) { | ||
WC_Payments::get_gateway()->tokenization_script(); | ||
WC_Payments::get_wc_payments_checkout()->enqueue_payment_scripts(); | ||
} |
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.
IMO, this whole block can be moved to a method in WC_Payment_Gateway_WCPay
and then you can use that method for all of these places. Without that, we do not need to write the same code in three places. We can use this because of the inheritance:
- UPE_Payment_Gateway extends WC_Payment_Gateway_WCPay
- UPE_Split_Payment_Gateway extends UPE_Payment_Gateway
Another possibility is to just hook this extracted method into the wp_enqueue_scripts
filter. With that, you just need to write this code one time in the top-level class WC_Payment_Gateway_WCPay
.
Something like this: https://gist.github.com/htdat/679c224b1c5a761be6082e44695b76ee
When an item with price 0 is added to the cart, it is expected that the payment form does not need to be displayed on the Checkout page. But, when the cart is updated via AJAX on the checkout page to be non-zero, the payment form should appear, because it is no longer a 0 priced order.
Fixes #4764
Fixes #5277
Changes proposed in this Pull Request
Testing instructions
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