-
Notifications
You must be signed in to change notification settings - Fork 212
Allow ECE display when default customer location is set to geolocate #4417
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?
Allow ECE display when default customer location is set to geolocate #4417
Conversation
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.
Pull Request Overview
This PR enables the display of express checkout element buttons when taxes are enabled, the product is virtual, tax calculation is based on the billing address, and the default customer location is set to geolocation.
- Updates test cases to include a default_customer_address parameter.
- Modifies the helper logic to allow ECE display for geolocation settings.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/phpunit/PaymentMethods/WC_Stripe_Express_Checkout_Helper_Test.php | Adds and adjusts test scenarios to cover the geolocation setup. |
includes/payment-methods/class-wc-stripe-express-checkout-helper.php | Updates the condition to permit ECE display when the default customer address is 'geolocation' or 'geolocation_ajax'. |
Comments suppressed due to low confidence (1)
includes/payment-methods/class-wc-stripe-express-checkout-helper.php:729
- Add a comment clarifying the meaning of 'geolocation' and 'geolocation_ajax' to document that they represent the geolocate settings (with and without page caching support).
&& ! in_array( $default_customer_address, [ 'geolocation', 'geolocation_ajax' ], 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.
Works as described! 🚢
Initially, it was disabled because we could not determine the customer's location with the setup above. But if location is geolocated, taxes should be accurate.
Small nit on the PR description: Taxes won't always be accurate. For example, my credit card billing address is Texas, but I'm on a business trip in Florida and purchased something while there.
But after discussing this as a team, and given several merchant feedbacks, we decided to relax this restriction. Given there is also a filter available (wc_stripe_should_hide_express_checkout_button_based_on_tax_setup
), any merchant who wishes to tweak the behavior can do so.
…omer-location-is-gelocated
Thanks for the review, Anne! And thanks for explaining it better. I may have understood it differently at first 👍 |
I have some high-level feedback on this change: I have concerns that this change in behaviour doesn't have a configuration setting, and that it is being enabled by default, with the only mechanism to disable being a filter. While some merchants have asked for this, pushing it on all merchants doesn't feel right, especially when we may be adding to their sales tax collection issues. I think it would be ideal to have this controlled by a configuration setting, I think we need to flip this behaviour around and have it be the same as before by default, and we then make it possible to enable the new behaviour via a filter. |
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 working as described, but I think we need the option off by default and available via a filter. Rather than iterating in this PR, I created #4423 to implement my suggestions.
@@ -1,6 +1,7 @@ | |||
*** Changelog *** | |||
|
|||
= 9.6.0 - xxxx-xx-xx = | |||
* Tweak - Allow the display of express payment methods when default customer location is set to "Geolocate" or "Geolocate (with page caching support)". |
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.
Two concerns:
- I think this should be flagged as an
Update
, not aTweak
- We should remove the trailing
.
* Tweak - Allow the display of express payment methods when default customer location is set to "Geolocate" or "Geolocate (with page caching support)". | |
* Update - Allow the display of express checkout when default customer location is set to "Geolocate" or "Geolocate (with page caching support)" |
if ( $is_taxable && $is_tax_based_on_billing && ! $needs_shipping | ||
// Allow ECE to be displayed if the default customer address is set to `geolocation` or `geolocation_ajax`. | ||
&& ! in_array( $default_customer_address, [ 'geolocation', 'geolocation_ajax' ], true ) ) { | ||
return 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.
I think we need a filter here rather than just defaulting on.
Fixes STRIPE-378
Fixes #4170
Changes proposed in this Pull Request:
This PR enables the display of the express checkout element buttons when taxes are enabled, the product is virtual, taxes are based on the billing address, but the default customer location is set to "Geolocate" or "Geolocate (with page caching support)".
Initially, it was disabled because we could not determine the customer's location with the setup above. But if location is geolocated, taxes should be accurate.
Testing instructions
tweak/allow-ece-display-when-default-customer-location-is-gelocated
)wp-admin/admin.php?page=wc-settings&tab=general
and set "Default customer location" to "Geolocate" or "Geolocate (with page caching support)"wp-admin/admin.php?page=wc-settings&tab=tax
and set "Calculate tax based on" to "Customer billing address" and saveChangelog entry
Changelog Entry Comment
Comment
Post merge