Skip to content

Commit 32fb813

Browse files
davidsvantessontechknowlogick
davidsvantesson
authored and
techknowlogick
committed
Allow repo admin to merge PR regardless of review status (#9611)
* Allow repo admin to merge even if review is not ok.
1 parent 4d06d10 commit 32fb813

File tree

13 files changed

+231
-119
lines changed

13 files changed

+231
-119
lines changed

‎models/branches.go

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,8 @@ func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool {
9393
return in
9494
}
9595

96-
// CanUserMerge returns if some user could merge a pull request to this protected branch
97-
func (protectBranch *ProtectedBranch) CanUserMerge(userID int64) bool {
96+
// IsUserMergeWhitelisted checks if some user is whitelisted to merge to this branch
97+
func (protectBranch *ProtectedBranch) IsUserMergeWhitelisted(userID int64) bool {
9898
if !protectBranch.EnableMergeWhitelist {
9999
return true
100100
}
@@ -348,27 +348,6 @@ func (repo *Repository) IsProtectedBranchForPush(branchName string, doer *User)
348348
return false, nil
349349
}
350350

351-
// IsProtectedBranchForMerging checks if branch is protected for merging
352-
func (repo *Repository) IsProtectedBranchForMerging(pr *PullRequest, branchName string, doer *User) (bool, error) {
353-
if doer == nil {
354-
return true, nil
355-
}
356-
357-
protectedBranch := &ProtectedBranch{
358-
RepoID: repo.ID,
359-
BranchName: branchName,
360-
}
361-
362-
has, err := x.Get(protectedBranch)
363-
if err != nil {
364-
return true, err
365-
} else if has {
366-
return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr) || protectedBranch.MergeBlockedByRejectedReview(pr), nil
367-
}
368-
369-
return false, nil
370-
}
371-
372351
// updateApprovalWhitelist checks whether the user whitelist changed and returns a whitelist with
373352
// the users from newWhitelist which have explicit read or write access to the repo.
374353
func updateApprovalWhitelist(repo *Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) {

‎models/pull.go

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -479,31 +479,6 @@ const (
479479
MergeStyleSquash MergeStyle = "squash"
480480
)
481481

482-
// CheckUserAllowedToMerge checks whether the user is allowed to merge
483-
func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) {
484-
if doer == nil {
485-
return ErrNotAllowedToMerge{
486-
"Not signed in",
487-
}
488-
}
489-
490-
if pr.BaseRepo == nil {
491-
if err = pr.GetBaseRepo(); err != nil {
492-
return fmt.Errorf("GetBaseRepo: %v", err)
493-
}
494-
}
495-
496-
if protected, err := pr.BaseRepo.IsProtectedBranchForMerging(pr, pr.BaseBranch, doer); err != nil {
497-
return fmt.Errorf("IsProtectedBranch: %v", err)
498-
} else if protected {
499-
return ErrNotAllowedToMerge{
500-
"The branch is protected",
501-
}
502-
}
503-
504-
return nil
505-
}
506-
507482
// SetMerged sets a pull request to merged and closes the corresponding issue
508483
func (pr *PullRequest) SetMerged() (err error) {
509484
if pr.HasMerged {

‎models/repo_permission.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss
271271
return
272272
}
273273

274-
// IsUserRepoAdmin return ture if user has admin right of a repo
274+
// IsUserRepoAdmin return true if user has admin right of a repo
275275
func IsUserRepoAdmin(repo *Repository, user *User) (bool, error) {
276276
return isUserRepoAdmin(x, repo, user)
277277
}

‎modules/auth/repo_form.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,7 @@ type MergePullRequestForm struct {
448448
Do string `binding:"Required;In(merge,rebase,rebase-merge,squash)"`
449449
MergeTitleField string
450450
MergeMessageField string
451+
ForceMerge *bool `json:"force_merge,omitempty"`
451452
}
452453

453454
// Validate validates the fields

‎modules/convert/convert.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func ToBranch(repo *models.Repository, b *git.Branch, c *git.Commit, bp *models.
5151
EnableStatusCheck: bp.EnableStatusCheck,
5252
StatusCheckContexts: bp.StatusCheckContexts,
5353
UserCanPush: bp.CanUserPush(user.ID),
54-
UserCanMerge: bp.CanUserMerge(user.ID),
54+
UserCanMerge: bp.IsUserMergeWhitelisted(user.ID),
5555
}
5656
}
5757

‎options/locale/locale_en-US.ini

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1062,7 +1062,8 @@ pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts.
10621062
pulls.no_merge_desc = This pull request cannot be merged because all repository merge options are disabled.
10631063
pulls.no_merge_helper = Enable merge options in the repository settings or merge the pull request manually.
10641064
pulls.no_merge_wip = This pull request can not be merged because it is marked as being a work in progress.
1065-
pulls.no_merge_status_check = This pull request cannot be merged because not all required status checkes are successful.
1065+
pulls.no_merge_not_ready = This pull request is not ready to be merged, check review status and status checks.
1066+
pulls.no_merge_access = You are not authorized to merge this pull request.
10661067
pulls.merge_pull_request = Merge Pull Request
10671068
pulls.rebase_merge_pull_request = Rebase and Merge
10681069
pulls.rebase_merge_commit_pull_request = Rebase and Merge (--no-ff)

‎routers/api/v1/repo/pull.go

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -600,22 +600,45 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
600600
return
601601
}
602602

603-
if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() {
604-
ctx.Status(http.StatusMethodNotAllowed)
603+
perm, err := models.GetUserRepoPermission(ctx.Repo.Repository, ctx.User)
604+
if err != nil {
605+
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
605606
return
606607
}
607608

608-
isPass, err := pull_service.IsPullCommitStatusPass(pr)
609+
allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, ctx.User)
609610
if err != nil {
610-
ctx.Error(http.StatusInternalServerError, "IsPullCommitStatusPass", err)
611+
ctx.Error(http.StatusInternalServerError, "IsUSerAllowedToMerge", err)
612+
return
613+
}
614+
if !allowedMerge {
615+
ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
611616
return
612617
}
613618

614-
if !isPass && !ctx.IsUserRepoAdmin() {
619+
if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() {
615620
ctx.Status(http.StatusMethodNotAllowed)
616621
return
617622
}
618623

624+
if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
625+
if !models.IsErrNotAllowedToMerge(err) {
626+
ctx.Error(http.StatusInternalServerError, "CheckPRReadyToMerge", err)
627+
return
628+
}
629+
if form.ForceMerge != nil && *form.ForceMerge {
630+
if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.User); err != nil {
631+
ctx.Error(http.StatusInternalServerError, "IsUserRepoAdmin", err)
632+
return
633+
} else if !isRepoAdmin {
634+
ctx.Error(http.StatusMethodNotAllowed, "Merge", "Only repository admin can merge if not all checks are ok (force merge)")
635+
}
636+
} else {
637+
ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
638+
return
639+
}
640+
}
641+
619642
if len(form.Do) == 0 {
620643
form.Do = string(models.MergeStyleMerge)
621644
}

‎routers/private/hook.go

Lines changed: 42 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"code.gitea.io/gitea/modules/private"
1818
"code.gitea.io/gitea/modules/repofiles"
1919
"code.gitea.io/gitea/modules/util"
20+
pull_service "code.gitea.io/gitea/services/pull"
2021

2122
"gitea.com/macaron/macaron"
2223
)
@@ -98,6 +99,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
9899
canPush = protectBranch.CanUserPush(opts.UserID)
99100
}
100101
if !canPush && opts.ProtectedBranchID > 0 {
102+
// Manual merge
101103
pr, err := models.GetPullRequestByID(opts.ProtectedBranchID)
102104
if err != nil {
103105
log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err)
@@ -106,24 +108,55 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
106108
})
107109
return
108110
}
109-
if !protectBranch.HasEnoughApprovals(pr) {
110-
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)
111-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
112-
"err": fmt.Sprintf("protected branch %s can not be pushed to and pr #%d does not have enough approvals", branchName, opts.ProtectedBranchID),
111+
user, err := models.GetUserByID(opts.UserID)
112+
if err != nil {
113+
log.Error("Unable to get User id %d Error: %v", opts.UserID, err)
114+
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
115+
"err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err),
116+
})
117+
return
118+
}
119+
perm, err := models.GetUserRepoPermission(repo, user)
120+
if err != nil {
121+
log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err)
122+
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
123+
"err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err),
113124
})
114125
return
115126
}
116-
if protectBranch.MergeBlockedByRejectedReview(pr) {
117-
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)
127+
allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user)
128+
if err != nil {
129+
log.Error("Error calculating if allowed to merge: %v", err)
130+
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
131+
"err": fmt.Sprintf("Error calculating if allowed to merge: %v", err),
132+
})
133+
return
134+
}
135+
if !allowedMerge {
136+
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)
118137
ctx.JSON(http.StatusForbidden, map[string]interface{}{
119-
"err": fmt.Sprintf("protected branch %s can not be pushed to and pr #%d has requested changes", branchName, opts.ProtectedBranchID),
138+
"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
120139
})
121140
return
122141
}
142+
// Manual merge only allowed if PR is ready (even if admin)
143+
if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
144+
if models.IsErrNotAllowedToMerge(err) {
145+
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())
146+
ctx.JSON(http.StatusForbidden, map[string]interface{}{
147+
"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()),
148+
})
149+
return
150+
}
151+
log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err)
152+
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
153+
"err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err),
154+
})
155+
}
123156
} else if !canPush {
124-
log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v", opts.UserID, branchName, repo)
157+
log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo)
125158
ctx.JSON(http.StatusForbidden, map[string]interface{}{
126-
"err": fmt.Sprintf("protected branch %s can not be pushed to", branchName),
159+
"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
127160
})
128161
return
129162
}

‎routers/repo/issue.go

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -903,6 +903,7 @@ func ViewIssue(ctx *context.Context) {
903903
pull := issue.PullRequest
904904
pull.Issue = issue
905905
canDelete := false
906+
ctx.Data["AllowMerge"] = false
906907

907908
if ctx.IsSigned {
908909
if err := pull.GetHeadRepo(); err != nil {
@@ -923,6 +924,20 @@ func ViewIssue(ctx *context.Context) {
923924
}
924925
}
925926
}
927+
928+
if err := pull.GetBaseRepo(); err != nil {
929+
log.Error("GetBaseRepo: %v", err)
930+
}
931+
perm, err := models.GetUserRepoPermission(pull.BaseRepo, ctx.User)
932+
if err != nil {
933+
ctx.ServerError("GetUserRepoPermission", err)
934+
return
935+
}
936+
ctx.Data["AllowMerge"], err = pull_service.IsUserAllowedToMerge(pull, perm, ctx.User)
937+
if err != nil {
938+
ctx.ServerError("IsUserAllowedToMerge", err)
939+
return
940+
}
926941
}
927942

928943
prUnit, err := repo.GetUnit(models.UnitTypePullRequests)
@@ -932,15 +947,6 @@ func ViewIssue(ctx *context.Context) {
932947
}
933948
prConfig := prUnit.PullRequestsConfig()
934949

935-
ctx.Data["AllowMerge"] = ctx.Repo.CanWrite(models.UnitTypeCode)
936-
if err := pull.CheckUserAllowedToMerge(ctx.User); err != nil {
937-
if !models.IsErrNotAllowedToMerge(err) {
938-
ctx.ServerError("CheckUserAllowedToMerge", err)
939-
return
940-
}
941-
ctx.Data["AllowMerge"] = false
942-
}
943-
944950
// Check correct values and select default
945951
if ms, ok := ctx.Data["MergeStyle"].(models.MergeStyle); !ok ||
946952
!prConfig.IsMergeStyleAllowed(ms) {

‎routers/repo/pull.go

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -600,6 +600,16 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
600600

601601
pr := issue.PullRequest
602602

603+
allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.Repo.Permission, ctx.User)
604+
if err != nil {
605+
ctx.ServerError("IsUserAllowedToMerge", err)
606+
return
607+
}
608+
if !allowedMerge {
609+
ctx.NotFound("MergePullRequest", nil)
610+
return
611+
}
612+
603613
if !pr.CanAutoMerge() || pr.HasMerged {
604614
ctx.NotFound("MergePullRequest", nil)
605615
return
@@ -611,15 +621,19 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
611621
return
612622
}
613623

614-
isPass, err := pull_service.IsPullCommitStatusPass(pr)
615-
if err != nil {
616-
ctx.ServerError("IsPullCommitStatusPass", err)
617-
return
618-
}
619-
if !isPass && !ctx.IsUserRepoAdmin() {
620-
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_status_check"))
621-
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
622-
return
624+
if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
625+
if !models.IsErrNotAllowedToMerge(err) {
626+
ctx.ServerError("Merge PR status", err)
627+
return
628+
}
629+
if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.User); err != nil {
630+
ctx.ServerError("IsUserRepoAdmin", err)
631+
return
632+
} else if !isRepoAdmin {
633+
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
634+
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
635+
return
636+
}
623637
}
624638

625639
if ctx.HasError() {

0 commit comments

Comments
 (0)