Skip to content
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

Use proper location for taxes when adding products via admin #30692

Merged
merged 3 commits into from Sep 22, 2021

Conversation

Konamiman
Copy link
Contributor

@Konamiman Konamiman commented Sep 10, 2021

All Submissions:

Changes proposed in this Pull Request:

When the woocommerce_adjust_non_base_location_prices is set to always use the customer location to calculate taxes, and "I will enter prices inclusive of tax" setting is set, adding a product to an order as an admin via the order editor will calculate the wrong price, because no customer/order data is passed to wc_get_price_excluding_tax and thus the location of the currently logged in user (the shop admin) is used instead of the location of the customer.

This PR fixes that by changing WC_AJAX::maybe_add_order_item so that it passes the order to WC_Abstract_Order->add_product, which in turn passes it to wc_get_price_excluding_tax which then obtains the customer from the order and uses his location to calculate the taxes to be substracted from the price.

Closes #30231.

How to test the changes in this Pull Request:

  1. Add the following snippet to your store: add_filter( 'woocommerce_adjust_non_base_location_prices', '__return_false' );
  2. Make sure that you have the "Prices entered with tax" option set as "Yes, I will enter prices inclusive of tax" (WooCommerce - Settings - Tax - Tax options).
  3. Let A be the country where your currently logged in user is based. Create an user based on a different country B.
  4. Go to WooCommerce - Settings - Tax - Standard rates and set 20% for country A and 10% for country B.
  5. Create a simple product, taxable with standard rates, with a price of 110.
  6. Create an order from admin area, set U as the customer.
  7. Add the product you just created to the order.

Without the fix, the price that will appear in the line item is 91,67 (110 minus 20% of taxes). With the fix, the price will be 100 (10% of taxes substracted this time).

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Fix - Use proper location for taxes when adding products via admin

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

The order is optional and must be passed as an "order" key in "$args".
If present, the order customer will be passed to "WC_Tax::get_rates"
in order to calculate the taxes to substract (provided that
"$product->is_taxable() && wc_prices_include_tax()")

Also "WC_Abstract_Order->add_product" and "WC_AJAX::maybe_add_order_item"
are modified to pass the order being processed so that ultimately
"wc_get_price_excluding_tax" uses it.

This prevents the base tax location to be wrongly used instead of the
customer location when the "woocommerce_adjust_non_base_location_prices"
hook is set to return false.
@Konamiman Konamiman self-assigned this Sep 10, 2021
@Konamiman Konamiman marked this pull request as ready for review September 13, 2021 08:13
@Konamiman Konamiman requested review from a team and vedanshujain and removed request for a team September 13, 2021 08:14
The test verifies that when an order is passed as an argument,
the order customer is passed to 'WC_Tax::get_rates'.
Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Given that this is a very fundamental change, what do you think about adding more tests? I think we can add at least for:

  1. add_product method when order is passed in $args with customer having different base address.
  2. add_product method when order is passed in $args with customer having similar base address.
  3. add_product method when order is passed in $args without customer.
  4. ``add_product` method without order.

Maybe some more variations for when fileter is disabled/enabled as well?

@Konamiman
Copy link
Contributor Author

@vedanshujain I think that since we are already testing that wc_get_price_excluding_tax is using the order it receives, it would be enough to add one test for verifying that add_product is passing the order to wc_get_price_excluding_tax and that the product is added with whatever price is returned by that function.

The test verifies that if an order is passed in arguments, that order
is passed in turn to wc_get_price_excluding_tax.
Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@vedanshujain vedanshujain merged commit 5cdf979 into trunk Sep 22, 2021
@vedanshujain vedanshujain deleted the fix/30231 branch September 22, 2021 09:44
@github-actions github-actions bot added this to the 5.9.0 milestone Sep 22, 2021
@github-actions
Copy link
Contributor

Hi @vedanshujain, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the status: needs changelog label
  • Add the status: needs testing instructions label

@Konamiman Konamiman added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] changelog added and removed release: add changelog Mark all PRs that have not had their changelog entries added. [auto] labels Oct 13, 2021
@tammullen tammullen added testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Oct 20, 2021
Konamiman added a commit that referenced this pull request Oct 22, 2021
PR #30692 modified 'wc_get_price_excluding_tax' so that if an order
is passed its customer will be passed to WC_Tax::get_rates in order
to use the proper location for the taxes to be discounted. The problem
is that when the order has no customer (it's "Guest") an invalid
customer (id=0) is passed, which has no location, and thus no taxes
are deducted whatsoever.

The fix consists of checking if the customer id from the order is 0,
and in that case no customer is passed to WC_Tax::get_rates, thus
the shop location is used for the taxes.
Konamiman added a commit that referenced this pull request Nov 5, 2021
PR #30692 modified 'wc_get_price_excluding_tax' so that if an order
is passed its customer will be passed to WC_Tax::get_rates in order
to use the proper location for the taxes to be discounted. The problem
is that when the order has no customer (it's "Guest") an invalid
customer (id=0) is passed, which has no location, and thus no taxes
are deducted whatsoever.

The fix consists of checking if the customer id from the order is 0,
and in that case no customer is passed to WC_Tax::get_rates, thus
the shop location is used for the taxes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong tax location when adding products to orders via admin (AJAX)
3 participants