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

4 files changed, 78 insertions, 21 deletions

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