Fix admin handling at merge of PR

routers/private/hook.go

@@ -99,7 +99,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)
@@ -139,19 +139,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)

services/pull/merge.go

@@ -472,9 +472,6 @@ func getDiffTree(repoPath, baseBranch, headBranch string) (string, 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
 	}

templates/repo/issue/view_content/pull.tmpl

@@ -124,7 +124,7 @@
 					</div>
 				{{end}}
 				{{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}}	
-				{{if or $.IsRepoAdmin (not $notAllOk)}}
+				{{if or (not $notAllOk) (and $.IsRepoAdmin .AllowMerge)}}
 					{{if $notAllOk}}
 						<div class="item text yellow">
 							<span class="octicon octicon-primitive-dot"></span>