diff --git a/models/branches.go b/models/branches.go index 21b23c75d..385817e4f 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 e8bb3f16d..73c9bc113 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 000000000..662d6c7b4 --- /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 26ed2bb69..c87549af9 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 691190427..712cd7ca4 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 dc5001ad4..c1e283f35 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 925903fee..46020acb6 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 c279c94b1..8872c7471 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 3503a5174..cec10a620 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.settings.block_rejected_reviews_desc"}}
+