Skip to content

Conversation

sapk
Copy link
Member

@sapk sapk commented Jul 6, 2017

And enforcement that :orgname is a org by using GetOrgByName inplace of GetUserByName and specific error check (GetOrgByName return ErrOrgNotExist not IsErrUserNotExist ) like it was done in CreateOrgRepo.
Before this PR we could use username as orgname.

This could also be done for enforcement of team in a other PR.

@sapk sapk force-pushed the better-org-err-handling branch from 8fc5644 to 8a97c00 Compare July 6, 2017 11:22
Return new error type
Use good error check
Use new method to check error
Update tests
@sapk sapk force-pushed the better-org-err-handling branch from 8a97c00 to 0d24812 Compare July 6, 2017 11:27
@sapk
Copy link
Member Author

sapk commented Jul 6, 2017

This is ok for review

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 6, 2017
@lunny lunny added this to the 1.2.0 milestone Jul 6, 2017
@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jul 6, 2017
@lunny
Copy link
Member

lunny commented Jul 6, 2017

LGTM

@tboerger tboerger 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 Jul 6, 2017
if err != nil {
if models.IsErrUserNotExist(err) {
if models.IsErrOrgNotExist(err) {
ctx.Status(404)
} else {
ctx.Error(500, "GetUserByName", err)
Copy link
Member

Choose a reason for hiding this comment

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

Please also fix 500 error name

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch

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

lafriks commented Jul 6, 2017

LGTM

@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 Jul 6, 2017
@lunny lunny merged commit 30787e4 into go-gitea:master Jul 6, 2017
@sapk sapk deleted the better-org-err-handling branch July 7, 2017 09:15
@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/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants