-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Make branch deletion URL more like GitHub's, fixes #1397 #1994
Make branch deletion URL more like GitHub's, fixes #1397 #1994
Conversation
Maybe add an integration test since merge test is ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this URL change prevent us from being able to support deleting arbitrary branches (even those not associated with any PR) in the future?
routers/repo/pull.go
Outdated
}) | ||
}() | ||
|
||
if !gitRepo.IsBranchExist(pr.HeadBranch) || pr.HeadBranch == "master" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be ctx.Repo.Repository.DefaultBranch
instead of "master"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
routers/repo/pull.go
Outdated
} | ||
|
||
if !ctx.User.IsWriterOfRepo(pr.HeadRepo) { | ||
ctx.Handle(404, "CleanUpPullRequest", nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
403 seems better than 404 here (although I'm no expert, could be wrong).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, 404 is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can see the PR you should get a 403, otherwise 404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will check for PR view rights in routes.go already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then 403 is correct
766b0e4
to
b90755e
Compare
Changed to checking for default repository branch name instead of "master" |
This URL meant to be used only for deleting PR branch and does very specific checks and functionality. For branch deletion from other places it would be better to create new URL with new handler |
When I try to delete the merged pull branch, it shows |
b90755e
to
f985907
Compare
84a1bd0
to
a459f37
Compare
Added integration test does fail without #2007 applied, so it covers both my PR :) |
The close comment and deletion comment is not shown but I think that should be another issue. So I give this LGTM. |
Another thinking: if pull branch is a protected branch? |
integrations/pull_merge_test.go
Outdated
respJSON := &struct { | ||
Redirect string | ||
}{} | ||
json.Unmarshal(resp.Body, respJSON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unchecked error, I'd recommend using the DecodeJSON(..)
helper function here:
DecodeJSON(t, resp, respJSON)
a459f37
to
77617da
Compare
@ethantkoenig fixed |
@lunny added checks to not allow deleting protected branches |
Missing comments are bug introduced in 255adc4 and needs to be fixed in other PR |
55c7128
to
0f64a3e
Compare
@ethantkoenig need your approval |
@lafriks Still need to address #1994 (comment) |
@ethantkoenig ok, done |
LGTM |
@lunny @ethantkoenig I added additional check so that code would not panic if forked repository has been deleted. |
Also fixes broken branch deletion as MergeCommitID is not in HeadBranch so it allways was giving error message that branch has new commits and it can not be deleted.
Fixes #1397