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

Integration PR for #6541 #6572

Merged
merged 2 commits into from Apr 11, 2019

Conversation

8 participants
@matthiasb85
Copy link
Contributor

commented Apr 10, 2019

Allow admin users to set a repositories visibility to public, even if FORCE_PRIVATE is to true (#6541)

Signed-off-by: Matthias Beckert beckert.matthias@googlemail.com

Allow admin users to set a repositoires visibility to public, even if…
… FORCE_PRIVATE is to true (#6541)

Signed-off-by: Matthias Beckert <beckert.matthias@googlemail.com>
@codecov-io

This comment has been minimized.

Copy link

commented Apr 10, 2019

Codecov Report

Merging #6572 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6572      +/-   ##
==========================================
+ Coverage   40.35%   40.35%   +<.01%     
==========================================
  Files         405      405              
  Lines       54260    54260              
==========================================
+ Hits        21896    21898       +2     
+ Misses      29349    29347       -2     
  Partials     3015     3015
Impacted Files Coverage Δ
routers/repo/setting.go 10.05% <0%> (ø) ⬆️
models/repo_list.go 67.89% <0%> (+1.05%) ⬆️

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 66fa092...bec3ae1. Read the comment docs.

@zeripath
Copy link
Contributor

left a comment

Although usually I'm against duplication I think this looks cleaner than a 3 way "and".

Of note this allows users to set their repositories to private again if they've been made non-private by an admin but that's probably reasonable.

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Apr 10, 2019

@matthiasb85

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

You are right, the user can always switch back to private. Nevertheless, the switch from private to public can only be performed by an administrator (in case of FORCE_PRIVAT). This is also the case, if the visibility is changed multiple times.

@@ -19,7 +19,11 @@
<div class="inline field">
<label>{{.i18n.Tr "repo.visibility"}}</label>
<div class="ui checkbox">
{{if .IsAdmin}}
<input name="private" type="checkbox" {{if .Repository.IsPrivate}}checked{{end}}>
{{else}}
<input name="private" type="checkbox" {{if .Repository.IsPrivate}}checked{{end}}{{if and $.ForcePrivate .Repository.IsPrivate}} readonly{{end}}>

This comment has been minimized.

Copy link
@sapk

sapk Apr 10, 2019

Member

To follow the same check/logic as in routers/repo/setting.go you just need to add (not .IsAdmin) to the and list.

Suggested change
<input name="private" type="checkbox" {{if .Repository.IsPrivate}}checked{{end}}{{if and $.ForcePrivate .Repository.IsPrivate}} readonly{{end}}>
<input name="private" type="checkbox" {{if .Repository.IsPrivate}}checked{{end}}{{if and $.ForcePrivate .Repository.IsPrivate (not .IsAdmin)}} readonly{{end}}>

This comment has been minimized.

Copy link
@sapk

sapk Apr 10, 2019

Member

I have just seen the comment of @zeripath and I will LGTM as it is not blocking.

@sapk

sapk approved these changes Apr 10, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Apr 10, 2019

@lunny lunny merged commit 5348573 into go-gitea:master Apr 11, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.