Worktime tracking for the organization level. #19808

kkovacs · 2022-05-25T21:15:45Z

Dear Gitea team,

first of all, thanks for the great work you're doing with this project.

I'm planning to introduce Gitea at a client site, and noticed that while there is time recording, there are no project-manager-friendly reports to actually make use of that data, as were also mentioned by others in #4870 #8684 and #13531.

Since I had a little time last weekend, I had put together something that I hope to be a useful contribution to this great project (while of course useful for me too).

This PR adds a new "Worktime" tab to the Organisation level. There is a date range selector (by default set to the current month), and there are three possible views:

by repository,
by milestone, and
by team member.

Happy to receive any feedback!

There are several possible future improvements of course (predefined date ranges, charts, a member time sheet, matrix of repos/members, etc) but I hope that even in this relatively simple state this would be useful to lots of people.

Keep up the good work!

Kristof

lafriks

Thanks for PR! Looks beautiful :)

All queries must be rewritten using xorm query builder and need to add filter to calculate and show only data from repositories user has access to (with issue/pr rights?). We should have function that return such conditions imho

kkovacs · 2022-05-26T18:40:19Z

Thanks for PR! Looks beautiful :)

Thank you, lafriks :) 😊

All queries must be rewritten using xorm query builder

OK, if I must :) I'll do it when I'll have some time. Hopefully within a week.

and need to add filter to calculate and show only data from repositories user has access to (with issue/pr rights?). We should have function that return such conditions imho

Hmm, currently the whole functionality is restricted to organization "owners" (by checking ctx.Org.IsOwner in parseTimes), and (according to the help text) "Owners have full access to all repositories and have administrator access to the organization." This is why there is no separate rights checking in the query.

My thinking was that I would rather NOT let people see partial (invalid) data, since the risk that such invalid data gets sent out in an invoice etc by somebody who don't have full rights to see everything within an organization is too high.

So I'm double-checking with you: would you consider the current "limited to owners" restriction enough, or still want additional permission check in the query?

Gusted · 2022-05-26T19:38:41Z

So I'm double-checking with you: would you consider the current "limited to owners" restriction enough, or still want additional permission check in the query?

The permission checking shouldn't be done by a helper helper, this should rather be done at router-level. So in this case you want to move the three lines in web.go to the group that starts from line 640, which already includes the owner checking.

lafriks · 2022-05-26T20:15:17Z

As @Gusted said it's in the wrong place and that's why I did not notice that it's only for owner team members.

Yes you understand correctly owner users will have full access to all org repositories.

If we leave it that only owners have access to this than it does not need that check in queries.

My use case would be that owner would be either lead developer of the team or devops team. Project manager who would be interested in this tab usually have read rights to all repos (and write rights to issues) thus he would not see this. But that's totally I don't mind seeing done in the future as other PR to improve this feature.

kkovacs · 2022-05-26T21:18:44Z

Thanks for the laser-precise advice, @Gusted, will do!

I totally see your point @lafriks. If (as you say) it's OK to deal with the more precise permissions in a future PR, then for simplicity's sake I'd rather leave this particular PR as "owner-only" (but change it of course as per Gusted's recommendation).

I will push later!

routers/web/org/times.go

kkovacs · 2022-06-05T23:12:08Z

Just two small questions:

I'd made the requested changes. Should I request a re-review, or is it enough that they are in the "resolved" state?
I've been looking at how you guys implement tests, and I think I could add tests to this functionality if I do some related refactoring for testability. Should I just add the new commits to this PR, or should this be a separate PR later?

Thanks in advance!

lunny · 2022-06-06T01:54:01Z

Just two small questions:

I'd made the requested changes. Should I request a re-review, or is it enough that they are in the "resolved" state?

I've been looking at how you guys implement tests, and I think I could add tests to this functionality if I do some related refactoring for testability. Should I just add the new commits to this PR, or should this be a separate PR later?

Thanks in advance!

Since this PR sent after v1.17 feature freezed, let's wait to merge after v1.17 release. Of course, I think we can re-review it at any time when maintainers have free time.

kkovacs · 2022-06-06T10:19:07Z

Thanks, I'll be requesting re-review then. (Github is a bit confusing for me regarding the right etiquette and the request UI)

kkovacs · 2022-06-13T15:10:17Z

Dear @lafriks and @Gusted, I just couldn't bear not having tests on the correctness of the queries, so I refactored a bit and added tests. Hope you don't mind.

P.s.: The build failed with "https://registry-1.docker.io/v2/woodpeckerci/plugin-codecov/manifests/next-alpine": received unexpected HTTP status: 500 Internal Server Error, which sounds like a docker registry issue? Can the build be restarted?

P.s.2: I think Github erroneously keeps "1 change requested" on this ticket, because the change was done – maybe it was confused by the rebase at the same time?

Anyway, I will leave this PR alone for now -- feel free to review (all change requests were completed) and to merge at the right time!

Gusted

Please make sure you have translated words and use the correct keys for those. The HTML part of this PR really need some tidying up to do.

modules/util/sec_to_hour.go

Gusted · 2022-06-17T19:35:05Z

routers/web/org/times.go

+	Name    string `xorm:"name"`
+	SumTime int64  `xorm:"sumtime"`
+}
+
+// resultTimesByMilestones is a struct for DB query results
+type resultTimesByMilestones struct {
+	RepoName     string `xorm:"reponame"`
+	Name         string `xorm:"name"`
+	ID           string `xorm:"id"`
+	SumTime      int64  `xorm:"sumtime"`
+	HideRepoName bool   `xorm:"-"`
+}
+
+// resultTimesByMembers is a struct for DB query results
+type resultTimesByMembers struct {
+	Name    string `xorm:"name"`
+	SumTime int64  `xorm:"sumtime"`


Usually you only use xorm tags for struct that will be stored into the database. Xorm should be able to figure out by itself which row corresponds to a field(if not, then add a xorm tag).

Changed. To let Xorm figure out by itself, the query builder part was also touched (sumtime => sum_time, so it recognizes SumTime, etc.

Gusted · 2022-06-17T19:39:43Z

routers/web/org/times.go

+	ctx.Data["RangeTo"] = to
+
+	// Prepare unix time values for SQL
+	gfrom, err := time.Parse("2006-01-02", from)


what does the g represent in the variable?

It stands for "Go", as in Go's Time object. The variable named from above is of String type, and here we convert to Go's Time. It's a temporary variable that is needed for the handling of err from the Parse function. Changed it to from2.

routers/web/org/times.go

templates/org/times/daterange.tmpl

templates/org/times/submenu.tmpl

kkovacs · 2022-06-27T13:48:01Z

Thanks for the detailed review - I haven't disappeared, I'll do the requested changes when I have some time!

kkovacs · 2022-07-06T21:49:39Z

Dear @Gusted, I've finally got around to make the changes you requested.

It's not completely clear to me whether it should be me or you who "resolve" the conversations, so I erred on the side of "resolving" the trivial ones, and leaving open the ones where you might want to see the responses.

One particular thing: please review my use of context.OrgAssignment for the m.Group in routers.

codecov-commenter · 2022-07-07T14:33:45Z

Codecov Report

❗ No coverage uploaded for pull request base (main@3c6c150). Click here to learn what that means.
The diff coverage is 46.20%.

@@           Coverage Diff           @@
##             main   #19808   +/-   ##
=======================================
  Coverage        ?   46.91%           
=======================================
  Files           ?      975           
  Lines           ?   135113           
  Branches        ?        0           
=======================================
  Hits            ?    63394           
  Misses          ?    63960           
  Partials        ?     7759

Impacted Files	Coverage Δ
modules/templates/helper.go	`45.86% <0.00%> (ø)`
routers/web/org/times.go	`39.13% <39.13%> (ø)`
modules/util/sec_to_hour.go	`100.00% <100.00%> (ø)`
routers/web/web.go	`86.61% <100.00%> (ø)`

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 3c6c150...3805fba. Read the comment docs.

Gusted · 2022-07-09T04:44:19Z

routers/web/org/times.go

+
+// parseTimes contains functionality that is required in all these functions,
+// like parsing the date from the request, setting default dates, etc.
+func parseTimes(ctx *context.Context) (unixfrom, unixto int64, err error) {


parseOrgTimes.

Gusted · 2022-07-09T04:44:28Z

routers/web/org/times.go

+func parseTimes(ctx *context.Context) (unixfrom, unixto int64, err error) {
+	// View variables
+	ctx.Data["PageIsOrgTimes"] = true
+	ctx.Data["AppSubURL"] = setting.AppSubURL


This shouldn't be set by this function.

Gusted · 2022-07-09T04:49:24Z

routers/web/org/times.go

+	// Show only the first RepoName, for nicer output.
+	prevreponame := ""
+	for i := 0; i < len(results); i++ {
+		res := &results[i]
+		if prevreponame == res.RepoName {
+			res.HideRepoName = true
+		}
+		prevreponame = res.RepoName
+	}


This should be moved outside this function.

Gusted · 2022-07-09T04:50:10Z

routers/web/org/times.go

+}
+
+// getTimesByRepos fetches data from DB to serve TimesByRepos.
+func getTimesByRepos(unixfrom, unixto, orgid int64) (results []resultTimesByRepos, err error) {


These kind of functions lives in models/ directory. routers/ is just connecting all the functions and do the nice things with the data.

Gusted · 2022-07-09T04:51:20Z

routers/web/web.go

+				m.Get("/by_repos", org.TimesByRepos)
+				m.Get("/by_members", org.TimesByMembers)
+				m.Get("/by_milestones", org.TimesByMilestones)
+			}, context.OrgAssignment(true, true))


Suggested change

}, context.OrgAssignment(true, true))

}, context.OrgAssignment(false, true))

Only org admin should be able to see it right?

Gusted · 2022-07-09T04:57:30Z

templates/org/menu.tmpl

@@ -21,6 +21,11 @@
 					<div class="ui black label">{{.NumTeams}}</div>
 				{{end}}
 			</a>
+			{{if .IsOrganizationOwner}}
+				<a class="{{if $.PageIsOrgTimes}}active{{end}} item" href="{{$.OrgLink}}/times/by_repos">
+					{{svg "octicon-clock"}}&nbsp;{{$.i18n.Tr "org.worktime"}}


Please change all occurences of $.i18n.Tr to $.locale.Tr

models/organization/org_time/org_times_test.go

models/organization/org_times.go

routers/web/org/times.go

bard · 2023-02-05T11:47:31Z

This looks mighty useful! Other than rebasing, are the changes required at this point only style-related?

kkovacs · 2023-02-16T09:50:03Z

This looks mighty useful! Other than rebasing, are the changes required at this point only style-related?

Thank you for the kind words, @bard! :) Nice comments like yours keep open source projects alive.

But, unfortunately, no. What is asked is another (the 5th) refactoring of the code. Also, the main branch in the meantime has diverged enough that rebasing the code is now at least half-day task in itself. It would be easier to integrate the whole stuff again. (I did try a quick rebase attempt before writing this, to see how much work was needed.)

Now, it's normal to have 1-2 rounds of change requests on a PR -- it is very important to keep the structure of a project consistent, enforce style so it's easy for everyone to touch each part, etc.

The problem is that when there were already 5 rounds, AND each round is asking for NEW things to change which were NOT mentioned before, I see no point in doing the integration work again, since there is no guarantee that it will not be in vain again. I unfortunately don't have THIS much free time on my hand. :( I've already spent about four times as much time complying with review requests as on the original feature itself...

The code still stands contributed, if someone is willing to integrate it in a way that satisfies the code stewards, that's great, I would be the most happy -- I too, still believe that this would increase the value of gitea a lot. I also had other project management centric ideas to expand on this feature that I planned to contribute in later separate PRs, but I wanted to keep it simple first. It didn't really work out :)

Sorry.

lunny · 2023-02-16T11:06:20Z

This looks mighty useful! Other than rebasing, are the changes required at this point only style-related?

Thank you for the kind words, @bard! :) Nice comments like yours keep open source projects alive.

But, unfortunately, no. What is asked is another (the 5th) refactoring of the code. Also, the main branch in the meantime has diverged enough that rebasing the code is now at least half-day task in itself. It would be easier to integrate the whole stuff again. (I did try a quick rebase attempt before writing this, to see how much work was needed.)

Now, it's normal to have 1-2 rounds of change requests on a PR -- it is very important to keep the structure of a project consistent, enforce style so it's easy for everyone to touch each part, etc.

The problem is that when there were already 5 rounds, AND each round is asking for NEW things to change which were NOT mentioned before, I see no point in doing the integration work again, since there is no guarantee that it will not be in vain again. I unfortunately don't have THIS much free time on my hand. :( I've already spent about four times as much time complying with review requests as on the original feature itself...

The code still stands contributed, if someone is willing to integrate it in a way that satisfies the code stewards, that's great, I would be the most happy -- I too, still believe that this would increase the value of gitea a lot. I also had other project management centric ideas to expand on this feature that I planned to contribute in later separate PRs, but I wanted to keep it simple first. It didn't really work out :)

Sorry.

Thanks for your time! What a pity this cannot be merged ASAP. I wish someone could pick it and continue.

…first parameter of the function.

kkovacs · 2023-02-16T14:14:12Z

OK, I'm giving this another try, because you guys were so nice. I've rebased it, implemented the last change request, and the build is passing.

If there are any more change requests, say so, but please try to ask in one batch, not with a thousand cuts, please? Thanks :)

bauermarkus · 2023-07-27T15:46:02Z

OK, I'm giving this another try, because you guys were so nice. I've rebased it, implemented the last change request, and the build is passing.

If there are any more change requests, say so, but please try to ask in one batch, not with a thousand cuts, please? Thanks :)

Thank you a lot for your work and your effort! That's such a wonderful feature!

bard · 2023-08-06T09:30:12Z

@lunny @delvh (paging because I see you in the PR, sorry if you're not the ones I should @): looking at this and #23113, both in limbo, it seems that time tracking in general in Gitea is on hold. Obviously I'm among those here who believe that small additions in this area would yield disproportionately high end user value — teams wouldn't have to find, integrate, and maintain a separate time tracking solution — but I respect that no software can be everything to everyone.

Any chance we could have the maintaners' view on the time tracking story in Gitea? At least we can let the wish go for good if necessary.

delvh · 2023-08-06T09:57:48Z

The problems I have with time tracking are the following:

I can see the benefit of allowing it for Gitea. However, if it is introduced without a clear roadmap of how it interacts with everything, it will simply be a burden. As @jolheiser would put it, before the whole functionality and its roadmap go through an RFC process (which we haven't established yet, so it would mostly be a long issue discussion at the moment), I don't think it's a good idea to simply merge it. It will create too much technical debt.
It is quite a struggle between keeping Gitea un-bloated and adding new features. While we mostly answer it with let's add it for any git/issue-related question, project-management features are another thing. While I too would like a better project management integration as GitHub does, I also know that it will drastically change the focus of Gitea, and thus be a lot of work.
As you can see, we currently have 2000 issues and 170 PRs open. Many good features are already ignored, simply because we cannot keep up with how much work there is to do. Or as I call it: The void of ignored PRs starting on page 2. The result of that is in praxis that larger PRs tend to be forgotten, especially if they introduce a not well-thought-out feature. No one wants to review that, there are more interesting/new/urgent PRs to review.

bard · 2023-08-06T10:48:23Z

That's fair. You see value in this (ancillary, I agree) area, but there are more urgent issues from the core area, and the lack of RFC process makes it difficult to achieve a well-integrated change, and the size of proposed changes makes it difficult to assess for impact and review.

I guess that for things to progress in the short term it would take either someone who has both a good understanding of the code base and a carefully thought-out story for project management that maintainers agree with (unlikely, as that sounds more like a core contributor and core contributors already have their hands full), or changes that are tiny and incremental enough that are easy to review and assess as well-fitting (I don't know if features in this area can actually be shaped this way).

Not great news obviously it helps a lot with setting expectations, so thank you for chiming in.

kkovacs marked this pull request as draft May 25, 2022 21:19

kkovacs marked this pull request as ready for review May 25, 2022 21:38

lunny added this to the 1.18.0 milestone May 26, 2022

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label May 26, 2022

lafriks requested changes May 26, 2022

View reviewed changes

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

kkovacs marked this pull request as draft May 26, 2022 21:22

kkovacs force-pushed the feature/worktime-for-org branch from 28cde92 to 44669f4 Compare May 26, 2022 21:44

lafriks reviewed May 26, 2022

View reviewed changes

routers/web/org/times.go Outdated Show resolved Hide resolved

kkovacs marked this pull request as ready for review May 26, 2022 22:05

Gusted reviewed May 27, 2022

View reviewed changes

routers/web/org/times.go Outdated Show resolved Hide resolved

routers/web/org/times.go Outdated Show resolved Hide resolved

kkovacs requested a review from lafriks June 6, 2022 10:16

kkovacs requested a review from Gusted June 6, 2022 10:19

Gusted reviewed Jun 17, 2022

View reviewed changes

kkovacs requested a review from Gusted July 7, 2022 14:51

Gusted reviewed Jul 9, 2022

View reviewed changes

kkovacs marked this pull request as draft August 10, 2022 18:28

kkovacs force-pushed the feature/worktime-for-org branch 2 times, most recently from 9f70fae to eef5842 Compare August 11, 2022 14:25

lunny reviewed Sep 19, 2022

View reviewed changes

models/organization/org_time/org_times_test.go Outdated Show resolved Hide resolved

lunny reviewed Sep 19, 2022

View reviewed changes

models/organization/org_times.go Show resolved Hide resolved

kkovacs force-pushed the feature/worktime-for-org branch from e4847ac to 53b460b Compare September 21, 2022 19:47

kkovacs force-pushed the feature/worktime-for-org branch from 53b460b to be5783e Compare October 19, 2022 16:44

lunny reviewed Oct 22, 2022

View reviewed changes

routers/web/org/times.go Outdated Show resolved Hide resolved

lunny modified the milestones: 1.18.0, 1.19.0 Oct 26, 2022

jolheiser modified the milestones: 1.19.0, 1.20.0 Jan 31, 2023

rebase of earlier feature/worktime-for-org branch

108705c

user added 2 commits February 16, 2023 13:14

Code review change implemented: The Organization object could be the …

851fd08

…first parameter of the function.

Updated CSS class on elements to get back the right look.

bd60f63

kkovacs force-pushed the feature/worktime-for-org branch from f041678 to bd60f63 Compare February 16, 2023 13:19

kkovacs requested review from lunny and Gusted and removed request for Gusted and lunny February 16, 2023 17:03

This was referenced Feb 26, 2023

Issue time estimate, meaningful time tracking #23112

Open

Issue time estimate, meaningful time tracking #23113

Open

delvh removed this from the 1.20.0 milestone Jun 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Worktime tracking for the organization level. #19808

Worktime tracking for the organization level. #19808

kkovacs commented May 25, 2022

lafriks left a comment •

edited

Loading

kkovacs commented May 26, 2022

Gusted commented May 26, 2022

lafriks commented May 26, 2022

kkovacs commented May 26, 2022

kkovacs commented Jun 5, 2022

lunny commented Jun 6, 2022

kkovacs commented Jun 6, 2022

kkovacs commented Jun 13, 2022

Gusted left a comment

Gusted Jun 17, 2022

kkovacs Jul 6, 2022

Gusted Jun 17, 2022

kkovacs Jul 6, 2022

kkovacs commented Jun 27, 2022

kkovacs commented Jul 6, 2022

codecov-commenter commented Jul 7, 2022

Gusted Jul 9, 2022

Gusted Jul 9, 2022

Gusted Jul 9, 2022

Gusted Jul 9, 2022

Gusted Jul 9, 2022

Gusted Jul 9, 2022

bard commented Feb 5, 2023

kkovacs commented Feb 16, 2023

lunny commented Feb 16, 2023

kkovacs commented Feb 16, 2023

bauermarkus commented Jul 27, 2023

bard commented Aug 6, 2023

delvh commented Aug 6, 2023 •

edited

Loading

bard commented Aug 6, 2023

	}, context.OrgAssignment(true, true))
	}, context.OrgAssignment(false, true))

Worktime tracking for the organization level. #19808

Are you sure you want to change the base?

Worktime tracking for the organization level. #19808

Conversation

kkovacs commented May 25, 2022

lafriks left a comment • edited Loading

Choose a reason for hiding this comment

kkovacs commented May 26, 2022

Gusted commented May 26, 2022

lafriks commented May 26, 2022

kkovacs commented May 26, 2022

kkovacs commented Jun 5, 2022

lunny commented Jun 6, 2022

kkovacs commented Jun 6, 2022

kkovacs commented Jun 13, 2022

Gusted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkovacs commented Jun 27, 2022

kkovacs commented Jul 6, 2022

codecov-commenter commented Jul 7, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bard commented Feb 5, 2023

kkovacs commented Feb 16, 2023

lunny commented Feb 16, 2023

kkovacs commented Feb 16, 2023

bauermarkus commented Jul 27, 2023

bard commented Aug 6, 2023

delvh commented Aug 6, 2023 • edited Loading

bard commented Aug 6, 2023

lafriks left a comment •

edited

Loading

delvh commented Aug 6, 2023 •

edited

Loading