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

Add configuration option for default permission to create Organizations #1686

Merged
merged 3 commits into from
May 8, 2017

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented May 5, 2017

Defaults to false

@lunny lunny added the type/enhancement An improvement of existing functionality label May 5, 2017
@lunny lunny added this to the 1.2.0 milestone May 5, 2017
@lunny
Copy link
Member

lunny commented May 6, 2017

LGTM

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 6, 2017
Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding new config variables should not break the old behaviour.

conf/app.ini Outdated
@@ -240,6 +240,9 @@ ENABLE_CAPTCHA = true
; Default value for KeepEmailPrivate
; New user will get the value of this setting copied into their profile
DEFAULT_KEEP_EMAIL_PRIVATE = false
; Default value for AllowCreateOrganization
; New user will get the value of this setting copied into their profile
DEFAULT_ALLOW_CREATE_ORGANIZATION = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default to true otherwise backwards compatability is broken.

@@ -45,12 +45,12 @@
margin: 0;

dd {
margin-left: 240px;
margin-left: 275px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this change necessary? Got any screenshots of how it looks before and after?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise "Default permission to create Organizations" did not if in, it was cut to something like "Default permission to crea..."

}
dt {
font-weight: bolder;
float: left;
width: 250px;
width: 285px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@@ -972,6 +972,7 @@ var Service struct {
EnableReverseProxyAutoRegister bool
EnableCaptcha bool
DefaultKeepEmailPrivate bool
DefaultAllowCreateOrganization bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to default to true to not break the old behaviour

@appleboy
Copy link
Member

appleboy commented May 8, 2017

Please update the default behavior as @bkcsoft suggestions. Otherwise L-G-T-M

@@ -992,6 +993,7 @@ func newService() {
Service.EnableReverseProxyAutoRegister = sec.Key("ENABLE_REVERSE_PROXY_AUTO_REGISTRATION").MustBool()
Service.EnableCaptcha = sec.Key("ENABLE_CAPTCHA").MustBool()
Service.DefaultKeepEmailPrivate = sec.Key("DEFAULT_KEEP_EMAIL_PRIVATE").MustBool()
Service.DefaultAllowCreateOrganization = sec.Key("DEFAULT_ALLOW_CREATE_ORGANIZATION").MustBool()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to set default to true here. Changing app.ini doesn't do anything

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 👍

@lafriks
Copy link
Member Author

lafriks commented May 8, 2017

Changed default to keep backward compatibility but I would like gitea to reconsider more sane default in the future. When adding this setting it should have not introduced such changes in default behavior. I see it could be useful only in deployments where gitea is used as SasS, in private repositories there would mostly also be only one person who will create organizations, in organizations that will do administrators.

@bkcsoft
Copy link
Member

bkcsoft commented May 8, 2017

LGTM

@lafriks
Copy link
Member Author

lafriks commented May 8, 2017

LG-TM, please work :)

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 8, 2017
@bkcsoft bkcsoft merged commit 51d0bec into go-gitea:master May 8, 2017
@lafriks lafriks deleted the no_default_organization_right branch June 21, 2017 06:10
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants