Skip to content

Add commit status on repo and user pull request lists #2519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from
13 changes: 13 additions & 0 deletions integrations/editor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,19 @@ func testEditFileToNewBranch(t *testing.T, session *TestSession, user, repo, bra
return resp
}

func testEditFileToNewBranchAndSendPull(t *testing.T, session *TestSession, user, repo, branch, targetBranch, filePath, newContent string) *httptest.ResponseRecorder {
testEditFileToNewBranch(t, session, user, repo, branch, targetBranch, filePath, newContent)

url := path.Join(user, repo, "compare", branch+"..."+targetBranch)
req := NewRequestWithValues(t, "POST", url,
map[string]string{
"_csrf": GetCSRF(t, session, url),
"title": "pull request from " + targetBranch,
},
)
return session.MakeRequest(t, req, http.StatusFound)
}

func TestEditFile(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user2")
Expand Down
128 changes: 128 additions & 0 deletions integrations/repo_pull_status_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
// Copyright 2017 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package integrations

import (
"fmt"
"net/http"
"path"
"strings"
"testing"

"code.gitea.io/gitea/models"
api "code.gitea.io/sdk/gitea"

"github.com/PuerkitoBio/goquery"
"github.com/stretchr/testify/assert"
)

var (
statesIcons = map[models.CommitStatusState]string{
models.CommitStatusPending: "circle icon yellow",
models.CommitStatusSuccess: "check icon green",
models.CommitStatusError: "warning icon red",
models.CommitStatusFailure: "remove icon red",
models.CommitStatusWarning: "warning sign icon yellow",
}
)

func TestRepoPullsWithStatus(t *testing.T) {
prepareTestEnv(t)

session := loginUser(t, "user2")

var size = 5
// create some pulls
for i := 0; i < size; i++ {
testEditFileToNewBranchAndSendPull(t, session, "user2", "repo16", "master", fmt.Sprintf("test%d", i), "readme.md", fmt.Sprintf("test%d", i))
}

// look for repo's pulls page
req := NewRequest(t, "GET", "/user2/repo16/pulls")
resp := session.MakeRequest(t, req, http.StatusOK)
doc := NewHTMLParser(t, resp.Body)

var indexes = make([]string, 0, size)
doc.doc.Find("li.item").Each(func(idx int, s *goquery.Selection) {
indexes = append(indexes, strings.TrimLeft(s.Find("div").Eq(1).Text(), "#"))
})

indexes = indexes[:5]

var status = make([]models.CommitStatusState, len(indexes))
for i := 0; i < len(indexes); i++ {
switch i {
case 0:
status[i] = models.CommitStatusPending
case 1:
status[i] = models.CommitStatusSuccess
case 2:
status[i] = models.CommitStatusError
case 3:
status[i] = models.CommitStatusFailure
case 4:
status[i] = models.CommitStatusWarning
default:
status[i] = models.CommitStatusSuccess
}
}

for i, index := range indexes {
// Request repository commits page
req = NewRequestf(t, "GET", "/user2/repo16/pulls/%s/commits", index)
resp = session.MakeRequest(t, req, http.StatusOK)
doc = NewHTMLParser(t, resp.Body)

// Get first commit URL
commitURL, exists := doc.doc.Find("#commits-table tbody tr td.sha a").Last().Attr("href")
assert.True(t, exists)
assert.NotEmpty(t, commitURL)

commitID := path.Base(commitURL)
// Call API to add status for commit
req = NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo16/statuses/"+commitID,
api.CreateStatusOption{
State: api.StatusState(status[i]),
TargetURL: "http://test.ci/",
Description: "",
Context: "testci",
},
)
session.MakeRequest(t, req, http.StatusCreated)

req = NewRequestf(t, "GET", "/user2/repo16/pulls/%s/commits", index)
resp = session.MakeRequest(t, req, http.StatusOK)
doc = NewHTMLParser(t, resp.Body)

commitURL, exists = doc.doc.Find("#commits-table tbody tr td.sha a").Last().Attr("href")
assert.True(t, exists)
assert.NotEmpty(t, commitURL)
assert.EqualValues(t, commitID, path.Base(commitURL))

cls, ok := doc.doc.Find("#commits-table tbody tr td.message i.commit-status").Last().Attr("class")
assert.True(t, ok)
assert.EqualValues(t, "commit-status "+statesIcons[status[i]], cls)
}

req = NewRequest(t, "GET", "/user2/repo16/pulls")
resp = session.MakeRequest(t, req, http.StatusOK)
doc = NewHTMLParser(t, resp.Body)

doc.doc.Find("li.item").Each(func(i int, s *goquery.Selection) {
cls, ok := s.Find("i.commit-status").Attr("class")
assert.True(t, ok)
assert.EqualValues(t, "commit-status "+statesIcons[status[i]], cls)
})

req = NewRequest(t, "GET", "/pulls?type=all&repo=16&sort=&state=open")
resp = session.MakeRequest(t, req, http.StatusOK)
doc = NewHTMLParser(t, resp.Body)

doc.doc.Find("li.item").Each(func(i int, s *goquery.Selection) {
cls, ok := s.Find("i.commit-status").Attr("class")
assert.True(t, ok)
assert.EqualValues(t, "commit-status "+statesIcons[status[i]], cls)
})
}
37 changes: 36 additions & 1 deletion models/fixtures/repo_unit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -220,4 +220,39 @@
repo_id: 28
type: 1
config: "{}"
created_unix: 1524304355
created_unix: 1524304355

-
id: 33
repo_id: 16
type: 1
config: "{}"
created_unix: 946684810

-
id: 34
repo_id: 16
type: 2
config: "{\"EnableTimetracker\":false,\"AllowOnlyContributorsToTrackTime\":false}"
created_unix: 946684810

-
id: 35
repo_id: 16
type: 3
config: "{}"
created_unix: 946684810

-
id: 36
repo_id: 16
type: 4
config: "{}"
created_unix: 946684810

-
id: 37
repo_id: 16
type: 5
config: "{}"
created_unix: 946684810
12 changes: 6 additions & 6 deletions models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,12 @@ type PullRequest struct {
BaseBranch string
ProtectedBranch *ProtectedBranch `xorm:"-"`
MergeBase string `xorm:"VARCHAR(40)"`

HasMerged bool `xorm:"INDEX"`
MergedCommitID string `xorm:"VARCHAR(40)"`
MergerID int64 `xorm:"INDEX"`
Merger *User `xorm:"-"`
MergedUnix util.TimeStamp `xorm:"updated INDEX"`
LastCommitID string `xorm:"-"`
HasMerged bool `xorm:"INDEX"`
MergedCommitID string `xorm:"VARCHAR(40)"`
MergerID int64 `xorm:"INDEX"`
Merger *User `xorm:"-"`
MergedUnix util.TimeStamp `xorm:"updated INDEX"`
}

// Note: don't try to get Issue because will end up recursive querying.
Expand Down
72 changes: 72 additions & 0 deletions models/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"code.gitea.io/gitea/modules/util"
api "code.gitea.io/sdk/gitea"

"github.com/go-xorm/builder"
"github.com/go-xorm/xorm"
)

Expand Down Expand Up @@ -157,6 +158,77 @@ func GetLatestCommitStatus(repo *Repository, sha string, page int) ([]*CommitSta
return statuses, x.In("id", ids).Find(&statuses)
}

// GetIssuesLatestCommitStatuses returns all statuses with given repoIDs and shas
func GetIssuesLatestCommitStatuses(issues []*Issue) ([][]*CommitStatus, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use GetLatestCommitStatus ?

If you need to fetch from multiple SHAs at the same time, just make a GetLatestCommitsStatus(repo *Repository, shas []string, page int) and fetch the shas before that 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I would return a map[string][]*CommitStatus to that you can ret[shas[0] when using them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OR, you can have a func (*Issue) PopulateLatestCommitStatus() that fetches them per Issue 🤔

var cond = builder.NewCond()
var repoCache = make(map[int64]*git.Repository)
var err error
for i := 0; i < len(issues); i++ {
var gitRepo *git.Repository
var ok bool
if gitRepo, ok = repoCache[issues[i].PullRequest.HeadRepoID]; !ok {
if err := issues[i].PullRequest.GetHeadRepo(); err != nil {
log.Error(4, "GetHeadRepo[%d, %d]: %v", issues[i].PullRequest.ID, issues[i].PullRequest.HeadRepoID, err)
continue
}

gitRepo, err = git.OpenRepository(issues[i].PullRequest.HeadRepo.RepoPath())
if err != nil {
log.Error(4, "OpenRepository[%d, %s]: %v", issues[i].PullRequest.ID, issues[i].PullRequest.HeadRepo.RepoPath(), err)
continue
}
repoCache[issues[i].PullRequest.HeadRepoID] = gitRepo
}

issues[i].PullRequest.LastCommitID, err = gitRepo.GetBranchCommitID(issues[i].PullRequest.HeadBranch)
if err != nil {
log.Error(4, "GetBranchCommitID[%d, %s]: %v", issues[i].PullRequest.ID, issues[i].PullRequest.HeadBranch, err)
continue
}

cond = cond.Or(builder.Eq{
"repo_id": issues[i].RepoID,
"sha": issues[i].PullRequest.LastCommitID,
})
}

var ids = make([]int64, 0, len(issues))
err = x.Table("commit_status").
Where(cond).
Select("max( id ) as id").
GroupBy("repo_id, sha, context").
OrderBy("max( id ) desc").
Find(&ids)
if err != nil {
return nil, err
}

var returns = make([][]*CommitStatus, len(issues))
if len(ids) == 0 {
return returns, nil
}

statuses := make(map[int64]*CommitStatus, len(ids))
err = x.In("id", ids).Find(&statuses)
if err != nil {
return nil, err
}

var repoIDsMap = make(map[string][]int64, len(issues))
for _, status := range statuses {
key := fmt.Sprintf("%d-%s", status.RepoID, status.SHA)
repoIDsMap[key] = append(repoIDsMap[key], status.ID)
}

for i := 0; i < len(issues); i++ {
key := fmt.Sprintf("%d-%s", issues[i].RepoID, issues[i].PullRequest.LastCommitID)
for _, id := range repoIDsMap[key] {
returns[i] = append(returns[i], statuses[id])
}
}
return returns, nil
}

// GetCommitStatus populates a given status for a given commit.
// NOTE: If ID or Index isn't given, and only Context, TargetURL and/or Description
// is given, the CommitStatus created _last_ will be returned.
Expand Down
18 changes: 17 additions & 1 deletion routers/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ func issues(ctx *context.Context, milestoneID int64, isPullOption util.OptionalB
}

// Get posters.
for i := range issues {
for i := 0; i < len(issues); i++ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

// Check read status
if !ctx.IsSigned {
issues[i].IsRead = true
Expand All @@ -203,6 +203,22 @@ func issues(ctx *context.Context, milestoneID int64, isPullOption util.OptionalB
return
}
}

if isPullOption == util.OptionalBoolTrue && len(issues) > 0 {
commitStatuses, err := models.GetIssuesLatestCommitStatuses(issues)
if err != nil {
ctx.ServerError("GetIssuesLatestCommitStatuses", err)
return
}

var issuesStates = make(map[int64]*models.CommitStatus, len(issues))
for i, statuses := range commitStatuses {
issuesStates[issues[i].PullRequest.ID] = models.CalcCommitStatus(statuses)
}

ctx.Data["IssuesStates"] = issuesStates
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there way to reduce all of this lines above to just 1 function call? I am thinking about doing just 1 sql select to get what you need. There are too many for loops and also nested loop.

I think we can add something like "StateLevel" or "StateCode" to CommitStatus table (there are only strings - CommitStatus.State- right now). Then we can do simple select to get "worst" status. For "latests" status there is CommitStatus.Index or max CommitStatus.ID.


ctx.Data["Issues"] = issues

// Get assignees.
Expand Down
14 changes: 14 additions & 0 deletions routers/user/home.go
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,20 @@ func Issues(ctx *context.Context) {
for _, issue := range issues {
issue.Repo = showReposMap[issue.RepoID]
}
if isPullList && len(issues) > 0 {
commitStatuses, err := models.GetIssuesLatestCommitStatuses(issues)
if err != nil {
ctx.ServerError("GetIssuesLatestCommitStatuses", err)
return
}

var issuesStates = make(map[int64]*models.CommitStatus, len(issues))
for i, statuses := range commitStatuses {
issuesStates[issues[i].PullRequest.ID] = models.CalcCommitStatus(statuses)
}

ctx.Data["IssuesStates"] = issuesStates
}

issueStats, err := models.GetUserIssueStats(models.UserIssueStatsOptions{
UserID: ctxUser.ID,
Expand Down
30 changes: 16 additions & 14 deletions templates/repo/commit_status.tmpl
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
{{if eq .State "pending"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status circle icon yellow"></i></a>
{{end}}
{{if eq .State "success"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status check icon green"></i></a>
{{end}}
{{if eq .State "error"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status warning icon red"></i></a>
{{end}}
{{if eq .State "failure"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status remove icon red"></i></a>
{{end}}
{{if eq .State "warning"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status warning sign icon yellow"></i></a>
{{if .}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should use tabs instead of spaces

{{if eq .State "pending"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status circle icon yellow"></i></a>
{{end}}
{{if eq .State "success"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status check icon green"></i></a>
{{end}}
{{if eq .State "error"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status warning icon red"></i></a>
{{end}}
{{if eq .State "failure"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status remove icon red"></i></a>
{{end}}
{{if eq .State "warning"}}
<a href="{{.TargetURL}}" target="_blank" rel="noopener noreferrer"><i class="commit-status warning sign icon yellow"></i></a>
{{end}}
{{end}}
4 changes: 3 additions & 1 deletion templates/repo/issue/list.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,9 @@
</div>
<div class="ui {{if .IsRead}}black{{else}}green{{end}} label">#{{.Index}}</div>
<a class="title has-emoji" href="{{$.Link}}/{{.Index}}">{{.Title}}</a>

{{if .IsPull}}
{{template "repo/commit_status" (index $.IssuesStates .ID)}}
{{end}}
{{if .Ref}}
<a class="ui label" href="{{$.RepoLink}}/src/branch/{{.Ref}}">{{.Ref}}</a>
{{end}}
Expand Down
Loading