diff --git a/models/issues/pull.go b/models/issues/pull.go index 1fb42915f60..4c8d02a990e 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -437,8 +437,8 @@ func (pr *PullRequest) IsChecking() bool { return pr.Status == PullRequestStatusChecking } -// CanAutoMerge returns true if this pull request can be merged automatically. -func (pr *PullRequest) CanAutoMerge() bool { +// IsStatusMergeable returns true if this pull request is mergeable to its base +func (pr *PullRequest) IsStatusMergeable() bool { return pr.Status == PullRequestStatusMergeable } diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index 583fc610cba..e01bec4fc46 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -417,8 +417,8 @@ func ViewIssue(ctx *context.Context) { return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) } - if issue.PullRequest != nil && !issue.PullRequest.IsChecking() && !setting.IsProd { - ctx.Data["PullMergeBoxReloadingInterval"] = 1 // in dev env, force using the reloading logic to make sure it won't break + if !setting.IsProd && issue.PullRequest != nil && !issue.PullRequest.IsChecking() && prViewInfo.MergeBoxData != nil { + prViewInfo.MergeBoxData.ReloadingInterval = 1 // in dev env, force using the reloading logic to make sure it won't break } ctx.HTML(http.StatusOK, tplIssueView) @@ -485,15 +485,12 @@ func prepareIssueViewSidebarDependency(ctx *context.Context, issue *issues_model ctx.Data["BlockingDependencies"], ctx.Data["BlockingDependenciesNotPermitted"] = checkBlockedByIssues(ctx, blocking) } -func preparePullViewSigning(ctx *context.Context, issue *issues_model.Issue) { - if !issue.IsPull { - return - } - pull := issue.PullRequest - ctx.Data["WillSign"] = false +func (prInfo *pullRequestViewInfo) prepareMergeBoxRequireSigning(ctx *context.Context) { + pull := prInfo.issue.PullRequest + willSign := false if ctx.Doer != nil { sign, key, _, err := asymkey_service.SignMerge(ctx, pull, ctx.Doer, ctx.Repo.GitRepo) - ctx.Data["WillSign"] = sign + willSign = sign ctx.Data["SigningKeyMergeDisplay"] = asymkey_model.GetDisplaySigningKey(key) if err != nil { if asymkey_service.IsErrWontSign(err) { @@ -506,6 +503,8 @@ func preparePullViewSigning(ctx *context.Context, issue *issues_model.Issue) { } else { ctx.Data["WontSignReason"] = "not_signed_in" } + ctx.Data["WillSign"] = willSign + prInfo.MergeBoxData.willSign = willSign } func prepareIssueViewSidebarWatch(ctx *context.Context, issue *issues_model.Issue) { @@ -574,6 +573,7 @@ func (prInfo *pullRequestViewInfo) prepareMergeBoxDeleteBranch(ctx *context.Cont isPullBranchDeletable = !exist } ctx.Data["IsPullBranchDeletable"] = isPullBranchDeletable + prInfo.MergeBoxData.isPullBranchDeletable = isPullBranchDeletable } func prepareIssueViewSidebarPin(ctx *context.Context, issue *issues_model.Issue) { @@ -826,8 +826,15 @@ func (prInfo *pullRequestViewInfo) prepareMergeBox(ctx *context.Context, issue * panic("impossible, issue must be the same") } + data := &pullMergeBoxData{} + prInfo.MergeBoxData = data + + statusCheckData := prInfo.StatusCheckData + if statusCheckData == nil { + statusCheckData = &pullCommitStatusCheckData{} // make the following logic easier, no need to keep checking "nil" + } + pull := issue.PullRequest - pull.Issue = issue canDelete := false allowMerge := false canWriteToHeadRepo := false @@ -896,7 +903,7 @@ func (prInfo *pullRequestViewInfo) prepareMergeBox(ctx *context.Context, issue * } } - ctx.Data["PullMergeBoxReloadingInterval"] = util.Iif(pull != nil && pull.IsChecking(), 2000, 0) + data.ReloadingInterval = util.Iif(pull != nil && pull.IsChecking(), 2000, 0) ctx.Data["CanWriteToHeadRepo"] = canWriteToHeadRepo ctx.Data["ShowMergeInstructions"] = canWriteToHeadRepo ctx.Data["AllowMerge"] = allowMerge @@ -949,28 +956,36 @@ func (prInfo *pullRequestViewInfo) prepareMergeBox(ctx *context.Context, issue * ctx.Data["DefaultSquashMergeMessage"] = defaultSquashMergeMessage ctx.Data["DefaultSquashMergeBody"] = defaultSquashMergeBody - pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.BaseRepoID, pull.BaseBranch) - if err != nil { - ctx.ServerError("LoadProtectedBranch", err) - return - } - + pb := prInfo.ProtectedBranchRule if pb != nil { pb.Repo = pull.BaseRepo ctx.Data["ProtectedBranch"] = pb - ctx.Data["IsBlockedByApprovals"] = !issues_model.HasEnoughApprovals(ctx, pb, pull) - ctx.Data["IsBlockedByRejection"] = issues_model.MergeBlockedByRejectedReview(ctx, pb, pull) - ctx.Data["IsBlockedByOfficialReviewRequests"] = issues_model.MergeBlockedByOfficialReviewRequests(ctx, pb, pull) - ctx.Data["IsBlockedByOutdatedBranch"] = issues_model.MergeBlockedByOutdatedBranch(pb, pull) + + data.isBlockedByApprovals = !issues_model.HasEnoughApprovals(ctx, pb, pull) + ctx.Data["IsBlockedByApprovals"] = data.isBlockedByApprovals + + data.isBlockedByRejection = issues_model.MergeBlockedByRejectedReview(ctx, pb, pull) + ctx.Data["IsBlockedByRejection"] = data.isBlockedByRejection + + data.isBlockedByOfficialReviewRequests = issues_model.MergeBlockedByOfficialReviewRequests(ctx, pb, pull) + ctx.Data["IsBlockedByOfficialReviewRequests"] = data.isBlockedByOfficialReviewRequests + + data.isBlockedByOutdatedBranch = issues_model.MergeBlockedByOutdatedBranch(pb, pull) + ctx.Data["IsBlockedByOutdatedBranch"] = data.isBlockedByOutdatedBranch + + data.isBlockedByChangedProtectedFiles = len(pull.ChangedProtectedFiles) != 0 + ctx.Data["IsBlockedByChangedProtectedFiles"] = data.isBlockedByChangedProtectedFiles + + data.requireSigned = pb.RequireSignedCommits + ctx.Data["RequireSigned"] = data.requireSigned + ctx.Data["GrantedApprovals"] = issues_model.GetGrantedApprovalsCount(ctx, pb, pull) - ctx.Data["RequireSigned"] = pb.RequireSignedCommits ctx.Data["ChangedProtectedFiles"] = pull.ChangedProtectedFiles - ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0 ctx.Data["ChangedProtectedFilesNum"] = len(pull.ChangedProtectedFiles) ctx.Data["RequireApprovalsWhitelist"] = pb.EnableApprovalsWhitelist } - preparePullViewSigning(ctx, issue) + prInfo.prepareMergeBoxRequireSigning(ctx) if ctx.Written() { return } @@ -984,14 +999,10 @@ func (prInfo *pullRequestViewInfo) prepareMergeBox(ctx *context.Context, issue * if pull.HasMerged || issue.IsClosed || !ctx.IsSigned { return false } - if pull.CanAutoMerge() || pull.IsWorkInProgress(ctx) || pull.IsChecking() { + if pull.IsStatusMergeable() || pull.IsWorkInProgress(ctx) || pull.IsChecking() { return false } - if allowMerge && prConfig.AllowManualMerge { - return true - } - - return false + return allowMerge && prConfig.AllowManualMerge } ctx.Data["StillCanManualMerge"] = stillCanManualMerge() @@ -1002,6 +1013,36 @@ func (prInfo *pullRequestViewInfo) prepareMergeBox(ctx *context.Context, issue * ctx.ServerError("GetScheduledMergeByPullID", err) return } + + enableStatusCheck := pb != nil && pb.EnableStatusCheck + ctx.Data["EnableStatusCheck"] = enableStatusCheck + + // Only show the merge box if the PR is not merged, or the branch is deletable. + // Otherwise, there is nothing to do, because the PR view page already contains enough information. + data.ShowMergeBox = !pull.HasMerged || data.isPullBranchDeletable + + isRepoAdmin := ctx.IsSigned && (ctx.Repo.IsAdmin() || ctx.Doer.IsAdmin) + + // admin can merge without checks, writer can merge when checks succeed + // admin and writer both can make an auto merge schedule (not affected by overridable blockers) + data.hasStatusCheckBlocker = enableStatusCheck && !statusCheckData.RequiredChecksState.IsSuccess() + + // this logic is from: + // {{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOfficialReviewRequests .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not $requiredStatusCheckState.IsSuccess))}} + // HINT: if a PR's status is not mergeable, then it is a non-overridable blocker, such logic is handled separately (see IsStatusMergeable) + data.HasOverridableBlockers = data.isBlockedByApprovals || data.isBlockedByRejection || + data.isBlockedByOfficialReviewRequests || data.isBlockedByOutdatedBranch || data.isBlockedByChangedProtectedFiles || + data.hasStatusCheckBlocker + + // this logic is from: + // {{$canMergeNow := and (or (and (not $.ProtectedBranch.BlockAdminMergeOverride) $.IsRepoAdmin) (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not .RequireSigned) .WillSign)}} + // HINT: legacy "(not .AllowMerge)" is not right (always false, does nothing), fixed here + // CanMergeNow means: if the doer has write permission, whether the PR can be merged now + adminCanOverrideBlockers := (pb == nil || !pb.BlockAdminMergeOverride) && isRepoAdmin + data.CanMergeNow = (!data.HasOverridableBlockers || adminCanOverrideBlockers) && // status checks are satisfied + (!data.requireSigned || data.willSign) // signing requirement is satisfied + + ctx.Data["PullMergeBoxData"] = prInfo.MergeBoxData } func prepareIssueViewContent(ctx *context.Context, issue *issues_model.Issue) { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index d09e12f4779..4ab497a1126 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -261,6 +261,25 @@ func GetMergedBaseCommitID(ctx *context.Context, issue *issues_model.Issue) stri return baseCommit } +type pullMergeBoxData struct { + ShowMergeBox bool + ReloadingInterval int + + HasOverridableBlockers bool + CanMergeNow bool + + // don't expose unneeded fields to templates, need more refactoring changes + hasStatusCheckBlocker bool + isPullBranchDeletable bool + + isBlockedByApprovals bool + isBlockedByRejection bool + isBlockedByOfficialReviewRequests bool + isBlockedByOutdatedBranch bool + isBlockedByChangedProtectedFiles bool + requireSigned, willSign bool +} + // pullRequestViewInfo is a structured type for viewing pull request // Refactoring plan: // * move dynamic template-data-based variable into this struct @@ -271,13 +290,11 @@ type pullRequestViewInfo struct { IsPullRequestBroken bool HeadBranchCommitID string - CompareInfo git_service.CompareInfo - MergeBoxInfo struct { - // TODO: move "merge box" related template variables here in the future - } - - StatusCheckData pullCommitStatusCheckData - CommitStatuses []*git_model.CommitStatus + CompareInfo git_service.CompareInfo + ProtectedBranchRule *git_model.ProtectedBranch + StatusCheckData *pullCommitStatusCheckData + CommitStatuses []*git_model.CommitStatus + MergeBoxData *pullMergeBoxData } func newPullRequestViewInfo() *pullRequestViewInfo { @@ -358,7 +375,8 @@ func (prInfo *pullRequestViewInfo) prepareViewFillCommitStatusInfo(ctx *context. } repo := ctx.Repo.Repository - statusCheckData := &prInfo.StatusCheckData + statusCheckData := &pullCommitStatusCheckData{} + prInfo.StatusCheckData = statusCheckData commitStatuses, err := git_model.GetLatestCommitStatus(ctx, ctx.Repo.Repository.ID, prInfo.CompareInfo.HeadCommitID, db.ListOptionsAll) if err != nil { @@ -374,7 +392,13 @@ func (prInfo *pullRequestViewInfo) prepareViewFillCommitStatusInfo(ctx *context. statusCheckData.LatestCommitStatus = git_model.CalcCommitStatus(commitStatuses) ctx.Data["LatestCommitStatuses"] = commitStatuses ctx.Data["LatestCommitStatus"] = statusCheckData.LatestCommitStatus - ctx.Data["StatusCheckData"] = &prInfo.StatusCheckData + ctx.Data["StatusCheckData"] = prInfo.StatusCheckData + + prInfo.ProtectedBranchRule, err = git_model.GetFirstMatchProtectedBranchRule(ctx, ctx.Repo.Repository.ID, prInfo.issue.PullRequest.BaseBranch) + if err != nil { + ctx.ServerError("GetFirstMatchProtectedBranchRule", err) + return + } if !prInfo.issue.IsClosed { prInfo.prepareViewFillCommitStatusInfoForOpen(ctx) @@ -382,8 +406,7 @@ func (prInfo *pullRequestViewInfo) prepareViewFillCommitStatusInfo(ctx *context. } func (prInfo *pullRequestViewInfo) prepareViewFillCommitStatusInfoForOpen(ctx *context.Context) { - issue := prInfo.issue - statusCheckData := &prInfo.StatusCheckData + statusCheckData := prInfo.StatusCheckData commitStatuses := prInfo.CommitStatuses runs, err := actions_service.GetRunsFromCommitStatuses(ctx, commitStatuses) if err != nil { @@ -399,16 +422,12 @@ func (prInfo *pullRequestViewInfo) prepareViewFillCommitStatusInfoForOpen(ctx *c statusCheckData.CanApprove = ctx.Repo.CanWrite(unit.TypeActions) } - pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, ctx.Repo.Repository.ID, issue.PullRequest.BaseBranch) - if err != nil { - ctx.ServerError("LoadProtectedBranch", err) - return - } + pb := prInfo.ProtectedBranchRule enableStatusCheck := pb != nil && pb.EnableStatusCheck - ctx.Data["EnableStatusCheck"] = enableStatusCheck if !enableStatusCheck { return } + var missingRequiredChecks []string for _, requiredContext := range pb.StatusCheckContexts { contextFound := false @@ -768,13 +787,8 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { return } - pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.BaseRepoID, pull.BaseBranch) - if err != nil { - ctx.ServerError("LoadProtectedBranch", err) - return - } - - if pb != nil { + pb := prViewInfo.ProtectedBranchRule + if prViewInfo.ProtectedBranchRule != nil { protectedFilePatterns := pb.GetProtectedFilePatterns() if len(protectedFilePatterns) != 0 { for _, file := range diff.Files { diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index b06fc723be1..1629b2b95e5 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -90,7 +90,7 @@ func RemoveScheduledAutoMerge(ctx context.Context, doer *user_model.User, pull * // StartPRCheckAndAutoMergeBySHA start an automerge check and auto merge task for all pull requests of repository and SHA func StartPRCheckAndAutoMergeBySHA(ctx context.Context, sha string, repo *repo_model.Repository) error { pulls, err := getPullRequestsByHeadSHA(ctx, sha, repo, func(pr *issues_model.PullRequest) bool { - return !pr.HasMerged && pr.CanAutoMerge() + return !pr.HasMerged && pr.IsStatusMergeable() }) if err != nil { return err diff --git a/services/automergequeue/automergequeue.go b/services/automergequeue/automergequeue.go index e8cc4512a7e..8cfdf3a5398 100644 --- a/services/automergequeue/automergequeue.go +++ b/services/automergequeue/automergequeue.go @@ -25,7 +25,7 @@ var AddToQueue = func(pr *issues_model.PullRequest, sha string) { // StartPRCheckAndAutoMerge start an automerge check and auto merge task for a pull request func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullRequest) { - if pull == nil || pull.HasMerged || !pull.CanAutoMerge() { + if pull == nil || pull.HasMerged || !pull.IsStatusMergeable() { return } diff --git a/services/pull/check.go b/services/pull/check.go index 996994cea95..bc706844b25 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -168,7 +168,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc return ErrIsWorkInProgress } - if !pr.CanAutoMerge() && !pr.IsEmpty() { + if !pr.IsStatusMergeable() && !pr.IsEmpty() { return ErrNotMergeableState } diff --git a/templates/repo/issue/view_content/pull_merge_box.tmpl b/templates/repo/issue/view_content/pull_merge_box.tmpl index 814e8c6c83b..84d703debc5 100644 --- a/templates/repo/issue/view_content/pull_merge_box.tmpl +++ b/templates/repo/issue/view_content/pull_merge_box.tmpl @@ -1,11 +1,10 @@ -{{if and .Issue.PullRequest.HasMerged (not .IsPullBranchDeletable)}} -{{/* Then the merge box will not be displayed because this page already contains enough information */}} -{{else}} +{{$data := $.PullMergeBoxData}} +{{if $data.ShowMergeBox}}
{{$statusCheckData := .StatusCheckData}} @@ -25,7 +24,7 @@ {{- else if and .AllowMerge .RequireSigned (not .WillSign)}}tw-text-red {{- else if .Issue.PullRequest.IsChecking}}tw-text-yellow {{- else if .Issue.PullRequest.IsEmpty}}tw-text-text-light - {{- else if .Issue.PullRequest.CanAutoMerge}}tw-text-green + {{- else if .Issue.PullRequest.IsStatusMergeable}}tw-text-green {{- else}}tw-text-red{{end}}">{{svg "octicon-git-merge" 40}}
{{if .LatestCommitStatus}} @@ -114,7 +113,7 @@ {{svg "octicon-alert"}} {{ctx.Locale.Tr "repo.pulls.is_ancestor"}}
- {{else if or .Issue.PullRequest.CanAutoMerge .Issue.PullRequest.IsEmpty}} + {{else if or .Issue.PullRequest.IsStatusMergeable .Issue.PullRequest.IsEmpty}} {{if .IsBlockedByApprovals}}
{{svg "octicon-x"}} @@ -170,11 +169,8 @@
{{end}} - {{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOfficialReviewRequests .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not $requiredStatusCheckState.IsSuccess))}} - - {{/* admin can merge without checks, writer can merge when checks succeed */}} - {{$canMergeNow := and (or (and (not $.ProtectedBranch.BlockAdminMergeOverride) $.IsRepoAdmin) (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not .RequireSigned) .WillSign)}} - {{/* admin and writer both can make an auto merge schedule */}} + {{$notAllOverridableChecksOk := $data.HasOverridableBlockers}} + {{$canMergeNow := $data.CanMergeNow}} {{if $canMergeNow}} {{if $notAllOverridableChecksOk}}