Make it possible to delete stale review requests that incorrectly remain in DB

models/fixtures/review.yml

@@ -207,10 +207,44 @@
 
 -
   id: 22
+  type: 1
+  reviewer_id: 5
+  issue_id: 3
+  content: "review accepted of user5 but dismissed"
+  original_author_id: 0
+  updated_unix: 946684000
+  created_unix: 946684000
+  dismissed: true
+
+-
+  id: 23
   type: 4
   reviewer_id: 5
   issue_id: 3
   content: "review request for user5"
   original_author_id: 0
   updated_unix: 946684817
   created_unix: 946684817
+
+-
+  id: 24
+  type: 3
+  reviewer_id: 5
+  issue_id: 3
+  content: "review rejected of user5 but dismissed"
+  original_author_id: 0
+  updated_unix: 946684827
+  created_unix: 946684827
+  dismissed: true
+
+-
+  id: 25
+  type: 3
+  reviewer_id: 15
+  reviewer_team_id: 0
+  issue_id: 3
+  content: "review rejected by user15 but dismissed"
+  original_author_id: 0
+  updated_unix: 946684899
+  created_unix: 946684899
+  dismissed: true

models/issues/pull_list_test.go

@@ -52,12 +52,12 @@ func TestPullRequestList_LoadReviews(t *testing.T) {
 	}
 	reviewList, err := prs.LoadReviews(t.Context())
 	assert.NoError(t, err)
-	// 1, 7, 8, 9, 10, 22
+	// 1, 7, 8, 9, 10, 23
 	assert.Len(t, reviewList, 6)
 	assert.EqualValues(t, 1, reviewList[0].ID)
 	assert.EqualValues(t, 7, reviewList[1].ID)
 	assert.EqualValues(t, 8, reviewList[2].ID)
 	assert.EqualValues(t, 9, reviewList[3].ID)
 	assert.EqualValues(t, 10, reviewList[4].ID)
-	assert.EqualValues(t, 22, reviewList[5].ID)
+	assert.EqualValues(t, 23, reviewList[5].ID)
 }

models/issues/pull_test.go

@@ -348,6 +348,22 @@ func TestGetApprovers(t *testing.T) {
 	approvers := pr.GetApprovers(t.Context())
 	expected := "Reviewed-by: User Five <user5@example.com>\nReviewed-by: Org Six <org6@example.com>\n"
 	assert.Equal(t, expected, approvers)
+
+	// comment-type and pending reviews should be ignored
+	pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2})
+	assert.EqualValues(t, 3, pr.IssueID)
+	setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false
+	approvers = pr.GetApprovers(t.Context())
+	expected = "Reviewed-by: User Five <user5@example.com>\nReviewed-by: user4 <user4@example.com>\n"
+	assert.Equal(t, expected, approvers)
+
+	// un-official reviews should now be ignored too
+	pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2})
+	assert.EqualValues(t, 3, pr.IssueID)
+	setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = true
+	approvers = pr.GetApprovers(t.Context())
+	expected = ""
+	assert.Equal(t, expected, approvers)
 }
 
 func TestGetPullRequestByMergedCommit(t *testing.T) {

models/issues/review.go

@@ -17,6 +17,7 @@ import (
 	access_model "code.gitea.io/gitea/models/perm/access"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/util"
@@ -390,6 +391,7 @@ func GetCurrentReview(ctx context.Context, reviewer *user_model.User, issue *Iss
 		Types:      []ReviewType{ReviewTypePending},
 		IssueID:    issue.ID,
 		ReviewerID: reviewer.ID,
+		Dismissed:  optional.Some(false),
 	})
 	if err != nil {
 		return nil, err
@@ -532,12 +534,18 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 }
 
 // GetReviewByIssueIDAndUserID get the latest review of reviewer for a pull request
-func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64) (*Review, error) {
+func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64, dismissed ...bool) (*Review, error) {
 	review := new(Review)
 
-	has, err := db.GetEngine(ctx).Where(
-		builder.In("type", ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
-			And(builder.Eq{"issue_id": issueID, "reviewer_id": userID, "original_author_id": 0})).
+	cond := builder.In("type", ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
+		And(builder.Eq{"issue_id": issueID, "reviewer_id": userID, "original_author_id": 0})
+
+	// apply optional filter for dismissed
+	if len(dismissed) != 0 {
+		cond = cond.And(builder.Eq{"dismissed": dismissed[0]})
+	}
+
+	has, err := db.GetEngine(ctx).Where(cond).
 		Desc("id").
 		Get(review)
 	if err != nil {
@@ -717,7 +725,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 // RemoveReviewRequest remove a review request from one reviewer
 func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
 	return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) {
-		review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID)
+		review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID, false)
 		if err != nil && !IsErrReviewNotExist(err) {
 			return nil, err
 		}

models/issues/review_list.go

@@ -163,7 +163,7 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (latestReviews, mig
 	reviews := make([]*Review, 0, 10)
 
 	// Get all reviews for the issue id
-	if err := db.GetEngine(ctx).Where("issue_id=?", issueID).OrderBy("updated_unix ASC").Find(&reviews); err != nil {
+	if err := db.GetEngine(ctx).Where("issue_id=? AND dismissed=?", issueID, false).OrderBy("updated_unix ASC").Find(&reviews); err != nil {
 		return nil, nil, err
 	}
 
@@ -175,7 +175,7 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (latestReviews, mig
 	reviewTeamsMap := make(map[int64][]*Review)       // key is reviewer team id
 	countedReivewTypes := []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest}
 	for _, review := range reviews {
-		if review.ReviewerTeamID == 0 && slices.Contains(countedReivewTypes, review.Type) && !review.Dismissed {
+		if review.ReviewerTeamID == 0 && slices.Contains(countedReivewTypes, review.Type) {
 			if review.OriginalAuthorID != 0 {
 				originalReviewersMap[review.OriginalAuthorID] = append(originalReviewersMap[review.OriginalAuthorID], review)
 			} else {

services/pull/pull.go

@@ -30,6 +30,7 @@ import (
 	"code.gitea.io/gitea/modules/globallock"
 	"code.gitea.io/gitea/modules/graceful"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/optional"
 	repo_module "code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/util"
@@ -1093,6 +1094,7 @@ func GetPullCommits(ctx context.Context, baseGitRepo *git.Repository, doer *user
 				issues_model.ReviewTypeComment,
 				issues_model.ReviewTypeReject,
 			},
+			Dismissed: optional.Some(false),
 		})
 
 		if err != nil && !issues_model.IsErrReviewNotExist(err) {

tests/integration/api_pull_review_test.go

@@ -40,23 +40,28 @@ func TestAPIPullReview(t *testing.T) {
 
 	var reviews []*api.PullReview
 	DecodeJSON(t, resp, &reviews)
-	require.Len(t, reviews, 8)
+	require.Len(t, reviews, 11)
 
 	for _, r := range reviews {
 		assert.Equal(t, pullIssue.HTMLURL(t.Context()), r.HTMLPullURL)
 	}
-	assert.EqualValues(t, 8, reviews[3].ID)
-	assert.EqualValues(t, "APPROVED", reviews[3].State)
-	assert.Equal(t, 0, reviews[3].CodeCommentsCount)
-	assert.True(t, reviews[3].Stale)
-	assert.False(t, reviews[3].Official)
-
-	assert.EqualValues(t, 10, reviews[5].ID)
-	assert.EqualValues(t, "REQUEST_CHANGES", reviews[5].State)
-	assert.Equal(t, 1, reviews[5].CodeCommentsCount)
-	assert.EqualValues(t, -1, reviews[5].Reviewer.ID) // ghost user
-	assert.False(t, reviews[5].Stale)
-	assert.True(t, reviews[5].Official)
+	if assert.EqualValues(t, 8, reviews[4].ID) {
+		assert.EqualValues(t, "APPROVED", reviews[4].State)
+		assert.Equal(t, 0, reviews[4].CodeCommentsCount)
+		assert.True(t, reviews[4].Stale)
+		assert.False(t, reviews[4].Official)
+	}
+
+	if assert.EqualValues(t, 10, reviews[6].ID) {
+		assert.EqualValues(t, "REQUEST_CHANGES", reviews[6].State)
+		assert.Equal(t, 1, reviews[6].CodeCommentsCount)
+		assert.EqualValues(t, -1, reviews[6].Reviewer.ID) // ghost user
+		assert.False(t, reviews[6].Stale)
+		assert.True(t, reviews[6].Official)
+	}
+	if assert.EqualValues(t, 25, reviews[10].ID) {
+		assert.True(t, reviews[10].Dismissed)
+	}
 
 	// test GetPullReview
 	req = NewRequestf(t, http.MethodGet, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d", repo.OwnerName, repo.Name, pullIssue.Index, reviews[3].ID).