Fix bug when pushing to a pull request which enabled dismiss approval automatically (#25882) (#26158)

Fix #25858
Backport #25882 

The option `dissmiss stale approvals` was listed on protected branch but
never implemented. This PR fixes that.

<img width="1006" alt="图片"

src="https://github.com/go-gitea/gitea/assets/81045/60bfa968-4db7-4c24-b8be-2e5978f91bb9">

<img width="1021" alt="图片"

src="https://github.com/go-gitea/gitea/assets/81045/8dabc14d-2dfe-40c2-94ed-24fcbf6e0e8f">

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

models/issues/pull.go

@@ -314,15 +314,13 @@ func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error {
 		return err
 	}
 
-	if len(reviews) > 0 {
+	if err = reviews.LoadReviewers(ctx); err != nil {
-		err = LoadReviewers(ctx, reviews)
-		if err != nil {
 		return err
 	}
 	for _, review := range reviews {
 		pr.RequestedReviewers = append(pr.RequestedReviewers, review.Reviewer)
 	}
-	}
+
 	return nil
 }
 

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
 }

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

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)

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)

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 {
+	if err := issues_model.DismissReview(ctx, review, isDismiss); err != nil {
-		return
+		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 {
+	if err := review.Issue.LoadAttributes(ctx); err != nil {
-		return
+		return nil, err
-	}
-	if err = review.Issue.LoadAttributes(ctx); err != nil {
-		return
 	}
 
 	comment, err = issue_service.CreateComment(ctx, &issues_model.CreateCommentOptions{