Redmine

In the same way the <h3>..</h3> tag should be fixed:

Current implementation (app/views/my/blocks/_timelog.html.erb:7):

l_hours_short entries.sum(&:hours)

Fix:

l_hours_short(entries.map{|t| (t.hours * 60).to_i}.sum / 60)

Go MAEDA wrote in #note-3:

Here is another solution. This patch will likely also fix the issue reported in #36897.

[...]

Sorry, using rationalize(Rational('1/60')) (approximation in minutes) breaks some tests. rationalize(Rational('1/3600')) (approximation in seconds) works fine.

Failure:
TimelogReportTest#test_report_all_time_by_day [test/functional/timelog_report_test.rb:87]:
Expected: "162:54" 
  Actual: "162:53".
Expected 0 to be >= 1.

bin/rails test test/functional/timelog_report_test.rb:84

diff --git a/lib/redmine/i18n.rb b/lib/redmine/i18n.rb
index 83ecb05eb..40b991f8a 100644
--- a/lib/redmine/i18n.rb
+++ b/lib/redmine/i18n.rb
@@ -93,8 +93,9 @@ module Redmine
       return "" if hours.blank?

       if Setting.timespan_format == 'minutes'
-        h = hours.floor
-        m = ((hours - h) * 60).round
+        rational_hours = hours.rationalize(Rational('1/3600'))
+        h = rational_hours.truncate
+        m = ((rational_hours - h) * 60).round
         "%d:%02d" % [h, m]
       else
         number_with_delimiter(sprintf('%.2f', hours.to_f), delimiter: nil)

The previous code hours = hours.to_f in the l_hours method would also accept strings such as "0.5" and format them correctly. With your proposed patch, this would break. I'm not sure if we actually do pass strings there, but I would assume that this can happen, e.g., when e.g. round-tripping raw params.

Also, while I thing the logic to use rationals is a nice and elegant solution, we likely have to check / update other places with similar logic where we calculate sums. This often happens in the database now with a SUM(hours) statement in SQL. Here, the database would again use floats right now and thus show inconsistent sums, depending on where they are calculated.

It might turn out that the only actual and consistent solution for this is to to replace the current fractional hours column with an integer minutes column and calculate the times based on that. Unfortunately, this would be a rather large change though...

Holger Just wrote in #note-12:

The previous code hours = hours.to_f in the l_hours method would also accept strings such as "0.5" and format them correctly. With your proposed patch, this would break. I'm not sure if we actually do pass strings there, but I would assume that this can happen, e.g., when e.g. round-tripping raw params.

I have fixed this issue in the new patch. The method now properly handles string inputs such as “0.5”.

Also, while I thing the logic to use rationals is a nice and elegant solution, we likely have to check / update other places with similar logic where we calculate sums. This often happens in the database now with a SUM(hours) statement in SQL. Here, the database would again use floats right now and thus show inconsistent sums, depending on where they are calculated.

You are correct that applying this patch will not resolve the inconsistency between results calculated using Ruby and results calculated using SQL. However, the overall inconsistencies in Redmine will be reduced with this patch.

The reason is that, before applying this patch, the TimeEntry#hours method rounds the return value to two decimal places (e.g. 0.17), while SQL SUM uses higher precision values (e.g. 0.16666666666666666). Therefore, errors in calculation will occur less frequently in SQL calculations compared to when the sum is calculated in Ruby code.

Test data preparation:

Issue.create!(project_id: 1, author_id: 1, tracker_id: 1, subject: 'test')
TimeEntry.create!(issue: issue, hours: '10m', user_id: 1, author_id: 1, spent_on: DateTime.now)
TimeEntry.create!(issue: issue, hours: '10m', user_id: 1, author_id: 1, spent_on: DateTime.now)
TimeEntry.create!(issue: issue, hours: '40m', user_id: 1, author_id: 1, spent_on: DateTime.now)

Calculating the sum using Ruby code (without the patch):
Float values are rounded to two decimal places (e.g. 0.17), so calculations using these values often return an incorrect value.

irb(main):001> TimeEntry.where(issue: Issue.last).map(&:hours).sum
  Issue Load (0.1ms)  SELECT "issues".* FROM "issues" ORDER BY "issues"."id" DESC LIMIT ?  [["LIMIT", 1]]
  TimeEntry Load (0.1ms)  SELECT "time_entries".* FROM "time_entries" WHERE "time_entries"."issue_id" = ?  [["issue_id", 16]]
=> 1.01

Calculating the sum using Ruby code (with the patch):

irb(main):003> TimeEntry.where(issue: Issue.last).map(&:hours).sum
  Issue Load (0.1ms)  SELECT "issues".* FROM "issues" ORDER BY "issues"."id" DESC LIMIT ?  [["LIMIT", 1]]
  TimeEntry Load (0.1ms)  SELECT "time_entries".* FROM "time_entries" WHERE "time_entries"."issue_id" = ?  [["issue_id", 16]]
=> (1/1)

Calculating the sum using SQL:
Since higher precision values (e.g. 0.16666666666666666) are used than the return values of TimeEntry#hours, it returns a correct value in many cases.

irb(main):002> TimeEntry.where(issue: Issue.last).sum(:hours)
  Issue Load (0.1ms)  SELECT "issues".* FROM "issues" ORDER BY "issues"."id" DESC LIMIT ?  [["LIMIT", 1]]
  TimeEntry Sum (0.1ms)  SELECT SUM("time_entries"."hours") FROM "time_entries" WHERE "time_entries"."issue_id" = ?  [["issue_id", 16]]
=> 1.0

In conclusion, this patch does not introduce new inconsistencies or worsen existing ones. While the SQL calculation results remain occasionally inaccurate both before and after applying the patch, the Ruby calculation results, which is often inaccurate, will always be accurate. As a result, overall inconsistencies will be reduced.