-
Notifications
You must be signed in to change notification settings - Fork 2
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
WooPayments Compatibility #142
Conversation
QA Update ✅I have checked this WooPayment compatibility in the Express pay buttons on the product page ✅ Express pay button on the block cart page ✅ Express pay button on the shortcode cart page ✅ Giftcard product ✅ Giftcard product on cart page - Express pay buttons do not appear ✅ Giftcard product on checkout page - Express pay buttons do not appear ✅ Split Payment Credit card + Giftcard ✅ Testing Environment
Steps to Test- As mentioned in the PR description.
|
@james-allan this PR is ready for your review. |
@diegocurbelo @james-allan checking in on Woo code review, let us know if we should assign elsewhere? |
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 look good an tested well. I left some non-blocking feedback.
@@ -32,7 +32,7 @@ | |||
if ( ! empty( $available_gateways ) ) { | |||
$plugin_gateways = wc_square()->get_gateway_ids(); | |||
foreach ( $available_gateways as $gateway ) { | |||
if ( ! $has_balance && ! in_array( Plugin::GATEWAY_ID, $plugin_gateways, true ) ) { | |||
if ( ! $has_balance && ! in_array( $gateway->id, $plugin_gateways, true ) ) { |
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.
Probably not something we need to look into right now but this logic seems a little odd. $has_balance
doesn't change between each looped element and so this whole loop could be avoided if ! $has_balance
.
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 quickly checked on this, and it doesn't look quite straightforward. @vikrampm1, could you please help by creating an issue for this? This way, the fix can go through proper testing. Thank you.
cc: @Sidsector9
Co-authored-by: James Allan <james.allan@automattic.com>
…o enhancement/121
Thanks for the review @james-allan. I have made suggested changes and responded to the feedback comments. Thank you. |
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.
Code changes look good, and it tests well 🚢
Regression / Smoke Test Report ✅Tested with Archive File created via "php woorelease.phar build repo_URL" (Composer version 2.5.5, npm version 10.7.0, node version 20.15.1) Status- Working expected with Plugin Archive/Zip file same as fix specific branch. Testing Environment
Next Step- Ready to Merge 🚀 |
All Submissions:
Changes proposed in this Pull Request:
PR adds compatibility with the WooPayments extension. it contains the following changes.
Apart from these compatibility changes PR also fixes one issue around split payments to ensure in case of split payment with the gift card only supported payment methods should be displayed.
Closes #121
Steps to test the changes in this Pull Request:
a. Visit the product page: ensure all the express pay buttons appear
b. Place the item in the cart: visit the cart and ensure all the express pay buttons appear
c. Place the item in the cart: visit the checkout page and ensure all the express pay buttons appear
a. Visit the product page: ensure no express pay buttons appear from the WooPayments
b. Place the item in the cart: visit the cart and no express pay buttons appear from the WooPayments
c. Place the item in the cart: visit the checkout page and no express pay buttons and no payment methods appear from the WooPayments
For overall compatibility do the smoke testing on the square extension.
Changelog entry