Resolve reimbursement tax calculation discrepancies #253
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjaminwil
changed the title
benjaminwil
force-pushed
the
fix-refunds
branch
3 times, most recently
from
beeeb39
to
700e810
Compare
benjaminwil
force-pushed
the
fix-refunds
branch
from
700e810
to
2fc2b72
Compare
|
@forkata @Noah-Silvera Just want to update you on the status of this draft. (I will be away for the rest of the week.) The commit "Fix tax calculation for orders with returned items" fixes the issue where bad reimbursements are generated during the RMA -> reimbursement admin flow. Check out the commit message for more info about But there is still something not right. If you examine the I haven't had time to delve in yet. I think the change Adam and I made is very correct but maybe something is still missing to account for the reporting API requests. |
benjaminwil
force-pushed
the
fix-refunds
branch
from
2fc2b72
to
ac6b450
Compare
forkata
reviewed
Jan 24, 2024
| def line_items_params(line_items) | ||
| { | ||
| line_items: valid_line_items(line_items).map do |line_item| | ||
| line_items: line_items.filter_map { |line_item| |
forkata
reviewed
Jan 24, 2024
forkata
reviewed
Jan 24, 2024
Our checkout and post-checkout feature specs share some `before` setup, and I wanted to extract it into a shared context to make the spec-specific set up more clear.
Noah-Silvera
force-pushed
the
fix-refunds
branch
from
ac6b450
to
04114a5
Compare
|
Just rebased to get the PR that adds a delay in here! |
While trying to make this spec pass and investigate an issue with
tax calculations we found during manual testing, I wanted to refactor
this spec so that:
1. It would be more clear which line items we were reimbursing for.
2. It would cover the case where a line item would be removed as well as
the case where a line item would not be removed but its quantity
would be changed
This test is pending as the reimbursement amounts populated are
currently wrong ($10.00 instead of $10.89):
Failures:
1) Refunding an order adds tax calculated by TaxJar to the order total
Failure/Error: expect(find(".reimbursement-refund-amount")).to have_content("$10.89")
expected to find text "$10.89" in "$10.00"
# ./spec/features/spree/admin/refund_spec.rb:72:in `block (2 levels) in <top (required)>'
I removed the VCR cassette because we will need to re-record it as part
of making the test pass.
Noah-Silvera
force-pushed
the
fix-refunds
branch
6 times, most recently
from
e4276c1
to
67c8275
Compare
benjaminwil
commented
Jan 30, 2024
Noah-Silvera
force-pushed
the
fix-refunds
branch
3 times, most recently
from
89fdc53
to
09b02f8
Compare
Before this commit, we were excluding returned and cancelled inventory units in two circumstances: 1. For tax calculations 2. For TaxJar's transaction reporting dashboard But this was incorrect. To explain this, we must look at orders with customer returns on them. We want to exclude returned and cancelled inventory units for reporting purposes (2), but we must continue calculating tax for any returned or cancelled units (1). The reason we must continue calculating tax is so that we have a record of how much tax must be reimbursed to customers when we generate new `Spree::Reimbursement`s from the admin. A bit more context: Before this change, whenever `order.recalculate` is called, we would recalculate taxes, excluding returned or cancelled items. This in a returned line item's `#additional_tax_total` being fully zeroed out. A partially returned line item's tax total would be in an even harder-to-understand state. Co-authored-by: Adam Mueller <adam@super.gd> Co-authored-by: Noah Silvera <noah@super.gd>
benjaminwil
force-pushed
the
fix-refunds
branch
from
09b02f8
to
0ce0c49
Compare
Noah-Silvera
force-pushed
the
fix-refunds
branch
2 times, most recently
from
f53d4f9
to
44ad5f6
Compare
This spec is intermittently failing on CI, and it's incredibly hard to reproduce why locally. We need to keep moving so pending this on CI for now Co-authored-by: benjamin wil <benjamin@super.gd>
Noah-Silvera
force-pushed
the
fix-refunds
branch
from
44ad5f6
to
3638ee1
Compare
Merged
forkata
approved these changes
Jan 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the goal of this PR?
While we were manually testing the extension for the 1.0 release, we discovered that going through the stock Solidus RMA -> customer return -> reimbursement flow would result in reimbursements being generated with the wrong amount of tax.
After the manual testing session, I discovered that the refund feature test was being skipped. I refactored it so we can more easily test two scenarios:
This is currently a draft. I am not sure what strategy we should use to resolve the discrepancy. But this gives us a great starting point for discussion.
Merge Checklist