[WIP] Refactor review related code

models/issues/pull.go

@@ -798,14 +798,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.IgnoreStaleApprovals {
-		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
@@ -819,11 +821,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
@@ -838,10 +842,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

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 {
@@ -103,11 +108,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
@@ -292,8 +297,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,
@@ -303,19 +314,46 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 		CommitID:     opts.CommitID,
 		Stale:        opts.Stale,
 	}
+
 	if 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
+		if opts.Type != ReviewTypePending {
+			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
+		// other types can be ignored, as they don't affect branch protection
+		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
+			}
+		}
+
+	} else if opts.ReviewerTeam != nil {
+		review.Type = ReviewTypeRequest
 		review.ReviewerTeamID = opts.ReviewerTeam.ID
+
+	} else {
+		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
-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
 	}
@@ -364,7 +402,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
@@ -374,7 +412,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
@@ -404,7 +442,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
@@ -440,7 +478,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
@@ -464,15 +502,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})).
-		Desc("id").
-		Get(review)
+	opt := &GetReviewOption{
+		Types:      []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest},
+		IssueID:    issueID,
+		ReviewerID: userID,
+	}
+	has, err := db.GetEngine(ctx).Where(opt.toCond()).Desc("review.id").Get(review)
 	if err != nil {
 		return nil, err
 	}
@@ -487,10 +573,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}).
-		Desc("id").
-		Get(review)
+	opt := &GetReviewOption{
+		IssueID: issueID,
+		TeamID:  teamID,
+	}
+	has, err := db.GetEngine(ctx).Where(opt.toCond()).Desc("review.id").Get(review)
 	if err != nil {
 		return nil, err
 	}
@@ -518,7 +605,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
 	}
 
@@ -589,7 +676,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
 	}
@@ -605,7 +692,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,
@@ -623,7 +710,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

models/issues/review_list.go

@@ -151,13 +151,15 @@ 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 {
 		return nil, err
 	}
 
 	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 {

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)

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)
 }

routers/api/v1/repo/pull_review.go

@@ -878,7 +878,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
 	}

routers/web/repo/pull.go

@@ -1018,7 +1018,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