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

Merged
merged 10 commits into from
Mar 3, 2023
15 changes: 9 additions & 6 deletions models/issues/pull_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,16 @@ func listPullRequestStatement(baseRepoID int64, opts *PullRequestsOptions) (*xor

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

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

prs, err := GetUnmergedPullRequestsByHeadInfo(p.Units[0].RepoID, branch)
prs, err := GetUnmergedPullRequestsByHeadInfo(p.Units[0].RepoID, branch, false)
if err != nil {
return false
}
Expand Down
2 changes: 1 addition & 1 deletion models/issues/pull_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func TestHasUnmergedPullRequestsByHeadInfo(t *testing.T) {

func TestGetUnmergedPullRequestsByHeadInfo(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(1, "branch2")
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(1, "branch2", false)
assert.NoError(t, err)
assert.Len(t, prs, 1)
for _, pr := range prs {
Expand Down
17 changes: 12 additions & 5 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1420,11 +1420,12 @@ func ViewIssue(ctx *context.Context) {
}

var (
role issues_model.RoleDescriptor
ok bool
marked = make(map[int64]issues_model.RoleDescriptor)
comment *issues_model.Comment
participants = make([]*user_model.User, 1, 10)
role issues_model.RoleDescriptor
ok bool
marked = make(map[int64]issues_model.RoleDescriptor)
comment *issues_model.Comment
participants = make([]*user_model.User, 1, 10)
latestCloseCommentID int64
)
if ctx.Repo.Repository.IsTimetrackerEnabled(ctx) {
if ctx.IsSigned {
Expand Down Expand Up @@ -1622,9 +1623,15 @@ func ViewIssue(ctx *context.Context) {
comment.Type == issues_model.CommentTypeStopTracking {
// drop error since times could be pruned from DB..
_ = comment.LoadTime()
} else if comment.Type == issues_model.CommentTypeClose {
// record ID of latest closed comment.
// if PR is closed, the comments whose type is CommentTypePullRequestPush(29) after latestCloseCommentID won't be rendered.
latestCloseCommentID = comment.ID
}
}

ctx.Data["LatestCloseCommentID"] = latestCloseCommentID

// Combine multiple label assignments into a single comment
combineLabelComments(issue)

Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C
ctx.Data["HeadBranchCommitID"] = headBranchSha
ctx.Data["PullHeadCommitID"] = sha

if pull.HeadRepo == nil || !headBranchExist || headBranchSha != sha {
if pull.HeadRepo == nil || !headBranchExist || (!pull.Issue.IsClosed && (headBranchSha != sha)) {
ctx.Data["IsPullRequestBroken"] = true
if pull.IsSameRepo() {
ctx.Data["HeadTarget"] = pull.HeadBranch
Expand Down
6 changes: 3 additions & 3 deletions services/pull/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
// If you don't let it run all the way then you will lose data
// TODO: graceful: AddTestPullRequestTask needs to become a queue!

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

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

var errs errlist
for _, branch := range branches {
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repo.ID, branch.Name)
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repo.ID, branch.Name, false)
if err != nil {
return err
}
Expand Down
6 changes: 5 additions & 1 deletion templates/repo/issue/view_content/comments.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,11 @@
</span>
</div>
{{else if and (eq .Type 29) (or (gt .CommitsNum 0) .IsForcePush)}}
<div class="timeline-item event" id="{{.HashTag}}">
<!-- If PR is closed, the comments whose type is CommentTypePullRequestPush(29) after latestCloseCommentID won't be rendered. //-->
{{if and .Issue.IsClosed (gt .ID $.LatestCloseCommentID)}}
{{continue}}
{{end}}`
<div class="timeline-item event" id="{{.HashTag}}">`
Comment on lines +703 to +704
Copy link
Contributor

@brechtvl brechtvl Mar 3, 2023

Choose a reason for hiding this comment

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

Some stray backticks ended up here, they can be seen in the timeline in the screenshots in the description.

Copy link
Contributor

Choose a reason for hiding this comment

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

<span class="badge">{{svg "octicon-repo-push"}}</span>
<span class="text grey muted-links">
{{template "shared/user/authorlink" .Poster}}
Expand Down