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.

Sign up for GitHub

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

Jump to bottom

Refactor repository transfer #33211

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Refactor repository transfer #33211

wants to merge 12 commits into from
+300 −218

Conversation

lunny
Copy link
Member

@lunny lunny commented Jan 11, 2025 •
edited
Loading

  • Both have RejectTransfer and CancelTransfer because the permission checks are not the same. CancelTransfer can be done by the doer or those who have admin permission to access this repository. RejectTransfer can be done by the receiver user if it's an individual or those who can create repositories if it's an organization.

  • Some tests are wrong, this PR corrects them.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 11, 2025
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 11, 2025
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Jan 11, 2025
@lunny lunny added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Jan 11, 2025
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Jan 11, 2025
@lunny lunny force-pushed the lunny/refactor_transfer branch from d42ace7 to 45ffedd Compare January 13, 2025 04:16
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Jan 13, 2025
@lunny lunny changed the title refactor transfer related code Refactor repository transfer Jan 13, 2025
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 13, 2025
@lunny lunny marked this pull request as ready for review January 13, 2025 21:11
@lunny lunny added this to the 1.24.0 milestone Jan 13, 2025
Zettat123
Zettat123 reviewed Jan 17, 2025
View reviewed changes
routers/api/v1/repo/transfer.go Outdated Show resolved Hide resolved
services/context/repo.go Outdated Show resolved Hide resolved
@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 Jan 20, 2025
@yp05327
Copy link
Contributor

yp05327 commented Jan 22, 2025

It seems that the transfer check don't check the user repo create limitation?
Then I can use another account to create the repo and transfer it to pass the check?

@lunny
Copy link
Member Author

lunny commented Jan 22, 2025

It seems that the transfer check don't check the user repo create limitation? Then I can use another account to create the repo and transfer it to pass the check?

You are right. This is a bug in the original logic. I will send another PR to fix it.

yp05327
yp05327 reviewed Jan 22, 2025
View reviewed changes
models/repo/transfer.go Outdated Show resolved Hide resolved
yp05327
yp05327 reviewed Jan 22, 2025
View reviewed changes
routers/web/repo/repo.go Outdated Show resolved Hide resolved
yp05327
yp05327 reviewed Jan 22, 2025
View reviewed changes
models/repo/transfer.go
Comment on lines +227 to +230
if _, err := user_model.GetUserByID(ctx, newOwner.ID); err != nil {
return err
}

Copy link
Contributor

@yp05327 yp05327 Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that this is unnecessary. If you got a User pointer, it should come from a GetUserByxxx function, so the user exist check is already done before calling this function, and even if newOwner is nil (although it is impossible), this will cause panic.

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 this test introduced a non-exist user. So we need to check it here.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yp05327 yp05327 yp05327 left review comments

@Zettat123 Zettat123 Zettat123 approved these changes

Assignees
No one assigned
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.
4 participants
@lunny @yp05327 @Zettat123 @GiteaBot