Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect handling of durations in SecToTime() #20589

Closed
edgar-bonet opened this issue Aug 1, 2022 · 3 comments · Fixed by #20610
Closed

Incorrect handling of durations in SecToTime() #20589

edgar-bonet opened this issue Aug 1, 2022 · 3 comments · Fixed by #20610
Labels
Milestone

Comments

@edgar-bonet
Copy link

Description

Pull request #18863 introduced the following code

years := duration / (3600 * 24 * 7 * 4 * 12)
months := (duration / (3600 * 24 * 30)) % 12
weeks := (duration / (3600 * 24 * 7)) % 4
days := (duration / (3600 * 24)) % 7

This is wrong for a couple of reasons:

  1. the week count reverts to zero (because of the modulus operation) after 4 weeks, which is not quite the same as a month

  2. these “years” are 336 days long, which is about 11 months

The consequence is that this function displays:

  • a duration of 340 days (about 11.2 months) as “1 year 11 months”

  • a duration of 29 days as “1 day”

  • a duration of 28 days as the empty string.

See the attached screenshot for the effect of this bug on the display of tracked time.

I suggest to instead use something along these lines:

years := (duration / 86400) / 365
months := (duration / 86400 - years * 365)  / 30
weeks := (duration / 86400 - years * 365 - month * 30) / 7
days := duration / 86400 - years * 365 - month * 30 - weeks * 7

I am not submitting a pull request because I have zero experience with the Go programming language.

Gitea Version

1.18.0+dev-196-ge56005f90

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

time_tracker
Screenshot from this test issue.

Git Version

No response

Operating System

No response

How are you running Gitea?

using https://try.gitea.io/

Database

No response

@noerw
Copy link
Member

noerw commented Aug 1, 2022

👀
I think we should just revert #18863 and use https://github.com/hako/durafmt instead, it also supports i18n.

edit: The formatted duration is written to the comments table in the DB, so it's not really feasible to translate that in the viewer's locale without reworking the whole thing. So maybe switching to durafmt is not worth it.

@edgar-bonet
Copy link
Author

edgar-bonet commented Aug 1, 2022

I wonder whether it is really useful to format those times in years, months, weeks and days.

In the specific context of recording the time spent working on an issue, I would expect the word “day” to mean a “full work day”. Maybe something around 8 hours or so, but certainly not 24 hours. And a week would likely be 5 days. If someone spent 730 hours working on that issue, “730 hours” would be more suitable than “1 month”.

I suggest using hours and minutes as the only time units in this context.

Edit: Even when the numbers get large, hours seem to be the preferred unit. See for example the 10,000 hour rule: no one would call it the “13.7 months rule”.

Gusted added a commit to Gusted/gitea that referenced this issue Aug 2, 2022
- Currently there are edge-cases where the code fails to produce the
correct results, this is mainly due to the misusage of the modulo
operator. Well this could be fixed, it's better to use a more performant
way and simply distract the previous number's cost to calculate the
variables.
- Resolves go-gitea#20589
@Gusted
Copy link
Contributor

Gusted commented Aug 2, 2022

@edgar-bonet Thank you for your detailed issue, opened a PR for this at #20610 with the appropriate comments.

Gusted added a commit to Gusted/gitea that referenced this issue Aug 2, 2022
- Backport of go-gitea#20610
  - Currently there are edge-cases where the code fails to produce the correct results, this is mainly due to the misusage of the modulo operator. Well this could be fixed, it's better to use a more performant way and simply distract the previous number's cost to calculate the variables.
  - Resolves go-gitea#20589
@lunny lunny added this to the 1.17.1 milestone Aug 7, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants