Repair rounding issue on totals in reports by pacodelaluna · Pull Request #13785 · openfoodfoundation/openfoodnetwork

pacodelaluna · 2025-12-04T15:20:41Z

What? Why?

Closes Rounding error on reports #13717

My previous refactoring on reports seems to have introduced a rounding issues on prices totals in reports.

What should we test?

Visit Reports in Admin, especially orders_and_fulfillment ones
Check that price totals are rounded to 2 decimals and not more

Release notes

Repair rounding issue on totals in reports

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

pacodelaluna

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.

rioug · 2025-12-04T22:48:45Z

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

rioug

See comment.

pacodelaluna

I have introduced the prices_sum method that I put inside ReportsHelper class. Tell me if any issue.

pacodelaluna · 2025-12-18T16:48:01Z

lib/reporting/reports/bulk_coop/base.rb

-          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)


By security, I think we can give 3 decimals for weight, from kilograms to grams.

pacodelaluna · 2025-12-18T16:51:34Z

lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals.rb

                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)


Same here, units can actually be kilograms, a test is covering the non-integer units case.

pacodelaluna · 2025-12-18T17:02:02Z

lib/reporting/reports/sales_tax/sales_tax_totals_by_producer.rb

-              .sum(&:to_f)
-          end&.sum(&:to_f)
+          prices_sum(
+            line_items(query_result_row).to_a.map do |line_item|


I found it clearer to convert nil case into empty array, easier to manage also.

rioug

Great ! thanks @pacodelaluna 🙏

dacook

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?

pacodelaluna · 2026-02-24T17:35:41Z

@dacook Sorry, I missed your message, I will take a look at this asap.

pacodelaluna

@dacook I have simplified the sum logic avoiding unnecessary conversions as you suggested. It worked fine directly. I had to adjust the prices retrieval for 2 lines on Payments report side.

app/helpers/reports_helper.rb

pacodelaluna · 2026-02-27T13:40:12Z

lib/reporting/reports/payments/payment_totals.rb

            paypal_price: proc { |orders| total_by_payment_method(orders, "PayPal") },
            outstanding_balance_price: proc { |orders|
-              orders.map(&:outstanding_balance).sum(&:to_f)
+              prices_sum(orders.map(&:outstanding_balance).map(&:amount))


Here, I have to check directly the amount attribute of the object, as the float conversion is not present anymore.

pacodelaluna · 2026-02-27T13:41:01Z

lib/reporting/reports/payments/payments_by_payment_type.rb

            distributor: proc { |payments| payments.first.order.distributor.name },
            payment_type: proc { |payments| payments.first.payment_method&.name },
-            total_price: proc { |payments| payments.map(&:amount).sum(&:to_f) }
+            total_price: proc { |payments| prices_sum(payments.map(&:amount)) }


Here also, I have to check directly the amount attribute of the object, as the conversion is not present anymore.

dacook

Thanks, now I see why to_f was added: to protect against nil values. Because prices are actually decimals (BigDecimal), we could have solved the rounding issue here by using to_d.

But I like the .compact.sum solution you used because the purpose is clearer 👍

This is a method that can be used for more than just prices, so I think could best be described as compact_sum. But in that case, it would be even better to not have the method, and simply use compact.sum!

Would you agree to use .compact.sum directly instead of prices_sum?

pacodelaluna

@dacook I have removed the helper method for simplicity, let me know if it is fine now.

spec/lib/reports/bulk_coop_report_spec.rb

It is the whole purpose of the spec.

dacook

Great, thanks for following up. Now there are no assumptions made on the datatype, the prices work and it seems everything else does too! 🏅

Edit: spec failure is unrelated flaky system spec and can be ignored.

Repair rounding issue on totals in reports

30f702e

github-project-automation bot added this to OFN Delivery board Dec 4, 2025

github-project-automation bot moved this to All the things 💤 in OFN Delivery board Dec 4, 2025

github-actions bot assigned pacodelaluna Dec 4, 2025

pacodelaluna commented Dec 4, 2025

View reviewed changes

sigmundpetersen moved this from All the things 💤 to Code review 🔎 in OFN Delivery board Dec 4, 2025

rioug requested changes Dec 4, 2025

View reviewed changes

github-project-automation bot moved this from Code review 🔎 to In Progress ⚙ in OFN Delivery board Dec 4, 2025

pacodelaluna force-pushed the repair-rounding-issue-on-totals-in-reports branch 2 times, most recently from 7c49d83 to fed8862 Compare December 18, 2025 16:45

Introduce a prices_sum helper to factorize the calculation on reports

8748bd7

pacodelaluna force-pushed the repair-rounding-issue-on-totals-in-reports branch from fed8862 to 8748bd7 Compare December 18, 2025 17:00

pacodelaluna commented Dec 18, 2025

View reviewed changes

rioug approved these changes Dec 19, 2025

View reviewed changes

rioug moved this from In Progress ⚙ to Code review 🔎 in OFN Delivery board Dec 19, 2025

dacook self-requested a review December 21, 2025 23:14

dacook requested changes Dec 21, 2025

View reviewed changes

github-project-automation bot moved this from Code review 🔎 to In Progress ⚙ in OFN Delivery board Dec 21, 2025

pacodelaluna added 2 commits February 26, 2026 18:04

Remove floating point overkill for prices sum

de52e21

Adjust logic and specs

08691f8

pacodelaluna commented Feb 27, 2026

View reviewed changes

sigmundpetersen moved this from In Progress ⚙ to Code review 🔎 in OFN Delivery board Feb 27, 2026

sigmundpetersen requested a review from dacook February 27, 2026 13:53

rioug approved these changes Mar 1, 2026

View reviewed changes

dacook requested changes Mar 2, 2026

View reviewed changes

github-project-automation bot moved this from Code review 🔎 to In Progress ⚙ in OFN Delivery board Mar 2, 2026

Remove helper method to use direct logic

a069e42

pacodelaluna force-pushed the repair-rounding-issue-on-totals-in-reports branch from 75f998a to a069e42 Compare March 2, 2026 21:43

pacodelaluna commented Mar 2, 2026

View reviewed changes

dacook reviewed Mar 3, 2026

View reviewed changes

spec/lib/reports/bulk_coop_report_spec.rb Show resolved Hide resolved

Restore required expectation

46de21e

It is the whole purpose of the spec.

dacook approved these changes Mar 3, 2026

View reviewed changes

dacook moved this from In Progress ⚙ to Test Ready 🧪 in OFN Delivery board Mar 3, 2026

Uh oh!

Conversation

pacodelaluna commented Dec 4, 2025

What? Why?

What should we test?

Release notes

Uh oh!

pacodelaluna left a comment

Choose a reason for hiding this comment

Uh oh!

rioug commented Dec 4, 2025

Uh oh!

rioug left a comment

Choose a reason for hiding this comment

Uh oh!

pacodelaluna left a comment

Choose a reason for hiding this comment

Uh oh!

pacodelaluna Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

pacodelaluna Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

pacodelaluna Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

rioug left a comment

Choose a reason for hiding this comment

Uh oh!

dacook left a comment

Choose a reason for hiding this comment

Uh oh!

pacodelaluna commented Feb 24, 2026

Uh oh!

pacodelaluna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pacodelaluna Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

pacodelaluna Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

dacook left a comment

Choose a reason for hiding this comment

Uh oh!

pacodelaluna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dacook left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dacook left a comment •

edited

Loading