[WIP] Refactor review related code

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

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
@@ -138,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
 	}
@@ -284,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,
@@ -295,19 +307,42 @@ 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")
 	}
-	return review, db.Insert(ctx, review)
+
+	if _, err := sess.Insert(review); err != nil {
+		return nil, err
+	}
+	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
 	}
@@ -356,7 +391,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
@@ -366,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
@@ -396,7 +431,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 +467,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
@@ -456,13 +491,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).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).
-		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
 	}
@@ -475,13 +560,14 @@ 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)
-
-	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 {
+func GetTeamReviewerByIssueIDAndTeamID(ctx context.Context, issueID, teamID int64) (*Review, error) {
+	review := new(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
 	}
 
@@ -508,7 +594,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
 	}
 
@@ -579,7 +665,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
 	}

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)

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
 	}

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

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
@@ -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 {