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

Backport #32139 by @Zettat123

Fix #31423

Co-authored-by: Zettat123 <zettat123@gmail.com>

diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml
index 74fc716180d1de83e35cdd8bc77f9d735761e031..8fde386e226d4eea3947618f722594a300b83414 100644 (file)
--- 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 ac97e24c2be0c3025018b2be973a2363e8ec0538..0438ceadae2892b133b50e739967c8ca24f9557d 100644 (file)
--- 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 74ea73a48f694e7d0509404c012afe07fe7ed452..6bed736847f5113e1a21c466e7a1efe0a8678927 100644 (file)
--- a/models/issues/pull.go
+++ b/models/issues/pull.go
@@ -387,7 +387,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 ca6fd6035b13032110c5eb1c58c8e65d5498921d..a39c12069b4fc76328ecfe89bcee476c980a9457 100644 (file)
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -364,7 +364,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 7b8c3d319c35a0ab8c2f42d6af1feade2d6480e6..ff6d1ea27f561572f118a3f213ec4e89aebb2295 100644 (file)
--- a/models/issues/review_list.go
+++ b/models/issues/review_list.go
@@ -63,7 +63,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
@@ -78,8 +78,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 ac1b84adebcc2ed06919b5acae5e23e9ef49fdc7..942121fd8f25721e58fb2296a08f69e52c540026 100644 (file)
--- 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 4b481790fb1a869ffe82533836517e2919dc6fa8..53291f7cbc8bb579602aab3b2ba089013ddbf848 100644 (file)
--- 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 e69c842a2d4b54e6f7522067be7538c287b6584b..95310cf0a503a7739c54069f7da85e825faa47d9 100644 (file)
--- a/services/pull/pull.go
+++ b/services/pull/pull.go
@@ -990,6 +990,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
 
@@ -1035,7 +1037,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 e303cd9a9d6eaa6ec41f14fa117166070038d3d9..a77dacf95cfb543ca28b37a84622e3a54bc3b469 100644 (file)
--- 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 bc544a30b53197a632ef78cf343fb3f1cf093f72..cadb0765c341eab0cc7a7ab8afc99a74ad41861f 100644 (file)
--- 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 (file)
index 0000000..477f017
--- /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)
+       })
+}