Worktime tracking for the organization level.

[kkovacs]

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

[kkovacs]

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]

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]

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]

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!

[kkovacs]

Just two small questions:

1. I'd made the requested changes. Should I request a re-review, or is it enough that they are in the "resolved" state?
2. 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]

Just two small questions:

1. I'd made the requested changes. Should I request a re-review, or is it enough that they are in the "resolved" state?
2. 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]

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

[kkovacs]

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!

[kkovacs]

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

[kkovacs]

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]

Codecov Report

:exclamation: 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]

Tests are failing as you need to make an authenticated request with a user

--- FAIL: TestOrgMembers (1.41s)

    org_test.go:124: 

        	Error Trace:	integration_test.go:506

        	            				org_test.go:124

        	Error:      	Not equal: 

        	            	expected: 200

        	            	actual  : 303

        	Test:       	TestOrgMembers

        	Messages:   	Request: GET /org/org25/members

    testlogger.go:78: 2022/08/11 14:58:27 ...eb/routing/logger.go:99:func1() [I] [62f51913] router: completed GET /org/org25/members for , 303 See Other in 0.4ms @ context/auth.go:28(context.Toggle)

    org_test.go:124: Response: <a href="/user/login">See Other</a>.

        

        

    testlogger.go:78: 2022/08/11 14:58:27 ...eb/routing/logger.go:99:func1() [I] [62f51913-2] router: completed GET /user/login for , 200 OK in 7.8ms @ auth/auth.go:150(auth.SignIn)

    testlogger.go:78: 2022/08/11 14:58:27 ...eb/routing/logger.go:99:func1() [I] [62f51913-3] router: completed POST /user/login for , 303 See Other in 342.8ms @ auth/auth.go:175(auth.SignInPost)

    org_test.go:129: 

        	Error Trace:	integration_test.go:506

        	            				integration_test.go:335

        	            				org_test.go:129

        	Error:      	Not equal: 

        	            	expected: 200

        	            	actual  : 303

        	Test:       	TestOrgMembers

        	Messages:   	Request: GET /org/org25/members

    org_test.go:129: Response: <a href="/user/login">See Other</a>.

        

        

    testlogger.go:78: 2022/08/11 14:58:27 ...eb/routing/logger.go:99:func1() [I] [62f51913-4] router: completed GET /org/org25/members for , 303 See Other in 0.8ms @ context/auth.go:28(context.Toggle)

    testlogger.go:78: 2022/08/11 14:58:27 ...nization/org_user.go:108:loadOrganizationOwners() [E] [62f51913-5] Organization does not have owner team: 25

    testlogger.go:78: 2022/08/11 14:58:27 ...eb/routing/logger.go:99:func1() [I] [62f51913-5] router: completed GET /org/org25/members for , 200 OK in 27.8ms @ org/members.go:25(org.Members)

[kkovacs]

Sorry it took this long to fix this test-fail, it was a bit hard to reproduce on my machine. I learned a lot about drone :) I rebased and hopefully now every test will succeed.

[molaie]

@kkovacs thanks for this great work. keep your excellent work.

[bard]

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

[kkovacs]

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]

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.

[kkovacs]

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]

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]

@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]

The problems I have with time tracking are the following:

1. 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.
2. 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.
3. 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]

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.