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

Author: davidsvantesson

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

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

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

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

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

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

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)

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.

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

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

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,

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.