Fix display time of milestones by renaissance14 · Pull Request #18753 · go-gitea/gitea

renaissance14 · 2022-02-13T17:29:41Z

There is an issue with the displaying of milestone update times. Currently instead of displaying 2 days 5h 45min 3s it displays 63h 45min 3s.

See: https://try.gitea.io/schorsch/milestone/milestones

This PR fixes this behavior

Testing

I also tested the function with the following script

package main
import (
  "fmt"
  "math"
)

func main() {
  for i := 1.0; i < 7; i++ {
    fmt.Println(SecToTime(int64(math.Pow(10, i))))
  }
}

// SecToTime converts an amount of seconds to a human-readable string (example: 66s -> 1min 6s)
func SecToTime(duration int64) string {
	seconds := duration % 60
	minutes := (duration / (60)) % 60
	hours := duration / (60 * 60) % 24
	days := duration / (60 * 60) / 24

	var hrs string

	if days > 1 {
		hrs = fmt.Sprintf("%d days", days)
	} else {
		hrs = fmt.Sprintf("%d day", days)
	}
	if hours > 0 {
		if days == 0 {
			hrs = fmt.Sprintf("%dh", hours)
		} else {
			hrs = fmt.Sprintf("%s %dh", hrs, hours)
		}
	}
	if minutes > 0 {
		if days == 0 && hours == 0 {
			hrs = fmt.Sprintf("%dmin", minutes)
		} else {
			hrs = fmt.Sprintf("%s %dmin", hrs, minutes)
		}
	}
	if seconds > 0 {
		if days == 0 && hours == 0 && minutes == 0 {
			hrs = fmt.Sprintf("%ds", seconds)
		} else {
			hrs = fmt.Sprintf("%s %ds", hrs, seconds)
		}
	}

	return hrs
}

Output:

10s
1min 40s
16min 40s
2h 46min 40s
1 day 3h 46min 40s
11 days 13h 46min 40s

silverwind · 2022-02-13T21:06:15Z

I imagine this function would better be moved to modules/util so it's reusable across the codebase (maybe there are already duplicate implementations?). Also, some automated tests need to be added.

renaissance14 · 2022-02-14T14:40:23Z

I imagine this function would better be moved to modules/util

I dont know how to do that xD

Also, some automated tests need to be added.

Why would you want to test this function?

lunny · 2022-02-14T16:19:38Z

Could you merge the test into the PR?

renaissance14 · 2022-02-14T18:24:25Z

@lunny what do you mean with merging?

Gusted · 2022-02-14T19:21:18Z

I dont know how to do that xD

Ctrl + c the code, Ctrl + v the code into the new location. Build the new code and check where the errors are(so you know where to fix the code by adding to correct import to the new code).

Why would you want to test this function?

In order to know if the function works, and once in the future if we touch the function, we know that it didn't break.

From the models/issue_stopwatch.go file to the modules/util package

renaissance14 · 2022-02-14T21:03:13Z

I have moved the function to the modules/utils package and created a test for it

- Update copyright notice dates to 2022 - Change `1 day 3h 5min 7s` to `1d 3h 5m 7s`

Gusted

Just a few more things.

renaissance14 · 2022-02-15T11:46:34Z

Alright done. Please do a squash-merge, thx

silverwind · 2022-02-15T12:29:54Z

CI test failures are related:

--- FAIL: TestAddTime (0.03s)
    issue_tracked_time_test.go:37: 
        	Error Trace:	issue_tracked_time_test.go:37
        	Error:      	Not equal: 
        	            	expected: "1h 1m 1s"
        	            	actual  : "1h 1min 1s"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-1h 1m 1s
        	            	+1h 1min 1s
        	Test:       	TestAddTime
--- FAIL: TestTotalTimes (0.02s)
    issue_tracked_time_test.go:89: 
        	Error Trace:	issue_tracked_time_test.go:89
        	Error:      	Not equal: 
        	            	expected: "6min 40s"
        	            	actual  : "6m 40s"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-6min 40s
        	            	+6m 40s
        	Test:       	TestTotalTimes

    issue_tracked_time_test.go:97: 
        	Error Trace:	issue_tracked_time_test.go:97

        	Error:      	Not equal: 
        	            	expected: "1h 1min 2s"
        	            	actual  : "1h 1m 2s"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-1h 1min 2s
        	            	+1h 1m 2s
        	Test:       	TestTotalTimes

Changed `1min` to `1m`

silverwind · 2022-02-15T16:04:13Z

Great work, very easy to review!

* giteaofficial/main: Various Mermaid improvements (go-gitea#18776) [skip ci] Updated translations via Crowdin Fix display time of milestones (go-gitea#18753)

* 'main' of https://github.com/go-gitea/gitea: (87 commits) Fix template bug of LFS lock (go-gitea#18784) Various Mermaid improvements (go-gitea#18776) [skip ci] Updated translations via Crowdin Fix display time of milestones (go-gitea#18753) [skip ci] Updated translations via Crowdin Prevent dangling GetAttribute calls (go-gitea#18754) Add example to render html files (go-gitea#18736) Fix a broken link in `commits_list_small.tmpl` (go-gitea#18763) Fix broken cancel button link on patch page (go-gitea#18718) Ignore the migrate if u2f_registration is not exist (go-gitea#18760) [skip ci] Updated translations via Crowdin Increase the size of the webauthn_credential credential_id field (go-gitea#18739) Fix isempty detection of git repository (go-gitea#18746) [skip ci] Updated translations via Crowdin Send mail to issue/pr assignee/reviewer also when OnMention is set (go-gitea#18707) Reduce CI go module downloads, add make targets (go-gitea#18708) Add number in queue status to monitor page (go-gitea#18712) Fix source code line highlighting (go-gitea#18729) Fix forked repositories missed tags (go-gitea#18719) [skip ci] Updated translations via Crowdin ...

* Fix display time of milestones * Move the SecToTime function From the models/issue_stopwatch.go file to the modules/util package * Rename the sec_to_time file * Updated formatting * Include copyright notice in sec_to_time.go * Apply PR review suggestions - Update copyright notice dates to 2022 - Change `1 day 3h 5min 7s` to `1d 3h 5m 7s` * Rename hrs var and combine conditions * Update unit tests to match new time pattern Changed `1min` to `1m` Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

Fix display time of milestones

db7c4a7

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 13, 2022

lunny added the type/enhancement An improvement of existing functionality label Feb 14, 2022

renaissance14 added 2 commits February 14, 2022 22:00

Move the SecToTime function

0130bed

From the models/issue_stopwatch.go file to the modules/util package

Rename the sec_to_time file

fecd959

renaissance14 added 2 commits February 14, 2022 22:07

Updated formatting

268f28d

Include copyright notice in sec_to_time.go

449f5cc

Gusted reviewed Feb 14, 2022

View reviewed changes

Comment thread modules/util/sec_to_time.go Outdated

Comment thread modules/util/sec_to_time.go Outdated

Comment thread modules/util/sec_to_time_test.go Outdated

Comment thread modules/util/sec_to_time.go

Apply PR review suggestions

5090a37

- Update copyright notice dates to 2022 - Change `1 day 3h 5min 7s` to `1d 3h 5m 7s`

renaissance14 requested a review from Gusted February 14, 2022 23:05

Gusted reviewed Feb 15, 2022

View reviewed changes

Comment thread modules/util/sec_to_time.go Outdated

Comment thread modules/util/sec_to_time.go Outdated

Rename hrs var and combine conditions

f0cf5c3

silverwind approved these changes Feb 15, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 15, 2022

Gusted approved these changes Feb 15, 2022

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 15, 2022

renaissance14 and others added 2 commits February 15, 2022 13:33

Update unit tests to match new time pattern

67efab5

Changed `1min` to `1m`

Merge branch 'main' into fix/milestone-update-time

b19873e

renaissance14 requested review from Gusted and silverwind February 15, 2022 15:46

Gusted approved these changes Feb 15, 2022

View reviewed changes

silverwind approved these changes Feb 15, 2022

View reviewed changes

lunny merged commit 609c916 into go-gitea:main Feb 15, 2022

lunny added this to the 1.17.0 milestone Feb 15, 2022

renaissance14 deleted the fix/milestone-update-time branch February 15, 2022 20:44

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Uh oh!

Conversation

renaissance14 commented Feb 13, 2022

Testing

Uh oh!

silverwind commented Feb 13, 2022

Uh oh!

renaissance14 commented Feb 14, 2022

Uh oh!

lunny commented Feb 14, 2022

Uh oh!

renaissance14 commented Feb 14, 2022

Uh oh!

Gusted commented Feb 14, 2022

Uh oh!

renaissance14 commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gusted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

renaissance14 commented Feb 15, 2022

Uh oh!

silverwind commented Feb 15, 2022

Uh oh!

silverwind commented Feb 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

renaissance14 commented Feb 14, 2022 •

edited

Loading