From 01ba4fb2f7ce5e42bd9aaa738195e81d4b2bc314 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sun, 17 Dec 2023 15:48:39 +0100 Subject: [PATCH 1/7] Add branch protection setting for ignoring stale approvals --- models/git/protected_branch.go | 1 + models/issues/pull.go | 2 +- models/migrations/v1_22/v284.go | 14 ++++++++++++++ modules/structs/repo_branch.go | 3 +++ options/locale/locale_en-US.ini | 2 ++ routers/api/v1/repo/branch.go | 5 +++++ routers/web/repo/setting/protected_branch.go | 1 + services/convert/convert.go | 1 + services/forms/repo_form.go | 1 + templates/repo/settings/protected_branch.tmpl | 7 +++++++ templates/swagger/v1_json.tmpl | 12 ++++++++++++ 11 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 models/migrations/v1_22/v284.go diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 528acc175a27a..13b01653ed8af 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -55,6 +55,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"` diff --git a/models/issues/pull.go b/models/issues/pull.go index c51a7daf4eca1..0250523c3e32c 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -831,7 +831,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)) diff --git a/models/migrations/v1_22/v284.go b/models/migrations/v1_22/v284.go new file mode 100644 index 0000000000000..1a4c786964909 --- /dev/null +++ b/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)) +} diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index e9927aec2797a..e96d276b2978e 100644 --- a/modules/structs/repo_branch.go +++ b/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"` diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index e57dd7794e4db..28b1aebfc01ac 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2312,6 +2312,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 = When new commits that change the content of the pull request are pushed to the branch, old approvals will be ignored. 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 diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 36c85c8a57034..edbfbcc568bad 100644 --- a/routers/api/v1/repo/branch.go +++ b/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 } diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index 73adfec95a0ee..98d6977b81f0d 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/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 diff --git a/services/convert/convert.go b/services/convert/convert.go index 366782390a758..873b913da9858 100644 --- a/services/convert/convert.go +++ b/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, diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 86599000a5882..780fc8800098a 100644 --- a/services/forms/repo_form.go +++ b/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 diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index a1e45d805ca85..7201ba546e528 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -149,6 +149,13 @@

{{ctx.Locale.Tr "repo.settings.dismiss_stale_approvals_desc"}}

+
+
+ + +

{{ctx.Locale.Tr "repo.settings.ignore_stale_approvals_desc"}}

+
+
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 6cf2beafec6e8..9343bc12157d2 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -16904,6 +16904,10 @@ "type": "boolean", "x-go-name": "DismissStaleApprovals" }, + "ignore_stale_approvals": { + "type": "boolean", + "x-go-name": "IgnoreStaleApprovals" + }, "enable_approvals_whitelist": { "type": "boolean", "x-go-name": "EnableApprovalsWhitelist" @@ -17545,6 +17549,10 @@ "type": "boolean", "x-go-name": "DismissStaleApprovals" }, + "ignore_stale_approvals": { + "type": "boolean", + "x-go-name": "IgnoreStaleApprovals" + }, "enable_approvals_whitelist": { "type": "boolean", "x-go-name": "EnableApprovalsWhitelist" @@ -18677,6 +18685,10 @@ "type": "boolean", "x-go-name": "DismissStaleApprovals" }, + "ignore_stale_approvals": { + "type": "boolean", + "x-go-name": "IgnoreStaleApprovals" + }, "enable_approvals_whitelist": { "type": "boolean", "x-go-name": "EnableApprovalsWhitelist" From d109630e2a354306e4fde7f149bf99b4d8c5cafb Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sun, 17 Dec 2023 16:23:52 +0100 Subject: [PATCH 2/7] Fix v1_json.tmpl property order --- templates/swagger/v1_json.tmpl | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 9343bc12157d2..f4d14c8d58202 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -16904,10 +16904,6 @@ "type": "boolean", "x-go-name": "DismissStaleApprovals" }, - "ignore_stale_approvals": { - "type": "boolean", - "x-go-name": "IgnoreStaleApprovals" - }, "enable_approvals_whitelist": { "type": "boolean", "x-go-name": "EnableApprovalsWhitelist" @@ -16928,6 +16924,10 @@ "type": "boolean", "x-go-name": "EnableStatusCheck" }, + "ignore_stale_approvals": { + "type": "boolean", + "x-go-name": "IgnoreStaleApprovals" + }, "merge_whitelist_teams": { "type": "array", "items": { @@ -17549,10 +17549,6 @@ "type": "boolean", "x-go-name": "DismissStaleApprovals" }, - "ignore_stale_approvals": { - "type": "boolean", - "x-go-name": "IgnoreStaleApprovals" - }, "enable_approvals_whitelist": { "type": "boolean", "x-go-name": "EnableApprovalsWhitelist" @@ -17573,6 +17569,10 @@ "type": "boolean", "x-go-name": "EnableStatusCheck" }, + "ignore_stale_approvals": { + "type": "boolean", + "x-go-name": "IgnoreStaleApprovals" + }, "merge_whitelist_teams": { "type": "array", "items": { @@ -18685,10 +18685,6 @@ "type": "boolean", "x-go-name": "DismissStaleApprovals" }, - "ignore_stale_approvals": { - "type": "boolean", - "x-go-name": "IgnoreStaleApprovals" - }, "enable_approvals_whitelist": { "type": "boolean", "x-go-name": "EnableApprovalsWhitelist" @@ -18709,6 +18705,10 @@ "type": "boolean", "x-go-name": "EnableStatusCheck" }, + "ignore_stale_approvals": { + "type": "boolean", + "x-go-name": "IgnoreStaleApprovals" + }, "merge_whitelist_teams": { "type": "array", "items": { From c5070a3ae58ccc1f11bd0a8d2175caca67014adb Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Mon, 18 Dec 2023 15:04:41 +0100 Subject: [PATCH 3/7] Disable "Ignore stale approvals" when "Dismiss stale approvals" is checked --- templates/repo/settings/protected_branch.tmpl | 6 +++--- web_src/js/features/repo-settings.js | 21 ++++++++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index 7201ba546e528..8394f552fe7a1 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -144,14 +144,14 @@
- +

{{ctx.Locale.Tr "repo.settings.dismiss_stale_approvals_desc"}}

-
+
- +

{{ctx.Locale.Tr "repo.settings.ignore_stale_approvals_desc"}}

diff --git a/web_src/js/features/repo-settings.js b/web_src/js/features/repo-settings.js index 74cc30835484a..88e341ba3a9b8 100644 --- a/web_src/js/features/repo-settings.js +++ b/web_src/js/features/repo-settings.js @@ -1,9 +1,9 @@ import $ from 'jquery'; -import {minimatch} from 'minimatch'; -import {createMonaco} from './codeeditor.js'; -import {onInputDebounce, toggleElem} from '../utils/dom.js'; +import { minimatch } from 'minimatch'; +import { createMonaco } from './codeeditor.js'; +import { onInputDebounce, toggleElem } from '../utils/dom.js'; -const {appSubUrl, csrfToken} = window.config; +const { appSubUrl, csrfToken } = window.config; export function initRepoSettingsCollaboration() { // Change collaborator access mode @@ -48,7 +48,7 @@ export function initRepoSettingSearchTeamBox() { minCharacters: 2, apiSettings: { url: `${appSubUrl}/org/${$searchTeamBox.attr('data-org-name')}/teams/-/search?q={query}`, - headers: {'X-Csrf-Token': csrfToken}, + headers: { 'X-Csrf-Token': csrfToken }, onResponse(response) { const items = []; $.each(response.data, (_i, item) => { @@ -58,7 +58,7 @@ export function initRepoSettingSearchTeamBox() { }); }); - return {results: items}; + return { results: items }; } }, searchFields: ['name', 'description'], @@ -69,7 +69,7 @@ export function initRepoSettingSearchTeamBox() { export function initRepoSettingGitHook() { if ($('.edit.githook').length === 0) return; const filename = document.querySelector('.hook-filename').textContent; - const _promise = createMonaco($('#content')[0], filename, {language: 'shell'}); + const _promise = createMonaco($('#content')[0], filename, { language: 'shell' }); } export function initRepoSettingBranches() { @@ -82,6 +82,13 @@ 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); + if (this.checked) { + $('#ignore_stale_approvals').prop('checked', false); + } + }); // show the `Matched` mark for the status checks that match the pattern const markMatchedStatusChecks = () => { From 5c5e816df14e9fdf8ad4f4bc25f66e71681939a5 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Mon, 18 Dec 2023 15:06:26 +0100 Subject: [PATCH 4/7] Apply delvh's suggested rephrasing of ignore_stale_approvals_desc --- options/locale/locale_en-US.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 28b1aebfc01ac..c1203f155c59c 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2313,7 +2313,7 @@ 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 = When new commits that change the content of the pull request are pushed to the branch, old approvals will be ignored. +settings.ignore_stale_approvals_desc = Do not count approvals that were made on older commits (stale reviews) towards how many approvals the PR has. 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 From b3b40c36a1801abb6eafd769c04a7b1896fedde2 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Mon, 18 Dec 2023 15:33:07 +0100 Subject: [PATCH 5/7] Fix linting errors --- templates/repo/settings/protected_branch.tmpl | 2 +- web_src/js/features/repo-settings.js | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index 8394f552fe7a1..9c0fbddf069f9 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -149,7 +149,7 @@

{{ctx.Locale.Tr "repo.settings.dismiss_stale_approvals_desc"}}

-
+
diff --git a/web_src/js/features/repo-settings.js b/web_src/js/features/repo-settings.js index 88e341ba3a9b8..50997b4b9c2a9 100644 --- a/web_src/js/features/repo-settings.js +++ b/web_src/js/features/repo-settings.js @@ -1,9 +1,9 @@ import $ from 'jquery'; -import { minimatch } from 'minimatch'; -import { createMonaco } from './codeeditor.js'; -import { onInputDebounce, toggleElem } from '../utils/dom.js'; +import {minimatch} from 'minimatch'; +import {createMonaco} from './codeeditor.js'; +import {onInputDebounce, toggleElem} from '../utils/dom.js'; -const { appSubUrl, csrfToken } = window.config; +const {appSubUrl, csrfToken} = window.config; export function initRepoSettingsCollaboration() { // Change collaborator access mode @@ -48,7 +48,7 @@ export function initRepoSettingSearchTeamBox() { minCharacters: 2, apiSettings: { url: `${appSubUrl}/org/${$searchTeamBox.attr('data-org-name')}/teams/-/search?q={query}`, - headers: { 'X-Csrf-Token': csrfToken }, + headers: {'X-Csrf-Token': csrfToken}, onResponse(response) { const items = []; $.each(response.data, (_i, item) => { @@ -58,7 +58,7 @@ export function initRepoSettingSearchTeamBox() { }); }); - return { results: items }; + return {results: items}; } }, searchFields: ['name', 'description'], @@ -69,7 +69,7 @@ export function initRepoSettingSearchTeamBox() { export function initRepoSettingGitHook() { if ($('.edit.githook').length === 0) return; const filename = document.querySelector('.hook-filename').textContent; - const _promise = createMonaco($('#content')[0], filename, { language: 'shell' }); + const _promise = createMonaco($('#content')[0], filename, {language: 'shell'}); } export function initRepoSettingBranches() { From 30eb0703f01cef93afcc21353ae7f027a6829e2f Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Tue, 19 Dec 2023 17:54:07 +0100 Subject: [PATCH 6/7] Update web_src/js/features/repo-settings.js Co-authored-by: delvh --- web_src/js/features/repo-settings.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/web_src/js/features/repo-settings.js b/web_src/js/features/repo-settings.js index 50997b4b9c2a9..04974200bb996 100644 --- a/web_src/js/features/repo-settings.js +++ b/web_src/js/features/repo-settings.js @@ -85,9 +85,6 @@ export function initRepoSettingBranches() { $('#dismiss_stale_approvals').on('change', function () { const $target = $('#ignore_stale_approvals_box'); $target.toggleClass('disabled', this.checked); - if (this.checked) { - $('#ignore_stale_approvals').prop('checked', false); - } }); // show the `Matched` mark for the status checks that match the pattern From 7bb2dd56caaa9ddebd323937f6b3952a8a814578 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Tue, 19 Dec 2023 17:54:16 +0100 Subject: [PATCH 7/7] Update options/locale/locale_en-US.ini Co-authored-by: delvh --- options/locale/locale_en-US.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index c1203f155c59c..1148a1c863a4b 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2313,7 +2313,7 @@ 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. +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