Skip to content

Fix error when add user has full name to team #2973

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

Merged
merged 3 commits into from
Nov 26, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 26, 2017

Will fix #2962.

@lunny lunny added this to the 1.4.0 milestone Nov 26, 2017
@codecov-io
Copy link

codecov-io commented Nov 26, 2017

Codecov Report

Merging #2973 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2973      +/-   ##
=========================================
- Coverage   26.82%   26.8%   -0.02%     
=========================================
  Files          90      90              
  Lines       17807   17807              
=========================================
- Hits         4777    4774       -3     
- Misses      12351   12353       +2     
- Partials      679     680       +1
Impacted Files Coverage Δ
modules/process/manager.go 69.56% <0%> (-4.35%) ⬇️

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 16c55e1...6a49159. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 26, 2017
@@ -76,6 +77,9 @@ func TeamsAction(ctx *context.Context) {
return
}
uname := ctx.Query("uname")
if strings.Contains(uname, "(") && strings.HasSuffix(uname, ")") {
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused as to why this helps. If uname has parentheses, then it is not a valid username, so GetUserByName(name) in line 84 will just return a ErrUserNotExist, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ethantkoenig Yes, that's what #2973 described.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because if some user has set FullName, the name submitted will be loginname (FullName) but not loginname, so it will return ErrUserNotExist. This PR will accept both two kinds requests.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Could we please add a comment explaining the situation?

// uname may be formatted as "username (fullname)"

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.

@ethantkoenig
Copy link
Member

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 Nov 26, 2017
@appleboy
Copy link
Member

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 Nov 26, 2017
@lafriks lafriks merged commit 061c501 into go-gitea:master Nov 26, 2017
@lunny lunny deleted the lunny/fix_add_team branch November 26, 2017 08:23
lunny added a commit to lunny/gitea that referenced this pull request Nov 26, 2017
* fix error when add user has full name to team

* add comment for extra uname check
@lafriks lafriks added the backport/done All backports for this PR have been created label Nov 26, 2017
lafriks pushed a commit that referenced this pull request Nov 26, 2017
* fix error when add user has full name to team

* add comment for extra uname check
@lunny
Copy link
Member Author

lunny commented Nov 27, 2017

also fixed #2726

@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
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.

Cannot add user to organization
6 participants