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

Fix display time of milestones #18753

Merged
merged 9 commits into from
Feb 15, 2022

Conversation

schorsch13
Copy link
Contributor

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

image

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
Copy link
Member

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.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 13, 2022
@lunny lunny added the type/enhancement An improvement of existing functionality label Feb 14, 2022
@schorsch13
Copy link
Contributor Author

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
Copy link
Member

lunny commented Feb 14, 2022

Could you merge the test into the PR?

@schorsch13
Copy link
Contributor Author

@lunny what do you mean with merging?

@Gusted
Copy link
Contributor

Gusted commented Feb 14, 2022

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
@schorsch13
Copy link
Contributor Author

schorsch13 commented Feb 14, 2022

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

modules/util/sec_to_time.go Outdated Show resolved Hide resolved
modules/util/sec_to_time.go Outdated Show resolved Hide resolved
modules/util/sec_to_time_test.go Outdated Show resolved Hide resolved
modules/util/sec_to_time.go Show resolved Hide resolved
- Update copyright notice dates to 2022
- Change `1 day 3h 5min 7s` to `1d 3h 5m 7s`
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more things.

modules/util/sec_to_time.go Outdated Show resolved Hide resolved
modules/util/sec_to_time.go Outdated Show resolved Hide resolved
@schorsch13
Copy link
Contributor Author

Alright done. Please do a squash-merge, thx

@GiteaBot 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
@GiteaBot 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
@silverwind
Copy link
Member

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

@silverwind
Copy link
Member

Great work, very easy to review!

@lunny lunny merged commit 609c916 into go-gitea:main Feb 15, 2022
@lunny lunny added this to the 1.17.0 milestone Feb 15, 2022
@schorsch13 schorsch13 deleted the fix/milestone-update-time branch February 15, 2022 20:44
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 16, 2022
* giteaofficial/main:
  Various Mermaid improvements (go-gitea#18776)
  [skip ci] Updated translations via Crowdin
  Fix display time of milestones (go-gitea#18753)
Caellion added a commit to Caellion/gitea that referenced this pull request Feb 16, 2022
* '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
  ...
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* 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>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants