From 95ef3d9c87c6fac08c9b54444c58a49bde59be1c Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Tue, 28 Feb 2023 15:05:18 +0800 Subject: [PATCH 1/9] Fix cannot reopen PR after commit --- models/issues/pull_list.go | 4 ++-- routers/web/repo/issue.go | 17 +++++++++++------ routers/web/repo/pull.go | 2 +- templates/repo/issue/view_content/comments.tmpl | 7 ++++++- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index f4efd916c867..a13ffe31e843 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -55,8 +55,8 @@ func listPullRequestStatement(baseRepoID int64, opts *PullRequestsOptions) (*xor func GetUnmergedPullRequestsByHeadInfo(repoID int64, branch string) ([]*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). + Where("head_repo_id = ? AND head_branch = ? AND has_merged = ? AND flow = ?", + repoID, branch, false, PullRequestFlowGithub). Join("INNER", "issue", "issue.id = pull_request.issue_id"). Find(&prs) } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index e4f94006155a..d49fe6585771 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1420,11 +1420,11 @@ 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) + participants = make([]*user_model.User, 1, 10) + latestCloseCommentIndex int ) if ctx.Repo.Repository.IsTimetrackerEnabled(ctx) { if ctx.IsSigned { @@ -1468,7 +1468,7 @@ func ViewIssue(ctx *context.Context) { // Render comments and and fetch participants. participants[0] = issue.Poster - for _, comment = range issue.Comments { + for i, comment := range issue.Comments { comment.Issue = issue if err := comment.LoadPoster(ctx); err != nil { @@ -1622,9 +1622,14 @@ 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 latest index of close comment. + latestCloseCommentIndex = i } } + ctx.Data["LatestCloseCommentIndex"] = latestCloseCommentIndex + // Combine multiple label assignments into a single comment combineLabelComments(issue) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 38b9f22cbf3d..75506441afa7 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -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 { ctx.Data["IsPullRequestBroken"] = true if pull.IsSameRepo() { ctx.Data["HeadTarget"] = pull.HeadBranch diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 94b46bd9f13a..2c3b14c36fdd 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -1,5 +1,6 @@ {{template "base/alert"}} -{{range .Issue.Comments}} +
+{{range $idx, $comment := .Issue.Comments}} {{if call $.ShouldShowCommentType .Type}} {{$createdStr:= TimeSinceUnix .CreatedUnix $.locale}} @@ -117,6 +118,9 @@ {{end}}
+ {{if and .Issue.IsClosed (eq $.LatestCloseCommentIndex $idx)}} + {{break}} + {{end}} {{else if eq .Type 28}}
{{svg "octicon-git-merge"}} @@ -796,3 +800,4 @@ {{end}} {{end}} {{end}} +
\ No newline at end of file From fd59c8c004a6c2c2d7cea9a8b523f88e35b61db9 Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Tue, 28 Feb 2023 16:41:24 +0800 Subject: [PATCH 2/9] fix lint --- templates/repo/issue/view_content/comments.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 2c3b14c36fdd..742147e915a8 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -800,4 +800,4 @@ {{end}} {{end}} {{end}} - \ No newline at end of file + From 799fb4bfc28470d7402924c82e611428e0f6eab0 Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Tue, 28 Feb 2023 21:53:03 +0800 Subject: [PATCH 3/9] skip test `headBranchSha != sha` when PR is closed --- routers/web/repo/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 75506441afa7..830c8ad62616 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -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 { + if pull.HeadRepo == nil || !headBranchExist || (!pull.Issue.IsClosed && (headBranchSha != sha)) { ctx.Data["IsPullRequestBroken"] = true if pull.IsSameRepo() { ctx.Data["HeadTarget"] = pull.HeadBranch From 210a6ced04abf1c072a79f7083baf5353b2878f0 Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Wed, 1 Mar 2023 11:25:12 +0800 Subject: [PATCH 4/9] record comment id instead of index of comment --- routers/web/repo/issue.go | 20 ++++++++++--------- .../repo/issue/view_content/comments.tmpl | 6 ++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index d49fe6585771..cd5f1219fd02 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1420,11 +1420,12 @@ func ViewIssue(ctx *context.Context) { } var ( - role issues_model.RoleDescriptor - ok bool - marked = make(map[int64]issues_model.RoleDescriptor) - participants = make([]*user_model.User, 1, 10) - latestCloseCommentIndex int + 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 { @@ -1468,7 +1469,7 @@ func ViewIssue(ctx *context.Context) { // Render comments and and fetch participants. participants[0] = issue.Poster - for i, comment := range issue.Comments { + for _, comment = range issue.Comments { comment.Issue = issue if err := comment.LoadPoster(ctx); err != nil { @@ -1623,12 +1624,13 @@ func ViewIssue(ctx *context.Context) { // drop error since times could be pruned from DB.. _ = comment.LoadTime() } else if comment.Type == issues_model.CommentTypeClose { - // record latest index of close comment. - latestCloseCommentIndex = i + // record ID of latest closed comment. + // if PR is closed, the comments after it won't be rendered. + latestCloseCommentID = comment.ID } } - ctx.Data["LatestCloseCommentIndex"] = latestCloseCommentIndex + ctx.Data["LatestCloseCommentID"] = latestCloseCommentID // Combine multiple label assignments into a single comment combineLabelComments(issue) diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 742147e915a8..9a5222cf5a48 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -1,6 +1,5 @@ {{template "base/alert"}} -
-{{range $idx, $comment := .Issue.Comments}} +{{range .Issue.Comments}} {{if call $.ShouldShowCommentType .Type}} {{$createdStr:= TimeSinceUnix .CreatedUnix $.locale}} @@ -118,7 +117,7 @@ {{end}}
- {{if and .Issue.IsClosed (eq $.LatestCloseCommentIndex $idx)}} + {{if and .Issue.IsClosed (eq $.LatestCloseCommentID .ID)}} {{break}} {{end}} {{else if eq .Type 28}} @@ -800,4 +799,3 @@ {{end}} {{end}} {{end}} - From 5f2417bdba84c26284e01854aca16f20d19d6e15 Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Wed, 1 Mar 2023 14:06:23 +0800 Subject: [PATCH 5/9] only skip render PULL_PUSH_EVENT when PR is closed --- templates/repo/issue/view_content/comments.tmpl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 9a5222cf5a48..3eecd4bba26e 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -117,9 +117,6 @@ {{end}} - {{if and .Issue.IsClosed (eq $.LatestCloseCommentID .ID)}} - {{break}} - {{end}} {{else if eq .Type 28}}
{{svg "octicon-git-merge"}} @@ -700,6 +697,9 @@
{{else if and (eq .Type 29) (or (gt .CommitsNum 0) .IsForcePush)}} + {{if and .Issue.IsClosed (gt .ID $.LatestCloseCommentID)}} + {{continue}} + {{end}}
{{svg "octicon-repo-push"}} From 92ae573063e9f531f4410f10191da611707849c9 Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Wed, 1 Mar 2023 15:15:29 +0800 Subject: [PATCH 6/9] update comments --- routers/web/repo/issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index cd5f1219fd02..9a0e93cfee77 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1625,7 +1625,7 @@ func ViewIssue(ctx *context.Context) { _ = comment.LoadTime() } else if comment.Type == issues_model.CommentTypeClose { // record ID of latest closed comment. - // if PR is closed, the comments after it won't be rendered. + // if PR is closed, the comments whose type is CommentTypePullRequestPush(29) after latestCloseCommentID won't be rendered. latestCloseCommentID = comment.ID } } From b6f348baf2cff4dae3467c0cac473b596df081d6 Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Thu, 2 Mar 2023 14:50:56 +0800 Subject: [PATCH 7/9] Add a switch to the GetUnmergedPullRequestsByHeadInfo func to control whether the status of PR must be open --- models/issues/pull_list.go | 14 ++++++++------ models/issues/pull_test.go | 2 +- services/pull/pull.go | 6 +++--- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index a13ffe31e843..d9e2d1b688c0 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -52,13 +52,15 @@ 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) { +func GetUnmergedPullRequestsByHeadInfo(repoID int64, branch string, mustOpenedPR 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 flow = ?", - repoID, branch, 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 mustOpenedPR { + sess.Where("issue.is_closed = ?", false) + } + return prs, sess.Find(&prs) } // CanMaintainerWriteToBranch check whether user is a maintainer and could write to the branch @@ -71,7 +73,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, true) if err != nil { return false } diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 8ce8eecc4aa8..e650b36edde8 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -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", true) assert.NoError(t, err) assert.Len(t, prs, 1) for _, pr := range prs { diff --git a/services/pull/pull.go b/services/pull/pull.go index 0d260c93b1ec..c17b96bd67c9 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -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, false) if err != nil { log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repoID, branch, err) return @@ -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, true) if err != nil { return err } @@ -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, true) if err != nil { return err } From cf9fea66a435d3745872b35ba2f708d391b647a5 Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Thu, 2 Mar 2023 22:03:48 +0800 Subject: [PATCH 8/9] reset the name of switch --- models/issues/pull_list.go | 6 +++--- models/issues/pull_test.go | 2 +- services/pull/pull.go | 6 +++--- templates/repo/issue/view_content/comments.tmpl | 5 +++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index d9e2d1b688c0..de718ba20bd5 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -52,12 +52,12 @@ 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, mustOpenedPR bool) ([]*PullRequest, error) { +func GetUnmergedPullRequestsByHeadInfo(repoID int64, branch string, notForceClose bool) ([]*PullRequest, error) { prs := make([]*PullRequest, 0, 2) sess := db.GetEngine(db.DefaultContext). Join("INNER", "issue", "issue.id = pull_request.issue_id"). Where("head_repo_id = ? AND head_branch = ? AND has_merged = ? AND flow = ?", repoID, branch, false, PullRequestFlowGithub) - if mustOpenedPR { + if !notForceClose { sess.Where("issue.is_closed = ?", false) } return prs, sess.Find(&prs) @@ -73,7 +73,7 @@ func CanMaintainerWriteToBranch(p access_model.Permission, branch string, user * return false } - prs, err := GetUnmergedPullRequestsByHeadInfo(p.Units[0].RepoID, branch, true) + prs, err := GetUnmergedPullRequestsByHeadInfo(p.Units[0].RepoID, branch, false) if err != nil { return false } diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index e650b36edde8..bcd33329eb9b 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -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", true) + prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(1, "branch2", false) assert.NoError(t, err) assert.Len(t, prs, 1) for _, pr := range prs { diff --git a/services/pull/pull.go b/services/pull/pull.go index c17b96bd67c9..a19e88b33b65 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -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, false) + 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 @@ -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, true) + prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repoID, branch, false) if err != nil { return err } @@ -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, true) + prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(repo.ID, branch.Name, false) if err != nil { return err } diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 3eecd4bba26e..e76561f5109e 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -697,10 +697,11 @@
{{else if and (eq .Type 29) (or (gt .CommitsNum 0) .IsForcePush)}} + {{if and .Issue.IsClosed (gt .ID $.LatestCloseCommentID)}} {{continue}} - {{end}} -
+ {{end}}` +
` {{svg "octicon-repo-push"}} {{template "shared/user/authorlink" .Poster}} From 8f4ed45b25821870d28c892f3d18beadce8c0794 Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Fri, 3 Mar 2023 10:58:38 +0800 Subject: [PATCH 9/9] fix the name of swicth --- models/issues/pull_list.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index de718ba20bd5..b781490146ba 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -52,12 +52,13 @@ 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, notForceClose bool) ([]*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) sess := db.GetEngine(db.DefaultContext). Join("INNER", "issue", "issue.id = pull_request.issue_id"). Where("head_repo_id = ? AND head_branch = ? AND has_merged = ? AND flow = ?", repoID, branch, false, PullRequestFlowGithub) - if !notForceClose { + if !includeClosed { sess.Where("issue.is_closed = ?", false) } return prs, sess.Find(&prs)