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

Shows total tracked time in issue and milestone list #3341

Merged
merged 39 commits into from
Apr 29, 2018

Conversation

jonasfranz
Copy link
Member

@jonasfranz jonasfranz commented Jan 9, 2018

Fixes #3003 (partially)

  • Shows total tracked time in issue and milestone list
  • Shows total tracked time at issue detail page

Milestone List:
Milestone List

Issue List:
Issue List

Issue Detail Page:
Issue Page

Show total tracked time at issue page

Signed-off-by: Jonas Franz <info@jonasfranz.software>
models/issue.go Outdated
@@ -68,6 +68,24 @@ func init() {
issueTasksDonePat = regexp.MustCompile(issueTasksDoneRegexpStr)
}

func (issue *Issue) totalTimes(e Engine) (string, error) {
times, err := GetTrackedTimes(FindTrackedTimesOptions{IssueID: issue.ID})
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to select total time from database already as single value

Copy link
Member Author

Choose a reason for hiding this comment

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

This would mean a higher maintenance effort, wouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Just reuse FindTrackedTimesOptions and use x.Where(options.ToCond()).Sum(&TrackedTime{}, "time")

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 9, 2018
Signed-off-by: Jonas Franz <info@jonasfranz.software>
@codecov-io
Copy link

codecov-io commented Jan 10, 2018

Codecov Report

Merging #3341 into master will increase coverage by 0.29%.
The diff coverage is 66.94%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3341      +/-   ##
=========================================
+ Coverage      23%   23.3%   +0.29%     
=========================================
  Files         126     126              
  Lines       24892   24999     +107     
=========================================
+ Hits         5726    5825      +99     
+ Misses      18289   18286       -3     
- Partials      877     888      +11
Impacted Files Coverage Δ
routers/repo/issue.go 0.96% <0%> (-0.01%) ⬇️
models/issue_stopwatch.go 75% <100%> (ø) ⬆️
models/issue.go 31.48% <41.17%> (+0.16%) ⬆️
models/issue_list.go 54.19% <69.23%> (+2.37%) ⬆️
models/issue_tracked_time.go 70.42% <72.72%> (-2.72%) ⬇️
models/issue_milestone.go 59.56% <79.54%> (+3.39%) ⬆️
models/login_source.go 0.84% <0%> (+0.84%) ⬆️
models/repo_unit.go 37.93% <0%> (+31.03%) ⬆️
models/repo_issue.go 52.63% <0%> (+52.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3028d1...e9562ad. Read the comment docs.

@lunny lunny added the type/enhancement An improvement of existing functionality label Jan 11, 2018
@lunny lunny added this to the 1.x.x milestone Jan 11, 2018
@lafriks
Copy link
Member

lafriks commented Jan 13, 2018

I don't know if this can be done with xorm but for list it would be better to load it with one select (group by and sum). Might be that @lunny can answer that

@lunny
Copy link
Member

lunny commented Jan 14, 2018

If you want join, just add Join.

e.Join("INNER", "issue", "issue.id = tracked_time.issue_id").Where(opts.ToCond()).SumInt(&TrackedTime{}, "tracked_time.time")

@jonasfranz
Copy link
Member Author

@lunny I am using this JOIN already (https://github.com/JonasFranzDEV/gitea/blob/187503832073280c4c994753778e1d69c307a000/models/issue_tracked_time.go#L71). I think that @lafriks meant that it would be better to make a query for everything like listing issues and sum of tracked time.

Adding unit tests for total times

Signed-off-by: Jonas Franz <info@jonasfranz.software>
models/issue.go Outdated
// IsTimetrackerEnabled returns true if the repo enables timetracking
func (issue *Issue) IsTimetrackerEnabled() bool {
if issue.loadRepo(x) != nil {
return false
Copy link
Member

Choose a reason for hiding this comment

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

If error is ignored than at least it should be logged

@lunny lunny modified the milestones: 1.x.x, 1.5.0 Feb 26, 2018
@lunny
Copy link
Member

lunny commented Feb 26, 2018

LGTM

@tboerger tboerger 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 26, 2018
models/issue.go Outdated

// TotalTimes returns the total amount of tracked time for the issue
func (issue *Issue) TotalTimes() string {
times, _ := issue.totalTimes(x)
Copy link
Member

Choose a reason for hiding this comment

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

Error should be at least logged

@@ -80,6 +80,10 @@
<span class="comment ui right"><i class="octicon octicon-comment"></i> {{.NumComments}}</span>
{{end}}

{{if and .IsTimetrackerEnabled .TotalTimes}}
Copy link
Member

Choose a reason for hiding this comment

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

Result of .TotalTimes be assigned probably to some variable otherwise currently if there is value this query will be executed twice

Add variable for totalTimes

Signed-off-by: Jonas Franz <info@jonasfranz.de>
Load TotalTrackedTimes by loading attributes of IssueList
Load TotalTrackedTimes by loading attributes of single issue
Add Sec2Time as helper to use it in templates

Signed-off-by: Jonas Franz <info@jonasfranz.software>
@lafriks
Copy link
Member

lafriks commented Apr 16, 2018

@JonasFranzDEV in model just like IssueList

Signed-off-by: Jonas Franz <info@jonasfranz.software>
@jonasfranz
Copy link
Member Author

@lafriks Done

Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
}
var trackedTimes = make(map[int64]int64, len(milestones))

// SELECT milestone_id, sum(time) as time FROM `issue`
Copy link
Member

Choose a reason for hiding this comment

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

This comment should be removed

Signed-off-by: Jonas Franz <info@jonasfranz.software>
@lafriks
Copy link
Member

lafriks commented Apr 19, 2018

Need to resolve conflicts but otherwise LG-TM

@jonasfranz
Copy link
Member Author

@lafriks Done

Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
@@ -64,6 +64,7 @@
<span class="issue-stats">
<i class="octicon octicon-issue-opened"></i> {{$.i18n.Tr "repo.issues.open_tab" .NumOpenIssues}}
<i class="octicon octicon-issue-closed"></i> {{$.i18n.Tr "repo.issues.close_tab" .NumClosedIssues}}
{{if .TotalTrackedTime}}<i class="octicon octicon-clock"></i> {{.TotalTrackedTime|Sec2Time}}{{end}}
Copy link
Member

Choose a reason for hiding this comment

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

This should probably also change to same as in issues.tmpl: {{if and .IsTimetrackerEnabled .TotalTrackedTime}}

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because .TotalTrackedTime is zero if timetracker is disabled because TotalTrackedTime is only loaded if it is enabled. (https://github.com/go-gitea/gitea/pull/3341/files#diff-ece22e0fe138da6e0fe0f25213406313R1142)

Copy link
Member

Choose a reason for hiding this comment

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

Than probably there also needs to be such check added so that it would behave same in both places: https://github.com/JonasFranzDEV/gitea/blob/d6237809c472de482cc31fb954af1a8fd6ef54b4/models/issue.go#L248

Copy link
Member Author

Choose a reason for hiding this comment

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

@lafriks Done

jonasfranz and others added 3 commits April 20, 2018 20:07
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.de>
@jonasfranz
Copy link
Member Author

@lafriks Build failure is not related to this PR, could you please merge?

@lunny lunny merged commit 8d5f58d into go-gitea:master Apr 29, 2018
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Jul 3, 2018
aswild added a commit to aswild/gitea that referenced this pull request Jul 6, 2018
* SECURITY
  * Limit uploaded avatar image-size to 4096x3072 by default (go-gitea#4353)
  * Do not allow to reuse TOTP passcode (go-gitea#3878)
* FEATURE
  * Add cli commands to regen hooks & keys (go-gitea#3979)
  * Add support for FIDO U2F (go-gitea#3971)
  * Added user language setting (go-gitea#3875)
  * LDAP Public SSH Keys synchronization (go-gitea#1844)
  * Add topic support (go-gitea#3711)
  * Multiple assignees (go-gitea#3705)
  * Add protected branch whitelists for merging (go-gitea#3689)
  * Global code search support (go-gitea#3664)
  * Add label descriptions (go-gitea#3662)
  * Add issue search via API (go-gitea#3612)
  * Add repository setting to enable/disable health checks (go-gitea#3607)
  * Emoji Autocomplete (go-gitea#3433)
  * Implements generator cli for secrets (go-gitea#3531)
* ENHANCEMENT
  * Add more webhooks support and refactor webhook templates directory (go-gitea#3929)
  * Add new option to allow only OAuth2/OpenID user registration (go-gitea#3910)
  * Add option to use paged LDAP search when synchronizing users (go-gitea#3895)
  * Symlink icons (go-gitea#1416)
  * Improve release page UI (go-gitea#3693)
  * Add admin dashboard option to run health checks (go-gitea#3606)
  * Add branch link in branch list (go-gitea#3576)
  * Reduce sql query times in retrieveFeeds (go-gitea#3547)
  * Option to enable or disable swagger endpoints (go-gitea#3502)
  * Add missing licenses (go-gitea#3497)
  * Reduce repo indexer disk usage (go-gitea#3452)
  * Enable caching on assets and avatars (go-gitea#3376)
  * Add repository search ordered by stars/forks. Forks column in admin repo list (go-gitea#3969)
  * Add Environment Variables to Docker template (go-gitea#4012)
  * LFS: make HTTP auth period configurable (go-gitea#4035)
  * Add config path as an optionial flag when changing pass via CLI (go-gitea#4184)
  * Refactor User Settings sections (go-gitea#3900)
  * Allow square brackets in external issue patterns (go-gitea#3408)
  * Add Attachment API (go-gitea#3478)
  * Add EnableTimetracking option to app settings (go-gitea#3719)
  * Add config option to enable or disable log executed SQL (go-gitea#3726)
  * Shows total tracked time in issue and milestone list (go-gitea#3341)
* TRANSLATION
  * Improve English grammar and consistency (go-gitea#3614)
* DEPLOYMENT
  * Allow Gitea to run as different USER in Docker (go-gitea#3961)
  * Provide compressed release binaries (go-gitea#3991)
  * Sign release binaries (go-gitea#4188)
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
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.

Add time tracking features for project management
8 participants