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 checkingctx.Org.IsOwner inparseTimes), 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?

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.

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.

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 willpush later!

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!

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.

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

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 changewas done – maybe it was confused by therebase 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!

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

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 ofcontext.OrgAssignment for them.Group in routers.

Codecov Report

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

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

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update3c6c150...3805fba. Read thecomment docs.

Made some changes, and the UI is kept as before

After fixing some problems (mismatched HTML tags, different SQL syntaxes), I think it is good enough now.

CI passes.