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

Fix cannot reopen after pushing commits to a closed PR (#23189) #23322

Conversation

sillyguodong
Copy link
Contributor

Backport: #23189
Close: #22784

  1. On GH, we can reopen a PR which was closed before after pushing commits. After reopening PR, we can see the commits that were pushed after closing PR in the time line. So the case of
    issue is a bug which needs to be fixed.

  2. After closing a PR and pushing commits, headBranchSha is not equal to sha(which is the last commit ID string of reference). If the judgement exists, the button of reopen will not display. So, skip the judgement if the status of PR is closed.

image

  1. Even if PR is already close, we should still insert comment record into DB when we push commits.
    So we should still call function CreatePushPullComment().

gitea/services/pull/pull.go

Lines 260 to 282 in 067b0c2

prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repoID, branch)
if err != nil {
log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repoID, branch, err)
return
}
for _, pr := range prs {
log.Trace("Updating PR[%d]: composing new test task", pr.ID)
if pr.Flow == issues_model.PullRequestFlowGithub {
if err := PushToBaseRepo(ctx, pr); err != nil {
log.Error("PushToBaseRepo: %v", err)
continue
}
} else {
continue
}
AddToTaskQueue(pr)
comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID)
if err == nil && comment != nil {
notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment)
}
}
So, I add a switch(includeClosed) to the
GetUnmergedPullRequestsByHeadInfo func to control whether the status of PR must be open. In this case, by setting includeClosed to true, we can query the closed PR.

image

  1. In the loop of comments, I use thelatestCloseCommentID variable to record the last occurrence of the close comment.
    In the go template, if the status of PR is closed, the comments whose type is CommentTypePullRequestPush(29) after latestCloseCommentID won't be rendered.

image e.g.
1). The initial status of the PR is opened.

image 2). Then I click the button of Close. PR is closed now.

image 3). I try to push a commit to this PR, even though its current status is closed.

image But in comments list, this commit do not display.This is as expected :)

image 4). Click the Reopen button, the commit which is pushed after closing PR display now.

image

Close: go-gitea#22784

1. On GH, we can reopen a PR which was closed before after pushing
commits. After reopening PR, we can see the commits that were pushed
after closing PR in the time line. So the case of
[issue](go-gitea#22784) is a bug which
needs to be fixed.

2. After closing a PR and pushing commits, `headBranchSha` is not equal
to `sha`(which is the last commit ID string of reference). If the
judgement exists, the button of reopen will not display. So, skip the
judgement if the status of PR is closed.

![image](https://user-images.githubusercontent.com/33891828/222037529-651fccf9-0bba-433e-b2f0-79c17e0cc812.png)

3. Even if PR is already close, we should still insert comment record
into DB when we push commits.
So we should still call  function `CreatePushPullComment()`.

https://github.com/go-gitea/gitea/blob/067b0c2664d127c552ccdfd264257caca4907a77/services/pull/pull.go#L260-L282
So, I add a switch(`includeClosed`) to the
`GetUnmergedPullRequestsByHeadInfo` func to control whether the status
of PR must be open. In this case, by setting `includeClosed` to `true`,
we can query the closed PR.

![image](https://user-images.githubusercontent.com/33891828/222621045-bb80987c-10c5-4eac-aa0c-1fb9c6aefb51.png)

4. In the loop of comments, I use the`latestCloseCommentID` variable to
record the last occurrence of the close comment.
In the go template, if the status of PR is closed, the comments whose
type is `CommentTypePullRequestPush(29)` after `latestCloseCommentID`
won't be rendered.

![image](https://user-images.githubusercontent.com/33891828/222058913-c91cf3e3-819b-40c5-8015-654b31eeccff.png)
e.g.
1). The initial status of the PR is opened.

![image](https://user-images.githubusercontent.com/33891828/222453617-33c5093e-f712-4cd6-8489-9f87e2075869.png)
2). Then I click the button of `Close`.  PR is closed now.

![image](https://user-images.githubusercontent.com/33891828/222453694-25c588a9-c121-4897-9ae5-0b13cf33d20b.png)
3). I try to push a commit to this PR, even though its current status is
closed.

![image](https://user-images.githubusercontent.com/33891828/222453916-361678fb-7321-410d-9e37-5a26e8095638.png)
But in comments list, this commit do not display.This is as expected :)

![image](https://user-images.githubusercontent.com/33891828/222454169-7617a791-78d2-404e-be5e-77d555f93313.png)
4). Click the `Reopen` button, the commit which is pushed after closing
PR display now.

![image](https://user-images.githubusercontent.com/33891828/222454533-897893b6-b96e-4701-b5cb-b1800f382b8f.png)

---------

Co-authored-by: Lunny Xiao <[email protected]>
@sillyguodong sillyguodong added type/bug lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Mar 6, 2023
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 6, 2023
@yardenshoham yardenshoham removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Mar 6, 2023
@GiteaBot GiteaBot 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 Mar 6, 2023
@delvh delvh added reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Mar 6, 2023
@lunny lunny added this to the 1.18.6 milestone Mar 6, 2023
@jolheiser
Copy link
Member

🎺 🤖

@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Mar 6, 2023
@jolheiser jolheiser merged commit 3dc2724 into go-gitea:release/v1.18 Mar 6, 2023
@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 6, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
@sillyguodong sillyguodong deleted the backport/pr_23189_backport_to_1.18 branch July 11, 2023 08:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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