Skip to content

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

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

Merged
merged 10 commits into from
Mar 3, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
95ef3d9
Fix cannot reopen PR after commit
sillyguodong Feb 28, 2023
fd59c8c
fix lint
sillyguodong Feb 28, 2023
799fb4b
skip test `headBranchSha != sha` when PR is closed
sillyguodong Feb 28, 2023
210a6ce
record comment id instead of index of comment
sillyguodong Mar 1, 2023
5f2417b
only skip render PULL_PUSH_EVENT when PR is closed
sillyguodong Mar 1, 2023
92ae573
update comments
sillyguodong Mar 1, 2023
b6f348b
Add a switch to the GetUnmergedPullRequestsByHeadInfo func to control…
sillyguodong Mar 2, 2023
cf9fea6
reset the name of switch
sillyguodong Mar 2, 2023
8f4ed45
fix the name of swicth
sillyguodong Mar 3, 2023
219ffc8
Merge branch 'main' into bugfix/issue_22784
lunny Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions models/issues/pull_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
17 changes: 11 additions & 6 deletions routers/web/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/pull.go
View file
Open in desktop
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 {
Copy link
Contributor

@brechtvl brechtvl Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We've run into cases where a PR had a difference in these SHAs and so was shown as broken, even when it had never been closed. But we have not been able to get a clear repro case yet.

Maybe this test can be skipped for closed PRs, but remain for open PRs? If what's shown in the PR and what's in the head branch is not the same, I think it's important to show that to avoid accidentally merging the wrong code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okey, now skip the test when only the PR is closed

ctx.Data["IsPullRequestBroken"] = true
if pull.IsSameRepo() {
ctx.Data["HeadTarget"] = pull.HeadBranch
Expand Down
7 changes: 6 additions & 1 deletion templates/repo/issue/view_content/comments.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{{template "base/alert"}}
{{range .Issue.Comments}}
<div>
{{range $idx, $comment := .Issue.Comments}}
{{if call $.ShouldShowCommentType .Type}}
{{$createdStr:= TimeSinceUnix .CreatedUnix $.locale}}

Expand Down Expand Up @@ -117,6 +118,9 @@
{{end}}
</span>
</div>
{{if and .Issue.IsClosed (eq $.LatestCloseCommentIndex $idx)}}
{{break}}
{{end}}
{{else if eq .Type 28}}
<div class="timeline-item event" id="{{.HashTag}}">
<span class="badge gt-bg-purple gt-text-white">{{svg "octicon-git-merge"}}</span>
Expand Down Expand Up @@ -796,3 +800,4 @@
{{end}}
{{end}}
{{end}}
</div>