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

Allow adding collaborators with (fullname) #3103

Merged
merged 3 commits into from
Dec 7, 2017

Conversation

svarlamov
Copy link
Contributor

Signed-off-by: Sasha Varlamov [email protected]

Targeting: #3102

Perhaps we should fork this out into a utility func? I'm pretty new to Gitea, so I just in-lined it same as #2973 but since this keeps popping up, I suppose we might want a shared function for this... Where should I put it and then refactor the #2973 fix?

@codecov-io
Copy link

codecov-io commented Dec 6, 2017

Codecov Report

Merging #3103 into master will decrease coverage by 0.02%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3103      +/-   ##
==========================================
- Coverage   34.08%   34.05%   -0.03%     
==========================================
  Files         273      274       +1     
  Lines       40018    40019       +1     
==========================================
- Hits        13639    13628      -11     
- Misses      24443    24453      +10     
- Partials     1936     1938       +2
Impacted Files Coverage Δ
routers/org/teams.go 0% <0%> (ø) ⬆️
routers/repo/setting.go 4.57% <0%> (ø) ⬆️
routers/utils/utils.go 100% <100%> (ø)
models/repo_indexer.go 45.54% <0%> (-5.45%) ⬇️
models/repo_list.go 65.62% <0%> (-1.57%) ⬇️
models/repo.go 38.15% <0%> (-0.19%) ⬇️

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 7ec6cdd...61465a7. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 6, 2017
@lunny lunny added this to the 1.4.0 milestone Dec 7, 2017
@lunny
Copy link
Member

lunny commented Dec 7, 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 Dec 7, 2017
@@ -367,6 +367,10 @@ func Collaboration(ctx *context.Context) {
// CollaborationPost response for actions for a collaboration of a repository
func CollaborationPost(ctx *context.Context) {
name := strings.ToLower(ctx.Query("collaborator"))
// name may be formatted as "username (fullname)"
if strings.Contains(name, "(") && strings.HasSuffix(name, ")") {
Copy link
Member

Choose a reason for hiding this comment

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

nit: can simplify to

if index := strings.Index(name, " ("); index >= 0 {
	name = name[:index]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just cut-n-pasted @lunny's fix. I agree with you on this. Where can I put a utility method and then refactor all of the usages? Sorry new to Gitea, so want to make sure I'm following your guys' best practices... Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I think adding a routers/utils/utils.go file and putting it there makes the most sense. This proposed function seems specific to route handlers, so I'd prefer to not add it to modules/util.

@svarlamov
Copy link
Contributor Author

So I moved it out into a new utils pkg within routers. I also refactored the two usages within routers/org/teams.go and routers/repo/setting.go. I'm sure that there are probably more places that need to have this added, but maybe that's a separate PR. Let's get this merged because it's a pretty big issue for users...

@ethantkoenig
Copy link
Member

LGTM. Thank you for adding tests!

@lunny
Copy link
Member

lunny commented Dec 7, 2017

make L-G-T-M 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 Dec 7, 2017
@lunny lunny merged commit 311c83a into go-gitea:master Dec 7, 2017
@lunny
Copy link
Member

lunny commented Dec 7, 2017

Please send a back port to branch release/v1.3

@svarlamov
Copy link
Contributor Author

@lunny So I should file another PR against that rel branch? New to the Gitea standards here :)

@ethantkoenig
Copy link
Member

@svarlamov Yes, see #3106 as an example of a backport.

@lunny
Copy link
Member

lunny commented Dec 7, 2017

@svarlamov
I just finished the back port locally:

git fetch release/v1.3
git co release/v1.3
git co -b lunny/fix_xxx
git cherry-pick xxxx
......

lafriks pushed a commit to lafriks-fork/gitea that referenced this pull request Dec 12, 2017
* Allow adding collaborators with (fullname)

Signed-off-by: Sasha Varlamov <[email protected]>

* Refactor username suffix to utils pkg

Signed-off-by: Sasha Varlamov <[email protected]>
@lafriks lafriks added the backport/done All backports for this PR have been created label Dec 12, 2017
appleboy pushed a commit that referenced this pull request Dec 12, 2017
* Allow adding collaborators with (fullname)

Signed-off-by: Sasha Varlamov <[email protected]>

* Refactor username suffix to utils pkg

Signed-off-by: Sasha Varlamov <[email protected]>
@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.

6 participants