Skip to content

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