From f4b8f6fc40ce2869135372a5c6ec6418d27ebfba Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Tue, 1 Oct 2024 09:58:55 +0800 Subject: [PATCH] Fix the logic of finding the latest pull review commit ID (#32139) Fix #31423 --- models/fixtures/comment.yml | 19 +++++++++++++ models/fixtures/review.yml | 19 +++++++++++++ models/issues/pull.go | 2 +- models/issues/review.go | 2 +- models/issues/review_list.go | 6 ++-- models/issues/review_test.go | 4 +-- routers/api/v1/repo/pull_review.go | 1 - services/pull/pull.go | 8 +++++- services/pull/review.go | 2 +- tests/integration/api_pull_review_test.go | 2 +- tests/integration/pull_commit_test.go | 34 +++++++++++++++++++++++ 11 files changed, 88 insertions(+), 11 deletions(-) create mode 100644 tests/integration/pull_commit_test.go diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml index 74fc716180..8fde386e22 100644 --- a/models/fixtures/comment.yml +++ b/models/fixtures/comment.yml @@ -83,3 +83,22 @@ issue_id: 2 # in repo_id 1 review_id: 20 created_unix: 946684810 + +- + id: 10 + type: 22 # review + poster_id: 5 + issue_id: 3 # in repo_id 1 + content: "reviewed by user5" + review_id: 21 + created_unix: 946684816 + +- + id: 11 + type: 27 # review request + poster_id: 2 + issue_id: 3 # in repo_id 1 + content: "review request for user5" + review_id: 22 + assignee_id: 5 + created_unix: 946684817 diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index ac97e24c2b..0438ceadae 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -179,3 +179,22 @@ content: "Review Comment" updated_unix: 946684810 created_unix: 946684810 + +- + id: 21 + type: 2 + reviewer_id: 5 + issue_id: 3 + content: "reviewed by user5" + commit_id: 4a357436d925b5c974181ff12a994538ddc5a269 + updated_unix: 946684816 + created_unix: 946684816 + +- + id: 22 + type: 4 + reviewer_id: 5 + issue_id: 3 + content: "review request for user5" + updated_unix: 946684817 + created_unix: 946684817 diff --git a/models/issues/pull.go b/models/issues/pull.go index 4a5782f836..b327ebc625 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -414,7 +414,7 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer) // Note: This doesn't page as we only expect a very limited number of reviews reviews, err := FindLatestReviews(ctx, FindReviewOptions{ - Type: ReviewTypeApprove, + Types: []ReviewType{ReviewTypeApprove}, IssueID: pr.IssueID, OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, }) diff --git a/models/issues/review.go b/models/issues/review.go index 9fb6d7573a..8b345e5fd8 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -389,7 +389,7 @@ func GetCurrentReview(ctx context.Context, reviewer *user_model.User, issue *Iss return nil, nil } reviews, err := FindReviews(ctx, FindReviewOptions{ - Type: ReviewTypePending, + Types: []ReviewType{ReviewTypePending}, IssueID: issue.ID, ReviewerID: reviewer.ID, }) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index 0ee28874ec..a5ceb21791 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -92,7 +92,7 @@ func (reviews ReviewList) LoadIssues(ctx context.Context) error { // FindReviewOptions represent possible filters to find reviews type FindReviewOptions struct { db.ListOptions - Type ReviewType + Types []ReviewType IssueID int64 ReviewerID int64 OfficialOnly bool @@ -107,8 +107,8 @@ func (opts *FindReviewOptions) toCond() builder.Cond { if opts.ReviewerID > 0 { cond = cond.And(builder.Eq{"reviewer_id": opts.ReviewerID}) } - if opts.Type != ReviewTypeUnknown { - cond = cond.And(builder.Eq{"type": opts.Type}) + if len(opts.Types) > 0 { + cond = cond.And(builder.In("type", opts.Types)) } if opts.OfficialOnly { cond = cond.And(builder.Eq{"official": true}) diff --git a/models/issues/review_test.go b/models/issues/review_test.go index ac1b84adeb..942121fd8f 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -63,7 +63,7 @@ func TestReviewType_Icon(t *testing.T) { func TestFindReviews(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) reviews, err := issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{ - Type: issues_model.ReviewTypeApprove, + Types: []issues_model.ReviewType{issues_model.ReviewTypeApprove}, IssueID: 2, ReviewerID: 1, }) @@ -75,7 +75,7 @@ func TestFindReviews(t *testing.T) { func TestFindLatestReviews(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) reviews, err := issues_model.FindLatestReviews(db.DefaultContext, issues_model.FindReviewOptions{ - Type: issues_model.ReviewTypeApprove, + Types: []issues_model.ReviewType{issues_model.ReviewTypeApprove}, IssueID: 11, }) assert.NoError(t, err) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index c2e4966498..34bbaf5600 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -83,7 +83,6 @@ func ListPullReviews(ctx *context.APIContext) { opts := issues_model.FindReviewOptions{ ListOptions: utils.GetListOptions(ctx), - Type: issues_model.ReviewTypeUnknown, IssueID: pr.IssueID, } diff --git a/services/pull/pull.go b/services/pull/pull.go index 154ff6c5c6..bab4e49998 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -995,6 +995,8 @@ type CommitInfo struct { } // GetPullCommits returns all commits on given pull request and the last review commit sha +// Attention: The last review commit sha must be from the latest review whose commit id is not empty. +// So the type of the latest review cannot be "ReviewTypeRequest". func GetPullCommits(ctx *gitea_context.Context, issue *issues_model.Issue) ([]CommitInfo, string, error) { pull := issue.PullRequest @@ -1040,7 +1042,11 @@ func GetPullCommits(ctx *gitea_context.Context, issue *issues_model.Issue) ([]Co lastreview, err := issues_model.FindLatestReviews(ctx, issues_model.FindReviewOptions{ IssueID: issue.ID, ReviewerID: ctx.Doer.ID, - Type: issues_model.ReviewTypeUnknown, + Types: []issues_model.ReviewType{ + issues_model.ReviewTypeApprove, + issues_model.ReviewTypeComment, + issues_model.ReviewTypeReject, + }, }) if err != nil && !issues_model.IsErrReviewNotExist(err) { diff --git a/services/pull/review.go b/services/pull/review.go index 3d5eca779f..78723a58ae 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -348,7 +348,7 @@ func DismissApprovalReviews(ctx context.Context, doer *user_model.User, pull *is reviews, err := issues_model.FindReviews(ctx, issues_model.FindReviewOptions{ ListOptions: db.ListOptionsAll, IssueID: pull.IssueID, - Type: issues_model.ReviewTypeApprove, + Types: []issues_model.ReviewType{issues_model.ReviewTypeApprove}, Dismissed: optional.Some(false), }) if err != nil { diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index bc544a30b5..cadb0765c3 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -38,7 +38,7 @@ func TestAPIPullReview(t *testing.T) { var reviews []*api.PullReview DecodeJSON(t, resp, &reviews) - if !assert.Len(t, reviews, 6) { + if !assert.Len(t, reviews, 8) { return } for _, r := range reviews { diff --git a/tests/integration/pull_commit_test.go b/tests/integration/pull_commit_test.go new file mode 100644 index 0000000000..477f01725d --- /dev/null +++ b/tests/integration/pull_commit_test.go @@ -0,0 +1,34 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "net/http" + "net/url" + "testing" + + pull_service "code.gitea.io/gitea/services/pull" + + "github.com/stretchr/testify/assert" +) + +func TestListPullCommits(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + session := loginUser(t, "user5") + req := NewRequest(t, "GET", "/user2/repo1/pulls/3/commits/list") + resp := session.MakeRequest(t, req, http.StatusOK) + + var pullCommitList struct { + Commits []pull_service.CommitInfo `json:"commits"` + LastReviewCommitSha string `json:"last_review_commit_sha"` + } + DecodeJSON(t, resp, &pullCommitList) + + if assert.Len(t, pullCommitList.Commits, 2) { + assert.Equal(t, "5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", pullCommitList.Commits[0].ID) + assert.Equal(t, "4a357436d925b5c974181ff12a994538ddc5a269", pullCommitList.Commits[1].ID) + } + assert.Equal(t, "4a357436d925b5c974181ff12a994538ddc5a269", pullCommitList.LastReviewCommitSha) + }) +}