From 651dbdb8bf594145eb051b19458ce4a489e5dbaa Mon Sep 17 00:00:00 2001 From: Parnic Date: Mon, 11 Apr 2022 09:49:33 -0500 Subject: [PATCH 1/6] Add status popup to issue list This gets the necessary data to the issuelist for it to support a clickable commit status icon which pops up the full list of commit statuses related to the commit. It accomplishes this without any additional queries or fetching as the existing codepath was already doing the necessary work but only returning the "last" status. All methods were wrapped to call the least-filtered version of each function in order to maximize code reuse. --- routers/web/repo/issue.go | 9 ++++--- services/pull/pull.go | 42 +++++++++++++++++++++------------ templates/shared/issuelist.tmpl | 2 +- 3 files changed, 34 insertions(+), 19 deletions(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 3ca193a15e73..a09a0094fcb0 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -269,14 +269,17 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption uti } } - commitStatus, err := pull_service.GetIssuesLastCommitStatus(ctx, issues) + commitStatuses, lastStatus, err := pull_service.GetIssuesAllCommitStatus(ctx, issues) if err != nil { - ctx.ServerError("GetIssuesLastCommitStatus", err) + ctx.ServerError("GetIssuesAllCommitStatus", err) return } ctx.Data["Issues"] = issues - ctx.Data["CommitStatus"] = commitStatus + ctx.Data["CommitStatuses"] = map[string]interface{}{ + "Status": lastStatus, + "Statuses": commitStatuses, + } // Get assignees. ctx.Data["Assignees"], err = models.GetRepoAssignees(repo) diff --git a/services/pull/pull.go b/services/pull/pull.go index 0537964b9de7..347620b1d759 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -728,16 +728,23 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *models.PullRequest) s // GetIssuesLastCommitStatus returns a map func GetIssuesLastCommitStatus(ctx context.Context, issues models.IssueList) (map[int64]*models.CommitStatus, error) { + _, lastStatus, err := GetIssuesAllCommitStatus(ctx, issues) + return lastStatus, err +} + +// GetIssuesAllCommitStatus returns a map +func GetIssuesAllCommitStatus(ctx context.Context, issues models.IssueList) (map[int64][]*models.CommitStatus, map[int64]*models.CommitStatus, error) { if err := issues.LoadPullRequests(); err != nil { - return nil, err + return nil, nil, err } if _, err := issues.LoadRepositories(); err != nil { - return nil, err + return nil, nil, err } var ( gitRepos = make(map[int64]*git.Repository) - res = make(map[int64]*models.CommitStatus) + res = make(map[int64][]*models.CommitStatus) + lastRes = make(map[int64]*models.CommitStatus) err error ) defer func() { @@ -760,28 +767,33 @@ func GetIssuesLastCommitStatus(ctx context.Context, issues models.IssueList) (ma gitRepos[issue.RepoID] = gitRepo } - status, err := getLastCommitStatus(gitRepo, issue.PullRequest) + statuses, lastStatus, err := getAllCommitStatus(gitRepo, issue.PullRequest) if err != nil { - log.Error("getLastCommitStatus: cant get last commit of pull [%d]: %v", issue.PullRequest.ID, err) + log.Error("getAllCommitStatus: cant get commit statuses of pull [%d]: %v", issue.PullRequest.ID, err) continue } - res[issue.PullRequest.ID] = status + res[issue.PullRequest.ID] = statuses + lastRes[issue.PullRequest.ID] = lastStatus } - return res, nil + return res, lastRes, nil } // getLastCommitStatus get pr's last commit status. PR's last commit status is the head commit id's last commit status func getLastCommitStatus(gitRepo *git.Repository, pr *models.PullRequest) (status *models.CommitStatus, err error) { - sha, err := gitRepo.GetRefCommitID(pr.GetGitRefName()) - if err != nil { - return nil, err - } + _, lastStatus, err := getAllCommitStatus(gitRepo, pr) + return lastStatus, err +} - statusList, _, err := models.GetLatestCommitStatus(pr.BaseRepo.ID, sha, db.ListOptions{}) - if err != nil { - return nil, err +// getAllCommitStatus get pr's commit statuses. +func getAllCommitStatus(gitRepo *git.Repository, pr *models.PullRequest) (statuses []*models.CommitStatus, lastStatus *models.CommitStatus, err error) { + sha, shaErr := gitRepo.GetRefCommitID(pr.GetGitRefName()) + if shaErr != nil { + return nil, nil, shaErr } - return models.CalcCommitStatus(statusList), nil + + statuses, _, err = models.GetLatestCommitStatus(pr.BaseRepo.ID, sha, db.ListOptions{}) + lastStatus = models.CalcCommitStatus(statuses) + return statuses, lastStatus, err } // IsHeadEqualWithBranch returns if the commits of branchName are available in pull request head diff --git a/templates/shared/issuelist.tmpl b/templates/shared/issuelist.tmpl index 002aa5b5abef..79f04165e8f6 100644 --- a/templates/shared/issuelist.tmpl +++ b/templates/shared/issuelist.tmpl @@ -34,7 +34,7 @@ {{RenderEmoji .Title}} {{if .IsPull}} {{if (index $.CommitStatus .PullRequest.ID)}} - {{template "repo/commit_status" (index $.CommitStatus .PullRequest.ID)}} + {{template "repo/commit_statuses" (index $.CommitStatuses .PullRequest.ID)}} {{end}} {{end}} From cb401b7bcbd02c1180e040c696843c3383735978 Mon Sep 17 00:00:00 2001 From: Parnic Date: Mon, 11 Apr 2022 10:30:39 -0500 Subject: [PATCH 2/6] Fix wrong variable reference --- templates/shared/issuelist.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/shared/issuelist.tmpl b/templates/shared/issuelist.tmpl index 79f04165e8f6..a4f091b9ec4b 100644 --- a/templates/shared/issuelist.tmpl +++ b/templates/shared/issuelist.tmpl @@ -33,7 +33,7 @@
{{RenderEmoji .Title}} {{if .IsPull}} - {{if (index $.CommitStatus .PullRequest.ID)}} + {{if (index $.CommitStatuses .PullRequest.ID)}} {{template "repo/commit_statuses" (index $.CommitStatuses .PullRequest.ID)}} {{end}} {{end}} From 4f42304df1328a1f7df91ea6e2af4efeddcf497d Mon Sep 17 00:00:00 2001 From: Parnic Date: Mon, 11 Apr 2022 11:48:03 -0500 Subject: [PATCH 3/6] Fix Details link not showing up And use a better method to build the dictionary on the fly instead of in the controller code. --- routers/web/repo/issue.go | 6 ++---- templates/shared/issuelist.tmpl | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index a09a0094fcb0..12a610f4c79e 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -276,10 +276,8 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption uti } ctx.Data["Issues"] = issues - ctx.Data["CommitStatuses"] = map[string]interface{}{ - "Status": lastStatus, - "Statuses": commitStatuses, - } + ctx.Data["CommitLastStatus"] = lastStatus + ctx.Data["CommitStatuses"] = commitStatuses // Get assignees. ctx.Data["Assignees"], err = models.GetRepoAssignees(repo) diff --git a/templates/shared/issuelist.tmpl b/templates/shared/issuelist.tmpl index a4f091b9ec4b..e81855851bf0 100644 --- a/templates/shared/issuelist.tmpl +++ b/templates/shared/issuelist.tmpl @@ -34,7 +34,7 @@ {{RenderEmoji .Title}} {{if .IsPull}} {{if (index $.CommitStatuses .PullRequest.ID)}} - {{template "repo/commit_statuses" (index $.CommitStatuses .PullRequest.ID)}} + {{template "repo/commit_statuses" dict "Status" (index $.CommitLastStatus .PullRequest.ID) "Statuses" (index $.CommitStatuses .PullRequest.ID) "root" $}} {{end}} {{end}} From 9e18bbe254595a62e13526441be16ff719d6acbf Mon Sep 17 00:00:00 2001 From: Parnic Date: Mon, 11 Apr 2022 12:02:05 -0500 Subject: [PATCH 4/6] Satisfy linter I would have preferred to leave this function in for future use, but `make lint` wants it gone, so it's gone. --- services/pull/pull.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 347620b1d759..3c3f2de42330 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -778,12 +778,6 @@ func GetIssuesAllCommitStatus(ctx context.Context, issues models.IssueList) (map return res, lastRes, nil } -// getLastCommitStatus get pr's last commit status. PR's last commit status is the head commit id's last commit status -func getLastCommitStatus(gitRepo *git.Repository, pr *models.PullRequest) (status *models.CommitStatus, err error) { - _, lastStatus, err := getAllCommitStatus(gitRepo, pr) - return lastStatus, err -} - // getAllCommitStatus get pr's commit statuses. func getAllCommitStatus(gitRepo *git.Repository, pr *models.PullRequest) (statuses []*models.CommitStatus, lastStatus *models.CommitStatus, err error) { sha, shaErr := gitRepo.GetRefCommitID(pr.GetGitRefName()) From 7406a8aa900e10024aa9d89c22dcdc5409ede95b Mon Sep 17 00:00:00 2001 From: Parnic Date: Mon, 11 Apr 2022 13:04:12 -0500 Subject: [PATCH 5/6] Also update other template users --- routers/web/user/home.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/routers/web/user/home.go b/routers/web/user/home.go index 33492aa209cf..e55bc108c802 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -579,7 +579,7 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { } } - commitStatus, err := pull_service.GetIssuesLastCommitStatus(ctx, issues) + commitStatuses, lastStatus, err := pull_service.GetIssuesAllCommitStatus(ctx, issues) if err != nil { ctx.ServerError("GetIssuesLastCommitStatus", err) return @@ -656,7 +656,8 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { } return 0 } - ctx.Data["CommitStatus"] = commitStatus + ctx.Data["CommitLastStatus"] = lastStatus + ctx.Data["CommitStatuses"] = commitStatuses ctx.Data["Repos"] = showRepos ctx.Data["Counts"] = issueCountByRepo ctx.Data["IssueStats"] = issueStats From 06412204956fac0a3ac655921583caca9bc00ee4 Mon Sep 17 00:00:00 2001 From: Parnic Date: Fri, 15 Apr 2022 10:56:28 -0500 Subject: [PATCH 6/6] Update function comments per review --- services/pull/pull.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 3c3f2de42330..f03621187132 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -726,13 +726,13 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *models.PullRequest) s return stringBuilder.String() } -// GetIssuesLastCommitStatus returns a map +// GetIssuesLastCommitStatus returns a map of issue ID to the most recent commit's latest status func GetIssuesLastCommitStatus(ctx context.Context, issues models.IssueList) (map[int64]*models.CommitStatus, error) { _, lastStatus, err := GetIssuesAllCommitStatus(ctx, issues) return lastStatus, err } -// GetIssuesAllCommitStatus returns a map +// GetIssuesAllCommitStatus returns a map of issue ID to a list of all statuses for the most recent commit as well as a map of issue ID to only the commit's latest status func GetIssuesAllCommitStatus(ctx context.Context, issues models.IssueList) (map[int64][]*models.CommitStatus, map[int64]*models.CommitStatus, error) { if err := issues.LoadPullRequests(); err != nil { return nil, nil, err