Skip to content

Commit e8e871b

Browse files
sillyguodonglunny
andauthored
Fix cannot reopen after pushing commits to a closed PR (#23189) (#23324)
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](#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]>
1 parent 6be6c19 commit e8e871b

File tree

6 files changed

+30
-16
lines changed

6 files changed

+30
-16
lines changed

Diff for: models/issues/pull_list.go

+9-6
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,16 @@ func listPullRequestStatement(baseRepoID int64, opts *PullRequestsOptions) (*xor
5252

5353
// GetUnmergedPullRequestsByHeadInfo returns all pull requests that are open and has not been merged
5454
// by given head information (repo and branch).
55-
func GetUnmergedPullRequestsByHeadInfo(repoID int64, branch string) ([]*PullRequest, error) {
55+
// arg `includeClosed` controls whether the SQL returns closed PRs
56+
func GetUnmergedPullRequestsByHeadInfo(repoID int64, branch string, includeClosed bool) ([]*PullRequest, error) {
5657
prs := make([]*PullRequest, 0, 2)
57-
return prs, db.GetEngine(db.DefaultContext).
58-
Where("head_repo_id = ? AND head_branch = ? AND has_merged = ? AND issue.is_closed = ? AND flow = ?",
59-
repoID, branch, false, false, PullRequestFlowGithub).
58+
sess := db.GetEngine(db.DefaultContext).
6059
Join("INNER", "issue", "issue.id = pull_request.issue_id").
61-
Find(&prs)
60+
Where("head_repo_id = ? AND head_branch = ? AND has_merged = ? AND flow = ?", repoID, branch, false, PullRequestFlowGithub)
61+
if !includeClosed {
62+
sess.Where("issue.is_closed = ?", false)
63+
}
64+
return prs, sess.Find(&prs)
6265
}
6366

6467
// CanMaintainerWriteToBranch check whether user is a maintainer and could write to the branch
@@ -71,7 +74,7 @@ func CanMaintainerWriteToBranch(p access_model.Permission, branch string, user *
7174
return false
7275
}
7376

74-
prs, err := GetUnmergedPullRequestsByHeadInfo(p.Units[0].RepoID, branch)
77+
prs, err := GetUnmergedPullRequestsByHeadInfo(p.Units[0].RepoID, branch, false)
7578
if err != nil {
7679
return false
7780
}

Diff for: models/issues/pull_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ func TestHasUnmergedPullRequestsByHeadInfo(t *testing.T) {
118118

119119
func TestGetUnmergedPullRequestsByHeadInfo(t *testing.T) {
120120
assert.NoError(t, unittest.PrepareTestDatabase())
121-
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(1, "branch2")
121+
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(1, "branch2", false)
122122
assert.NoError(t, err)
123123
assert.Len(t, prs, 1)
124124
for _, pr := range prs {

Diff for: routers/web/repo/issue.go

+12-5
Original file line numberDiff line numberDiff line change
@@ -1415,11 +1415,12 @@ func ViewIssue(ctx *context.Context) {
14151415
}
14161416

14171417
var (
1418-
role issues_model.RoleDescriptor
1419-
ok bool
1420-
marked = make(map[int64]issues_model.RoleDescriptor)
1421-
comment *issues_model.Comment
1422-
participants = make([]*user_model.User, 1, 10)
1418+
role issues_model.RoleDescriptor
1419+
ok bool
1420+
marked = make(map[int64]issues_model.RoleDescriptor)
1421+
comment *issues_model.Comment
1422+
participants = make([]*user_model.User, 1, 10)
1423+
latestCloseCommentID int64
14231424
)
14241425
if ctx.Repo.Repository.IsTimetrackerEnabled(ctx) {
14251426
if ctx.IsSigned {
@@ -1626,9 +1627,15 @@ func ViewIssue(ctx *context.Context) {
16261627
comment.Type == issues_model.CommentTypeStopTracking {
16271628
// drop error since times could be pruned from DB..
16281629
_ = comment.LoadTime()
1630+
} else if comment.Type == issues_model.CommentTypeClose {
1631+
// record ID of latest closed comment.
1632+
// if PR is closed, the comments whose type is CommentTypePullRequestPush(29) after latestCloseCommentID won't be rendered.
1633+
latestCloseCommentID = comment.ID
16291634
}
16301635
}
16311636

1637+
ctx.Data["LatestCloseCommentID"] = latestCloseCommentID
1638+
16321639
// Combine multiple label assignments into a single comment
16331640
combineLabelComments(issue)
16341641

Diff for: routers/web/repo/pull.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -587,7 +587,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C
587587
ctx.Data["HeadBranchCommitID"] = headBranchSha
588588
ctx.Data["PullHeadCommitID"] = sha
589589

590-
if pull.HeadRepo == nil || !headBranchExist || headBranchSha != sha {
590+
if pull.HeadRepo == nil || !headBranchExist || (!pull.Issue.IsClosed && (headBranchSha != sha)) {
591591
ctx.Data["IsPullRequestBroken"] = true
592592
if pull.IsSameRepo() {
593593
ctx.Data["HeadTarget"] = pull.HeadBranch

Diff for: services/pull/pull.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
257257
// If you don't let it run all the way then you will lose data
258258
// TODO: graceful: AddTestPullRequestTask needs to become a queue!
259259

260-
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repoID, branch)
260+
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repoID, branch, true)
261261
if err != nil {
262262
log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repoID, branch, err)
263263
return
@@ -500,7 +500,7 @@ func (errs errlist) Error() string {
500500

501501
// CloseBranchPulls close all the pull requests who's head branch is the branch
502502
func CloseBranchPulls(doer *user_model.User, repoID int64, branch string) error {
503-
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repoID, branch)
503+
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repoID, branch, false)
504504
if err != nil {
505505
return err
506506
}
@@ -536,7 +536,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re
536536

537537
var errs errlist
538538
for _, branch := range branches {
539-
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repo.ID, branch.Name)
539+
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repo.ID, branch.Name, false)
540540
if err != nil {
541541
return err
542542
}

Diff for: templates/repo/issue/view_content/comments.tmpl

+4
Original file line numberDiff line numberDiff line change
@@ -697,6 +697,10 @@
697697
</span>
698698
</div>
699699
{{else if and (eq .Type 29) (or (gt .CommitsNum 0) .IsForcePush)}}
700+
<!-- If PR is closed, the comments whose type is CommentTypePullRequestPush(29) after latestCloseCommentID won't be rendered. //-->
701+
{{if and .Issue.IsClosed (gt .ID $.LatestCloseCommentID)}}
702+
{{continue}}
703+
{{end}}
700704
<div class="timeline-item event" id="{{.HashTag}}">
701705
<span class="badge">{{svg "octicon-repo-push"}}</span>
702706
<span class="text grey muted-links">

0 commit comments

Comments
 (0)