Repair rounding issue on totals in reports #13785
Conversation
pacodelaluna
left a comment
pacodelaluna
left a comment
There was a problem hiding this comment.
I have decided to enforce rounding to 2 decimals.
I was thinking to add a specific method to calculate the sum from a list, not sure yet if it will help much actually. Let me know if you think it is worth it.
I think that's a good idea, there is a lot of summing of list, so having a method that does that and handles the rounding should help to not forget the rounding in the future |
github-project-automation
bot
moved this from Code review 🔎
to In Progress ⚙
in OFN Delivery board
pacodelaluna
force-pushed
the
repair-rounding-issue-on-totals-in-reports
branch
2 times, most recently
from
7c49d83 to
fed8862
Compare
pacodelaluna
force-pushed
the
repair-rounding-issue-on-totals-in-reports
branch
from
fed8862 to
8748bd7
Compare
pacodelaluna
left a comment
pacodelaluna
left a comment
There was a problem hiding this comment.
I have introduced the prices_sum method that I put inside ReportsHelper class. Tell me if any issue.
| line_items.map { |li| scaled_final_weight_volume(li) }.sum(&:to_f) | ||
| line_items.map { |li| | ||
| scaled_final_weight_volume(li) | ||
| }.sum(&:to_f).round(3) |
There was a problem hiding this comment.
By security, I think we can give 3 decimals for weight, from kilograms to grams.
| total_units = rows.map(&:total_units) | ||
| summary_total_units = if total_units.all?(&:present?) | ||
| rows.map(&:total_units).sum(&:to_f) | ||
| rows.map(&:total_units).sum(&:to_f).round(3) |
There was a problem hiding this comment.
Same here, units can actually be kilograms, a test is covering the non-integer units case.
| .sum(&:to_f) | ||
| end&.sum(&:to_f) | ||
| prices_sum( | ||
| line_items(query_result_row).to_a.map do |line_item| |
There was a problem hiding this comment.
I found it clearer to convert nil case into empty array, easier to manage also.
rioug
left a comment
rioug
left a comment
There was a problem hiding this comment.
Great ! thanks @pacodelaluna 🙏
dacook
left a comment
dacook
left a comment
There was a problem hiding this comment.
This limits any currency amounts to 2 decimal places. I think this is ok, because I don't know of any currencies where a third decimal place is needed.
But I think there's a better way to solve it, by removing the rounding issue introduced by using Floats. Prices are stored as Decimal, so why can't we sum these values directly?
I see that Float was introduced in the previous PR, but am not sure why. Can you try removing the to_f part?
github-project-automation
bot
moved this from Code review 🔎
to In Progress ⚙
in OFN Delivery board
3 participants
What? Why?
My previous refactoring on reports seems to have introduced a rounding issues on prices totals in reports.
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):