Increase scale on pre tax amount #459
Conversation
The `pre_tax_amount` column is used when calculating VATs. When the
precision is too low for this field, rounding errors occur.
For a line item with a price of 4.92 $ and a tax rate of 17%, the
calculation has to go like this:
```
4.92 / 1.17 = 4.205128205128205
line_item.save
-->
pre_tax_amount: 4.205128205128205
pre_tax_amount (rounded): 4.21
included_tax_amount 71 ct.
amount 4.92
```
Without this PR, whenever the line item is saved and reloaded, and then
the taxes are re-calculated, the calculation wrongly goes like this:
```
4.92 / 1.17 = 4.205128205128205
line_item.save
-->
pre_tax_amount: 4.21
included_tax_amount: 72 ct.
amount: 4.92
```
That's an error: `4.21 + 72 != 2.92`.
This should not affect any display_* methods, as those use
`Spree::Money`, which takes care of good rounding.
Fixes #6472
On existing installations, the migrations `IncreaseScaleOnPreTaxAmounts` will fail because of existing shipments not having a `pre_tax_amount` set. Fixes #6525 Fixes #6527
| # set pre_tax_amount on shipments to discounted_amount - included_tax_total | ||
| # so that the null: false option on the shipment pre_tax_amount doesn't generate | ||
| # errors. | ||
| # |
There was a problem hiding this comment.
can we do this with a
Spree::Shipment.where(pre_tax_amount: nil).update_all('pre_tax_amount = (cost + promo_total) - included_tax_total)rather than the exec? its a nit-pick but will ensure right table names or whatever are used.
There was a problem hiding this comment.
I prefer what is there. This is a migration, where the table names are already hardcoded.
There was a problem hiding this comment.
Assuming that IS the name of your spree shipments table...
There was a problem hiding this comment.
I've been going back and forth these days on whether to use SQL or to use AR for migrations. For me, I would have done:
Spree::Shipment.where(pre_tax_amount: nil).find_each do |shipment|
pta = shipment.cost + shipment.promo_total - shipment.included_tax_total
shipment.update_attribute 'pre_tax_amount', pta
endThis way its all inside of AR and none of the callbacks happen; I think. If the table name may change, then maybe:
class Shipment < Spree::Base
self.table_name = 'spree_shipments'
end
Shipment.find_each do |shipment|
...
endThere was a problem hiding this comment.
If you look two lines lower in this file the table name is also hardcoded. Table names should be hardcoded in migrations, we don't want migrations to change behaviour.
SQL is always preferable to AR in migrations for the same reason.
|
Yea this one is 👍 by me. Also, BEDMAS. |
|
👍 |
Increase scale on pre tax amount
Commits from @mamhoff in spree 3.0 which increase the scale on the pre_tax_amount columns.
Needed for #440