Refactor pull request view (3) (#37439)

Move some complex logic to backend

Rename `pr.CanAutoMerge` to `pr.IsStatusMergeable`

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.
+// IsStatusMergeable returns true if this pull request is mergeable to its base
-func (pr *PullRequest) CanAutoMerge() bool {
+func (pr *PullRequest) IsStatusMergeable() bool {
 	return pr.Status == PullRequestStatusMergeable
 }
 

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 {
+	if !setting.IsProd && issue.PullRequest != nil && !issue.PullRequest.IsChecking() && prViewInfo.MergeBoxData != nil {
-		ctx.Data["PullMergeBoxReloadingInterval"] = 1 // in dev env, force using the reloading logic to make sure it won't break
+		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) {
+func (prInfo *pullRequestViewInfo) prepareMergeBoxRequireSigning(ctx *context.Context) {
-	if !issue.IsPull {
+	pull := prInfo.issue.PullRequest
-		return
+	willSign := false
-	}
-	pull := issue.PullRequest
-	ctx.Data["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)
+	pb := prInfo.ProtectedBranchRule
-	if err != nil {
-		ctx.ServerError("LoadProtectedBranch", err)
-		return
-	}
-
 	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)
+		data.isBlockedByApprovals = !issues_model.HasEnoughApprovals(ctx, pb, pull)
-		ctx.Data["IsBlockedByOfficialReviewRequests"] = issues_model.MergeBlockedByOfficialReviewRequests(ctx, pb, pull)
+		ctx.Data["IsBlockedByApprovals"] = data.isBlockedByApprovals
-		ctx.Data["IsBlockedByOutdatedBranch"] = issues_model.MergeBlockedByOutdatedBranch(pb, pull)
+
+		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 allowMerge && prConfig.AllowManualMerge
-			return true
-		}
-
-		return false
 	}
 
 	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) {

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
+	CompareInfo         git_service.CompareInfo
-	MergeBoxInfo struct {
+	ProtectedBranchRule *git_model.ProtectedBranch
-		// TODO: move "merge box" related template variables here in the future
+	StatusCheckData     *pullCommitStatusCheckData
-	}
+	CommitStatuses      []*git_model.CommitStatus
-
+	MergeBoxData        *pullMergeBoxData
-	StatusCheckData pullCommitStatusCheckData
-	CommitStatuses  []*git_model.CommitStatus
 }
 
 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)
+	pb := prInfo.ProtectedBranchRule
-	if err != nil {
-		ctx.ServerError("LoadProtectedBranch", err)
-		return
-	}
 	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)
+	pb := prViewInfo.ProtectedBranchRule
-	if err != nil {
+	if prViewInfo.ProtectedBranchRule != nil {
-		ctx.ServerError("LoadProtectedBranch", err)
-		return
-	}
-
-	if pb != nil {
 		protectedFilePatterns := pb.GetProtectedFilePatterns()
 		if len(protectedFilePatterns) != 0 {
 			for _, file := range diff.Files {

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

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
 	}
 

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
 		}
 

templates/repo/issue/view_content/pull_merge_box.tmpl

@@ -1,11 +1,10 @@
-{{if and .Issue.PullRequest.HasMerged (not .IsPullBranchDeletable)}}
+{{$data := $.PullMergeBoxData}}
-{{/* Then the merge box will not be displayed because this page already contains enough information */}}
+{{if $data.ShowMergeBox}}
-{{else}}
 <div class="timeline-item comment pull-merge-box"
 	data-global-init="initRepoPullMergeBox"
-	{{if .PullMergeBoxReloadingInterval}}
+	{{if $data.ReloadingInterval}}
-		data-pull-merge-box-reloading-interval="{{.PullMergeBoxReloadingInterval}}"
+		data-pull-merge-box-reloading-interval="{{$data.ReloadingInterval}}"
-		data-pull-link="{{.Issue.Link}}"
+		data-pull-link="{{$.Issue.Link}}"
 	{{end}}
 >
 	{{$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}}</div>
 	<div class="content">
 		{{if .LatestCommitStatus}}
@@ -114,7 +113,7 @@
 					{{svg "octicon-alert"}}
 					{{ctx.Locale.Tr "repo.pulls.is_ancestor"}}
 				</div>
-			{{else if or .Issue.PullRequest.CanAutoMerge .Issue.PullRequest.IsEmpty}}
+			{{else if or .Issue.PullRequest.IsStatusMergeable .Issue.PullRequest.IsEmpty}}
 				{{if .IsBlockedByApprovals}}
 					<div class="item">
 						{{svg "octicon-x"}}
@@ -170,11 +169,8 @@
 					</div>
 				{{end}}
 
-				{{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOfficialReviewRequests .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not $requiredStatusCheckState.IsSuccess))}}
+				{{$notAllOverridableChecksOk := $data.HasOverridableBlockers}}
-
+				{{$canMergeNow := $data.CanMergeNow}}
-				{{/* 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 */}}
 
 				{{if $canMergeNow}}
 					{{if $notAllOverridableChecksOk}}