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

Allow users with explicit read access to give approvals #8382

Merged
merged 2 commits into from Oct 8, 2019

Conversation

@guillep2k
Copy link
Member

commented Oct 5, 2019

Fixes #8297

Currently teams with read-only access can be added as whitelisted approvers, but not users.

This PR corrects that in that it allows the addition of users with read-only access to the whitelisted approvers as long as the user was explicitly added as a collaborator to the repository.

@techknowlogick techknowlogick added this to the 1.10.0 milestone Oct 5, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Oct 5, 2019

Codecov Report

Merging #8382 into master will decrease coverage by 0.01%.
The diff coverage is 26.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8382      +/-   ##
==========================================
- Coverage   41.82%   41.81%   -0.02%     
==========================================
  Files         497      497              
  Lines       65611    65631      +20     
==========================================
+ Hits        27440    27441       +1     
- Misses      34657    34674      +17     
- Partials     3514     3516       +2
Impacted Files Coverage Δ
models/repo.go 48.01% <0%> (-0.18%) ⬇️
routers/repo/setting_protected_branch.go 39.36% <0%> (ø) ⬆️
models/branches.go 48.7% <42.85%> (-0.46%) ⬇️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
models/gpg_key.go 55.83% <0%> (-0.84%) ⬇️
models/repo_list.go 74.14% <0%> (+0.97%) ⬆️

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 736ad8f...484c9fa. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 label Oct 5, 2019
@worthy7

This comment has been minimized.

Copy link

commented Oct 5, 2019

@guillep2k Thanks for this. May I request we make a small text change to the UI so that it's very clear how this works.

  1. Allow only to merge pull request with enough positive reviews of whitelisted users or teams.
    -> Allow merging of pull requests only with enough positive reviews from approvers.
  2. Whitelisted reviewers: -> Approvers:
  3. hint text underneath Contributing teams or users of this repository
  4. Add a checkbox first saying "Use PR Approvals", when unchecked the two options are disabled, when checked, the "number of approvals" is minimum 1 and the input for Approvers should be "required" (input required). Doing a dynamic calculation to see if they match is not worth it since team sizes can change in the future
@guillep2k

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2019

@worthy7 This PR only corrects the inconsistency about allowing individual users as approvers. This will not fix your GUI problem from #8297. I'm sorry I misunderstood your issue back there.

@worthy7

This comment has been minimized.

Copy link

commented Oct 5, 2019

Ah I see you linked them but regardless this PR is still an improvement, I'm grateful for you actively trying to fix things.

My comment above could be just another issue about "interface". Feel free to ignore it and proceed with the PR if you like

@lunny
lunny approved these changes Oct 5, 2019
@guillep2k

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2019

@guillep2k Thanks for this. May I request we make a small text change to the UI so that it's very clear how this works.

1. `Allow only to merge pull request with enough positive reviews of whitelisted users or teams.`
   -> `Allow merging of pull requests only with enough positive reviews from approvers.`

2. `Whitelisted reviewers:` -> `Approvers:`

3. hint text underneath `Contributing teams or users of this repository`

4. Add a checkbox first saying "Use PR Approvals", when unchecked the two options are disabled, when checked, the "number of approvals" is minimum 1 and the input for Approvers should be "required" (input required). Doing a dynamic calculation to see if they match is not worth it since team sizes can change in the future

May be you can PR for that after this is merged? No golang knowledge needed for it.

@worthy7

This comment has been minimized.

Copy link

commented Oct 6, 2019

@worthy7

This comment has been minimized.

Copy link

commented Oct 7, 2019

Took a look, seems Gitea is very unix-only for dev, I'm on Windows and it's a work computer (so I'm not keen on setting up a whole VM dev environment to do it - sorry)
I could edit the files and push and then someone else could simply pull and test it, but that's not going to be as straightforward as me just creating a mock design for someone else to copy. Anyway, I'll open a separate issue.

@6543

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

@lunny add lable backport done?

@techknowlogick techknowlogick merged commit 4843723 into go-gitea:master Oct 8, 2019
2 checks passed
2 checks passed
approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
@guillep2k guillep2k deleted the guillep2k:fix-8297 branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.