From 666038a06df6356a06d309966a925bf00253e3fa Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 27 Jul 2023 10:36:54 +0800 Subject: [PATCH] Fix bug when pushing to a pull request which enabled dismiss approval automatically (#25882) (#26158) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix #25858 Backport #25882 The option `dissmiss stale approvals` was listed on protected branch but never implemented. This PR fixes that. 图片 图片 --- models/issues/issue.go | 2 +- models/issues/pull.go | 14 ++- models/issues/review.go | 139 +--------------------------- models/issues/review_list.go | 172 +++++++++++++++++++++++++++++++++++ models/issues/review_test.go | 16 ++-- services/pull/pull.go | 11 +++ services/pull/review.go | 61 +++++++++++-- 7 files changed, 252 insertions(+), 163 deletions(-) create mode 100644 models/issues/review_list.go diff --git a/models/issues/issue.go b/models/issues/issue.go index 364d53ba31..97f6429d58 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -552,7 +552,7 @@ func GetIssueWithAttrsByID(id int64) (*Issue, error) { // GetIssuesByIDs return issues with the given IDs. func GetIssuesByIDs(ctx context.Context, issueIDs []int64) (IssueList, error) { - issues := make([]*Issue, 0, 10) + issues := make([]*Issue, 0, len(issueIDs)) return issues, db.GetEngine(ctx).In("id", issueIDs).Find(&issues) } diff --git a/models/issues/pull.go b/models/issues/pull.go index 3f37d8d243..444ae49e05 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -314,15 +314,13 @@ func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { return err } - if len(reviews) > 0 { - err = LoadReviewers(ctx, reviews) - if err != nil { - return err - } - for _, review := range reviews { - pr.RequestedReviewers = append(pr.RequestedReviewers, review.Reviewer) - } + if err = reviews.LoadReviewers(ctx); err != nil { + return err } + for _, review := range reviews { + pr.RequestedReviewers = append(pr.RequestedReviewers, review.Reviewer) + } + return nil } diff --git a/models/issues/review.go b/models/issues/review.go index e342ce4d15..83ca7d3d87 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -162,27 +162,6 @@ func (r *Review) LoadReviewer(ctx context.Context) (err error) { return err } -// LoadReviewers loads reviewers -func LoadReviewers(ctx context.Context, reviews []*Review) (err error) { - reviewerIds := make([]int64, len(reviews)) - for i := 0; i < len(reviews); i++ { - reviewerIds[i] = reviews[i].ReviewerID - } - reviewers, err := user_model.GetPossibleUserByIDs(ctx, reviewerIds) - if err != nil { - return err - } - - userMap := make(map[int64]*user_model.User, len(reviewers)) - for _, reviewer := range reviewers { - userMap[reviewer.ID] = reviewer - } - for _, review := range reviews { - review.Reviewer = userMap[review.ReviewerID] - } - return nil -} - // LoadReviewerTeam loads reviewer team func (r *Review) LoadReviewerTeam(ctx context.Context) (err error) { if r.ReviewerTeamID == 0 || r.ReviewerTeam != nil { @@ -236,50 +215,6 @@ func GetReviewByID(ctx context.Context, id int64) (*Review, error) { } } -// FindReviewOptions represent possible filters to find reviews -type FindReviewOptions struct { - db.ListOptions - Type ReviewType - IssueID int64 - ReviewerID int64 - OfficialOnly bool -} - -func (opts *FindReviewOptions) toCond() builder.Cond { - cond := builder.NewCond() - if opts.IssueID > 0 { - cond = cond.And(builder.Eq{"issue_id": opts.IssueID}) - } - if opts.ReviewerID > 0 { - cond = cond.And(builder.Eq{"reviewer_id": opts.ReviewerID}) - } - if opts.Type != ReviewTypeUnknown { - cond = cond.And(builder.Eq{"type": opts.Type}) - } - if opts.OfficialOnly { - cond = cond.And(builder.Eq{"official": true}) - } - return cond -} - -// FindReviews returns reviews passing FindReviewOptions -func FindReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error) { - reviews := make([]*Review, 0, 10) - sess := db.GetEngine(ctx).Where(opts.toCond()) - if opts.Page > 0 { - sess = db.SetSessionPagination(sess, &opts) - } - return reviews, sess. - Asc("created_unix"). - Asc("id"). - Find(&reviews) -} - -// CountReviews returns count of reviews passing FindReviewOptions -func CountReviews(opts FindReviewOptions) (int64, error) { - return db.GetEngine(db.DefaultContext).Where(opts.toCond()).Count(&Review{}) -} - // CreateReviewOptions represent the options to create a review. Type, Issue and Reviewer are required. type CreateReviewOptions struct { Content string @@ -512,76 +447,6 @@ func SubmitReview(doer *user_model.User, issue *Issue, reviewType ReviewType, co return review, comm, committer.Commit() } -// GetReviewOptions represent filter options for GetReviews -type GetReviewOptions struct { - IssueID int64 - ReviewerID int64 - Dismissed util.OptionalBool -} - -// GetReviews return reviews based on GetReviewOptions -func GetReviews(ctx context.Context, opts *GetReviewOptions) ([]*Review, error) { - if opts == nil { - return nil, fmt.Errorf("opts are nil") - } - - sess := db.GetEngine(ctx) - - if opts.IssueID != 0 { - sess = sess.Where("issue_id=?", opts.IssueID) - } - if opts.ReviewerID != 0 { - sess = sess.Where("reviewer_id=?", opts.ReviewerID) - } - if !opts.Dismissed.IsNone() { - sess = sess.Where("dismissed=?", opts.Dismissed.IsTrue()) - } - - reviews := make([]*Review, 0, 4) - return reviews, sess.Find(&reviews) -} - -// GetReviewsByIssueID gets the latest review of each reviewer for a pull request -func GetReviewsByIssueID(issueID int64) ([]*Review, error) { - reviews := make([]*Review, 0, 10) - - sess := db.GetEngine(db.DefaultContext) - - // Get latest review of each reviewer, sorted in order they were made - 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) - 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 { - return nil, err - } - - if len(teamReviewRequests) > 0 { - reviews = append(reviews, teamReviewRequests...) - } - - return reviews, nil -} - -// GetReviewersFromOriginalAuthorsByIssueID gets the latest review of each original authors for a pull request -func GetReviewersFromOriginalAuthorsByIssueID(issueID int64) ([]*Review, error) { - reviews := make([]*Review, 0, 10) - - // Get latest review of each reviewer, sorted in order they were made - if err := db.GetEngine(db.DefaultContext).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 original_author_id <> 0 GROUP BY issue_id, original_author_id) ORDER BY review.updated_unix ASC", - issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest). - Find(&reviews); err != nil { - return nil, err - } - - return reviews, 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) @@ -633,7 +498,7 @@ func MarkReviewsAsNotStale(issueID int64, commitID string) (err error) { } // DismissReview change the dismiss status of a review -func DismissReview(review *Review, isDismiss bool) (err error) { +func DismissReview(ctx context.Context, review *Review, isDismiss bool) (err error) { if review.Dismissed == isDismiss || (review.Type != ReviewTypeApprove && review.Type != ReviewTypeReject) { return nil } @@ -644,7 +509,7 @@ func DismissReview(review *Review, isDismiss bool) (err error) { return ErrReviewNotExist{} } - _, err = db.GetEngine(db.DefaultContext).ID(review.ID).Cols("dismissed").Update(review) + _, err = db.GetEngine(ctx).ID(review.ID).Cols("dismissed").Update(review) return err } diff --git a/models/issues/review_list.go b/models/issues/review_list.go new file mode 100644 index 0000000000..c044fe915a --- /dev/null +++ b/models/issues/review_list.go @@ -0,0 +1,172 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package issues + +import ( + "context" + + "code.gitea.io/gitea/models/db" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/container" + "code.gitea.io/gitea/modules/util" + + "xorm.io/builder" +) + +type ReviewList []*Review + +// LoadReviewers loads reviewers +func (reviews ReviewList) LoadReviewers(ctx context.Context) error { + reviewerIds := make([]int64, len(reviews)) + for i := 0; i < len(reviews); i++ { + reviewerIds[i] = reviews[i].ReviewerID + } + reviewers, err := user_model.GetPossibleUserByIDs(ctx, reviewerIds) + if err != nil { + return err + } + + userMap := make(map[int64]*user_model.User, len(reviewers)) + for _, reviewer := range reviewers { + userMap[reviewer.ID] = reviewer + } + for _, review := range reviews { + review.Reviewer = userMap[review.ReviewerID] + } + return nil +} + +func (reviews ReviewList) LoadIssues(ctx context.Context) error { + issueIds := container.Set[int64]{} + for i := 0; i < len(reviews); i++ { + issueIds.Add(reviews[i].IssueID) + } + + issues, err := GetIssuesByIDs(ctx, issueIds.Values()) + if err != nil { + return err + } + if _, err := issues.LoadRepositories(ctx); err != nil { + return err + } + issueMap := make(map[int64]*Issue, len(issues)) + for _, issue := range issues { + issueMap[issue.ID] = issue + } + + for _, review := range reviews { + review.Issue = issueMap[review.IssueID] + } + return nil +} + +// FindReviewOptions represent possible filters to find reviews +type FindReviewOptions struct { + db.ListOptions + Type ReviewType + IssueID int64 + ReviewerID int64 + OfficialOnly bool + Dismissed util.OptionalBool +} + +func (opts *FindReviewOptions) toCond() builder.Cond { + cond := builder.NewCond() + if opts.IssueID > 0 { + cond = cond.And(builder.Eq{"issue_id": opts.IssueID}) + } + if opts.ReviewerID > 0 { + cond = cond.And(builder.Eq{"reviewer_id": opts.ReviewerID}) + } + if opts.Type != ReviewTypeUnknown { + cond = cond.And(builder.Eq{"type": opts.Type}) + } + if opts.OfficialOnly { + cond = cond.And(builder.Eq{"official": true}) + } + if !opts.Dismissed.IsNone() { + cond = cond.And(builder.Eq{"dismissed": opts.Dismissed.IsTrue()}) + } + return cond +} + +// FindReviews returns reviews passing FindReviewOptions +func FindReviews(ctx context.Context, opts FindReviewOptions) (ReviewList, error) { + reviews := make([]*Review, 0, 10) + sess := db.GetEngine(ctx).Where(opts.toCond()) + if opts.Page > 0 && !opts.IsListAll() { + sess = db.SetSessionPagination(sess, &opts) + } + return reviews, sess. + Asc("created_unix"). + Asc("id"). + Find(&reviews) +} + +// FindLatestReviews returns only latest reviews per user, passing FindReviewOptions +func FindLatestReviews(ctx context.Context, opts FindReviewOptions) (ReviewList, error) { + reviews := make([]*Review, 0, 10) + cond := opts.toCond() + sess := db.GetEngine(ctx).Where(cond) + if opts.Page > 0 { + sess = db.SetSessionPagination(sess, &opts) + } + + sess.In("id", builder. + Select("max ( id ) "). + From("review"). + Where(cond). + GroupBy("reviewer_id")) + + return reviews, sess. + Asc("created_unix"). + Asc("id"). + Find(&reviews) +} + +// CountReviews returns count of reviews passing FindReviewOptions +func CountReviews(opts FindReviewOptions) (int64, error) { + return db.GetEngine(db.DefaultContext).Where(opts.toCond()).Count(&Review{}) +} + +// GetReviewersFromOriginalAuthorsByIssueID gets the latest review of each original authors for a pull request +func GetReviewersFromOriginalAuthorsByIssueID(issueID int64) (ReviewList, error) { + reviews := make([]*Review, 0, 10) + + // Get latest review of each reviewer, sorted in order they were made + if err := db.GetEngine(db.DefaultContext).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 original_author_id <> 0 GROUP BY issue_id, original_author_id) ORDER BY review.updated_unix ASC", + issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest). + Find(&reviews); err != nil { + return nil, err + } + + return reviews, nil +} + +// GetReviewsByIssueID gets the latest review of each reviewer for a pull request +func GetReviewsByIssueID(issueID int64) (ReviewList, error) { + reviews := make([]*Review, 0, 10) + + sess := db.GetEngine(db.DefaultContext) + + // Get latest review of each reviewer, sorted in order they were made + 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) + 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 { + return nil, err + } + + if len(teamReviewRequests) > 0 { + reviews = append(reviews, teamReviewRequests...) + } + + return reviews, nil +} diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 0cb621812c..5b8767cf7e 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -147,7 +147,7 @@ func TestGetReviewersByIssueID(t *testing.T) { allReviews, err = issues_model.GetReviewsByIssueID(issue.ID) assert.NoError(t, err) - assert.NoError(t, issues_model.LoadReviewers(db.DefaultContext, allReviews)) + assert.NoError(t, allReviews.LoadReviewers(db.DefaultContext)) if assert.Len(t, allReviews, 3) { for i, review := range allReviews { assert.Equal(t, expectedReviews[i].Reviewer, review.Reviewer) @@ -167,46 +167,46 @@ func TestDismissReview(t *testing.T) { assert.False(t, requestReviewExample.Dismissed) assert.False(t, approveReviewExample.Dismissed) - assert.NoError(t, issues_model.DismissReview(rejectReviewExample, true)) + assert.NoError(t, issues_model.DismissReview(db.DefaultContext, rejectReviewExample, true)) rejectReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 9}) requestReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 11}) assert.True(t, rejectReviewExample.Dismissed) assert.False(t, requestReviewExample.Dismissed) - assert.NoError(t, issues_model.DismissReview(requestReviewExample, true)) + assert.NoError(t, issues_model.DismissReview(db.DefaultContext, requestReviewExample, true)) rejectReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 9}) requestReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 11}) assert.True(t, rejectReviewExample.Dismissed) assert.False(t, requestReviewExample.Dismissed) assert.False(t, approveReviewExample.Dismissed) - assert.NoError(t, issues_model.DismissReview(requestReviewExample, true)) + assert.NoError(t, issues_model.DismissReview(db.DefaultContext, requestReviewExample, true)) rejectReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 9}) requestReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 11}) assert.True(t, rejectReviewExample.Dismissed) assert.False(t, requestReviewExample.Dismissed) assert.False(t, approveReviewExample.Dismissed) - assert.NoError(t, issues_model.DismissReview(requestReviewExample, false)) + assert.NoError(t, issues_model.DismissReview(db.DefaultContext, requestReviewExample, false)) rejectReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 9}) requestReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 11}) assert.True(t, rejectReviewExample.Dismissed) assert.False(t, requestReviewExample.Dismissed) assert.False(t, approveReviewExample.Dismissed) - assert.NoError(t, issues_model.DismissReview(requestReviewExample, false)) + assert.NoError(t, issues_model.DismissReview(db.DefaultContext, requestReviewExample, false)) rejectReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 9}) requestReviewExample = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 11}) assert.True(t, rejectReviewExample.Dismissed) assert.False(t, requestReviewExample.Dismissed) assert.False(t, approveReviewExample.Dismissed) - assert.NoError(t, issues_model.DismissReview(rejectReviewExample, false)) + assert.NoError(t, issues_model.DismissReview(db.DefaultContext, rejectReviewExample, false)) assert.False(t, rejectReviewExample.Dismissed) assert.False(t, requestReviewExample.Dismissed) assert.False(t, approveReviewExample.Dismissed) - assert.NoError(t, issues_model.DismissReview(approveReviewExample, true)) + assert.NoError(t, issues_model.DismissReview(db.DefaultContext, approveReviewExample, true)) assert.False(t, rejectReviewExample.Dismissed) assert.False(t, requestReviewExample.Dismissed) assert.True(t, approveReviewExample.Dismissed) diff --git a/services/pull/pull.go b/services/pull/pull.go index 8f2befa8ff..55c400947b 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -302,6 +302,17 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, if err := issues_model.MarkReviewsAsStale(pr.IssueID); err != nil { log.Error("MarkReviewsAsStale: %v", err) } + + // dismiss all approval reviews if protected branch rule item enabled. + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { + log.Error("GetFirstMatchProtectedBranchRule: %v", err) + } + if pb != nil && pb.DismissStaleApprovals { + if err := DismissApprovalReviews(ctx, doer, pr); err != nil { + log.Error("DismissApprovalReviews: %v", err) + } + } } if err := issues_model.MarkReviewsAsNotStale(pr.IssueID, newCommitID); err != nil { log.Error("MarkReviewsAsNotStale: %v", err) diff --git a/services/pull/review.go b/services/pull/review.go index 6feffe4ec4..6b8373c089 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -316,6 +316,52 @@ func SubmitReview(ctx context.Context, doer *user_model.User, gitRepo *git.Repos return review, comm, nil } +// DismissApprovalReviews dismiss all approval reviews because of new commits +func DismissApprovalReviews(ctx context.Context, doer *user_model.User, pull *issues_model.PullRequest) error { + reviews, err := issues_model.FindReviews(ctx, issues_model.FindReviewOptions{ + ListOptions: db.ListOptions{ + ListAll: true, + }, + IssueID: pull.IssueID, + Type: issues_model.ReviewTypeApprove, + Dismissed: util.OptionalBoolFalse, + }) + if err != nil { + return err + } + + if err := reviews.LoadIssues(ctx); err != nil { + return err + } + + return db.WithTx(ctx, func(subCtx context.Context) error { + for _, review := range reviews { + if err := issues_model.DismissReview(subCtx, review, true); err != nil { + return err + } + + comment, err := issue_service.CreateComment(ctx, &issues_model.CreateCommentOptions{ + Doer: doer, + Content: "New commits pushed, approval review dismissed automatically according to repository settings", + Type: issues_model.CommentTypeDismissReview, + ReviewID: review.ID, + Issue: review.Issue, + Repo: review.Issue.Repo, + }) + if err != nil { + return err + } + + comment.Review = review + comment.Poster = doer + comment.Issue = review.Issue + + notification.NotifyPullReviewDismiss(ctx, doer, review, comment) + } + return nil + }) +} + // DismissReview dismissing stale review by repo admin func DismissReview(ctx context.Context, reviewID, repoID int64, message string, doer *user_model.User, isDismiss, dismissPriors bool) (comment *issues_model.Comment, err error) { review, err := issues_model.GetReviewByID(ctx, reviewID) @@ -337,12 +383,12 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string, return nil, fmt.Errorf("reviews's repository is not the same as the one we expect") } - if err = issues_model.DismissReview(review, isDismiss); err != nil { - return + if err := issues_model.DismissReview(ctx, review, isDismiss); err != nil { + return nil, err } if dismissPriors { - reviews, err := issues_model.GetReviews(ctx, &issues_model.GetReviewOptions{ + reviews, err := issues_model.FindReviews(ctx, issues_model.FindReviewOptions{ IssueID: review.IssueID, ReviewerID: review.ReviewerID, Dismissed: util.OptionalBoolFalse, @@ -351,7 +397,7 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string, return nil, err } for _, oldReview := range reviews { - if err = issues_model.DismissReview(oldReview, true); err != nil { + if err = issues_model.DismissReview(ctx, oldReview, true); err != nil { return nil, err } } @@ -361,11 +407,8 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string, return nil, nil } - if err = review.Issue.LoadPullRequest(ctx); err != nil { - return - } - if err = review.Issue.LoadAttributes(ctx); err != nil { - return + if err := review.Issue.LoadAttributes(ctx); err != nil { + return nil, err } comment, err = issue_service.CreateComment(ctx, &issues_model.CreateCommentOptions{