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

Turn WC_Order::get_tax_location public #36953

Merged
merged 3 commits into from Mar 9, 2023
Merged

Conversation

senadir
Copy link
Member

@senadir senadir commented Feb 24, 2023

This PR turns the get_tax_location function public, this is needed so plugins like Avatax can get the tax location to recalculate taxes (for subscriptions for example). An alternative would be to duplicate the function.

How to test the changes in this Pull Request:

This is a safe backward compatible change

  1. Go to an on hold order in WooCommerce admin.
  2. Change an item price.
  3. Recalculate totals, it should work fine.

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 created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?
  • Have you included testing instructions?

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.

@senadir senadir added this to the 7.6.0 milestone Feb 24, 2023
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 24, 2023
@senadir senadir self-assigned this Feb 24, 2023
@senadir senadir added focus: calculation Issues related to calculations (for example, rounding issues). focus: tax Issues related to taxes. focus: order Issues related to orders. category: extensibility and removed plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Feb 24, 2023
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #36953 (5d25edd) into trunk (6f8f35b) will increase coverage by 0.0%.
The diff coverage is 71.4%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             trunk   #36953   +/-   ##
========================================
  Coverage     46.7%    46.7%           
  Complexity   17188    17188           
========================================
  Files          429      429           
  Lines        64821    64826    +5     
========================================
+ Hits         30251    30254    +3     
- Misses       34570    34572    +2     
Impacted Files Coverage Δ
...ocommerce/includes/abstracts/abstract-wc-order.php 86.7% <0.0%> (-0.2%) ⬇️
.../includes/import/class-wc-product-csv-importer.php 75.1% <100.0%> (+0.2%) ⬆️

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2023

Test Results Summary

Commit SHA: 5d25edd

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 16s
E2E Tests189006019514m 57s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

Copy link
Contributor

@jonathansadowski jonathansadowski left a comment

Choose a reason for hiding this comment

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

I have some concerns that there will always be versions of WooCommerce where this method isn't public, and simply changing it to public would encourage non-defensive coding, and perhaps lead to future errors here.

What do you think about having a simple public wrapper method that calls the protected method?

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 7, 2023
…d public wrapper get_taxable_location()for it.
@wavvves
Copy link
Contributor

wavvves commented Mar 7, 2023

I have some concerns that there will always be versions of WooCommerce where this method isn't public, and simply changing it to public would encourage non-defensive coding, and perhaps lead to future errors here.

What do you think about having a simple public wrapper method that calls the protected method?

Agree, I've made some changes to accommodate for this.

Copy link
Contributor

@jonathansadowski jonathansadowski left a comment

Choose a reason for hiding this comment

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

👍 thanks for updating

@wavvves wavvves merged commit 46b8137 into trunk Mar 9, 2023
20 checks passed
@wavvves wavvves deleted the add/turn-get-tax-location-public branch March 9, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: extensibility focus: calculation Issues related to calculations (for example, rounding issues). focus: order Issues related to orders. focus: tax Issues related to taxes. plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants