Fix the logic of finding the latest pull review commit ID (#32139)

Zettat123 · web-flow · commit f4b8f6fc40ce · 2024-10-01T01:58:55.000Z
Fix #31423
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
+	})
+}

Original file line number	Diff line number	Diff line change
`@@ -389,7 +389,7 @@ func GetCurrentReview(ctx context.Context, reviewer user_model.User, issue Iss`
`389`	`389`	`return nil, nil`
`390`	`390`	`}`
`391`	`391`	`reviews, err := FindReviews(ctx, FindReviewOptions{`
`392`		`- Type: ReviewTypePending,`
	`392`	`+ Types: []ReviewType{ReviewTypePending},`
`393`	`393`	`IssueID: issue.ID,`
`394`	`394`	`ReviewerID: reviewer.ID,`
`395`	`395`	`})`
Original file line number	Diff line number	Diff line change
`@@ -83,7 +83,6 @@ func ListPullReviews(ctx *context.APIContext) {`
`83`	`83`
`84`	`84`	`opts := issues_model.FindReviewOptions{`
`85`	`85`	`ListOptions: utils.GetListOptions(ctx),`
`86`		`- Type: issues_model.ReviewTypeUnknown,`
`87`	`86`	`IssueID: pr.IssueID,`
`88`	`87`	`}`
`89`	`88`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,7 @@ func TestAPIPullReview(t *testing.T) {`
`38`	`38`
`39`	`39`	`var reviews []*api.PullReview`
`40`	`40`	`DecodeJSON(t, resp, &reviews)`
`41`		`- if !assert.Len(t, reviews, 6) {`
	`41`	`+ if !assert.Len(t, reviews, 8) {`
`42`	`42`	`return`
`43`	`43`	`}`
`44`	`44`	`for _, r := range reviews {`