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

Fix default for allowing new organization creation for new users #7017

Merged
merged 3 commits into from
May 24, 2019

Conversation

jpicht
Copy link
Contributor

@jpicht jpicht commented May 22, 2019

When creating users DefaultAllowCreateOrganization was ignored.

This is simple two-line fix for issue #6542.

When creating users DefaultAllowCreateOrganization was ignored.

Signed-off-by: Julian Picht <julian.picht@gmail.com>
@zeripath
Copy link
Contributor

zeripath commented May 22, 2019

~~Looking at the code I think these two settings need to be coalesced. It doesn't appear that DEFAULT_ALLOW_CREATE_ORGANIZATION is wired into anything at all. ~~

@zeripath
Copy link
Contributor

zeripath commented May 22, 2019

We should probably just kill DEFAULT_ALLOW_CREATE_ORGANIZATION let's just rewire it back in and fix the test

@jpicht
Copy link
Contributor Author

jpicht commented May 22, 2019

I can certainly do that as well. Should there be a warning if somebody has it set in the config file? Is there a place to add a message to, so it will end up in the release notes?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 22, 2019
@zeripath
Copy link
Contributor

zeripath commented May 22, 2019

Hmm... let's just fix the test. I think that's the correct thing to do.

Make line 263:

gitea/models/user_test.go

Lines 262 to 264 in 6eb53ac

}
for _, v := range tt {

setting.Service.DefaultAllowCreateOrganization = true

Signed-off-by: Julian Picht <julian.picht@gmail.com>
@jpicht
Copy link
Contributor Author

jpicht commented May 23, 2019

I ran the drone test locally before creating this pull request. It seems the test did not fail for me. What did I do wrong? The output can be found here: https://gist.github.com/jpicht/6337485ab466776fded3ee22089d45dc

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 23, 2019
@zeripath
Copy link
Contributor

zeripath commented May 23, 2019

You didn't run the unit tests - that is make test - your logs indicate you only ran the integration tests

@codecov-io
Copy link

Codecov Report

Merging #7017 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7017      +/-   ##
==========================================
- Coverage    41.5%   41.48%   -0.02%     
==========================================
  Files         440      440              
  Lines       59453    59452       -1     
==========================================
- Hits        24675    24663      -12     
- Misses      31558    31570      +12     
+ Partials     3220     3219       -1
Impacted Files Coverage Δ
models/user.go 50.88% <100%> (-0.05%) ⬇️
models/unit.go 62.16% <0%> (-5.41%) ⬇️
modules/log/event.go 64.46% <0%> (-1.53%) ⬇️
models/repo_list.go 72.08% <0%> (-1.02%) ⬇️
routers/repo/view.go 42.02% <0%> (-1.02%) ⬇️

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 6eb53ac...7301c69. Read the comment docs.

@jpicht
Copy link
Contributor Author

jpicht commented May 23, 2019

You didn't run the unit tests - that is make test - your logs indicate you only ran the integration tests

Thanks! That step should probably be added to the contributors guide.

@GiteaBot GiteaBot 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 24, 2019
@lafriks
Copy link
Member

lafriks commented May 24, 2019

Make lgtm work

@lafriks lafriks merged commit 8cd4c22 into go-gitea:master May 24, 2019
@lafriks lafriks changed the title FIX issue #6542 Fix default for allowing new organization creation for new users May 24, 2019
@lafriks
Copy link
Member

lafriks commented May 24, 2019

Please send backport to release/v1.8 branch

lafriks pushed a commit that referenced this pull request May 24, 2019
…) (#7034)

* FIX issue 6542

When creating users DefaultAllowCreateOrganization was ignored.

Signed-off-by: Julian Picht <julian.picht@gmail.com>

* fix TestCreateUser_Issue5882

Signed-off-by: Julian Picht <julian.picht@gmail.com>
@lafriks lafriks added the backport/done All backports for this PR have been created label May 24, 2019
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
…gitea#7017)

Fixed go-gitea#6542

When creating users DefaultAllowCreateOrganization was ignored.

Signed-off-by: Julian Picht <julian.picht@gmail.com>

* fix TestCreateUser_Issue5882

Signed-off-by: Julian Picht <julian.picht@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants