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

@@ -12,6 +12,7 @@ import (
 "code.gitea.io/gitea/models/unittest"
 user_model "code.gitea.io/gitea/models/user"
 "code.gitea.io/gitea/modules/setting"
+"code.gitea.io/gitea/modules/test"
 
 "github.com/stretchr/testify/assert"
 "github.com/stretchr/testify/require"
@@ -344,10 +345,25 @@ func TestGetApprovers(t *testing.T) {
 pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 5})
 // Official reviews are already deduplicated. Allow unofficial reviews
 // to assert that there are no duplicated approvers.
-setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false
+defer test.MockVariableValue(&setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, false)()
 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)
+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 optional.Option[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 dismissed.Has() {
+cond = cond.And(builder.Eq{"dismissed": dismissed.Value()})
+}
+
+has, err := db.GetEngine(ctx).Where(cond).
 Desc("id").
 Get(review)
 if err != nil {
@@ -647,7 +655,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) {
 sess := db.GetEngine(ctx)
 
-review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID)
+review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID, optional.None[bool]())
 if err != nil && !IsErrReviewNotExist(err) {
 return nil, err
 }
@@ -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, optional.Some(false))
 if err != nil && !IsErrReviewNotExist(err) {
 return nil, err
 }
@@ -752,7 +760,7 @@ func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user
 
 // Recalculate the latest official review for reviewer
 func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64) error {
-review, err := GetReviewByIssueIDAndUserID(ctx, issueID, reviewerID)
+review, err := GetReviewByIssueIDAndUserID(ctx, issueID, reviewerID, optional.None[bool]())
 if err != nil && !IsErrReviewNotExist(err) {
 return err
 }
@@ -844,7 +852,7 @@ func RemoveTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organi
 
 if official {
 // recalculate which is the latest official review from that team
-review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, -reviewer.ID)
+review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, -reviewer.ID, optional.None[bool]())
 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, ReviewTypeComment}
 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/issue/assignee.go

@@ -14,6 +14,7 @@ import (
 "code.gitea.io/gitea/models/unit"
 user_model "code.gitea.io/gitea/models/user"
 "code.gitea.io/gitea/modules/log"
+"code.gitea.io/gitea/modules/optional"
 notify_service "code.gitea.io/gitea/services/notify"
 )
 
@@ -116,7 +117,7 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
 }
 }
 
-lastReview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID)
+lastReview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID, optional.None[bool]())
 if err != nil && !issues_model.IsErrReviewNotExist(err) {
 return err
 }

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"
@@ -1102,6 +1103,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).