diff --git a/routers/private/hook.go b/routers/private/hook.go index 6a07de15ff..7044fdac22 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -224,7 +224,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { canPush = protectBranch.CanUserPush(opts.UserID) } if !canPush && opts.ProtectedBranchID > 0 { - // Manual merge + // Merge (from UI or API) pr, err := models.GetPullRequestByID(opts.ProtectedBranchID) if err != nil { log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err) @@ -264,19 +264,21 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { }) return } - // Manual merge only allowed if PR is ready (even if admin) - if err := pull_service.CheckPRReadyToMerge(pr); err != nil { - if models.IsErrNotAllowedToMerge(err) { - log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error()) - ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()), + // Check all status checks and reviews is ok, unless repo admin which can bypass this. + if !perm.IsAdmin() { + if err := pull_service.CheckPRReadyToMerge(pr); err != nil { + if models.IsErrNotAllowedToMerge(err) { + log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error()) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()), + }) + return + } + log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err), }) - return } - log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err), - }) } } else if !canPush { log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo) diff --git a/services/pull/merge.go b/services/pull/merge.go index 5e077f6dc0..b423c663ea 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -487,9 +487,6 @@ func IsSignedIfRequired(pr *models.PullRequest, doer *models.User) (bool, error) // IsUserAllowedToMerge check if user is allowed to merge PR with given permissions and branch protections func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *models.User) (bool, error) { - if p.IsAdmin() { - return true, nil - } if !p.CanWrite(models.UnitTypeCode) { return false, nil } diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 115ea2d119..f8a82f1a0f 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -133,9 +133,9 @@ {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }} {{end}} - {{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}} - {{if and (or $.IsRepoAdmin (not $notAllOk)) (or (not .RequireSigned) .WillSign)}} - {{if $notAllOk}} + {{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}} + {{if and (or $.IsRepoAdmin (not $notAllOverridableChecksOk)) (or (not .RequireSigned) .WillSign)}} + {{if $notAllOverridableChecksOk}}