Allow repo admin to merge PR regardless of review status

models/branches.go

@@ -91,8 +91,8 @@ func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool {
 	return in
 }
 
-// CanUserMerge returns if some user could merge a pull request to this protected branch
-func (protectBranch *ProtectedBranch) CanUserMerge(userID int64) bool {
+// IsUserMergeWhitelisted checks if some user is whitelisted to merge to this branch
+func (protectBranch *ProtectedBranch) IsUserMergeWhitelisted(userID int64) bool {
 	if !protectBranch.EnableMergeWhitelist {
 		return true
 	}
@@ -343,27 +343,6 @@ func (repo *Repository) IsProtectedBranchForPush(branchName string, doer *User)
 	return false, nil
 }
 
-// IsProtectedBranchForMerging checks if branch is protected for merging
-func (repo *Repository) IsProtectedBranchForMerging(pr *PullRequest, branchName string, doer *User) (bool, error) {
-	if doer == nil {
-		return true, nil
-	}
-
-	protectedBranch := &ProtectedBranch{
-		RepoID:     repo.ID,
-		BranchName: branchName,
-	}
-
-	has, err := x.Get(protectedBranch)
-	if err != nil {
-		return true, err
-	} else if has {
-		return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr) || protectedBranch.MergeBlockedByRejectedReview(pr), nil
-	}
-
-	return false, nil
-}
-
 // updateApprovalWhitelist checks whether the user whitelist changed and returns a whitelist with
 // the users from newWhitelist which have explicit read or write access to the repo.
 func updateApprovalWhitelist(repo *Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) {

models/pull.go

@@ -608,31 +608,6 @@ const (
 	MergeStyleSquash MergeStyle = "squash"
 )
 
-// CheckUserAllowedToMerge checks whether the user is allowed to merge
-func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) {
-	if doer == nil {
-		return ErrNotAllowedToMerge{
-			"Not signed in",
-		}
-	}
-
-	if pr.BaseRepo == nil {
-		if err = pr.GetBaseRepo(); err != nil {
-			return fmt.Errorf("GetBaseRepo: %v", err)
-		}
-	}
-
-	if protected, err := pr.BaseRepo.IsProtectedBranchForMerging(pr, pr.BaseBranch, doer); err != nil {
-		return fmt.Errorf("IsProtectedBranch: %v", err)
-	} else if protected {
-		return ErrNotAllowedToMerge{
-			"The branch is protected",
-		}
-	}
-
-	return nil
-}
-
 // SetMerged sets a pull request to merged and closes the corresponding issue
 func (pr *PullRequest) SetMerged() (err error) {
 	if pr.HasMerged {

models/repo_permission.go

@@ -271,7 +271,7 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss
 	return
 }
 
-// IsUserRepoAdmin return ture if user has admin right of a repo
+// IsUserRepoAdmin return true if user has admin right of a repo
 func IsUserRepoAdmin(repo *Repository, user *User) (bool, error) {
 	return isUserRepoAdmin(x, repo, user)
 }

modules/auth/repo_form.go

@@ -447,6 +447,7 @@ type MergePullRequestForm struct {
 	Do                string `binding:"Required;In(merge,rebase,rebase-merge,squash)"`
 	MergeTitleField   string
 	MergeMessageField string
+	ForceMerge        *bool `json:"force_merge,omitempty"`
 }
 
 // Validate validates the fields

modules/convert/convert.go

@@ -51,7 +51,7 @@ func ToBranch(repo *models.Repository, b *git.Branch, c *git.Commit, bp *models.
 		EnableStatusCheck:   bp.EnableStatusCheck,
 		StatusCheckContexts: bp.StatusCheckContexts,
 		UserCanPush:         bp.CanUserPush(user.ID),
-		UserCanMerge:        bp.CanUserMerge(user.ID),
+		UserCanMerge:        bp.IsUserMergeWhitelisted(user.ID),
 	}
 }
 

options/locale/locale_en-US.ini

@@ -1062,7 +1062,8 @@ pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts.
 pulls.no_merge_desc = This pull request cannot be merged because all repository merge options are disabled.
 pulls.no_merge_helper = Enable merge options in the repository settings or merge the pull request manually.
 pulls.no_merge_wip = This pull request can not be merged because it is marked as being a work in progress.
-pulls.no_merge_status_check = This pull request cannot be merged because not all required status checkes are successful.
+pulls.no_merge_not_ready = This pull request is not ready to be merged, check status of reviews and status checks.
+pulls.no_merge_access = You are not authorized to merge this pull request.
 pulls.merge_pull_request = Merge Pull Request
 pulls.rebase_merge_pull_request = Rebase and Merge
 pulls.rebase_merge_commit_pull_request = Rebase and Merge (--no-ff)

routers/api/v1/repo/pull.go

@@ -597,22 +597,41 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
 		return
 	}
 
-	if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() {
-		ctx.Status(http.StatusMethodNotAllowed)
+	perm, err := models.GetUserRepoPermission(ctx.Repo.Repository, ctx.User)
+	if err != nil {
+		ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
 		return
 	}
 
-	isPass, err := pull_service.IsPullCommitStatusPass(pr)
+	allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, ctx.User)
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "IsPullCommitStatusPass", err)
+		ctx.Error(http.StatusInternalServerError, "IsUSerAllowedToMerge", err)
+		return
+	}
+	if !allowedMerge {
+		ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
 		return
 	}
 
-	if !isPass && !ctx.IsUserRepoAdmin() {
+	if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() {
 		ctx.Status(http.StatusMethodNotAllowed)
 		return
 	}
 
+	if err := pull_service.CheckPrReadyToMerge(pr); err != nil {
+		if form.ForceMerge != nil && *form.ForceMerge {
+			if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.User); err != nil {
+				ctx.Error(http.StatusInternalServerError, "IsUserRepoAdmin", err)
+				return
+			} else if !isRepoAdmin {
+				ctx.Error(http.StatusMethodNotAllowed, "Merge", "Only repository admin can merge if not all checks are ok (force merge)")
+			}
+		} else {
+			ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
+			return
+		}
+	}
+
 	if len(form.Do) == 0 {
 		form.Do = string(models.MergeStyleMerge)
 	}

routers/private/hook.go

@@ -17,6 +17,7 @@ import (
 	"code.gitea.io/gitea/modules/private"
 	"code.gitea.io/gitea/modules/repofiles"
 	"code.gitea.io/gitea/modules/util"
+	pull_service "code.gitea.io/gitea/services/pull"
 
 	"gitea.com/macaron/macaron"
 )
@@ -98,6 +99,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 				canPush = protectBranch.CanUserPush(opts.UserID)
 			}
 			if !canPush && opts.ProtectedBranchID > 0 {
+				// Manual merge
 				pr, err := models.GetPullRequestByID(opts.ProtectedBranchID)
 				if err != nil {
 					log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err)
@@ -106,24 +108,55 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 					})
 					return
 				}
-				if !protectBranch.HasEnoughApprovals(pr) {
-					log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v and pr #%d does not have enough approvals", 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 does not have enough approvals", branchName, opts.ProtectedBranchID),
+				user, err := models.GetUserByID(opts.UserID)
+				if err != nil {
+					log.Error("Unable to get User id %d Error: %v", opts.UserID, err)
+					ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+						"err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err),
+					})
+					return
+				}
+				perm, err := models.GetUserRepoPermission(repo, user)
+				if err != nil {
+					log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err)
+					ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+						"err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err),
 					})
 					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)
+				allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user)
+				if err != nil {
+					log.Error("Error calculating if allowed to merge: %v", err)
+					ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+						"err": fmt.Sprintf("Error calculating if allowed to merge: %v", err),
+					})
+					return
+				}
+				if !allowedMerge {
+					log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v and is not allowed to merge pr #%d", 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),
+						"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
 					})
 					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()),
+						})
+						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 cannot push to protected branch: %s in %-v", opts.UserID, branchName, repo)
+				log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo)
 				ctx.JSON(http.StatusForbidden, map[string]interface{}{
-					"err": fmt.Sprintf("protected branch %s can not be pushed to", branchName),
+					"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
 				})
 				return
 			}

routers/repo/issue.go

@@ -903,6 +903,7 @@ func ViewIssue(ctx *context.Context) {
 		pull := issue.PullRequest
 		pull.Issue = issue
 		canDelete := false
+		ctx.Data["AllowMerge"] = false
 
 		if ctx.IsSigned {
 			if err := pull.GetHeadRepo(); err != nil {
@@ -923,6 +924,20 @@ func ViewIssue(ctx *context.Context) {
 					}
 				}
 			}
+
+			if err := pull.GetBaseRepo(); err != nil {
+				log.Error("GetBaseRepo: %v", err)
+			}
+			perm, err := models.GetUserRepoPermission(pull.BaseRepo, ctx.User)
+			if err != nil {
+				ctx.ServerError("GetUserRepoPermission", err)
+				return
+			}
+			ctx.Data["AllowMerge"], err = pull_service.IsUserAllowedToMerge(pull, perm, ctx.User)
+			if err != nil {
+				ctx.ServerError("IsUserAllowedToMerge", err)
+				return
+			}
 		}
 
 		prUnit, err := repo.GetUnit(models.UnitTypePullRequests)
@@ -932,15 +947,6 @@ func ViewIssue(ctx *context.Context) {
 		}
 		prConfig := prUnit.PullRequestsConfig()
 
-		ctx.Data["AllowMerge"] = ctx.Repo.CanWrite(models.UnitTypeCode)
-		if err := pull.CheckUserAllowedToMerge(ctx.User); err != nil {
-			if !models.IsErrNotAllowedToMerge(err) {
-				ctx.ServerError("CheckUserAllowedToMerge", err)
-				return
-			}
-			ctx.Data["AllowMerge"] = false
-		}
-
 		// Check correct values and select default
 		if ms, ok := ctx.Data["MergeStyle"].(models.MergeStyle); !ok ||
 			!prConfig.IsMergeStyleAllowed(ms) {

routers/repo/pull.go

@@ -600,6 +600,16 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
 
 	pr := issue.PullRequest
 
+	allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.Repo.Permission, ctx.User)
+	if err != nil {
+		ctx.ServerError("IsUserAllowedToMerge", err)
+		return
+	}
+	if !allowedMerge {
+		ctx.NotFound("MergePullRequest", nil)
+		return
+	}
+
 	if !pr.CanAutoMerge() || pr.HasMerged {
 		ctx.NotFound("MergePullRequest", nil)
 		return
@@ -611,15 +621,19 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
 		return
 	}
 
-	isPass, err := pull_service.IsPullCommitStatusPass(pr)
-	if err != nil {
-		ctx.ServerError("IsPullCommitStatusPass", err)
-		return
-	}
-	if !isPass && !ctx.IsUserRepoAdmin() {
-		ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_status_check"))
-		ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
-		return
+	if err := pull_service.CheckPrReadyToMerge(pr); err != nil {
+		if !models.IsErrNotAllowedToMerge(err) {
+			ctx.ServerError("Merge PR status", err)
+			return
+		}
+		if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.User); err != nil {
+			ctx.ServerError("IsUserRepoAdmin", err)
+			return
+		} else if !isRepoAdmin {
+			ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
+			ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
+			return
+		}
 	}
 
 	if ctx.HasError() {