-
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
Add support for WooCommerce Deposits when using Apple Pay and Google Pay #7910
Add support for WooCommerce Deposits when using Apple Pay and Google Pay #7910
Conversation
Thank you for starting this PR @peterwilsoncc. I fixed the remaining issues and updated the test instruction. |
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 @peterwilsoncc @gpressutto5 for improving this area! 🚀
I went through the code and am leaving a few comments. The testing result will follow in a separate comment because it takes me a bit longer and I want to post these comments in the meantime to speed up the process.
client/checkout/woopay/express-button/use-express-checkout-product-handler.js
Outdated
Show resolved
Hide resolved
client/payment-request/index.js
Outdated
let allowedFieldNames = applyFilters( | ||
'wcpayPaymentRequestAllowedFieldNames', | ||
[] | ||
); | ||
// Ensure allowedFieldNames is an array. | ||
if ( ! Array.isArray( allowedFieldNames ) ) { | ||
allowedFieldNames = [ allowedFieldNames ]; | ||
} | ||
$.each( formData, ( i, field ) => { | ||
if ( /^addon-/.test( field.name ) ) { | ||
if ( | ||
allowedFieldNames.includes( field.name ) || | ||
/^(addon-|wc_)/.test( field.name ) | ||
) { |
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.
Non-blocking and nice-to-have (or a good candidate for a separate small issue): It'd be beneficial to streamline this data
variable appending implementation with the identical one from woopay/express-button/use-express-checkout-product-handler.js
, to avoid code duplication and all the potential future regressions because of it. Both are located in different places which might be complicating things, but just a little bit.
tests/unit/test-class-wc-payments-payment-request-button-handler.php
Outdated
Show resolved
Hide resolved
client/checkout/woopay/express-button/use-express-checkout-product-handler.js
Show resolved
Hide resolved
I tested deposits with virtual and physical products. I successfully tested physical product payment by paying the deposit first and paying the rest with a pay-for-order flow. One thing that doesn't work for me as expected yet is the product price within the Google Pay window for a virtual product with deposits enabled. It isn't updated, although the final order reflects the deposit amount only. @gpressutto5 may I ask you to double-check this if possible? Thanks! |
Thanks @timur27! It is working fine here, maybe it was fixed by something I changed now. Could you clear cache and test it again, please? |
Thanks for picking this up @gpressutto5! It looks in much better shape now than it did before I went on holiday. I've tested it with the following and it seems fine:
For payment plan purchases, I am seeing JS errors on this branch:
The key product details are:
I'm not seeing this bug on trunk so I think it's coming from this branch rather than deposits. |
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.
It is working fine here, maybe it was fixed by something I changed now. Could you clear cache and test it again, please?
@gpressutto5 It works for me now, thank you! I'm approving the PR from my side. Feel free to 🚢 once threads from Peter are resolved.
E2E tests are failing on the env setup stage, I assume it has something to do with how the PR is opened from a fork.
Co-authored-by: Peter Wilson <[email protected]>
I created #8110 to run CI tests, and they passed. The tests that are not passing are the same that are broken in The payment itself is working as expected. The remaining issue is just the tax price being displayed incorrectly when you click the ℹ button with a deposit in the cart, but we are still investigating if taxes are broken in WooCommerce Deposits. @timur27 would you please review the new changes? |
I'll take a look. I've been working through the tax display issues on the Deposits#569. Switching the store to USD and the default tax rates revealed the error for me. I'll need to test any changes against other payment gateways to make sure any changes to the Deposits code doesn't result in undercharging elsewhere. |
@gpressutto5 I've created Deposits#583 for the data storage issue. Modifying the data as items are added to the cart (be it in session storage or a draft order) makes sense to me but we'll need to identify the risk involved in making the change. |
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.
Hey @gpressutto5 @peterwilsoncc! Thanks again for all your efforts so far! 🚀 I left two comments and look forward to following them up with you all.
In terms of testing, I've tested deposits and deposit plans, and it works well for me, with the caveat that there are some open issues you moved to separate tickets. 👍
Overall, I think it'd be great to merge this PR as soon as possible and keep all the non-critical items or items from different areas for follow-up issues due to PR's complexity at this point.
@gpressutto5 @timur27 I was considering this overnight and how it could be fixed in Deposits and worked around in Payments for the time being. The risk of adding a work around in Payments is that once it's fixed, the maths may be doubled up here and reintroduce a problem. Until deposits is fixed, would it be possible to remove itemisation from Apple Pay for carts containing a deposit? The total is correct. Provided, of course, that the Apple terms and conditions allow for this. |
Hi @peterwilsoncc. Itemization is not an issue anymore, only taxes, and removing it for carts with a deposit would require even more code specific for Deposits. I suggest one of the following:
I like the option 2 better as it does not require updating WCPay, only WC Deposits. |
Hi @peterwilsoncc, did you have a chance to look at the options I suggested? It would be great to merge or close this PR as soon as possible to avoid conflicts. |
@gpressutto5 I think two is best, is it possible to remove itemization when there is a deposit so customers aren't confused. I looked at doing it in deposits with the |
This reverts commit 9c54d6b.
Thanks for changing the filter in 369509e, I've tested it with the following and it works as expected: /**
* Hide itemization for Apple Pay and Google Pay when deposits are present.
*
* Compatibility with WooCommerce Payments. This filter is used to hide itemization
* due to an issue in which the itemized amount shows the full price of the product
* rather than the deposit amount.
*
* @todo Remove this filter once the issue is resolved in the extension.
* @see https://github.com/woocommerce/woocommerce-deposits/issues/583
*
* @param bool $hide_itemization Whether to hide itemization for Apple Pay and Google Pay.
* @return bool Modified value based on whether the cart has deposits.
*/
public function wcpay_payment_request_hide_itemization( $hide_itemization ) {
// Hide itemization for carts containing deposits.
if ( $this->has_deposit() ) {
$hide_itemization = true;
}
return $hide_itemization;
} |
The build is failing here because it is a fork. I created #8110 only to run CI, and it is passing. |
97df788
Thank you so much for your help wrapping this up and catching all the gotchas and items I was unaware of. Once this is released, we'll declare compatibility in the Deposits readme for the relevant versions of Payments. |
…Pay (#7910) Co-authored-by: Guilherme Pressutto <[email protected]>
Fixes #7907
Fixes woocommerce/woocommerce-deposits#569
Changes proposed in this Pull Request
This modifies the add to cart AJAX request made during express payments to include field names prefixed with
wc_
. This is to account for fields added by extensions, for example the deposits extensions fieldswc_deposit_option
andwc_deposit_payment_plan
.The goal is to include extension fields during an express payment request so if an input affects the price, that is reflected in the data added to the cart.
Testing instructions
This has been tested with the deposits extension.
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