Defect #36897

Wrong formatting of date in Time Entries

Added by Denis Yurashku 6 months ago. Updated 3 months ago.

Status:ConfirmedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Time tracking
Target version:Candidate for next minor release
Resolution: Affected version:

Description

Date is formatted wrong at "<PROJECT> – Time entries" ("7:60" instead of "8:00", for example). See screenshot.

This patch fixes it.

Format_hours.diff Magnifier (450 Bytes) Denis Yurashku, 2022-04-05 14:30

Снимок экрана от 2022-04-05 16-59-25_0.png - Screenshot of bug. (71.6 KB) Denis Yurashku, 2022-04-05 16:03

Format_hours_edit.diff Magnifier (443 Bytes) Denis Yurashku, 2022-04-28 10:14

issue-page.png (98.8 KB) Go MAEDA, 2022-07-19 08:17

spent-time-page.png (88.4 KB) Go MAEDA, 2022-07-19 08:18


Related issues

Related to Redmine - Feature #23996: Introduce a setting to change the display format of times... Closed

History

#1 Updated by Denis Yurashku 6 months ago

I've encountered this in 4.0.6, but AFAIK this bugged part of code still not fixed in 5.0.0.

#2 Updated by Holger Just 6 months ago

  • Related to Feature #23996: Introduce a setting to change the display format of timespans to HH:MM added

#3 Updated by Holger Just 6 months ago

  • Target version set to Candidate for next minor release

This whole thing can probably be simplified to just

def format_hours(hours)
  return "" if hours.blank?

  if Setting.timespan_format == 'minutes'
    "%d:%02d" % (hours * 60).divmod(60)
  else
    l_number("%.2f", hours.to_f)
  end
end

#4 Updated by Denis Yurashku 6 months ago

Probably yes.

But as far as I understood from how hours are displayed in Time entries, they need to be `.round`-ed (not just dropping the fractional part).
I.e. if you've spent 7:59:30 (7.991666666666667 hrs), for human it's 8:00 (we don't count seconds in Time entries). :)
Patch gives "8:00" too.

I think like this.

#5 Updated by Holger Just 6 months ago

Hmmm, with my version, trailing "seconds" indeed would just be cut off instead of being rounded. Thus, semantically 7:59:01 and 7:59:59 would both be shown as "7:59" which is different from the current behavior.

If we round the minutes, we would again have to deal with the case of 7.99 hours being rounded up to "7:60" however.

Maybe this version could work then:

h, m = (hours * 60).divmod(60).map!(&:round)
if m == 60
  h += 1
  m = 0
end
"%d:%02d" % [h, m]

The if block could also be shortened to the equivalent one-liner:

h, m = h+1, 0 if m == 60

In any case, we can be sure that after the first line, m is an Integer between 0 and 60 (inclusive). If it was rounded up to 60, we set to to 0 and increase the hour by one in the if block.

This should then be equivalent to the proposed version in the original patch, but with a little less math involved and a little bit more straight-forward.

At Planio, we run this code for some time now which is a mixture of both approaches:

h = hours.floor
m = ((hours - h) * 60).round
if m == 60
  h += 1
  m = 0
end
"%d:%02d" % [h, m]

Here, the first two lines (which are the same as in Redmine core) return the exact same result as h, m = (hours * 60).divmod(60).map!(&:round).

#6 Updated by Denis Yurashku 6 months ago

Holger Just wrote:

At Planio, we run this code for some time now which is a mixture of both approaches:

h = hours.floor
m = ((hours - h) * 60).round
if m == 60
  h += 1
  m = 0
end
"%d:%02d" % [h, m]

Here, the first two lines (which are the same as in Redmine core) return the exact same result as h, m = (hours * 60).divmod(60).map!(&:round).

Yes, I think we can go this way. :)

#7 Updated by Denis Yurashku 5 months ago

So, the adjusted patch.

#8 Updated by Denis Yurashku 4 months ago

Any movements on this?

Is it "Confirmed"?

#9 Updated by Go MAEDA 3 months ago

Denis Yurashku wrote:

Any movements on this?

Is it "Confirmed"?

I still cannot reproduce the issue. Could you tell me detailed steps to output "7:60"? Without this, I cannot write test code.

#10 Updated by Holger Just 3 months ago

You can reproduce the issue by creating a time entry with decimal logged hours slightly below 8 hours, e.g. 7.991666666666667 hours. Here, the fractional minutes will be rounded up to 60 minutes because 0.991666666666667 * 60 = 59.50000000000002. This value is always allowed as the time entry form also allows entering float values (which are then parsed as floats in lib/redmine/core_ext/string/conversions.rb. External time tracking apps might round durations by the second and then send those exact values which are accepted by Redmine.

Programmatically, you can also create this situation from "full" minutes by logging exactly 75 time entries of 44 minutes each (i.e. 0.7333333333333333 hours). The sum of those time entries then comes at 54.99999999999999. Here, the minutes part is again rounded up to 60 by the existing code.

Depending on the database and the rounding of the floats involved, this situation may also be created with other times as shown in the screenshot as we always round the raw values to two decimal places before summing them in TimeEntryQuery#total_for_hours which may introduce additional error margins.

#11 Updated by Go MAEDA 3 months ago

Holger Just wrote:

You can reproduce the issue by creating a time entry with decimal logged hours slightly below 8 hours, e.g. 7.991666666666667 hours. Here, the fractional minutes will be rounded up to 60 minutes because 0.991666666666667 * 60 = 59.50000000000002.

I have confirmed the issue. The spent time value 7.991666666666667 is displayed as "7:60 h" on the issue page and displayed as "8:00 h" after applying the patch.

But I am not sure if the value should be displayed as "8:00 h" there because it is displayed as "7:59" on the "Spent time" tab. The reason why you see different values on each page is that the Spent time page uses a value 7.99 rounded by TimeEntry#hours (source:tags/5.0.2/app/models/time_entry.rb#L196), while Issue page uses a value 7.991666666666667 obtained from the database (source:tags/5.0.2/app/models/issue.rb#L1140) without using TimeEntry#hours.

Also available in: Atom PDF