Fix RMA amount calculator #1590
Conversation
| @@ -36,7 +36,7 @@ | |||
| order.adjustments.first.update_attributes(amount: adjustment_amount) | |||
| end | |||
|
|
|||
| it { is_expected.to eq line_item_price - (adjustment_amount.abs / line_item_quantity) } | |||
| it { is_expected.to be_within(0.1).of(line_item_price - (adjustment_amount.abs / line_item_quantity)) } | |||
There was a problem hiding this comment.
Thanks for the PR.
Being within 0.1 (10 cents) is probably not good enough here. We should test for an exact amount.
This test passes both before and after the change made in default_refund_amount.rb. Is it possible to get a spec which clearly shows how the existing code was incorrect.
There was a problem hiding this comment.
The problem is that the existing code use a percentage to calculate the value of each line item. This works without problem when quantity is factor of 100 , but in other cases this returns a repeating decimal percentage and the result is a little bit different.
Ex:
existing code:
def percentage_of_line_item(inventory_unit)
1 / BigDecimal.new(inventory_unit)
end
...
30 * percentage_of_line_item(3) # 30 * 0.333333333333333333 = 9.99999999999999999
what we want is 10 not 9.99999999999999999
with fix we have:
30 / 3 # 10
Replaced the test from / line_item_quantity to * ( line_item_price / line_item_total ) because the code https://github.com/nebulab/solidus/pull/4/files#diff-f271b0c846fcbc382f43fdfa7f6a0c89R24 use a percentage instead a division
DanielePalombo
force-pushed
the
fix-rma-calculator
branch
from
6d086c8 to
369fcb6
Compare
| @@ -36,7 +37,7 @@ | |||
| order.adjustments.first.update_attributes(amount: adjustment_amount) | |||
| end | |||
|
|
|||
| it { is_expected.to eq line_item_price - (adjustment_amount.abs / line_item_quantity) } | |||
| it { is_expected.to eq line_item_price - (adjustment_amount.abs * ( line_item_price / line_item_total )) } | |||
There was a problem hiding this comment.
Can you replace this call with the actual number its supposed to be, please. Doing it on the computed amount from the setup makes it tougher to figure out what should be happening (though this calculation would be nice to keep in a comment or something).
You can take a look at the tax specs and see how much more readability it adds when you're really trying to figure out whats happening: https://github.com/solidusio/solidus/blob/master/core/spec/models/spree/calculator/default_tax_spec.rb
There was a problem hiding this comment.
ok, i replaced my code.
During my investigation i found round_to_two_places method in Spree::Calculator::DefaultTax. Maybe we could use this method? I made this commit with my proposal.
DanielePalombo
force-pushed
the
fix-rma-calculator
branch
2 times, most recently
from
ba6a3df to
4bf75f6
Compare
There was a problem hiding this comment.
@DanielePalombo Would you mind rebasing on the latest master branch? The discounted_amount method has been changed to total_before_tax.
Otherwise, this still LGTM.
|
Ok, I'll rebase it next weekend. |
RMA calculator use the percentage to calculate the weighted line item amount, but when the quantiy isn't a factor of 100 the result percentage is a repeating decimal and the calculator makes an error. The fix replace the percentage calculation with a division of result. ref solidusio#1564
DanielePalombo
force-pushed
the
fix-rma-calculator
branch
from
4bf75f6 to
8d8f3b5
Compare
|
I rebased it. |
cbrunsdon
dismissed
their stale review
Good enough for me, you could have picked numbers that divided easier or, aye, rounded it, but this is fine too
RMA calculator use the percentage to calculate the weighted line item
amount, but when the percentage is a repeating decimal the calculator
makes an error. Replaced the percentage calculation with a division.
ref #1564