From 4568fcee4ca5318ff0f47dae6855c45a349ae0da Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Tue, 19 Dec 2023 22:36:14 +0100
Subject: [PATCH 01/28] add index to columns we use filter

---
 models/issues/review.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 3db73a09ebcb..1ee5bed5e9ac 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -103,11 +103,11 @@ func (rt ReviewType) Icon() string {
 
 // Review represents collection of code comments giving feedback for a PR
 type Review struct {
-	ID               int64 `xorm:"pk autoincr"`
-	Type             ReviewType
+	ID               int64              `xorm:"pk autoincr"`
+	Type             ReviewType         `xorm:"index"`
 	Reviewer         *user_model.User   `xorm:"-"`
 	ReviewerID       int64              `xorm:"index"`
-	ReviewerTeamID   int64              `xorm:"NOT NULL DEFAULT 0"`
+	ReviewerTeamID   int64              `xorm:"index NOT NULL DEFAULT 0"`
 	ReviewerTeam     *organization.Team `xorm:"-"`
 	OriginalAuthor   string
 	OriginalAuthorID int64

From 8f75edc0cb168080cdaeb33c538441b40056aa54 Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Tue, 19 Dec 2023 22:52:31 +0100
Subject: [PATCH 02/28] database done right!!!

---
 models/issues/review.go | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 1ee5bed5e9ac..d483ba4a3034 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -460,8 +460,10 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64) (*Review, error) {
 	review := new(Review)
 
-	has, err := db.GetEngine(ctx).SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND original_author_id = 0 AND type in (?, ?, ?))",
-		issueID, userID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
+	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})).
+		OrderBy("id").Desc().
 		Get(review)
 	if err != nil {
 		return nil, err
@@ -475,13 +477,13 @@ func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64) (*R
 }
 
 // GetTeamReviewerByIssueIDAndTeamID get the latest review request of reviewer team for a pull request
-func GetTeamReviewerByIssueIDAndTeamID(ctx context.Context, issueID, teamID int64) (review *Review, err error) {
-	review = new(Review)
+func GetTeamReviewerByIssueIDAndTeamID(ctx context.Context, issueID, teamID int64) (*Review, error) {
+	review := new(Review)
 
-	var has bool
-	if has, err = db.GetEngine(ctx).SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = ?)",
-		issueID, teamID).
-		Get(review); err != nil {
+	has, err := db.GetEngine(ctx).Where(builder.Eq{"issue_id": issueID, "reviewer_team_id": teamID}).
+		OrderBy("id").Desc().
+		Get(review)
+	if err != nil {
 		return nil, err
 	}
 

From d54ec891c1ffb4e68ba5b7ff802133e8a7ba9596 Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Wed, 20 Dec 2023 02:08:35 +0100
Subject: [PATCH 03/28] single source of truth for reviewType who affect
 reviews

---
 models/issues/review.go            | 15 ++++++++++-----
 routers/api/v1/repo/pull_review.go |  2 +-
 routers/web/repo/pull_review.go    |  6 +++---
 services/pull/review.go            |  4 ++--
 4 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index d483ba4a3034..8f5132ea9bdb 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -85,6 +85,11 @@ const (
 	ReviewTypeRequest
 )
 
+// AffectReview indicate if this review type alter a pull state
+func (rt ReviewType) AffectReview() bool {
+	return rt == ReviewTypeApprove || rt == ReviewTypeReject
+}
+
 // Icon returns the corresponding icon for the review type
 func (rt ReviewType) Icon() string {
 	switch rt {
@@ -366,7 +371,7 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 			return nil, nil, ContentEmptyErr{}
 		}
 
-		if reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject {
+		if reviewType.AffectReview() {
 			// Only reviewers latest review of type approve and reject shall count as "official", so existing reviews needs to be cleared
 			if _, err := db.Exec(ctx, "UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
 				return nil, nil, err
@@ -396,7 +401,7 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 			return nil, nil, ContentEmptyErr{}
 		}
 
-		if reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject {
+		if reviewType.AffectReview() {
 			// Only reviewers latest review of type approve and reject shall count as "official", so existing reviews needs to be cleared
 			if _, err := db.Exec(ctx, "UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
 				return nil, nil, err
@@ -432,7 +437,7 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 	}
 
 	// try to remove team review request if need
-	if issue.Repo.Owner.IsOrganization() && (reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject) {
+	if issue.Repo.Owner.IsOrganization() && reviewType.AffectReview() {
 		teamReviewRequests := make([]*Review, 0, 10)
 		if err := sess.SQL("SELECT * FROM review WHERE issue_id = ? AND reviewer_team_id > 0 AND type = ?", issue.ID, ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
 			return nil, nil, err
@@ -510,7 +515,7 @@ func MarkReviewsAsNotStale(ctx context.Context, issueID int64, commitID string)
 
 // DismissReview change the dismiss status of a review
 func DismissReview(ctx context.Context, review *Review, isDismiss bool) (err error) {
-	if review.Dismissed == isDismiss || (review.Type != ReviewTypeApprove && review.Type != ReviewTypeReject) {
+	if review.Dismissed == isDismiss || !review.Type.AffectReview() {
 		return nil
 	}
 
@@ -581,7 +586,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 		return nil, err
 	}
 
-	// skip it when reviewer hase been request to review
+	// skip it when reviewer has been request to review
 	if review != nil && review.Type == ReviewTypeRequest {
 		return nil, nil
 	}
diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go
index 7b9445be4c06..e325e07620e9 100644
--- a/routers/api/v1/repo/pull_review.go
+++ b/routers/api/v1/repo/pull_review.go
@@ -879,7 +879,7 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors
 		return
 	}
 
-	if review.Type != issues_model.ReviewTypeApprove && review.Type != issues_model.ReviewTypeReject {
+	if !review.Type.AffectReview() {
 		ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because it's type is not Approve or change request")
 		return
 	}
diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go
index 1359af9d3bfa..47d490a2bcf8 100644
--- a/routers/web/repo/pull_review.go
+++ b/routers/web/repo/pull_review.go
@@ -199,13 +199,13 @@ func SubmitReview(ctx *context.Context) {
 	}
 
 	reviewType := form.ReviewType()
-	switch reviewType {
-	case issues_model.ReviewTypeUnknown:
+	switch {
+	case reviewType == issues_model.ReviewTypeUnknown:
 		ctx.ServerError("ReviewType", fmt.Errorf("unknown ReviewType: %s", form.Type))
 		return
 
 	// can not approve/reject your own PR
-	case issues_model.ReviewTypeApprove, issues_model.ReviewTypeReject:
+	case reviewType.AffectReview():
 		if issue.IsPoster(ctx.Doer.ID) {
 			var translated string
 			if reviewType == issues_model.ReviewTypeApprove {
diff --git a/services/pull/review.go b/services/pull/review.go
index e48f3801547d..968c473a5b76 100644
--- a/services/pull/review.go
+++ b/services/pull/review.go
@@ -270,7 +270,7 @@ func SubmitReview(ctx context.Context, doer *user_model.User, gitRepo *git.Repos
 	}
 
 	var stale bool
-	if reviewType != issues_model.ReviewTypeApprove && reviewType != issues_model.ReviewTypeReject {
+	if !reviewType.AffectReview() {
 		stale = false
 	} else {
 		headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
@@ -368,7 +368,7 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string,
 		return nil, err
 	}
 
-	if review.Type != issues_model.ReviewTypeApprove && review.Type != issues_model.ReviewTypeReject {
+	if !review.Type.AffectReview() {
 		return nil, fmt.Errorf("not need to dismiss this review because it's type is not Approve or change request")
 	}
 

From 1c27639d576933d1dc3b3aa532b7b5ba91c77ff8 Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Wed, 20 Dec 2023 02:09:23 +0100
Subject: [PATCH 04/28] rename func to be more apropriate

---
 models/issues/review.go         | 6 +++---
 models/issues/review_test.go    | 6 +++---
 routers/web/repo/pull.go        | 2 +-
 routers/web/repo/pull_review.go | 2 +-
 services/pull/review.go         | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 8f5132ea9bdb..eeda540f007e 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -311,8 +311,8 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 	return review, db.Insert(ctx, review)
 }
 
-// GetCurrentReview returns the current pending review of reviewer for given issue
-func GetCurrentReview(ctx context.Context, reviewer *user_model.User, issue *Issue) (*Review, error) {
+// GetCurrentPendingReview returns the current pending review of reviewer for given issue
+func GetCurrentPendingReview(ctx context.Context, reviewer *user_model.User, issue *Issue) (*Review, error) {
 	if reviewer == nil {
 		return nil, nil
 	}
@@ -361,7 +361,7 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 
 	official := false
 
-	review, err := GetCurrentReview(ctx, doer, issue)
+	review, err := GetCurrentPendingReview(ctx, doer, issue)
 	if err != nil {
 		if !IsErrReviewNotExist(err) {
 			return nil, nil, err
diff --git a/models/issues/review_test.go b/models/issues/review_test.go
index 1868cb1bfab2..b2cda9d7866a 100644
--- a/models/issues/review_test.go
+++ b/models/issues/review_test.go
@@ -84,19 +84,19 @@ func TestFindLatestReviews(t *testing.T) {
 	assert.Equal(t, "singular review from org6 and final review for this pr", reviews[1].Content)
 }
 
-func TestGetCurrentReview(t *testing.T) {
+func TestGetCurrentPendingReview(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 
-	review, err := issues_model.GetCurrentReview(db.DefaultContext, user, issue)
+	review, err := issues_model.GetCurrentPendingReview(db.DefaultContext, user, issue)
 	assert.NoError(t, err)
 	assert.NotNil(t, review)
 	assert.Equal(t, issues_model.ReviewTypePending, review.Type)
 	assert.Equal(t, "Pending Review", review.Content)
 
 	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 7})
-	review2, err := issues_model.GetCurrentReview(db.DefaultContext, user2, issue)
+	review2, err := issues_model.GetCurrentPendingReview(db.DefaultContext, user2, issue)
 	assert.Error(t, err)
 	assert.True(t, issues_model.IsErrReviewNotExist(err))
 	assert.Nil(t, review2)
diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go
index ec109ed665c4..4703bc120828 100644
--- a/routers/web/repo/pull.go
+++ b/routers/web/repo/pull.go
@@ -979,7 +979,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
 		return
 	}
 
-	currentReview, err := issues_model.GetCurrentReview(ctx, ctx.Doer, issue)
+	currentReview, err := issues_model.GetCurrentPendingReview(ctx, ctx.Doer, issue)
 	if err != nil && !issues_model.IsErrReviewNotExist(err) {
 		ctx.ServerError("GetCurrentReview", err)
 		return
diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go
index 47d490a2bcf8..1f6427754e63 100644
--- a/routers/web/repo/pull_review.go
+++ b/routers/web/repo/pull_review.go
@@ -34,7 +34,7 @@ func RenderNewCodeCommentForm(ctx *context.Context) {
 	if !issue.IsPull {
 		return
 	}
-	currentReview, err := issues_model.GetCurrentReview(ctx, ctx.Doer, issue)
+	currentReview, err := issues_model.GetCurrentPendingReview(ctx, ctx.Doer, issue)
 	if err != nil && !issues_model.IsErrReviewNotExist(err) {
 		ctx.ServerError("GetCurrentReview", err)
 		return
diff --git a/services/pull/review.go b/services/pull/review.go
index 968c473a5b76..20a5dc73a8bc 100644
--- a/services/pull/review.go
+++ b/services/pull/review.go
@@ -118,7 +118,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 		return comment, nil
 	}
 
-	review, err := issues_model.GetCurrentReview(ctx, doer, issue)
+	review, err := issues_model.GetCurrentPendingReview(ctx, doer, issue)
 	if err != nil {
 		if !issues_model.IsErrReviewNotExist(err) {
 			return nil, err

From d248a0a5f100a375f113b30a7bf8fac9e6630aad Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Wed, 20 Dec 2023 02:09:59 +0100
Subject: [PATCH 05/28] use introduced GetReviewOption

---
 models/issues/pull.go   | 36 ++++++++++++---------
 models/issues/review.go | 69 +++++++++++++++++++++++++++++++++++------
 2 files changed, 80 insertions(+), 25 deletions(-)

diff --git a/models/issues/pull.go b/models/issues/pull.go
index 34bea921a0d2..e97899cd6e1c 100644
--- a/models/issues/pull.go
+++ b/models/issues/pull.go
@@ -797,14 +797,16 @@ func HasEnoughApprovals(ctx context.Context, protectBranch *git_model.ProtectedB
 
 // GetGrantedApprovalsCount returns the number of granted approvals for pr. A granted approval must be authored by a user in an approval whitelist.
 func GetGrantedApprovalsCount(ctx context.Context, protectBranch *git_model.ProtectedBranch, pr *PullRequest) int64 {
-	sess := db.GetEngine(ctx).Where("issue_id = ?", pr.IssueID).
-		And("type = ?", ReviewTypeApprove).
-		And("official = ?", true).
-		And("dismissed = ?", false)
+	opt := &GetReviewOption{
+		Dismissed: util.OptionalBoolFalse,
+		Official:  util.OptionalBoolTrue,
+		Types:     []ReviewType{ReviewTypeApprove},
+		IssueID:   pr.IssueID,
+	}
 	if protectBranch.DismissStaleApprovals {
-		sess = sess.And("stale = ?", false)
+		opt.Stale = util.OptionalBoolFalse
 	}
-	approvals, err := sess.Count(new(Review))
+	approvals, err := db.GetEngine(ctx).Where(opt.toCond()).Count(new(Review))
 	if err != nil {
 		log.Error("GetGrantedApprovalsCount: %v", err)
 		return 0
@@ -818,11 +820,13 @@ func MergeBlockedByRejectedReview(ctx context.Context, protectBranch *git_model.
 	if !protectBranch.BlockOnRejectedReviews {
 		return false
 	}
-	rejectExist, err := db.GetEngine(ctx).Where("issue_id = ?", pr.IssueID).
-		And("type = ?", ReviewTypeReject).
-		And("official = ?", true).
-		And("dismissed = ?", false).
-		Exist(new(Review))
+	opt := &GetReviewOption{
+		Dismissed: util.OptionalBoolFalse,
+		Official:  util.OptionalBoolTrue,
+		Types:     []ReviewType{ReviewTypeReject},
+		IssueID:   pr.IssueID,
+	}
+	rejectExist, err := db.GetEngine(ctx).Where(opt.toCond()).Exist(new(Review))
 	if err != nil {
 		log.Error("MergeBlockedByRejectedReview: %v", err)
 		return true
@@ -837,10 +841,12 @@ func MergeBlockedByOfficialReviewRequests(ctx context.Context, protectBranch *gi
 	if !protectBranch.BlockOnOfficialReviewRequests {
 		return false
 	}
-	has, err := db.GetEngine(ctx).Where("issue_id = ?", pr.IssueID).
-		And("type = ?", ReviewTypeRequest).
-		And("official = ?", true).
-		Exist(new(Review))
+	opt := &GetReviewOption{
+		Official: util.OptionalBoolTrue,
+		Types:    []ReviewType{ReviewTypeRequest},
+		IssueID:  pr.IssueID,
+	}
+	has, err := db.GetEngine(ctx).Where(opt.toCond()).Exist(new(Review))
 	if err != nil {
 		log.Error("MergeBlockedByOfficialReviewRequests: %v", err)
 		return true
diff --git a/models/issues/review.go b/models/issues/review.go
index eeda540f007e..066e432bb23d 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -461,15 +461,63 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 	return review, comm, committer.Commit()
 }
 
+type GetReviewOption struct {
+	Dismissed  util.OptionalBool
+	Official   util.OptionalBool
+	Stale      util.OptionalBool
+	Types      []ReviewType
+	IssueID    int64
+	ReviewerID int64
+	TeamID     int64
+}
+
+func (o *GetReviewOption) toCond() builder.Cond {
+	cond := builder.And(builder.Eq{"review.issue_id": o.IssueID})
+
+	if !o.Dismissed.IsNone() {
+		cond = cond.And(builder.Eq{"review.dismissed": o.Dismissed.IsTrue()})
+	}
+	if !o.Official.IsNone() {
+		cond = cond.And(builder.Eq{"review.official": o.Official.IsTrue()})
+	}
+	if !o.Stale.IsNone() {
+		cond = cond.And(builder.Eq{"review.stale": o.Stale.IsTrue()})
+	}
+	if len(o.Types) != 0 {
+		cond = cond.And(builder.In("review.type", o.Types))
+	}
+	if o.ReviewerID > 0 {
+		cond = cond.And(builder.Eq{"review.reviewer_id": o.ReviewerID, "original_author_id": 0})
+	}
+	if o.TeamID > 0 {
+		cond = cond.And(builder.Eq{"review.reviewer_team_id": o.TeamID})
+	}
+
+	return cond
+}
+
+// GetReviewByOption get the latest review for a pull request filtered by given option
+func GetReviewByOption(ctx context.Context, opt *GetReviewOption) (*Review, error) {
+	review := new(Review)
+	has, err := db.GetEngine(ctx).Where(opt.toCond()).OrderBy("id").Desc().Get(review)
+	if err != nil {
+		return nil, err
+	}
+	if !has {
+		return nil, ErrReviewNotExist{}
+	}
+	return review, nil
+}
+
 // GetReviewByIssueIDAndUserID get the latest review of reviewer for a pull request
 func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64) (*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})).
-		OrderBy("id").Desc().
-		Get(review)
+	opt := &GetReviewOption{
+		Types:      []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest},
+		IssueID:    issueID,
+		ReviewerID: userID,
+	}
+	has, err := db.GetEngine(ctx).Where(opt.toCond()).OrderBy("id").Desc().Get(review)
 	if err != nil {
 		return nil, err
 	}
@@ -484,10 +532,11 @@ func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64) (*R
 // GetTeamReviewerByIssueIDAndTeamID get the latest review request of reviewer team for a pull request
 func GetTeamReviewerByIssueIDAndTeamID(ctx context.Context, issueID, teamID int64) (*Review, error) {
 	review := new(Review)
-
-	has, err := db.GetEngine(ctx).Where(builder.Eq{"issue_id": issueID, "reviewer_team_id": teamID}).
-		OrderBy("id").Desc().
-		Get(review)
+	opt := &GetReviewOption{
+		IssueID: issueID,
+		TeamID:  teamID,
+	}
+	has, err := db.GetEngine(ctx).Where(opt.toCond()).OrderBy("id").Desc().Get(review)
 	if err != nil {
 		return nil, err
 	}

From 9d44369bfc980319ad3e3dc7ce43dd532ac36b1b Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Wed, 20 Dec 2023 02:10:44 +0100
Subject: [PATCH 06/28] CreateReview now make sure to clean up and not leafe
 reviews in strange state

---
 models/issues/review.go | 42 +++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 066e432bb23d..d4bb509fe45c 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -143,6 +143,7 @@ func (r *Review) LoadCodeComments(ctx context.Context) (err error) {
 	if r.CodeComments != nil {
 		return err
 	}
+
 	if err = r.loadIssue(ctx); err != nil {
 		return err
 	}
@@ -289,8 +290,14 @@ func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organizatio
 
 // CreateReview creates a new review based on opts
 func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error) {
+	ctx, committer, err := db.TxContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+	defer committer.Close()
+	sess := db.GetEngine(ctx)
+
 	review := &Review{
-		Type:         opts.Type,
 		Issue:        opts.Issue,
 		IssueID:      opts.Issue.ID,
 		Reviewer:     opts.Reviewer,
@@ -300,15 +307,38 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 		CommitID:     opts.CommitID,
 		Stale:        opts.Stale,
 	}
-	if opts.Reviewer != nil {
+
+	switch {
+	case opts.Reviewer != nil:
+		review.Type = opts.Type
 		review.ReviewerID = opts.Reviewer.ID
-	} else {
-		if review.Type != ReviewTypeRequest {
-			review.Type = ReviewTypeRequest
+
+		opt := &GetReviewOption{
+			ReviewerID: opts.Reviewer.ID,
+			IssueID:    opts.Issue.ID,
 		}
+		// make sure user review requests are cleared
+		sess.Where(opt.toCond().And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review))
+		// make sure if the created review gets dismissed no old review surface
+		if opts.Type.AffectReview() {
+			if _, err := sess.Where(opt.toCond().And(builder.In("type", ReviewTypeApprove, ReviewTypeReject))).
+				Cols("dismissed").Update(&Review{Dismissed: true}); err != nil {
+				return nil, err
+			}
+		}
+
+	case opts.ReviewerTeam != nil:
+		review.Type = ReviewTypeRequest
 		review.ReviewerTeamID = opts.ReviewerTeam.ID
+
+	default:
+		return nil, fmt.Errorf("provide either reviewer or reviewer team")
+	}
+
+	if _, err := sess.Insert(review); err != nil {
+		return nil, err
 	}
-	return review, db.Insert(ctx, review)
+	return review, committer.Commit()
 }
 
 // GetCurrentPendingReview returns the current pending review of reviewer for given issue

From 9f34f0314809eced0eee0082cd7d782cf18c60ac Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Wed, 20 Dec 2023 02:10:57 +0100
Subject: [PATCH 07/28] note todo

---
 models/issues/review_list.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/models/issues/review_list.go b/models/issues/review_list.go
index ed3d0bd028a0..4073c6d01d4a 100644
--- a/models/issues/review_list.go
+++ b/models/issues/review_list.go
@@ -151,6 +151,7 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, error)
 	sess := db.GetEngine(ctx)
 
 	// Get latest review of each reviewer, sorted in order they were made
+	// TODO: use FindLatestReviews()
 	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) AND dismissed = ? AND original_author_id = 0 GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
 		issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest, false).
 		Find(&reviews); err != nil {
@@ -158,6 +159,7 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, error)
 	}
 
 	teamReviewRequests := make([]*Review, 0, 5)
+	// TODO: use FindLatestReviews()
 	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id <> 0 AND original_author_id = 0 GROUP BY issue_id, reviewer_team_id) ORDER BY review.updated_unix ASC",
 		issueID).
 		Find(&teamReviewRequests); err != nil {

From 189db7309fe6b395c5931f658c15b49a19711e72 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Wed, 20 Dec 2023 02:15:06 +0100
Subject: [PATCH 08/28] Update models/issues/review.go

---
 models/issues/review.go | 1 -
 1 file changed, 1 deletion(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index d4bb509fe45c..bb95e6e45afe 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -143,7 +143,6 @@ func (r *Review) LoadCodeComments(ctx context.Context) (err error) {
 	if r.CodeComments != nil {
 		return err
 	}
-
 	if err = r.loadIssue(ctx); err != nil {
 		return err
 	}

From 70b1b6d4c16d97f13edbf7be84a2c0917c615caa Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Wed, 20 Dec 2023 02:49:24 +0100
Subject: [PATCH 09/28] remove ReviewID on CommentTypeReviewRequest as its not
 important and review will be deleted later anyway

---
 models/issues/review.go | 1 -
 1 file changed, 1 deletion(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index bb95e6e45afe..4716de126a4a 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -698,7 +698,6 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 		Issue:           issue,
 		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 		AssigneeID:      reviewer.ID, // Use AssigneeID as reviewer ID
-		ReviewID:        review.ID,
 	})
 	if err != nil {
 		return nil, err

From 1ace5703043100edd06738fdf39d41d164a04c57 Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Wed, 20 Dec 2023 03:02:29 +0100
Subject: [PATCH 10/28] wrap errors

---
 models/issues/comment_list.go | 3 +++
 services/issue/assignee.go    | 9 +++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go
index 93af45870ed6..a98e66ed5fde 100644
--- a/models/issues/comment_list.go
+++ b/models/issues/comment_list.go
@@ -428,6 +428,9 @@ func (comments CommentList) loadReviews(ctx context.Context) error {
 	}
 
 	for _, comment := range comments {
+		if comment.Type == CommentTypeReviewRequest {
+			continue
+		}
 		comment.Review = reviews[comment.ReviewID]
 		if comment.Review == nil {
 			if comment.ReviewID > 0 {
diff --git a/services/issue/assignee.go b/services/issue/assignee.go
index 27fc695533e3..dfefb011e3a1 100644
--- a/services/issue/assignee.go
+++ b/services/issue/assignee.go
@@ -5,6 +5,7 @@ package issue
 
 import (
 	"context"
+	"fmt"
 
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/organization"
@@ -97,20 +98,20 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
 
 	permReviewer, err := access_model.GetUserRepoPermission(ctx, issue.Repo, reviewer)
 	if err != nil {
-		return err
+		return fmt.Errorf("GetUserRepoPermission: %w", err)
 	}
 
 	if permDoer == nil {
 		permDoer = new(access_model.Permission)
 		*permDoer, err = access_model.GetUserRepoPermission(ctx, issue.Repo, doer)
 		if err != nil {
-			return err
+			return fmt.Errorf("GetUserRepoPermission: %w", err)
 		}
 	}
 
 	lastreview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID)
 	if err != nil && !issues_model.IsErrReviewNotExist(err) {
-		return err
+		return fmt.Errorf("GetReviewByIssueIDAndUserID: %w", err)
 	}
 
 	var pemResult bool
@@ -135,7 +136,7 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
 		if !pemResult {
 			pemResult, err = issues_model.IsOfficialReviewer(ctx, issue, doer)
 			if err != nil {
-				return err
+				return fmt.Errorf("IsOfficialReviewer :%w", err)
 			}
 			if !pemResult {
 				return issues_model.ErrNotValidReviewRequest{

From fd51505cf0b4e77ba17615ba190719eabd54df76 Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Wed, 20 Dec 2023 03:13:00 +0100
Subject: [PATCH 11/28] finish

---
 models/issues/review.go | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 4716de126a4a..3d0dedea4abb 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -317,7 +317,9 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 			IssueID:    opts.Issue.ID,
 		}
 		// make sure user review requests are cleared
-		sess.Where(opt.toCond().And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review))
+		if _, err := sess.Where(opt.toCond().And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil {
+			return nil, err
+		}
 		// make sure if the created review gets dismissed no old review surface
 		if opts.Type.AffectReview() {
 			if _, err := sess.Where(opt.toCond().And(builder.In("type", ReviewTypeApprove, ReviewTypeReject))).
@@ -546,7 +548,7 @@ func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64) (*R
 		IssueID:    issueID,
 		ReviewerID: userID,
 	}
-	has, err := db.GetEngine(ctx).Where(opt.toCond()).OrderBy("id").Desc().Get(review)
+	has, err := db.GetEngine(ctx).Where(opt.toCond()).Desc("review.id").Get(review)
 	if err != nil {
 		return nil, err
 	}
@@ -565,7 +567,7 @@ func GetTeamReviewerByIssueIDAndTeamID(ctx context.Context, issueID, teamID int6
 		IssueID: issueID,
 		TeamID:  teamID,
 	}
-	has, err := db.GetEngine(ctx).Where(opt.toCond()).OrderBy("id").Desc().Get(review)
+	has, err := db.GetEngine(ctx).Where(opt.toCond()).Desc("review.id").Get(review)
 	if err != nil {
 		return nil, err
 	}

From 448184167eff1019e36964074ff4cac090b35af3 Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Wed, 20 Dec 2023 03:14:08 +0100
Subject: [PATCH 12/28] not use anymore

---
 models/issues/review.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 3d0dedea4abb..448d7ca30218 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -682,7 +682,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 		}
 	}
 
-	review, err = CreateReview(ctx, CreateReviewOptions{
+	_, err = CreateReview(ctx, CreateReviewOptions{
 		Type:     ReviewTypeRequest,
 		Issue:    issue,
 		Reviewer: reviewer,

From 2bba3bbeeb24f180a1c56a44dc008c4b09437966 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Wed, 20 Dec 2023 11:56:27 +0100
Subject: [PATCH 13/28] RequestReview get deleted on review. So we dont have to
 try to load them on comments.

---
 models/issues/comment_list.go | 3 +++
 models/issues/review.go       | 3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go
index 93af45870ed6..a98e66ed5fde 100644
--- a/models/issues/comment_list.go
+++ b/models/issues/comment_list.go
@@ -428,6 +428,9 @@ func (comments CommentList) loadReviews(ctx context.Context) error {
 	}
 
 	for _, comment := range comments {
+		if comment.Type == CommentTypeReviewRequest {
+			continue
+		}
 		comment.Review = reviews[comment.ReviewID]
 		if comment.Review == nil {
 			if comment.ReviewID > 0 {
diff --git a/models/issues/review.go b/models/issues/review.go
index 3db73a09ebcb..4437d1629518 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -595,7 +595,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 		}
 	}
 
-	review, err = CreateReview(ctx, CreateReviewOptions{
+	_, err = CreateReview(ctx, CreateReviewOptions{
 		Type:     ReviewTypeRequest,
 		Issue:    issue,
 		Reviewer: reviewer,
@@ -613,7 +613,6 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 		Issue:           issue,
 		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 		AssigneeID:      reviewer.ID, // Use AssigneeID as reviewer ID
-		ReviewID:        review.ID,
 	})
 	if err != nil {
 		return nil, err

From cf0f77dcf3fa6e4bc041bba7a0cac25aa7688760 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Wed, 20 Dec 2023 12:28:25 +0100
Subject: [PATCH 14/28] Clean up old reviews on creating a new one

---
 models/issues/review.go | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 3db73a09ebcb..c28a2d107836 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -284,8 +284,14 @@ func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organizatio
 
 // CreateReview creates a new review based on opts
 func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error) {
+	ctx, committer, err := db.TxContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+	defer committer.Close()
+	sess := db.GetEngine(ctx)
+
 	review := &Review{
-		Type:         opts.Type,
 		Issue:        opts.Issue,
 		IssueID:      opts.Issue.ID,
 		Reviewer:     opts.Reviewer,
@@ -295,15 +301,37 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 		CommitID:     opts.CommitID,
 		Stale:        opts.Stale,
 	}
-	if opts.Reviewer != nil {
+
+	switch {
+	case opts.Reviewer != nil:
+		review.Type = opts.Type
 		review.ReviewerID = opts.Reviewer.ID
-	} else {
-		if review.Type != ReviewTypeRequest {
-			review.Type = ReviewTypeRequest
+
+		reviewCond := builder.Eq{"reviewer_id": opts.Reviewer.ID, "issue_id": opts.Issue.ID}
+		// make sure user review requests are cleared
+		if _, err := sess.Where(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil {
+			return nil, err
 		}
+		// make sure if the created review gets dismissed no old review surface
+		if opts.Type == ReviewTypeApprove || opts.Type == ReviewTypeReject {
+			if _, err := sess.Where(reviewCond.And(builder.In("type", ReviewTypeApprove, ReviewTypeReject))).
+				Cols("dismissed").Update(&Review{Dismissed: true}); err != nil {
+				return nil, err
+			}
+		}
+
+	case opts.ReviewerTeam != nil:
+		review.Type = ReviewTypeRequest
 		review.ReviewerTeamID = opts.ReviewerTeam.ID
+
+	default:
+		return nil, fmt.Errorf("provide either reviewer or reviewer team")
+	}
+
+	if _, err := sess.Insert(review); err != nil {
+		return nil, err
 	}
-	return review, db.Insert(ctx, review)
+	return review, committer.Commit()
 }
 
 // GetCurrentReview returns the current pending review of reviewer for given issue

From 3c21bd769cc2acef073715ee38259f079e8874a4 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Wed, 20 Dec 2023 12:49:32 +0100
Subject: [PATCH 15/28] func caller use the created comment to retrieve created
 review too

---
 models/issues/comment.go | 3 +++
 models/issues/review.go  | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/models/issues/comment.go b/models/issues/comment.go
index ba5aed9c652e..97255dffeea4 100644
--- a/models/issues/comment.go
+++ b/models/issues/comment.go
@@ -696,6 +696,9 @@ func (c *Comment) LoadReactions(ctx context.Context, repo *repo_model.Repository
 }
 
 func (c *Comment) loadReview(ctx context.Context) (err error) {
+	if c.ReviewID == 0 {
+		return nil
+	}
 	if c.Review == nil {
 		if c.Review, err = GetReviewByID(ctx, c.ReviewID); err != nil {
 			return err
diff --git a/models/issues/review.go b/models/issues/review.go
index 4437d1629518..7b47682b2b3e 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -595,7 +595,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 		}
 	}
 
-	_, err = CreateReview(ctx, CreateReviewOptions{
+	review, err = CreateReview(ctx, CreateReviewOptions{
 		Type:     ReviewTypeRequest,
 		Issue:    issue,
 		Reviewer: reviewer,
@@ -618,6 +618,9 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 		return nil, err
 	}
 
+	// func caller use the created comment to retrieve created review too.
+	comment.Review = review
+
 	return comment, committer.Commit()
 }
 

From aa70e2ef015ec7af0a4ed039fb58f5e2111b4c73 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Wed, 20 Dec 2023 19:52:33 +0100
Subject: [PATCH 16/28] fix conflict

---
 models/issues/review.go | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 1e9ecd4bf548..448d7ca30218 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -543,21 +543,12 @@ func GetReviewByOption(ctx context.Context, opt *GetReviewOption) (*Review, erro
 // GetReviewByIssueIDAndUserID get the latest review of reviewer for a pull request
 func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64) (*Review, error) {
 	review := new(Review)
-<<<<<<< HEAD
 	opt := &GetReviewOption{
 		Types:      []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest},
 		IssueID:    issueID,
 		ReviewerID: userID,
 	}
 	has, err := db.GetEngine(ctx).Where(opt.toCond()).Desc("review.id").Get(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})).
-		Desc("id").
-		Get(review)
->>>>>>> main
 	if err != nil {
 		return nil, err
 	}
@@ -572,18 +563,11 @@ func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64) (*R
 // GetTeamReviewerByIssueIDAndTeamID get the latest review request of reviewer team for a pull request
 func GetTeamReviewerByIssueIDAndTeamID(ctx context.Context, issueID, teamID int64) (*Review, error) {
 	review := new(Review)
-<<<<<<< HEAD
 	opt := &GetReviewOption{
 		IssueID: issueID,
 		TeamID:  teamID,
 	}
 	has, err := db.GetEngine(ctx).Where(opt.toCond()).Desc("review.id").Get(review)
-=======
-
-	has, err := db.GetEngine(ctx).Where(builder.Eq{"issue_id": issueID, "reviewer_team_id": teamID}).
-		Desc("id").
-		Get(review)
->>>>>>> main
 	if err != nil {
 		return nil, err
 	}

From e08e30685ba2cadfaf777884e55379ac6360e170 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Thu, 21 Dec 2023 09:58:03 +0100
Subject: [PATCH 17/28] else if

---
 models/issues/review.go | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index b1962925d544..6b6dec7323d6 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -302,8 +302,7 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 		Stale:        opts.Stale,
 	}
 
-	switch {
-	case opts.Reviewer != nil:
+	if opts.Reviewer != nil {
 		review.Type = opts.Type
 		review.ReviewerID = opts.Reviewer.ID
 
@@ -320,11 +319,11 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 			}
 		}
 
-	case opts.ReviewerTeam != nil:
+	} else if opts.ReviewerTeam != nil {
 		review.Type = ReviewTypeRequest
 		review.ReviewerTeamID = opts.ReviewerTeam.ID
 
-	default:
+	} else {
 		return nil, fmt.Errorf("provide either reviewer or reviewer team")
 	}
 

From 8a06ccb37dfbe8104e9a555899d8edd539739814 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Sat, 23 Dec 2023 14:09:36 +0100
Subject: [PATCH 18/28] handle ReviewTypePending

---
 models/issues/review.go | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 6b6dec7323d6..7ec39e59fc58 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -308,8 +308,10 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 
 		reviewCond := builder.Eq{"reviewer_id": opts.Reviewer.ID, "issue_id": opts.Issue.ID}
 		// make sure user review requests are cleared
-		if _, err := sess.Where(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil {
-			return nil, err
+		if ots.Type != ReviewTypePending {
+			if _, err := sess.Where(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil {
+				return nil, err
+			}
 		}
 		// make sure if the created review gets dismissed no old review surface
 		if opts.Type == ReviewTypeApprove || opts.Type == ReviewTypeReject {

From 9fcea4ca0a3df53e3a89e8ce1efc1922df927fae Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Sat, 23 Dec 2023 14:14:18 +0100
Subject: [PATCH 19/28] Update models/issues/review.go

---
 models/issues/review.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 7ec39e59fc58..f66f305fdc7f 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -308,7 +308,7 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 
 		reviewCond := builder.Eq{"reviewer_id": opts.Reviewer.ID, "issue_id": opts.Issue.ID}
 		// make sure user review requests are cleared
-		if ots.Type != ReviewTypePending {
+		if opts.Type != ReviewTypePending {
 			if _, err := sess.Where(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil {
 				return nil, err
 			}

From 372922183a93ba0dd3ee0f2dfd1505084a7d02b4 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Sat, 30 Dec 2023 22:40:57 +0100
Subject: [PATCH 20/28] add back

---
 models/issues/review.go | 1 +
 1 file changed, 1 insertion(+)

diff --git a/models/issues/review.go b/models/issues/review.go
index 3737311f6ba4..32d61f36b438 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -615,6 +615,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 		Issue:           issue,
 		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 		AssigneeID:      reviewer.ID, // Use AssigneeID as reviewer ID
+		ReviewID:        review.ID,
 	})
 	if err != nil {
 		return nil, err

From ec62290945d03e697c0947623110c780cac75109 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Sat, 13 Jan 2024 02:44:31 +0100
Subject: [PATCH 21/28] add note why we do skip it

---
 models/issues/comment_list.go | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go
index a98e66ed5fde..d664259687de 100644
--- a/models/issues/comment_list.go
+++ b/models/issues/comment_list.go
@@ -428,6 +428,9 @@ func (comments CommentList) loadReviews(ctx context.Context) error {
 	}
 
 	for _, comment := range comments {
+		// skip review request as the comment struct already has all the info needed to display the infos.
+		// also it would throw errors for all review requests who has been replaced by actual reviews
+		// as they do not exist in database anymore.
 		if comment.Type == CommentTypeReviewRequest {
 			continue
 		}

From 4d4c6d709292e827a11b10797164ddb00ab32748 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Fri, 19 Jan 2024 23:34:19 +0100
Subject: [PATCH 22/28] fix merge conflict resolve

---
 models/issues/pull.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/models/issues/pull.go b/models/issues/pull.go
index ec1132b61dea..f0dc34fe0f3f 100644
--- a/models/issues/pull.go
+++ b/models/issues/pull.go
@@ -803,7 +803,7 @@ func GetGrantedApprovalsCount(ctx context.Context, protectBranch *git_model.Prot
 		Types:     []ReviewType{ReviewTypeApprove},
 		IssueID:   pr.IssueID,
 	}
-	if protectBranch.DismissStaleApprovals {
+	if protectBranch.IgnoreStaleApprovals {
 		opt.Stale = util.OptionalBoolFalse
 	}
 	approvals, err := db.GetEngine(ctx).Where(opt.toCond()).Count(new(Review))

From 34ea361e66e3ec963b3e29fdd3739b916a4c8461 Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Mon, 12 Feb 2024 22:15:21 +0100
Subject: [PATCH 23/28] ignore error

---
 models/issues/comment.go | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/models/issues/comment.go b/models/issues/comment.go
index 2cdb5d3e2578..a586caf1b57e 100644
--- a/models/issues/comment.go
+++ b/models/issues/comment.go
@@ -700,6 +700,10 @@ func (c *Comment) loadReview(ctx context.Context) (err error) {
 	}
 	if c.Review == nil {
 		if c.Review, err = GetReviewByID(ctx, c.ReviewID); err != nil {
+			// review request which has been replaced by actual reviews doesn't exist in database anymore, so ignorem them.
+			if c.Type == CommentTypeReviewRequest {
+				return nil
+			}
 			return err
 		}
 	}

From ba24e2699ab658ce3b161365cb5b1094cf551fbe Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Mon, 12 Feb 2024 22:40:20 +0100
Subject: [PATCH 24/28] ignore error msg

---
 models/issues/comment_list.go | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go
index d664259687de..9f62fa150f88 100644
--- a/models/issues/comment_list.go
+++ b/models/issues/comment_list.go
@@ -436,7 +436,8 @@ func (comments CommentList) loadReviews(ctx context.Context) error {
 		}
 		comment.Review = reviews[comment.ReviewID]
 		if comment.Review == nil {
-			if comment.ReviewID > 0 {
+			// review request which has been replaced by actual reviews doesn't exist in database anymore, so don't log errors for them.
+			if comment.ReviewID > 0 && comment.Type != CommentTypeReviewRequest {
 				log.Error("comment with review id [%d] but has no review record", comment.ReviewID)
 			}
 			continue

From 2d0d3b43d43a0921a4f2ca276888f127599a2730 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Mon, 12 Feb 2024 22:42:09 +0100
Subject: [PATCH 25/28] Update models/issues/comment_list.go

---
 models/issues/comment_list.go | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go
index 9f62fa150f88..cb7df3270d6a 100644
--- a/models/issues/comment_list.go
+++ b/models/issues/comment_list.go
@@ -428,12 +428,6 @@ func (comments CommentList) loadReviews(ctx context.Context) error {
 	}
 
 	for _, comment := range comments {
-		// skip review request as the comment struct already has all the info needed to display the infos.
-		// also it would throw errors for all review requests who has been replaced by actual reviews
-		// as they do not exist in database anymore.
-		if comment.Type == CommentTypeReviewRequest {
-			continue
-		}
 		comment.Review = reviews[comment.ReviewID]
 		if comment.Review == nil {
 			// review request which has been replaced by actual reviews doesn't exist in database anymore, so don't log errors for them.

From d4049d9c3c7eccb2a0e7b5786d6916e10b4dbfdf Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Thu, 15 Feb 2024 18:15:34 +0100
Subject: [PATCH 26/28] return result for unittest.AssertCount* helper func

---
 models/unittest/unit_tests.go | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/models/unittest/unit_tests.go b/models/unittest/unit_tests.go
index d47bceea1ea1..75898436fc52 100644
--- a/models/unittest/unit_tests.go
+++ b/models/unittest/unit_tests.go
@@ -131,8 +131,8 @@ func AssertSuccessfulInsert(t assert.TestingT, beans ...any) {
 }
 
 // AssertCount assert the count of a bean
-func AssertCount(t assert.TestingT, bean, expected any) {
-	assert.EqualValues(t, expected, GetCount(t, bean))
+func AssertCount(t assert.TestingT, bean, expected any) bool {
+	return assert.EqualValues(t, expected, GetCount(t, bean))
 }
 
 // AssertInt64InRange assert value is in range [low, high]
@@ -150,7 +150,7 @@ func GetCountByCond(t assert.TestingT, tableName string, cond builder.Cond) int6
 }
 
 // AssertCountByCond test the count of database entries matching bean
-func AssertCountByCond(t assert.TestingT, tableName string, cond builder.Cond, expected int) {
-	assert.EqualValues(t, expected, GetCountByCond(t, tableName, cond),
+func AssertCountByCond(t assert.TestingT, tableName string, cond builder.Cond, expected int) bool {
+	return assert.EqualValues(t, expected, GetCountByCond(t, tableName, cond),
 		"Failed consistency test, the counted bean (of table %s) was %+v", tableName, cond)
 }

From 2db7e7c02a3f0b74db52932a2c54a00915250a42 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Thu, 15 Feb 2024 19:32:08 +0100
Subject: [PATCH 27/28] Add TestAPIPullReviewStayDismissed test

---
 tests/integration/api_pull_review_test.go | 116 ++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go
index daa136b21e45..634e43e24ca3 100644
--- a/tests/integration/api_pull_review_test.go
+++ b/tests/integration/api_pull_review_test.go
@@ -13,11 +13,14 @@ import (
 	issues_model "code.gitea.io/gitea/models/issues"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/json"
 	api "code.gitea.io/gitea/modules/structs"
+	issue_service "code.gitea.io/gitea/services/issue"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
+	"xorm.io/builder"
 )
 
 func TestAPIPullReview(t *testing.T) {
@@ -314,3 +317,116 @@ func TestAPIPullReviewRequest(t *testing.T) {
 		AddTokenAuth(token)
 	MakeRequest(t, req, http.StatusNoContent)
 }
+
+func TestAPIPullReviewStayDismissed(t *testing.T) {
+	// This test against issue https://github.com/go-gitea/gitea/issues/28542
+	// where old reviews surface after a review request got dismissed.
+	defer tests.PrepareTestEnv(t)()
+	pullIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3})
+	assert.NoError(t, pullIssue.LoadAttributes(db.DefaultContext))
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: pullIssue.RepoID})
+	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	session2 := loginUser(t, user2.LoginName)
+	token2 := getTokenForLoggedInUser(t, session2, auth_model.AccessTokenScopeWriteRepository)
+	user8 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 8})
+	session8 := loginUser(t, user8.LoginName)
+	token8 := getTokenForLoggedInUser(t, session8, auth_model.AccessTokenScopeWriteRepository)
+
+	// user2 request user8 again
+	req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
+		Reviewers: []string{"user8"},
+	}).AddTokenAuth(token2)
+	MakeRequest(t, req, http.StatusCreated)
+
+	reviewsCountCheck(t,
+		"check we have only one review request",
+		pullIssue.ID, user8.ID, 0, 1, 1, false)
+
+	// user8 reviews it as accept
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
+		Event: "APPROVED",
+		Body:  "lgtm",
+	}).AddTokenAuth(token8)
+	MakeRequest(t, req, http.StatusOK)
+
+	reviewsCountCheck(t,
+		"check we have one valid approval",
+		pullIssue.ID, user8.ID, 0, 0, 1, true)
+
+	// emulate of auto-dismiss lgtm on a protected branch that where a pull just got an update
+	_, err := db.GetEngine(db.DefaultContext).Where("issue_id = ? AND reviewer_id = ?", pullIssue.ID, user8.ID).
+		Cols("dismissed").Update(&issues_model.Review{Dismissed: true})
+	assert.NoError(t, err)
+
+	// user2 request user8 again
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
+		Reviewers: []string{"user8"},
+	}).AddTokenAuth(token2)
+	MakeRequest(t, req, http.StatusCreated)
+
+	reviewsCountCheck(t,
+		"check we have no valid approval and one review request",
+		pullIssue.ID, user8.ID, 1, 1, 2, false)
+
+	// user8 dismiss review
+	_, err = issue_service.ReviewRequest(db.DefaultContext, pullIssue, user8, user8, false)
+	assert.NoError(t, err)
+
+	reviewsCountCheck(t,
+		"check new review request is now dismissed",
+		pullIssue.ID, user8.ID, 1, 0, 1, false)
+
+	// add a new valid approval
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
+		Event: "APPROVED",
+		Body:  "lgtm",
+	}).AddTokenAuth(token8)
+	MakeRequest(t, req, http.StatusOK)
+
+	reviewsCountCheck(t,
+		"check that old reviews requests are deleted",
+		pullIssue.ID, user8.ID, 1, 0, 2, true)
+
+	// now add a change request witch should dismiss the approval
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
+		Event: "REQUEST_CHANGES",
+		Body:  "please change XYZ",
+	}).AddTokenAuth(token8)
+	MakeRequest(t, req, http.StatusOK)
+
+	reviewsCountCheck(t,
+		"check that old reviews are dismissed",
+		pullIssue.ID, user8.ID, 2, 0, 3, false)
+}
+
+func reviewsCountCheck(t *testing.T, name string, issueID, reviewerID int64, expectedDismissed, expectedRequested, expectedTotal int, expectApproval bool) {
+	t.Run(name, func(t *testing.T) {
+		unittest.AssertCountByCond(t, "review", builder.Eq{
+			"issue_id":    issueID,
+			"reviewer_id": reviewerID,
+			"dismissed":   true,
+		}, expectedDismissed)
+
+		unittest.AssertCountByCond(t, "review", builder.Eq{
+			"issue_id":    issueID,
+			"reviewer_id": reviewerID,
+		}, expectedTotal)
+
+		unittest.AssertCountByCond(t, "review", builder.Eq{
+			"issue_id":    issueID,
+			"reviewer_id": reviewerID,
+			"type":        issues_model.ReviewTypeRequest,
+		}, expectedRequested)
+
+		approvalCount := 0
+		if expectApproval {
+			approvalCount = 1
+		}
+		unittest.AssertCountByCond(t, "review", builder.Eq{
+			"issue_id":    issueID,
+			"reviewer_id": reviewerID,
+			"type":        issues_model.ReviewTypeApprove,
+			"dismissed":   false,
+		}, approvalCount)
+	})
+}

From b3d062d3a0aa1bfaae292aebae6970ce0831cbd0 Mon Sep 17 00:00:00 2001
From: "m.huber" <m.huber@kithara.com>
Date: Mon, 19 Feb 2024 13:55:07 +0100
Subject: [PATCH 28/28] test against rerequest and add some codecomments

---
 models/issues/review.go                   |  1 +
 tests/integration/api_pull_review_test.go | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 108efc21b95a..fc110630e0fa 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -322,6 +322,7 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 			}
 		}
 		// make sure if the created review gets dismissed no old review surface
+		// other types can be ignored, as they don't affect branch protection
 		if opts.Type == ReviewTypeApprove || opts.Type == ReviewTypeReject {
 			if _, err := sess.Where(reviewCond.And(builder.In("type", ReviewTypeApprove, ReviewTypeReject))).
 				Cols("dismissed").Update(&Review{Dismissed: true}); err != nil {
diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go
index 634e43e24ca3..ab6d33cd5bee 100644
--- a/tests/integration/api_pull_review_test.go
+++ b/tests/integration/api_pull_review_test.go
@@ -332,9 +332,9 @@ func TestAPIPullReviewStayDismissed(t *testing.T) {
 	session8 := loginUser(t, user8.LoginName)
 	token8 := getTokenForLoggedInUser(t, session8, auth_model.AccessTokenScopeWriteRepository)
 
-	// user2 request user8 again
+	// user2 request user8
 	req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
-		Reviewers: []string{"user8"},
+		Reviewers: []string{user8.LoginName},
 	}).AddTokenAuth(token2)
 	MakeRequest(t, req, http.StatusCreated)
 
@@ -342,6 +342,16 @@ func TestAPIPullReviewStayDismissed(t *testing.T) {
 		"check we have only one review request",
 		pullIssue.ID, user8.ID, 0, 1, 1, false)
 
+	// user2 request user8 again, it is expected to be ignored
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
+		Reviewers: []string{user8.LoginName},
+	}).AddTokenAuth(token2)
+	MakeRequest(t, req, http.StatusCreated)
+
+	reviewsCountCheck(t,
+		"check we have only one review request, even after re-request it again",
+		pullIssue.ID, user8.ID, 0, 1, 1, false)
+
 	// user8 reviews it as accept
 	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
 		Event: "APPROVED",
@@ -360,7 +370,7 @@ func TestAPIPullReviewStayDismissed(t *testing.T) {
 
 	// user2 request user8 again
 	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
-		Reviewers: []string{"user8"},
+		Reviewers: []string{user8.LoginName},
 	}).AddTokenAuth(token2)
 	MakeRequest(t, req, http.StatusCreated)