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

Fix number of orders under tax report #38525

Merged
merged 6 commits into from Jun 15, 2023

Conversation

octaedro
Copy link
Contributor

@octaedro octaedro commented May 30, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

Under WooCommerce > Reports > Taxes, the number of orders displayed is incorrect. Currently, it shows the count of applied taxes instead of the actual number of orders that are charging taxes.

Before
Screenshot 2023-05-31 at 11 27 43

After
Screenshot 2023-05-31 at 11 10 13

Partially fixes issue 38347.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Set up a store with a tax plugin like Avalara Avatax.
  2. Create a product and create an order. You may want to use a California address so multiple tax rules are applied.
  3. Complete the order and make sure all the actions are run under WooCommerce > Status > Scheduled Actions
  4. Go to WooCommerce > Reports > Taxes
  5. Tax numbers should show correctly here.
  6. Now checkout trunk, rebuild the project, and refresh the page.
  7. The bug should be visible again.

@octaedro octaedro self-assigned this May 30, 2023
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label May 30, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 30, 2023

Test Results Summary

Commit SHA: da5ab47

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 52s
E2E Tests1950010020515m 37s

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.

@octaedro octaedro requested a review from a team May 31, 2023 14:32
@octaedro octaedro marked this pull request as ready for review May 31, 2023 14:33
@github-actions
Copy link
Contributor

github-actions bot commented May 31, 2023

Hi @louwie17, @nathanss,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@nathanss nathanss self-requested a review June 1, 2023 13:49
Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

Hey @octaedro thanks for digging into this, it looks like a fun one :)
I left a couple thoughts and suggestions on how to address this.
I think the underlying issue where the tax_rate is zero for Avatax is causing them all to loop together.

Speaking of tax_rate it looks like in our Analytics > Taxes table we are relying to much on wp_woocommerce_tax_rates table. So if a user deletes an old tax rate that data won't show up within the tax table in Analytics anymore. This is why when I enabled Avatax and deleted my other tax rates the table remained empty. As Avatax doesn't use the wp_woocommerce_tax_rates table.

I think we may want to use similar logic as the legacy reports tax page when displaying tax rates.

Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

Hey @octaedro thanks for digging into this, it looks like a fun one :)
I left a couple thoughts and suggestions on how to address this.
I think the underlying issue where the tax_rate is zero for Avatax is causing them all to loop together.

Speaking of tax_rate it looks like in our Analytics > Taxes table we are relying to much on wp_woocommerce_tax_rates table. So if a user deletes an old tax rate that data won't show up within the tax table in Analytics anymore. This is why when I enabled Avatax and deleted my other tax rates the table remained empty. As Avatax doesn't use the wp_woocommerce_tax_rates table.

I think we may want to use similar logic as the legacy reports tax page when displaying tax rates.

@nathanss
Copy link
Contributor

nathanss commented Jun 1, 2023

Might be related to what @louwie17 found... But I found this bug when displaying "Last month" (there were no orders on that time period):

image

Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

Tested well, just left two minor comments, aside from that this will be good to go.

@octaedro octaedro requested a review from louwie17 June 6, 2023 13:36
Copy link
Contributor

@nathanss nathanss left a comment

Choose a reason for hiding this comment

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

The issue I found previously was fixed.

I have no comments regarding the code, and it seems Lourens already made a thorough review of it, so I'm approving it.

@octaedro octaedro merged commit d74904b into trunk Jun 15, 2023
17 checks passed
@octaedro octaedro deleted the fix/38347_tax_report_count_number_of_orders branch June 15, 2023 14:11
@github-actions github-actions bot added this to the 7.9.0 milestone Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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