NYANDEV/forgejo
NYANDEV/forgejo
4
0
Fork 1
mirror of https://codeberg.org/forgejo/forgejo.git synced 2024-11-08 18:04:14 +01:00
Code Issues 2 Projects 1 Releases Packages Wiki Activity

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

Browse source 
Fix #25858 

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">
This commit is contained in:
Lunny Xiao 2023-07-20 15:18:52 +08:00 committed by GitHub
parent cf467119ff
commit 01c04607c7
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 251 additions and 183 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
models/issues/issue.go
View file

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

14
models/issues/pull.go
View file

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

160
models/issues/review.go
View file

@ -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,71 +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)
}
// FindLatestReviews returns only latest reviews per user, passing FindReviewOptions
func FindLatestReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, 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{})
}
// CreateReviewOptions represent the options to create a review. Type, Issue and Reviewer are required.
type CreateReviewOptions struct {
Content string
@ -533,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)
@ -654,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
}
@ -665,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
}

172
models/issues/review_list.go Normal file
View file

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

16
models/issues/review_test.go
View file

@ -159,7 +159,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)
@ -179,46 +179,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)

11
services/pull/pull.go
View file

@ -309,6 +309,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)

55
services/pull/review.go
View file

@ -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 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,9 +407,6 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string,
return nil, nil
}
if err := review.Issue.LoadPullRequest(ctx); err != nil {
return nil, err
}
if err := review.Issue.LoadAttributes(ctx); err != nil {
return nil, err
}

4
services/repository/push.go
View file

@ -257,12 +257,12 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum]
}
notification.NotifyPushCommits(ctx, pusher, repo, opts, commits)
if err = git_model.UpdateBranch(ctx, repo.ID, opts.PusherID, branch, newCommit); err != nil {
return fmt.Errorf("git_model.UpdateBranch %s:%s failed: %v", repo.FullName(), branch, err)
}
notification.NotifyPushCommits(ctx, pusher, repo, opts, commits)
// Cache for big repository
if err := CacheRef(graceful.GetManager().HammerContext(), repo, gitRepo, opts.RefFullName); err != nil {
log.Error("repo_module.CacheRef %s/%s failed: %v", repo.ID, branch, err)