From 25531c71a78b98af91f25d5e6eaa362e5fc54a86 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Thu, 9 Jan 2020 02:47:45 +0100 Subject: [PATCH] Mark PR reviews as stale at push and allow to dismiss stale approvals (#9532) Fix #5997. If a push causes the patch/diff of a PR towards target branch to change, all existing reviews for the PR will be set and shown as stale. New branch protection option to dismiss stale approvals are added. To show that a review is not based on the latest PR changes, an hourglass is shown --- models/branches.go | 41 ++++---- models/migrations/migrations.go | 2 + models/migrations/v118.go | 26 +++++ models/review.go | 30 +++++- modules/auth/repo_form.go | 19 ++-- modules/git/repo_compare.go | 6 ++ modules/repofiles/update.go | 4 +- options/locale/locale_en-US.ini | 2 + routers/repo/pull.go | 2 +- routers/repo/pull_review.go | 4 +- routers/repo/setting_protected_branch.go | 1 + services/pull/merge.go | 2 +- services/pull/pull.go | 96 ++++++++++++++++++- services/pull/review.go | 37 +++++-- templates/repo/diff/comment_form.tmpl | 1 + templates/repo/diff/new_review.tmpl | 1 + templates/repo/issue/view_content/pull.tmpl | 5 + templates/repo/settings/protected_branch.tmpl | 8 ++ 18 files changed, 244 insertions(+), 43 deletions(-) create mode 100644 models/migrations/v118.go diff --git a/models/branches.go b/models/branches.go index 385817e4f9..1932e06db3 100644 --- a/models/branches.go +++ b/models/branches.go @@ -32,21 +32,23 @@ type ProtectedBranch struct { BranchName string `xorm:"UNIQUE(s)"` CanPush bool `xorm:"NOT NULL DEFAULT false"` EnableWhitelist bool - WhitelistUserIDs []int64 `xorm:"JSON TEXT"` - WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` - EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` - WhitelistDeployKeys bool `xorm:"NOT NULL DEFAULT false"` - MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` - MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` - EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"` - StatusCheckContexts []string `xorm:"JSON TEXT"` - EnableApprovalsWhitelist bool `xorm:"NOT NULL DEFAULT false"` - ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"` - ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` - RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` - BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"` - CreatedUnix timeutil.TimeStamp `xorm:"created"` - UpdatedUnix timeutil.TimeStamp `xorm:"updated"` + WhitelistUserIDs []int64 `xorm:"JSON TEXT"` + WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` + WhitelistDeployKeys bool `xorm:"NOT NULL DEFAULT false"` + MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"` + StatusCheckContexts []string `xorm:"JSON TEXT"` + EnableApprovalsWhitelist bool `xorm:"NOT NULL DEFAULT false"` + ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` + BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"` + DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"` + + CreatedUnix timeutil.TimeStamp `xorm:"created"` + UpdatedUnix timeutil.TimeStamp `xorm:"updated"` } // IsProtected returns if the branch is protected @@ -155,10 +157,13 @@ func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool { // GetGrantedApprovalsCount returns the number of granted approvals for pr. A granted approval must be authored by a user in an approval whitelist. func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest) int64 { - approvals, err := x.Where("issue_id = ?", pr.IssueID). + sess := x.Where("issue_id = ?", pr.IssueID). And("type = ?", ReviewTypeApprove). - And("official = ?", true). - Count(new(Review)) + And("official = ?", true) + if protectBranch.DismissStaleApprovals { + sess = sess.And("stale = ?", false) + } + approvals, err := sess.Count(new(Review)) if err != nil { log.Error("GetGrantedApprovalsCount: %v", err) return 0 diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 73c9bc1138..f26566b045 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -290,6 +290,8 @@ var migrations = []Migration{ NewMigration("Extend TrackedTimes", extendTrackedTimes), // v117 -> v118 NewMigration("Add block on rejected reviews branch protection", addBlockOnRejectedReviews), + // v118 -> v119 + NewMigration("Add commit id and stale to reviews", addReviewCommitAndStale), } // Migrate database to current version diff --git a/models/migrations/v118.go b/models/migrations/v118.go new file mode 100644 index 0000000000..c79cbb8ae3 --- /dev/null +++ b/models/migrations/v118.go @@ -0,0 +1,26 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "xorm.io/xorm" +) + +func addReviewCommitAndStale(x *xorm.Engine) error { + type Review struct { + CommitID string `xorm:"VARCHAR(40)"` + Stale bool `xorm:"NOT NULL DEFAULT false"` + } + + type ProtectedBranch struct { + DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"` + } + + // Old reviews will have commit ID set to "" and not stale + if err := x.Sync2(new(Review)); err != nil { + return err + } + return x.Sync2(new(ProtectedBranch)) +} diff --git a/models/review.go b/models/review.go index bc7dfbcd14..2838cfa316 100644 --- a/models/review.go +++ b/models/review.go @@ -53,7 +53,9 @@ type Review struct { IssueID int64 `xorm:"index"` Content string `xorm:"TEXT"` // Official is a review made by an assigned approver (counts towards approval) - Official bool `xorm:"NOT NULL DEFAULT false"` + Official bool `xorm:"NOT NULL DEFAULT false"` + CommitID string `xorm:"VARCHAR(40)"` + Stale bool `xorm:"NOT NULL DEFAULT false"` CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` @@ -169,6 +171,8 @@ type CreateReviewOptions struct { Issue *Issue Reviewer *User Official bool + CommitID string + Stale bool } // IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals) @@ -200,6 +204,8 @@ func createReview(e Engine, opts CreateReviewOptions) (*Review, error) { ReviewerID: opts.Reviewer.ID, Content: opts.Content, Official: opts.Official, + CommitID: opts.CommitID, + Stale: opts.Stale, } if _, err := e.Insert(review); err != nil { return nil, err @@ -258,7 +264,7 @@ func IsContentEmptyErr(err error) bool { } // SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist -func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content string) (*Review, *Comment, error) { +func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, commitID string, stale bool) (*Review, *Comment, error) { sess := x.NewSession() defer sess.Close() if err := sess.Begin(); err != nil { @@ -295,6 +301,8 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin Reviewer: doer, Content: content, Official: official, + CommitID: commitID, + Stale: stale, }) if err != nil { return nil, nil, err @@ -322,8 +330,10 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin review.Issue = issue review.Content = content review.Type = reviewType + review.CommitID = commitID + review.Stale = stale - if _, err := sess.ID(review.ID).Cols("content, type, official").Update(review); err != nil { + if _, err := sess.ID(review.ID).Cols("content, type, official, commit_id, stale").Update(review); err != nil { return nil, nil, err } } @@ -374,3 +384,17 @@ func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) { return reviews, nil } + +// MarkReviewsAsStale marks existing reviews as stale +func MarkReviewsAsStale(issueID int64) (err error) { + _, err = x.Exec("UPDATE `review` SET stale=? WHERE issue_id=?", true, issueID) + + return +} + +// MarkReviewsAsNotStale marks existing reviews as not stale for a giving commit SHA +func MarkReviewsAsNotStale(issueID int64, commitID string) (err error) { + _, err = x.Exec("UPDATE `review` SET stale=? WHERE issue_id=? AND commit_id=?", false, issueID, commitID) + + return +} diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index c87549af92..28615ebbc4 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -172,6 +172,7 @@ type ProtectBranchForm struct { ApprovalsWhitelistUsers string ApprovalsWhitelistTeams string BlockOnRejectedReviews bool + DismissStaleApprovals bool } // Validate validates the fields @@ -456,12 +457,13 @@ func (f *MergePullRequestForm) Validate(ctx *macaron.Context, errs binding.Error // CodeCommentForm form for adding code comments for PRs type CodeCommentForm struct { - Content string `binding:"Required"` - Side string `binding:"Required;In(previous,proposed)"` - Line int64 - TreePath string `form:"path" binding:"Required"` - IsReview bool `form:"is_review"` - Reply int64 `form:"reply"` + Content string `binding:"Required"` + Side string `binding:"Required;In(previous,proposed)"` + Line int64 + TreePath string `form:"path" binding:"Required"` + IsReview bool `form:"is_review"` + Reply int64 `form:"reply"` + LatestCommitID string } // Validate validates the fields @@ -471,8 +473,9 @@ func (f *CodeCommentForm) Validate(ctx *macaron.Context, errs binding.Errors) bi // SubmitReviewForm for submitting a finished code review type SubmitReviewForm struct { - Content string - Type string `binding:"Required;In(approve,comment,reject)"` + Content string + Type string `binding:"Required;In(approve,comment,reject)"` + CommitID string } // Validate validates the fields diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 53b8af4bb4..683ac7a7ee 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -112,3 +112,9 @@ func (repo *Repository) GetPatch(base, head string, w io.Writer) error { return NewCommand("format-patch", "--binary", "--stdout", base+"..."+head). RunInDirPipeline(repo.Path, w, nil) } + +// GetDiffFromMergeBase generates and return patch data from merge base to head +func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error { + return NewCommand("diff", "-p", "--binary", base+"..."+head). + RunInDirPipeline(repo.Path, w, nil) +} diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 19c3d5b51c..7ad4a4d388 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -477,7 +477,7 @@ func PushUpdate(repo *models.Repository, branch string, opts PushUpdateOptions) log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name) - go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true) + go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID) if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) @@ -528,7 +528,7 @@ func PushUpdates(repo *models.Repository, optsList []*PushUpdateOptions) error { log.Trace("TriggerTask '%s/%s' by %s", repo.Name, opts.Branch, pusher.Name) - go pull_service.AddTestPullRequestTask(pusher, repo.ID, opts.Branch, true) + go pull_service.AddTestPullRequestTask(pusher, repo.ID, opts.Branch, true, opts.OldCommitID, opts.NewCommitID) if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index f6ff12250a..a00df07d93 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1413,6 +1413,8 @@ settings.protect_approvals_whitelist_enabled = Restrict approvals to whitelisted settings.protect_approvals_whitelist_enabled_desc = Only reviews from whitelisted users or teams will count to the required approvals. Without approval whitelist, reviews from anyone with write access count to the required approvals. settings.protect_approvals_whitelist_users = Whitelisted reviewers: settings.protect_approvals_whitelist_teams = Whitelisted teams for reviews: +settings.dismiss_stale_approvals = Dismiss stale approvals +settings.dismiss_stale_approvals_desc = When new commits that change the content of the pull request are pushed to the branch, old approvals will be dismissed. settings.add_protected_branch = Enable protection settings.delete_protected_branch = Disable protection settings.update_protect_branch_success = Branch protection for branch '%s' has been updated. diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 1a5c4a036f..93e98e0c33 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -841,7 +841,7 @@ func TriggerTask(ctx *context.Context) { log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name) - go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true) + go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, "", "") ctx.Status(202) } diff --git a/routers/repo/pull_review.go b/routers/repo/pull_review.go index b596d2578b..558473eb08 100644 --- a/routers/repo/pull_review.go +++ b/routers/repo/pull_review.go @@ -37,12 +37,14 @@ func CreateCodeComment(ctx *context.Context, form auth.CodeCommentForm) { comment, err := pull_service.CreateCodeComment( ctx.User, + ctx.Repo.GitRepo, issue, signedLine, form.Content, form.TreePath, form.IsReview, form.Reply, + form.LatestCommitID, ) if err != nil { ctx.ServerError("CreateCodeComment", err) @@ -95,7 +97,7 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) { } } - _, comm, err := pull_service.SubmitReview(ctx.User, issue, reviewType, form.Content) + _, comm, err := pull_service.SubmitReview(ctx.User, ctx.Repo.GitRepo, issue, reviewType, form.Content, form.CommitID) if err != nil { if models.IsContentEmptyErr(err) { ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty")) diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index 8872c7471f..da28ac50be 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -245,6 +245,7 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) } } protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews + protectBranch.DismissStaleApprovals = f.DismissStaleApprovals err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{ UserIDs: whitelistUsers, diff --git a/services/pull/merge.go b/services/pull/merge.go index 8aa38bf11e..b38c2e72f2 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -64,7 +64,7 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor } defer func() { - go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false) + go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") }() // Clone base repo. diff --git a/services/pull/pull.go b/services/pull/pull.go index 6be9c2da17..b459d81cf7 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -5,10 +5,14 @@ package pull import ( + "bufio" + "bytes" "context" "fmt" "os" "path" + "strings" + "time" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/git" @@ -16,6 +20,8 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" issue_service "code.gitea.io/gitea/services/issue" + + "github.com/unknwon/com" ) // NewPullRequest creates new pull request with labels for repository. @@ -168,7 +174,7 @@ func addHeadRepoTasks(prs []*models.PullRequest) { // AddTestPullRequestTask adds new test tasks by given head/base repository and head/base branch, // and generate new patch for testing as needed. -func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSync bool) { +func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSync bool, oldCommitID, newCommitID string) { log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", repoID, branch) graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { // There is no sensible way to shut this down ":-(" @@ -191,6 +197,22 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy } if err == nil { for _, pr := range prs { + if newCommitID != "" && newCommitID != git.EmptySHA { + changed, err := checkIfPRContentChanged(pr, oldCommitID, newCommitID) + if err != nil { + log.Error("checkIfPRContentChanged: %v", err) + } + if changed { + // Mark old reviews as stale if diff to mergebase has changed + if err := models.MarkReviewsAsStale(pr.IssueID); err != nil { + log.Error("MarkReviewsAsStale: %v", err) + } + } + if err := models.MarkReviewsAsNotStale(pr.IssueID, newCommitID); err != nil { + log.Error("MarkReviewsAsNotStale: %v", err) + } + } + pr.Issue.PullRequest = pr notification.NotifyPullRequestSynchronized(doer, pr) } @@ -211,6 +233,78 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy }) } +// checkIfPRContentChanged checks if diff to target branch has changed by push +// A commit can be considered to leave the PR untouched if the patch/diff with its merge base is unchanged +func checkIfPRContentChanged(pr *models.PullRequest, oldCommitID, newCommitID string) (hasChanged bool, err error) { + + if err = pr.GetHeadRepo(); err != nil { + return false, fmt.Errorf("GetHeadRepo: %v", err) + } else if pr.HeadRepo == nil { + // corrupt data assumed changed + return true, nil + } + + if err = pr.GetBaseRepo(); err != nil { + return false, fmt.Errorf("GetBaseRepo: %v", err) + } + + headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) + if err != nil { + return false, fmt.Errorf("OpenRepository: %v", err) + } + defer headGitRepo.Close() + + // Add a temporary remote. + tmpRemote := "checkIfPRContentChanged-" + com.ToStr(time.Now().UnixNano()) + if err = headGitRepo.AddRemote(tmpRemote, models.RepoPath(pr.BaseRepo.MustOwner().Name, pr.BaseRepo.Name), true); err != nil { + return false, fmt.Errorf("AddRemote: %s/%s-%s: %v", pr.HeadRepo.OwnerName, pr.HeadRepo.Name, tmpRemote, err) + } + defer func() { + if err := headGitRepo.RemoveRemote(tmpRemote); err != nil { + log.Error("checkIfPRContentChanged: RemoveRemote: %s/%s-%s: %v", pr.HeadRepo.OwnerName, pr.HeadRepo.Name, tmpRemote, err) + } + }() + // To synchronize repo and get a base ref + _, base, err := headGitRepo.GetMergeBase(tmpRemote, pr.BaseBranch, pr.HeadBranch) + if err != nil { + return false, fmt.Errorf("GetMergeBase: %v", err) + } + + diffBefore := &bytes.Buffer{} + diffAfter := &bytes.Buffer{} + if err := headGitRepo.GetDiffFromMergeBase(base, oldCommitID, diffBefore); err != nil { + // If old commit not found, assume changed. + log.Debug("GetDiffFromMergeBase: %v", err) + return true, nil + } + if err := headGitRepo.GetDiffFromMergeBase(base, newCommitID, diffAfter); err != nil { + // New commit should be found + return false, fmt.Errorf("GetDiffFromMergeBase: %v", err) + } + + diffBeforeLines := bufio.NewScanner(diffBefore) + diffAfterLines := bufio.NewScanner(diffAfter) + + for diffBeforeLines.Scan() && diffAfterLines.Scan() { + if strings.HasPrefix(diffBeforeLines.Text(), "index") && strings.HasPrefix(diffAfterLines.Text(), "index") { + // file hashes can change without the diff changing + continue + } else if strings.HasPrefix(diffBeforeLines.Text(), "@@") && strings.HasPrefix(diffAfterLines.Text(), "@@") { + // the location of the difference may change + continue + } else if !bytes.Equal(diffBeforeLines.Bytes(), diffAfterLines.Bytes()) { + return true, nil + } + } + + if diffBeforeLines.Scan() || diffAfterLines.Scan() { + // Diffs not of equal length + return true, nil + } + + return false, nil +} + // PushToBaseRepo pushes commits from branches of head repository to // corresponding branches of base repository. // FIXME: Only push branches that are actually updates? diff --git a/services/pull/review.go b/services/pull/review.go index 5b453e87f4..2bc8240230 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -18,7 +18,7 @@ import ( ) // CreateCodeComment creates a comment on the code line -func CreateCodeComment(doer *models.User, issue *models.Issue, line int64, content string, treePath string, isReview bool, replyReviewID int64) (*models.Comment, error) { +func CreateCodeComment(doer *models.User, gitRepo *git.Repository, issue *models.Issue, line int64, content string, treePath string, isReview bool, replyReviewID int64, latestCommitID string) (*models.Comment, error) { var ( existsReview bool @@ -73,6 +73,7 @@ func CreateCodeComment(doer *models.User, issue *models.Issue, line int64, conte Reviewer: doer, Issue: issue, Official: false, + CommitID: latestCommitID, }) if err != nil { return nil, err @@ -94,7 +95,7 @@ func CreateCodeComment(doer *models.User, issue *models.Issue, line int64, conte if !isReview && !existsReview { // Submit the review we've just created so the comment shows up in the issue view - if _, _, err = SubmitReview(doer, issue, models.ReviewTypeComment, ""); err != nil { + if _, _, err = SubmitReview(doer, gitRepo, issue, models.ReviewTypeComment, "", latestCommitID); err != nil { return nil, err } } @@ -159,16 +160,36 @@ func createCodeComment(doer *models.User, repo *models.Repository, issue *models } // SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist -func SubmitReview(doer *models.User, issue *models.Issue, reviewType models.ReviewType, content string) (*models.Review, *models.Comment, error) { - review, comm, err := models.SubmitReview(doer, issue, reviewType, content) - if err != nil { - return nil, nil, err - } - +func SubmitReview(doer *models.User, gitRepo *git.Repository, issue *models.Issue, reviewType models.ReviewType, content, commitID string) (*models.Review, *models.Comment, error) { pr, err := issue.GetPullRequest() if err != nil { return nil, nil, err } + + var stale bool + if reviewType != models.ReviewTypeApprove && reviewType != models.ReviewTypeReject { + stale = false + } else { + headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName()) + if err != nil { + return nil, nil, err + } + + if headCommitID == commitID { + stale = false + } else { + stale, err = checkIfPRContentChanged(pr, commitID, headCommitID) + if err != nil { + return nil, nil, err + } + } + } + + review, comm, err := models.SubmitReview(doer, issue, reviewType, content, commitID, stale) + if err != nil { + return nil, nil, err + } + notification.NotifyPullRequestReview(pr, review, comm) return review, comm, nil diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 573e7d4638..822b1e54f6 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -4,6 +4,7 @@ {{end}}
{{$.root.CsrfTokenHtml}} + diff --git a/templates/repo/diff/new_review.tmpl b/templates/repo/diff/new_review.tmpl index 68d8f893f2..4981c9ef48 100644 --- a/templates/repo/diff/new_review.tmpl +++ b/templates/repo/diff/new_review.tmpl @@ -7,6 +7,7 @@
{{.CsrfTokenHtml}} +
{{$.i18n.Tr "repo.diff.review.header"}} diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 2dc76dcf2e..b1e6feeba0 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -13,6 +13,11 @@ {{else}}grey{{end}}"> + {{if .Stale}} + + + + {{end}} diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index bf74a12330..c6701ce8a9 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -211,6 +211,14 @@

{{.i18n.Tr "repo.settings.block_rejected_reviews_desc"}}

+
+
+ + +

{{.i18n.Tr "repo.settings.dismiss_stale_approvals_desc"}}

+
+
+