Admin teams should get all units regardless of form selection

[adelowo]

Fixes #4771 and kind of related to #4713

[codecov-io]

Codecov Report

Merging #4772 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #4772      +/-   ##
==========================================
- Coverage   37.37%   37.32%   -0.05%     
==========================================
  Files         312      312              
  Lines       46379    46412      +33     
==========================================
- Hits        17332    17323       -9     
- Misses      26567    26606      +39     
- Partials     2480     2483       +3

Impacted Files	Coverage Δ
routers/org/teams.go	0% <0%> (ø)	⬆️
models/repo_indexer.go	44.49% <0%> (-3.82%)	⬇️

---

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 4c1f1f9...11a8aba. Read the comment docs.

[lafriks]

What if you only want team to have admin rights only for issues but still not be able to see code?

[adelowo]

Is that intended behaviour ? If yes, then I can close this PR. Like I mentioned in #4771 , the behaviour I expected was all permissions be granted.

Again, if you select "admin permissions", you cannot select any unit from the UI ( yet it still reads in units from the form )

gitea/routers/org/teams.go

Line 192 in 3c39b63

for _, tp := range form.Units {

Which is what prompted this fix. If this PR is closed and your comment is the expected behaviour, then the UI should allow users still select permissions unit when "admin" is chosen .

[adelowo]

@lafriks Wouldn't that be write permission with only the issue unit ?

[lafriks]

@adelowo not really, IMHO with write permission you can not create new labels/milestones

[adelowo]

Just tested and it turns out with write permissions, you can create both labels and milestones

[adelowo]

@lafriks I think this is a bug since if for example you untick all the units ( with either read/write), and then go on to choose admin permission for the team, the users in the team would be unable to do anything even though they have admin permissions ( technically, admin permissions over nothing )...

If you don't touch any of the units, just select admin permissions, then everything is fine.. There is a reproduction how to in #4771

[lafriks]

Was not that fixed already with #4719 ?

[adelowo]

No, this is a different bug report/fix... Should units given to an admin be read from the form being posted as in

gitea/routers/org/teams.go

Line 192 in 3c39b63

for _, tp := range form.Units {

If I had seen this bug earlier, I wouldn't not have sent #4719 but this instead since we can let other permission types read units from the form and just give the admin team all available units...

The major question is should admins be able to select units or an admin team should get everything ? . If yes, then the UI should take that into consideration and be fixed ( see above screenshots ).

Ideally, I'd want to revert #4719 (this PR already does that anyways) since #4713 is a deeper problem than what it initially looked like.... With it, admins can potentially not have any permissions at all

Were you able to follow the reproduction steps ?

[lunny]

@lafriks #4719 didn't fix the bug I think.

[techknowlogick]

Removing backport/v1.5 as a label because 1.5.3 just went out, and we should focus on getting 1.6 out as stable.

[lunny]

I think this is unnecessary since admin team should have all the units. So save it on database is not a good idea. I think #5314 has fixed almost all the unit permission problem.

[adelowo]

@lunny I will test to make sure it is actually fixed. Closing this by the way