Add branch protection setting for ignoring stale approvals (#28498)

Fixes #27114.

* In Gitea 1.12 (#9532), a "dismiss stale approvals" branch protection
setting was introduced, for ignoring stale reviews when verifying the
approval count of a pull request.
* In Gitea 1.14 (#12674), the "dismiss review" feature was added.
* This caused confusion with users (#25858), as "dismiss" now means 2
different things.
* In Gitea 1.20 (#25882), the behavior of the "dismiss stale approvals"
branch protection was modified to actually dismiss the stale review.

For some users this new behavior of dismissing the stale reviews is not
desirable.

So this PR reintroduces the old behavior as a new "ignore stale
approvals" branch protection setting.

---------

Co-authored-by: delvh <[email protected]>

Author: jpraet

models/git/protected_branch.go

@@ -54,6 +54,7 @@ type ProtectedBranch struct {
 	BlockOnOfficialReviewRequests bool     `xorm:"NOT NULL DEFAULT false"`
 	BlockOnOutdatedBranch         bool     `xorm:"NOT NULL DEFAULT false"`
 	DismissStaleApprovals         bool     `xorm:"NOT NULL DEFAULT false"`
+	IgnoreStaleApprovals          bool     `xorm:"NOT NULL DEFAULT false"`
 	RequireSignedCommits          bool     `xorm:"NOT NULL DEFAULT false"`
 	ProtectedFilePatterns         string   `xorm:"TEXT"`
 	UnprotectedFilePatterns       string   `xorm:"TEXT"`

models/issues/pull.go

@@ -801,7 +801,7 @@ func GetGrantedApprovalsCount(ctx context.Context, protectBranch *git_model.Prot
 		And("type = ?", ReviewTypeApprove).
 		And("official = ?", true).
 		And("dismissed = ?", false)
-	if protectBranch.DismissStaleApprovals {
+	if protectBranch.IgnoreStaleApprovals {
 		sess = sess.And("stale = ?", false)
 	}
 	approvals, err := sess.Count(new(Review))

models/migrations/v1_22/v284.go

@@ -0,0 +1,14 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_22 //nolint
+import (
+	"xorm.io/xorm"
+)
+
+func AddIgnoreStaleApprovalsColumnToProtectedBranchTable(x *xorm.Engine) error {
+	type ProtectedBranch struct {
+		IgnoreStaleApprovals bool `xorm:"NOT NULL DEFAULT false"`
+	}
+	return x.Sync(new(ProtectedBranch))
+}

modules/structs/repo_branch.go

@@ -43,6 +43,7 @@ type BranchProtection struct {
 	BlockOnOfficialReviewRequests bool     `json:"block_on_official_review_requests"`
 	BlockOnOutdatedBranch         bool     `json:"block_on_outdated_branch"`
 	DismissStaleApprovals         bool     `json:"dismiss_stale_approvals"`
+	IgnoreStaleApprovals          bool     `json:"ignore_stale_approvals"`
 	RequireSignedCommits          bool     `json:"require_signed_commits"`
 	ProtectedFilePatterns         string   `json:"protected_file_patterns"`
 	UnprotectedFilePatterns       string   `json:"unprotected_file_patterns"`
@@ -75,6 +76,7 @@ type CreateBranchProtectionOption struct {
 	BlockOnOfficialReviewRequests bool     `json:"block_on_official_review_requests"`
 	BlockOnOutdatedBranch         bool     `json:"block_on_outdated_branch"`
 	DismissStaleApprovals         bool     `json:"dismiss_stale_approvals"`
+	IgnoreStaleApprovals          bool     `json:"ignore_stale_approvals"`
 	RequireSignedCommits          bool     `json:"require_signed_commits"`
 	ProtectedFilePatterns         string   `json:"protected_file_patterns"`
 	UnprotectedFilePatterns       string   `json:"unprotected_file_patterns"`
@@ -100,6 +102,7 @@ type EditBranchProtectionOption struct {
 	BlockOnOfficialReviewRequests *bool    `json:"block_on_official_review_requests"`
 	BlockOnOutdatedBranch         *bool    `json:"block_on_outdated_branch"`
 	DismissStaleApprovals         *bool    `json:"dismiss_stale_approvals"`
+	IgnoreStaleApprovals          *bool    `json:"ignore_stale_approvals"`
 	RequireSignedCommits          *bool    `json:"require_signed_commits"`
 	ProtectedFilePatterns         *string  `json:"protected_file_patterns"`
 	UnprotectedFilePatterns       *string  `json:"unprotected_file_patterns"`

options/locale/locale_en-US.ini

@@ -2315,6 +2315,8 @@ 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.ignore_stale_approvals = Ignore stale approvals
+settings.ignore_stale_approvals_desc = Do not count approvals that were made on older commits (stale reviews) towards how many approvals the PR has. Irrelevant if stale reviews are already dismissed.
 settings.require_signed_commits = Require Signed Commits
 settings.require_signed_commits_desc = Reject pushes to this branch if they are unsigned or unverifiable.
 settings.protect_branch_name_pattern = Protected Branch Name Pattern

routers/api/v1/repo/branch.go

@@ -615,6 +615,7 @@ func CreateBranchProtection(ctx *context.APIContext) {
 		BlockOnRejectedReviews:        form.BlockOnRejectedReviews,
 		BlockOnOfficialReviewRequests: form.BlockOnOfficialReviewRequests,
 		DismissStaleApprovals:         form.DismissStaleApprovals,
+		IgnoreStaleApprovals:          form.IgnoreStaleApprovals,
 		RequireSignedCommits:          form.RequireSignedCommits,
 		ProtectedFilePatterns:         form.ProtectedFilePatterns,
 		UnprotectedFilePatterns:       form.UnprotectedFilePatterns,
@@ -786,6 +787,10 @@ func EditBranchProtection(ctx *context.APIContext) {
 		protectBranch.DismissStaleApprovals = *form.DismissStaleApprovals
 	}
 
+	if form.IgnoreStaleApprovals != nil {
+		protectBranch.IgnoreStaleApprovals = *form.IgnoreStaleApprovals
+	}
+
 	if form.RequireSignedCommits != nil {
 		protectBranch.RequireSignedCommits = *form.RequireSignedCommits
 	}

routers/web/repo/setting/protected_branch.go

@@ -228,6 +228,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 	protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews
 	protectBranch.BlockOnOfficialReviewRequests = f.BlockOnOfficialReviewRequests
 	protectBranch.DismissStaleApprovals = f.DismissStaleApprovals
+	protectBranch.IgnoreStaleApprovals = f.IgnoreStaleApprovals
 	protectBranch.RequireSignedCommits = f.RequireSignedCommits
 	protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns
 	protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns

services/convert/convert.go

@@ -158,6 +158,7 @@ func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch) *api
 		BlockOnOfficialReviewRequests: bp.BlockOnOfficialReviewRequests,
 		BlockOnOutdatedBranch:         bp.BlockOnOutdatedBranch,
 		DismissStaleApprovals:         bp.DismissStaleApprovals,
+		IgnoreStaleApprovals:          bp.IgnoreStaleApprovals,
 		RequireSignedCommits:          bp.RequireSignedCommits,
 		ProtectedFilePatterns:         bp.ProtectedFilePatterns,
 		UnprotectedFilePatterns:       bp.UnprotectedFilePatterns,

services/forms/repo_form.go

@@ -212,6 +212,7 @@ type ProtectBranchForm struct {
 	BlockOnOfficialReviewRequests bool
 	BlockOnOutdatedBranch         bool
 	DismissStaleApprovals         bool
+	IgnoreStaleApprovals          bool
 	RequireSignedCommits          bool
 	ProtectedFilePatterns         string
 	UnprotectedFilePatterns       string

templates/repo/settings/protected_branch.tmpl

@@ -144,11 +144,18 @@
 				</div>
 				<div class="field">
 					<div class="ui checkbox">
-						<input name="dismiss_stale_approvals" type="checkbox" {{if .Rule.DismissStaleApprovals}}checked{{end}}>
+						<input id="dismiss_stale_approvals" name="dismiss_stale_approvals" type="checkbox" {{if .Rule.DismissStaleApprovals}}checked{{end}}>
 						<label>{{ctx.Locale.Tr "repo.settings.dismiss_stale_approvals"}}</label>
 						<p class="help">{{ctx.Locale.Tr "repo.settings.dismiss_stale_approvals_desc"}}</p>
 					</div>
 				</div>
+				<div id="ignore_stale_approvals_box" class="field {{if .Rule.DismissStaleApprovals}}disabled{{end}}">
+					<div class="ui checkbox">
+						<input id="ignore_stale_approvals" name="ignore_stale_approvals" type="checkbox" {{if .Rule.IgnoreStaleApprovals}}checked{{end}}>
+						<label>{{ctx.Locale.Tr "repo.settings.ignore_stale_approvals"}}</label>
+						<p class="help">{{ctx.Locale.Tr "repo.settings.ignore_stale_approvals_desc"}}</p>
+					</div>
+				</div>
 				<div class="grouped fields">
 					<div class="field">
 						<div class="ui checkbox">

templates/swagger/v1_json.tmpl

@@ -82,6 +82,10 @@ export function initRepoSettingBranches() {
     const $target = $($(this).attr('data-target'));
     if (this.checked) $target.addClass('disabled'); // only disable, do not auto enable
   });
+  $('#dismiss_stale_approvals').on('change', function () {
+    const $target = $('#ignore_stale_approvals_box');
+    $target.toggleClass('disabled', this.checked);
+  });
 
   // show the `Matched` mark for the status checks that match the pattern
   const markMatchedStatusChecks = () => {