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

Make branch deletion URL more like GitHub's, fixes #1397 #1994

Merged
merged 5 commits into from
Jun 21, 2017
Merged

Make branch deletion URL more like GitHub's, fixes #1397 #1994

merged 5 commits into from
Jun 21, 2017

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Jun 17, 2017 •
edited
Loading

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

@lunny lunny added the type/bug label Jun 17, 2017
@lunny lunny added this to the 1.2.0 milestone Jun 17, 2017
@lunny
Copy link
Member

lunny commented Jun 17, 2017

Maybe add an integration test since merge test is ready.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 17, 2017
ethantkoenig
ethantkoenig suggested changes Jun 17, 2017
View reviewed changes
Copy link
Member

@ethantkoenig ethantkoenig left a 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" {
Copy link
Member

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

Copy link
Member

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)
Copy link
Member

@ethantkoenig ethantkoenig Jun 17, 2017 •
edited
Loading

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

Copy link
Member

Choose a reason for hiding this comment

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

No, 404 is better.

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

Choose a reason for hiding this comment

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

Then 403 is correct

@lafriks lafriks force-pushed the pr_delete_branch_link_change branch from 766b0e4 to b90755e Compare June 18, 2017 18:59
@lafriks
Copy link
Member Author

lafriks commented Jun 18, 2017

Changed to checking for default repository branch name instead of "master"

@lafriks
Copy link
Member Author

lafriks commented Jun 18, 2017

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

@lunny
Copy link
Member

lunny commented Jun 19, 2017

When I try to delete the merged pull branch, it shows lunny/test2 cannot be deleted because it has new commits after merging.

@lafriks
Copy link
Member Author

lafriks commented Jun 19, 2017

@lunny did you do it for fork? Does it works on try gitea? If both answers are yes, than you are hitting this bug #2007

@lafriks lafriks mentioned this pull request Jun 19, 2017
Merged
@lafriks lafriks force-pushed the pr_delete_branch_link_change branch from b90755e to f985907 Compare June 19, 2017 22:32
@lafriks lafriks force-pushed the pr_delete_branch_link_change branch 2 times, most recently from 84a1bd0 to a459f37 Compare June 19, 2017 22:55
@lafriks
Copy link
Member Author

lafriks commented Jun 19, 2017

Added integration test does fail without #2007 applied, so it covers both my PR :)

@lunny
Copy link
Member

lunny commented Jun 20, 2017

The close comment and deletion comment is not shown but I think that should be another issue. So I give this 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 Jun 20, 2017
@lunny
Copy link
Member

lunny commented Jun 20, 2017

Another thinking: if pull branch is a protected branch?

ethantkoenig
ethantkoenig suggested changes Jun 20, 2017
View reviewed changes
integrations/pull_merge_test.go Outdated
respJSON := &struct {
Redirect string
}{}
json.Unmarshal(resp.Body, respJSON)
Copy link
Member

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)

@lafriks lafriks force-pushed the pr_delete_branch_link_change branch from a459f37 to 77617da Compare June 20, 2017 07:09
@lafriks
Copy link
Member Author

lafriks commented Jun 20, 2017

@ethantkoenig fixed

@lafriks
Copy link
Member Author

lafriks commented Jun 20, 2017

@lunny added checks to not allow deleting protected branches

@lafriks
Copy link
Member Author

lafriks commented Jun 20, 2017

Missing comments are bug introduced in 255adc4 and needs to be fixed in other PR

@lafriks lafriks force-pushed the pr_delete_branch_link_change branch from 55c7128 to 0f64a3e Compare June 20, 2017 10:24
@lafriks
Copy link
Member Author

lafriks commented Jun 20, 2017

@ethantkoenig need your approval

@ethantkoenig
Copy link
Member

@lafriks Still need to address #1994 (comment)

@lafriks
Copy link
Member Author

lafriks commented Jun 20, 2017

@ethantkoenig ok, done

@ethantkoenig
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 Jun 20, 2017
@lafriks
Copy link
Member Author

lafriks commented Jun 20, 2017

@lunny @ethantkoenig I added additional check so that code would not panic if forked repository has been deleted.
There are some other places where it does panic but for them I will create new PR

@lunny lunny merged commit 0a5dc64 into go-gitea:master Jun 21, 2017
@lafriks lafriks deleted the pr_delete_branch_link_change branch June 21, 2017 06:09
@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.
Reviewers

@lunny lunny lunny left review comments

@bkcsoft bkcsoft bkcsoft left review comments

@ethantkoenig ethantkoenig ethantkoenig approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

Cannot delete branch with slash in name after PR
5 participants
@lafriks @lunny @ethantkoenig @bkcsoft @tboerger