From 245c96744bacc731c57ad0fef0aba5b53ca94e6b Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 26 Apr 2026 15:26:47 +0800 Subject: [PATCH] fix --- models/issues/review.go | 4 +- routers/web/repo/issue_view.go | 59 ++++++++-------- routers/web/repo/pull.go | 118 +++++++++++++------------------ templates/repo/commits_list.tmpl | 2 +- 4 files changed, 84 insertions(+), 99 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index 9f6fed779062e..694e384ded469 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -908,8 +908,8 @@ func MarkConversation(ctx context.Context, comment *Comment, doer *user_model.Us // CanMarkConversation Add or remove Conversation mark for a code comment permission check // the PR writer , official reviewer and poster can do it func CanMarkConversation(ctx context.Context, issue *Issue, doer *user_model.User) (permResult bool, err error) { - if doer == nil || issue == nil { - return false, errors.New("issue or doer is nil") + if doer == nil { + return false, nil } if err = issue.LoadRepo(ctx); err != nil { diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index 778720ebdaaf7..583fc610cba2a 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -117,19 +117,6 @@ func roleDescriptor(ctx *context.Context, repo *repo_model.Repository, poster *u return roleDesc, nil } -func getBranchData(ctx *context.Context, issue *issues_model.Issue) { - ctx.Data["BaseBranch"] = nil - ctx.Data["HeadBranch"] = nil - ctx.Data["HeadUserName"] = nil - ctx.Data["BaseName"] = ctx.Repo.Repository.OwnerName - if issue.IsPull { - pull := issue.PullRequest - ctx.Data["BaseBranch"] = pull.BaseBranch - ctx.Data["HeadBranch"] = pull.HeadBranch - ctx.Data["HeadUserName"] = pull.MustHeadUserName(ctx) - } -} - // checkBlockedByIssues return canRead and notPermitted func checkBlockedByIssues(ctx *context.Context, blockers []*issues_model.DependencyInfo) (canRead, notPermitted []*issues_model.DependencyInfo) { repoPerms := make(map[int64]access_model.Permission) @@ -379,6 +366,7 @@ func ViewIssue(ctx *context.Context) { } pageMetaData.LabelsData.SetSelectedLabels(issue.Labels) + prViewInfo := newPullRequestViewInfo() prepareFuncs := []func(*context.Context, *issues_model.Issue){ prepareIssueViewContent, prepareIssueViewCommentsAndSidebarParticipants, @@ -389,8 +377,8 @@ func ViewIssue(ctx *context.Context) { } if issue.IsPull { prepareFuncs = append(prepareFuncs, - func(ctx *context.Context, issue *issues_model.Issue) { preparePullViewPullInfo(ctx, issue) }, - preparePullViewReviewAndMerge, + prViewInfo.prepareViewInfo, + prViewInfo.prepareMergeBox, ) } for _, prepareFunc := range prepareFuncs { @@ -405,7 +393,7 @@ func ViewIssue(ctx *context.Context) { if issue.PullRequest.HasMerged { ctx.Data["DisableStatusChange"] = issue.PullRequest.HasMerged } else { - ctx.Data["DisableStatusChange"] = ctx.Data["IsPullRequestBroken"] == true && issue.IsClosed + ctx.Data["DisableStatusChange"] = prViewInfo.IsPullRequestBroken && issue.IsClosed } } @@ -445,11 +433,12 @@ func ViewPullMergeBox(ctx *context.Context) { ctx.NotFound(nil) return } - preparePullViewPullInfo(ctx, issue) + prViewInfo := newPullRequestViewInfo() + prViewInfo.prepareViewInfo(ctx, issue) if ctx.Written() { return } - preparePullViewReviewAndMerge(ctx, issue) + prViewInfo.prepareMergeBox(ctx, issue) if ctx.Written() { return } @@ -566,14 +555,11 @@ func prepareIssueViewSidebarTimeTracker(ctx *context.Context, issue *issues_mode } } -func preparePullViewDeleteBranch(ctx *context.Context, issue *issues_model.Issue, canDelete bool) { - if !issue.IsPull { - return - } - pull := issue.PullRequest +func (prInfo *pullRequestViewInfo) prepareMergeBoxDeleteBranch(ctx *context.Context, canDelete bool) { + pull := prInfo.issue.PullRequest isPullBranchDeletable := canDelete && pull.HeadRepo != nil && - (!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"]) + (!pull.HasMerged || prInfo.HeadBranchCommitID == prInfo.CompareInfo.HeadCommitID) if isPullBranchDeletable { isPullBranchDeletable, _ = git_model.IsBranchExist(ctx, pull.HeadRepo.ID, pull.HeadBranch) } @@ -835,10 +821,9 @@ func prepareIssueViewCommentsAndSidebarParticipants(ctx *context.Context, issue ctx.Data["NumParticipants"] = len(participants) } -func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Issue) { - getBranchData(ctx, issue) - if !issue.IsPull { - return +func (prInfo *pullRequestViewInfo) prepareMergeBox(ctx *context.Context, issue *issues_model.Issue) { + if prInfo.issue != issue { + panic("impossible, issue must be the same") } pull := issue.PullRequest @@ -849,6 +834,22 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss pull_service.StartPullRequestCheckOnView(ctx, pull) + ctx.Data["GetCommitMessages"] = "" + if !prInfo.IsPullRequestBroken { + var err error + ctx.Data["UpdateAllowed"], ctx.Data["UpdateByRebaseAllowed"], err = pull_service.IsUserAllowedToUpdate(ctx, pull, ctx.Doer) + if err != nil { + ctx.ServerError("IsUserAllowedToUpdate", err) + return + } + ctx.Data["GetCommitMessages"] = pull_service.GetSquashMergeCommitMessages(ctx, pull) + } + + if pull.IsFilesConflicted() { + ctx.Data["IsPullFilesConflicted"] = true + ctx.Data["ConflictedFiles"] = pull.ConflictedFiles + } + if ctx.IsSigned { if err := pull.LoadHeadRepo(ctx); err != nil { log.Error("LoadHeadRepo: %v", err) @@ -974,7 +975,7 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss return } - preparePullViewDeleteBranch(ctx, issue, canDelete) + prInfo.prepareMergeBoxDeleteBranch(ctx, canDelete) if ctx.Written() { return } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index c208bc1907a98..d09e12f4779f2 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -161,7 +161,8 @@ func getPullInfo(ctx *context.Context) (issue *issues_model.Issue, ok bool) { return issue, true } -func setMergeTarget(ctx *context.Context, pull *issues_model.PullRequest) { +func (prInfo *pullRequestViewInfo) setTemplateDataMergeTarget(ctx *context.Context) { + pull := prInfo.issue.PullRequest if ctx.Repo.Owner.Name == pull.MustHeadUserName(ctx) { ctx.Data["HeadTarget"] = pull.HeadBranch } else if pull.HeadRepo == nil { @@ -260,11 +261,13 @@ func GetMergedBaseCommitID(ctx *context.Context, issue *issues_model.Issue) stri return baseCommit } -// PullRequestViewInfo is a structured type for viewing pull request +// pullRequestViewInfo is a structured type for viewing pull request // Refactoring plan: // * move dynamic template-data-based variable into this struct // * let backend handle complex logic, prepare everything, avoid plenty of "if" blocks in tmpl -type PullRequestViewInfo struct { +type pullRequestViewInfo struct { + issue *issues_model.Issue + IsPullRequestBroken bool HeadBranchCommitID string @@ -277,41 +280,47 @@ type PullRequestViewInfo struct { CommitStatuses []*git_model.CommitStatus } -func preparePullViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *PullRequestViewInfo { - prInfo := &PullRequestViewInfo{} - ctx.Data["PullRequestViewInfo"] = prInfo // TODO: after the complete refactoring, decouple the variable from template data +func newPullRequestViewInfo() *pullRequestViewInfo { + return &pullRequestViewInfo{} +} + +func (prInfo *pullRequestViewInfo) prepareViewInfo(ctx *context.Context, issue *issues_model.Issue) { + prInfo.issue = issue ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes if err := issue.PullRequest.LoadHeadRepo(ctx); err != nil { ctx.ServerError("LoadHeadRepo", err) - return nil + return } if err := issue.PullRequest.LoadBaseRepo(ctx); err != nil { ctx.ServerError("LoadBaseRepo", err) - return nil + return } + // for the PR target branch selector + ctx.Data["BaseBranch"] = issue.PullRequest.BaseBranch + ctx.Data["HeadBranch"] = issue.PullRequest.HeadBranch + ctx.Data["HeadUserName"] = issue.PullRequest.MustHeadUserName(ctx) + if issue.PullRequest.HasMerged { - prepareViewMergedPullInfo(ctx, issue) + prInfo.prepareViewMergedPullInfo(ctx) } else { - prepareViewOpenPullInfo(ctx, issue) + prInfo.prepareViewOpenPullInfo(ctx) } - return prInfo } -func preparePullViewFillInfo(ctx *context.Context, issue *issues_model.Issue, baseRef git.RefName) { - preparePullViewFillCompareInfo(ctx, issue, baseRef) +func (prInfo *pullRequestViewInfo) prepareViewFillInfo(ctx *context.Context, baseRef git.RefName) { + prInfo.prepareViewFillCompareInfo(ctx, baseRef) if ctx.Written() { return } - preparePullViewFillCommitStatusInfo(ctx, issue) + prInfo.prepareViewFillCommitStatusInfo(ctx) } -func preparePullViewFillCompareInfo(ctx *context.Context, issue *issues_model.Issue, baseRef git.RefName) { +func (prInfo *pullRequestViewInfo) prepareViewFillCompareInfo(ctx *context.Context, baseRef git.RefName) { var err error - prInfo := ctx.Data["PullRequestViewInfo"].(*PullRequestViewInfo) - pull := issue.PullRequest + pull := prInfo.issue.PullRequest prInfo.CompareInfo, err = git_service.GetCompareInfo(ctx, ctx.Repo.Repository, ctx.Repo.Repository, ctx.Repo.GitRepo, baseRef, git.RefName(pull.GetGitHeadRefName()), false, false) if err != nil { isKnownErrorForBroken := gitcmd.IsStdErrorNotValidObjectName(err) || @@ -339,12 +348,10 @@ func preparePullViewFillCompareInfo(ctx *context.Context, issue *issues_model.Is ctx.Data["IsPullRequestBroken"] = prInfo.IsPullRequestBroken ctx.Data["NumCommits"] = len(prInfo.CompareInfo.Commits) ctx.Data["NumFiles"] = prInfo.CompareInfo.NumFiles - setMergeTarget(ctx, issue.PullRequest) + prInfo.setTemplateDataMergeTarget(ctx) } -func preparePullViewFillCommitStatusInfo(ctx *context.Context, issue *issues_model.Issue) { - prInfo := ctx.Data["PullRequestViewInfo"].(*PullRequestViewInfo) - +func (prInfo *pullRequestViewInfo) prepareViewFillCommitStatusInfo(ctx *context.Context) { headCommitID := prInfo.CompareInfo.HeadCommitID if headCommitID == "" { return @@ -369,13 +376,13 @@ func preparePullViewFillCommitStatusInfo(ctx *context.Context, issue *issues_mod ctx.Data["LatestCommitStatus"] = statusCheckData.LatestCommitStatus ctx.Data["StatusCheckData"] = &prInfo.StatusCheckData - if !issue.IsClosed { - preparePullViewFillCommitStatusInfoForOpen(ctx, issue) + if !prInfo.issue.IsClosed { + prInfo.prepareViewFillCommitStatusInfoForOpen(ctx) } } -func preparePullViewFillCommitStatusInfoForOpen(ctx *context.Context, issue *issues_model.Issue) { - prInfo := ctx.Data["PullRequestViewInfo"].(*PullRequestViewInfo) +func (prInfo *pullRequestViewInfo) prepareViewFillCommitStatusInfoForOpen(ctx *context.Context) { + issue := prInfo.issue statusCheckData := &prInfo.StatusCheckData commitStatuses := prInfo.CommitStatuses runs, err := actions_service.GetRunsFromCommitStatuses(ctx, commitStatuses) @@ -439,10 +446,10 @@ func preparePullViewFillCommitStatusInfoForOpen(ctx *context.Context, issue *iss } // prepareViewMergedPullInfo show meta information for a merged pull request view page -func prepareViewMergedPullInfo(ctx *context.Context, issue *issues_model.Issue) { +func (prInfo *pullRequestViewInfo) prepareViewMergedPullInfo(ctx *context.Context) { ctx.Data["HasMerged"] = true - baseCommit := GetMergedBaseCommitID(ctx, issue) - preparePullViewFillInfo(ctx, issue, git.RefName(baseCommit)) + baseCommit := GetMergedBaseCommitID(ctx, prInfo.issue) + prInfo.prepareViewFillInfo(ctx, git.RefName(baseCommit)) } type pullCommitStatusCheckData struct { @@ -495,49 +502,31 @@ func getViewPullHeadBranchCommitID(ctx *context.Context, pull *issues_model.Pull return "", util.ErrNotExist } -func prepareViewOpenPullInfo(ctx *context.Context, issue *issues_model.Issue) { - prInfo := ctx.Data["PullRequestViewInfo"].(*PullRequestViewInfo) - pull := issue.PullRequest +func (prInfo *pullRequestViewInfo) prepareViewOpenPullInfo(ctx *context.Context) { + pull := prInfo.issue.PullRequest if exist, _ := git_model.IsBranchExist(ctx, pull.BaseRepo.ID, pull.BaseBranch); !exist { // if base branch doesn't exist, prepare from the merge base ctx.Data["BaseBranchNotExist"] = true - preparePullViewFillInfo(ctx, issue, git.RefName(pull.MergeBase)) + prInfo.prepareViewFillInfo(ctx, git.RefName(pull.MergeBase)) return } - preparePullViewFillInfo(ctx, issue, git.RefNameFromBranch(pull.BaseBranch)) + prInfo.prepareViewFillInfo(ctx, git.RefNameFromBranch(pull.BaseBranch)) if ctx.Written() { return } - if !prInfo.IsPullRequestBroken { - var err error - ctx.Data["UpdateAllowed"], ctx.Data["UpdateByRebaseAllowed"], err = pull_service.IsUserAllowedToUpdate(ctx, pull, ctx.Doer) - if err != nil { - ctx.ServerError("IsUserAllowedToUpdate", err) - return - } - ctx.Data["GetCommitMessages"] = pull_service.GetSquashMergeCommitMessages(ctx, pull) - } else { - ctx.Data["GetCommitMessages"] = "" - } - - ctx.Data["HeadBranchCommitID"] = prInfo.HeadBranchCommitID ctx.Data["PullHeadCommitID"] = prInfo.CompareInfo.HeadCommitID if prInfo.CompareInfo.HeadCommitID == prInfo.CompareInfo.MergeBase { ctx.Data["IsNothingToCompare"] = true } + // this one is used by both sidebar and merge-box if pull.IsWorkInProgress(ctx) { ctx.Data["IsPullWorkInProgress"] = true ctx.Data["WorkInProgressPrefix"] = pull.GetWorkInProgressPrefix(ctx) } - - if pull.IsFilesConflicted() { - ctx.Data["IsPullFilesConflicted"] = true - ctx.Data["ConflictedFiles"] = pull.ConflictedFiles - } } func createRequiredContextMatcher(requiredContext string) func(string) bool { @@ -596,8 +585,8 @@ func ViewPullCommits(ctx *context.Context) { if !ok { return } - - prViewInfo := preparePullViewPullInfo(ctx, issue) + prViewInfo := newPullRequestViewInfo() + prViewInfo.prepareViewInfo(ctx, issue) if ctx.Written() { return } @@ -626,7 +615,6 @@ func ViewPullCommits(ctx *context.Context) { if ctx.Written() { return } - getBranchData(ctx, issue) ctx.HTML(http.StatusOK, tplPullCommits) } @@ -652,7 +640,8 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { gitRepo := ctx.Repo.GitRepo - prViewInfo := preparePullViewPullInfo(ctx, issue) + prViewInfo := newPullRequestViewInfo() + prViewInfo.prepareViewInfo(ctx, issue) if ctx.Written() { return } @@ -704,8 +693,6 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { beforeCommitID = beforeCommit.ID.String() } - ctx.Data["Username"] = ctx.Repo.Owner.Name - ctx.Data["Reponame"] = ctx.Repo.Repository.Name ctx.Data["MergeBase"] = prCompareInfo.MergeBase ctx.Data["AfterCommitID"] = afterCommitID ctx.Data["BeforeCommitID"] = beforeCommitID @@ -788,10 +775,10 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { } if pb != nil { - glob := pb.GetProtectedFilePatterns() - if len(glob) != 0 { + protectedFilePatterns := pb.GetProtectedFilePatterns() + if len(protectedFilePatterns) != 0 { for _, file := range diff.Files { - file.IsProtected = pb.IsProtectedFile(glob, file.Name) + file.IsProtected = pb.IsProtectedFile(protectedFilePatterns, file.Name) } } } @@ -824,11 +811,9 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { } ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0 - if ctx.IsSigned && ctx.Doer != nil { - if ctx.Data["CanMarkConversation"], err = issues_model.CanMarkConversation(ctx, issue, ctx.Doer); err != nil { - ctx.ServerError("CanMarkConversation", err) - return - } + if ctx.Data["CanMarkConversation"], err = issues_model.CanMarkConversation(ctx, issue, ctx.Doer); err != nil { + ctx.ServerError("CanMarkConversation", err) + return } setCompareContext(ctx, beforeCommit, afterCommit, ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) @@ -860,8 +845,7 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { ctx.Data["CurrentReview"] = currentReview ctx.Data["PendingCodeCommentNumber"] = numPendingCodeComments - getBranchData(ctx, issue) - ctx.Data["IsIssuePoster"] = ctx.IsSigned && issue.IsPoster(ctx.Doer.ID) + ctx.Data["IsIssuePoster"] = ctx.Doer != nil && issue.IsPoster(ctx.Doer.ID) ctx.Data["HasIssuesOrPullsWritePermission"] = ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled diff --git a/templates/repo/commits_list.tmpl b/templates/repo/commits_list.tmpl index a0722307a7011..d520c0fbcf7fe 100644 --- a/templates/repo/commits_list.tmpl +++ b/templates/repo/commits_list.tmpl @@ -30,7 +30,7 @@ {{$commitBaseLink = printf "%s/wiki/commit" $commitRepoLink}} {{else if $.PageIsPullCommits}} {{$commitBaseLink = printf "%s/pulls/%d/commits" $commitRepoLink $.Issue.Index}} - {{else if $.Reponame}} + {{else}} {{$commitBaseLink = printf "%s/commit" $commitRepoLink}} {{end}} {{template "repo/commit_sign_badge" dict "Commit" . "CommitBaseLink" $commitBaseLink "CommitSignVerification" .Verification}}