-
Notifications
You must be signed in to change notification settings - Fork 213
Checking if taxes are enabled when verifying if ECE should be displayed #4420
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
base: develop
Are you sure you want to change the base?
Checking if taxes are enabled when verifying if ECE should be displayed #4420
Conversation
…ing-for-ece-display
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 believe we fixed this before in #3563, and somehow lost it during refactors. Thanks for catching and fixing.
Added a suggestion to use the WooCommerce-provided function.
includes/payment-methods/class-wc-stripe-express-checkout-helper.php
Outdated
Show resolved
Hide resolved
📈 PHP Unit Code Coverage Report
|
…ing-for-ece-display
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 we should set $taxes_enabled
to true
in the unit tests, except for the two cases that specifically test for the "taxes are disabled" scenario. That way, we know the behavior is due to the scenario it is testing, and not only because taxes are enabled/or disabled, e.g. ECE should always be available in the Pay for Order page.
tests/phpunit/PaymentMethods/WC_Stripe_Express_Checkout_Helper_Test.php
Outdated
Show resolved
Hide resolved
tests/phpunit/PaymentMethods/WC_Stripe_Express_Checkout_Helper_Test.php
Outdated
Show resolved
Hide resolved
tests/phpunit/PaymentMethods/WC_Stripe_Express_Checkout_Helper_Test.php
Outdated
Show resolved
Hide resolved
tests/phpunit/PaymentMethods/WC_Stripe_Express_Checkout_Helper_Test.php
Outdated
Show resolved
Hide resolved
tests/phpunit/PaymentMethods/WC_Stripe_Express_Checkout_Helper_Test.php
Outdated
Show resolved
Hide resolved
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.
LGTM, thanks for fixing!
Just realized I was the one who removed the check when I added per-product tax logic 🙈
Fixes STRIPE-538
Changes proposed in this Pull Request:
This is a minor fix for an edge case when a store enabled taxes but disabled them afterwards. The value of
woocommerce_tax_based_on
would still exist, making the express checkout buttons hidden when falling into the tax-based logic for virtual products.Testing instructions
develop
branch on your test environmentfix/check-if-taxes-are-enabled-when-checking-for-ece-display
)Changelog entry
Changelog Entry Comment
Comment
Post merge