diff --git a/models/issues/pull.go b/models/issues/pull.go index 2cb1e1b971e9c..8719a3e8b5a5c 100644 --- a/models/issues/pull.go +++ b/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 diff --git a/models/issues/review.go b/models/issues/review.go index fc110630e0fa8..308393be81cba 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 { @@ -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 @@ -314,17 +319,20 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error review.Type = opts.Type review.ReviewerID = opts.Reviewer.ID - reviewCond := builder.Eq{"reviewer_id": opts.Reviewer.ID, "issue_id": opts.Issue.ID} + 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(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil { + 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 == ReviewTypeApprove || opts.Type == ReviewTypeReject { - if _, err := sess.Where(reviewCond.And(builder.In("type", ReviewTypeApprove, ReviewTypeReject))). + 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 } @@ -344,8 +352,8 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error 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 } @@ -394,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 @@ -404,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 @@ -434,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 @@ -470,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 @@ -494,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 } @@ -517,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 } @@ -548,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 } @@ -619,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 } @@ -635,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, @@ -653,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 diff --git a/models/issues/review_list.go b/models/issues/review_list.go index 282f18b4f7f65..d0328dd448d24 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 { diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 1868cb1bfab25..b2cda9d7866a4 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/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 07d8f4877bb3d..91cf38208ae6c 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/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 } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 7052467e64e7d..bbb4b56c3dd38 100644 --- a/routers/web/repo/pull.go +++ b/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 diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index f84510b39d85d..1762b37755904 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -36,7 +36,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 @@ -210,13 +210,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/issue/assignee.go b/services/issue/assignee.go index 27fc695533e3a..dfefb011e3a16 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{ diff --git a/services/pull/review.go b/services/pull/review.go index d4ea97561253e..cdc8d1571cab5 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -119,7 +119,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 @@ -271,7 +271,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()) @@ -369,7 +369,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") }