-
Notifications
You must be signed in to change notification settings - Fork 10.7k
fix/31971: tax calculation and coupon sequence #32010
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
Conversation
|
@barryhughes @swatipawarGS note that this PR is up for review, please assign reviewer(s) as necessary, thanks! |
vedanshujain
left a comment
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.
Looks good, great work! Can you also add a unit test for this please, these kind of issues are pretty tricky to solve, and we can easily end up accidentally breaking this fix in the future if it's not covered by automated tests.
|
@Sidsector9 note the request for a unit test to help cover this, thanks! |
|
I am facing issues while writing tests. In the first attempt, I was able to write the test case. I confirmed that the site was available. Visiting localhost opened the WordPress site but yet the "waiting for testing environment" error was shown. To fix this, I deleted everything and attempted to write from scratch. But now I see this error: I am not sure what the problem is. I followed the exact steps outlined here. Any idea how to proceed? |
|
hey @Sidsector9, please write a php unit test for this instead of an end-to-end test. PHPUnit test should be relatively much simpler. See this file on instructions for setting it up: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/tests/README.md You would probably want to add your test to this file: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/tests/php/includes/class-wc-ajax-test.php . Some other examples of calculation-based tests - https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/tests/php/includes/class-wc-cart-totals-test.php. Don't hesitate to ask if there are any issues in setup, PHPUnit is generally used internally so it's quite possible that there are gaps in documentation. |
|
@vedanshujain thanks for the guidance. I wrote the skeleton of the test:
Where I am stuck at is testing these AJAX calls. I don't have much experience with testing Ajax calls in unit tests. I saw a similar test in the same file but the method being tested is refactored for better unit testing. My aim for this test is that the Order Total should be same when these 2 methods are called in any order:
How can I achieve this? |
|
hey @Sidsector9 Are you having issues with the post request? Feel free to refactor the original methods if they help in better testing, it will likely be more work than this issue scopes. If you want to continue testing without refactoring, then perhaps you can write two different tests, and pass the required data in a POST variable from the test? Although it might be easier to refactor the parts where it does not interact with the POST variables into different methods and test them instead. |
|
Closing in favour of #33812 |


All Submissions:
Changes proposed in this Pull Request:
This PR fixes the order in which the coupon is applied.
Closes #31971
How to test the changes in this Pull Request:
Please follow the instruction provided in the issue.
I have tested 2 (P)roducts, 3 Standard Tax rates (inclusive/exclusive) and 2 (C)oupons with the following sequences in the order page and the results have been consistent.
Other information:
Changelog entry
FOR PR REVIEWER ONLY: