From ea707f5a77d83d023cb02ce8e44c4b95ac83ef30 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Fri, 3 Jan 2020 18:47:10 +0100 Subject: [PATCH] Add branch protection option to block merge on requested changes. (#9592) * Add branch protection option to block merge on requested changes. * Add migration step * Fix check to correct negation * Apply suggestions from code review Language improvement. Co-Authored-By: John Olheiser <42128690+jolheiser@users.noreply.github.com> * Copyright year. Co-authored-by: John Olheiser <42128690+jolheiser@users.noreply.github.com> Co-authored-by: Lauris BH --- models/branches.go | 20 ++++++++++++++++++- models/migrations/migrations.go | 2 ++ models/migrations/v117.go | 17 ++++++++++++++++ modules/auth/repo_form.go | 1 + options/locale/locale_en-US.ini | 3 +++ routers/private/hook.go | 7 +++++++ routers/repo/issue.go | 3 ++- routers/repo/setting_protected_branch.go | 1 + templates/repo/issue/view_content/pull.tmpl | 6 ++++++ templates/repo/settings/protected_branch.tmpl | 7 +++++++ 10 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 models/migrations/v117.go diff --git a/models/branches.go b/models/branches.go index 21b23c75d9..385817e4f9 100644 --- a/models/branches.go +++ b/models/branches.go @@ -44,6 +44,7 @@ type ProtectedBranch struct { 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"` } @@ -166,6 +167,23 @@ func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest) return approvals } +// MergeBlockedByRejectedReview returns true if merge is blocked by rejected reviews +func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullRequest) bool { + if !protectBranch.BlockOnRejectedReviews { + return false + } + rejectExist, err := x.Where("issue_id = ?", pr.IssueID). + And("type = ?", ReviewTypeReject). + And("official = ?", true). + Exist(new(Review)) + if err != nil { + log.Error("MergeBlockedByRejectedReview: %v", err) + return true + } + + return rejectExist +} + // GetProtectedBranchByRepoID getting protected branch by repo ID func GetProtectedBranchByRepoID(repoID int64) ([]*ProtectedBranch, error) { protectedBranches := make([]*ProtectedBranch, 0) @@ -340,7 +358,7 @@ func (repo *Repository) IsProtectedBranchForMerging(pr *PullRequest, branchName if err != nil { return true, err } else if has { - return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr), nil + return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr) || protectedBranch.MergeBlockedByRejectedReview(pr), nil } return false, nil diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index e8bb3f16d4..73c9bc1138 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -288,6 +288,8 @@ var migrations = []Migration{ NewMigration("add user_id prefix to existing user avatar name", renameExistingUserAvatarName), // v116 -> v117 NewMigration("Extend TrackedTimes", extendTrackedTimes), + // v117 -> v118 + NewMigration("Add block on rejected reviews branch protection", addBlockOnRejectedReviews), } // Migrate database to current version diff --git a/models/migrations/v117.go b/models/migrations/v117.go new file mode 100644 index 0000000000..662d6c7b46 --- /dev/null +++ b/models/migrations/v117.go @@ -0,0 +1,17 @@ +// Copyright 2020 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 addBlockOnRejectedReviews(x *xorm.Engine) error { + type ProtectedBranch struct { + BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"` + } + + return x.Sync2(new(ProtectedBranch)) +} diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index 26ed2bb692..c87549af92 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -171,6 +171,7 @@ type ProtectBranchForm struct { EnableApprovalsWhitelist bool ApprovalsWhitelistUsers string ApprovalsWhitelistTeams string + BlockOnRejectedReviews bool } // Validate validates the fields diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 6911904271..712cd7ca41 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1054,6 +1054,7 @@ pulls.is_checking = "Merge conflict checking is in progress. Try again in few mo pulls.required_status_check_failed = Some required checks were not successful. pulls.required_status_check_administrator = As an administrator, you may still merge this pull request. pulls.blocked_by_approvals = "This Pull Request doesn't have enough approvals yet. %d of %d approvals granted." +pulls.blocked_by_rejection = "This Pull Request has changes requested by an official reviewer." pulls.can_auto_merge_desc = This pull request can be merged automatically. pulls.cannot_auto_merge_desc = This pull request cannot be merged automatically due to conflicts. pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts. @@ -1417,6 +1418,8 @@ settings.update_protect_branch_success = Branch protection for branch '%s' has b settings.remove_protected_branch_success = Branch protection for branch '%s' has been disabled. settings.protected_branch_deletion = Disable Branch Protection settings.protected_branch_deletion_desc = Disabling branch protection allows users with write permission to push to the branch. Continue? +settings.block_rejected_reviews = Block merge on rejected reviews +settings.block_rejected_reviews_desc = Merging will not be possible when changes are requested by official reviewers, even if there are enough approvals. settings.default_branch_desc = Select a default repository branch for pull requests and code commits: settings.choose_branch = Choose a branch… settings.no_protected_branch = There are no protected branches. diff --git a/routers/private/hook.go b/routers/private/hook.go index dc5001ad4e..c1e283f357 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -113,6 +113,13 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { }) return } + if protectBranch.MergeBlockedByRejectedReview(pr) { + log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v and pr #%d has requested changes", opts.UserID, branchName, repo, pr.Index) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("protected branch %s can not be pushed to and pr #%d has requested changes", branchName, opts.ProtectedBranchID), + }) + return + } } else if !canPush { log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v", opts.UserID, branchName, repo) ctx.JSON(http.StatusForbidden, map[string]interface{}{ diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 925903fee4..46020acb6d 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -962,7 +962,8 @@ func ViewIssue(ctx *context.Context) { } if pull.ProtectedBranch != nil { cnt := pull.ProtectedBranch.GetGrantedApprovalsCount(pull) - ctx.Data["IsBlockedByApprovals"] = pull.ProtectedBranch.RequiredApprovals > 0 && cnt < pull.ProtectedBranch.RequiredApprovals + ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull) + ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull) ctx.Data["GrantedApprovals"] = cnt } ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index c279c94b1b..8872c7471f 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -244,6 +244,7 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) approvalsWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.ApprovalsWhitelistTeams, ",")) } } + protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{ UserIDs: whitelistUsers, diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 3503a51742..cec10a620e 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -41,6 +41,7 @@ {{else if .IsFilesConflicted}}grey {{else if .IsPullRequestBroken}}red {{else if .IsBlockedByApprovals}}red + {{else if .IsBlockedByRejection}}red {{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}red {{else if .Issue.PullRequest.IsChecking}}yellow {{else if .Issue.PullRequest.CanAutoMerge}}green @@ -100,6 +101,11 @@ {{$.i18n.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}} + {{else if .IsBlockedByRejection}} +
+ + {{$.i18n.Tr "repo.pulls.blocked_by_rejection"}} +
{{else if .Issue.PullRequest.IsChecking}}
diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index b6305861af..bf74a12330 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -204,6 +204,13 @@
{{end}} +
+
+ + +

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

+
+
-- 2.39.5