Defect #36897
closedThe minutes part of a time entry is displayed as 60 instead of being carried over
Added by Denis Yurashku over 2 years ago. Updated 3 months ago.
0%
Description
Date is formatted wrong at "<PROJECT> – Time entries" ("7:60" instead of "8:00", for example). See screenshot.
This patch fixes it.
Files
Format_hours.diff (450 Bytes) Format_hours.diff | Denis Yurashku, 2022-04-05 14:30 | ||
Снимок экрана от 2022-04-05 16-59-25_0.png (71.6 KB) Снимок экрана от 2022-04-05 16-59-25_0.png | Screenshot of bug. | Denis Yurashku, 2022-04-05 16:03 | |
Format_hours_edit.diff (443 Bytes) Format_hours_edit.diff | Denis Yurashku, 2022-04-28 10:14 | ||
issue-page.png (98.8 KB) issue-page.png | Go MAEDA, 2022-07-19 08:17 | ||
spent-time-page.png (88.4 KB) spent-time-page.png | Go MAEDA, 2022-07-19 08:18 | ||
36897.patch (1.64 KB) 36897.patch | Go MAEDA, 2024-07-22 06:03 |
Related issues
Updated by Denis Yurashku over 2 years ago
I've encountered this in 4.0.6, but AFAIK this bugged part of code still not fixed in 5.0.0.
Updated by Holger Just over 2 years ago
- Related to Feature #23996: Introduce a setting to change the display format of timespans to HH:MM added
Updated by Holger Just over 2 years 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
Updated by Denis Yurashku over 2 years 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.
Updated by Holger Just over 2 years 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)
.
Updated by Denis Yurashku over 2 years 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. :)
Updated by Denis Yurashku over 2 years ago
- File Format_hours_edit.diff Format_hours_edit.diff added
So, the adjusted patch.
Updated by Denis Yurashku over 2 years ago
Any movements on this?
Is it "Confirmed"?
Updated by Go MAEDA over 2 years 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.
Updated by Holger Just over 2 years 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.
Updated by Go MAEDA over 2 years ago
- File issue-page.png issue-page.png added
- File spent-time-page.png spent-time-page.png added
- Tracker changed from Patch to Defect
- Status changed from New to Confirmed
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 because0.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.
Updated by Go MAEDA 4 months ago
- Related to Defect #40914: Fix precision issues in TimeEntry#hours calculation by returning Rational instead of Float added
Updated by Go MAEDA 3 months ago
- File 36897.patch 36897.patch added
Go MAEDA wrote in #note-13:
This issue will be fixed if the patch proposed in #40914 is merged.
I would like to propose a new patch to address this issue. This patch can resolve the reported problem without applying the patch in #40914.
This patch converts the given value hours to the nearest minute using (hours * 60).round
. Since the converted value is an integer, the minute value obtained from minutes.divmod(60)
will always be an integer within the range of 0 to 59. This ensures that edge cases where the minute value would be 60, such as 7:60, are eliminated.
Additionally, whether the timespan_format
setting is minutes
or decimal
, the same rounded minute value is used. This reduces instances where the displayed values appear inconsistent depending on the setting (for example, displaying '8:00' when the setting is minutes
but '7.99' when the setting is decimal).