Skip to content

Commit 81daf26

Browse files
lunnylunny
authored
Fix wrong hint when status checking is running on pull request view (#9886)
* Fix wrong hint when status checking is running on pull request view * fix lint * fix test * fix test * fix wrong tmpl * fix import * rename function name
1 parent cca13ae commit 81daf26

File tree

9 files changed

+159
-84
lines changed

9 files changed

+159
-84
lines changed

‎integrations/pull_status_test.go‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"strings"
1212
"testing"
1313

14-
"code.gitea.io/gitea/models"
1514
api "code.gitea.io/gitea/modules/structs"
1615

1716
"github.com/stretchr/testify/assert"
@@ -48,20 +47,20 @@ func TestPullCreate_CommitStatus(t *testing.T) {
4847

4948
commitID := path.Base(commitURL)
5049

51-
statusList := []models.CommitStatusState{
52-
models.CommitStatusPending,
53-
models.CommitStatusError,
54-
models.CommitStatusFailure,
55-
models.CommitStatusWarning,
56-
models.CommitStatusSuccess,
50+
statusList := []api.CommitStatusState{
51+
api.CommitStatusPending,
52+
api.CommitStatusError,
53+
api.CommitStatusFailure,
54+
api.CommitStatusWarning,
55+
api.CommitStatusSuccess,
5756
}
5857

59-
statesIcons := map[models.CommitStatusState]string{
60-
models.CommitStatusPending: "circle icon yellow",
61-
models.CommitStatusSuccess: "check icon green",
62-
models.CommitStatusError: "warning icon red",
63-
models.CommitStatusFailure: "remove icon red",
64-
models.CommitStatusWarning: "warning sign icon yellow",
58+
statesIcons := map[api.CommitStatusState]string{
59+
api.CommitStatusPending: "circle icon yellow",
60+
api.CommitStatusSuccess: "check icon green",
61+
api.CommitStatusError: "warning icon red",
62+
api.CommitStatusFailure: "remove icon red",
63+
api.CommitStatusWarning: "warning sign icon yellow",
6564
}
6665

6766
// Update commit status, and check if icon is updated as well

‎models/commit_status.go‎

Lines changed: 13 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -19,52 +19,19 @@ import (
1919
"xorm.io/xorm"
2020
)
2121

22-
// CommitStatusState holds the state of a Status
23-
// It can be "pending", "success", "error", "failure", and "warning"
24-
type CommitStatusState string
25-
26-
// IsWorseThan returns true if this State is worse than the given State
27-
func (css CommitStatusState) IsWorseThan(css2 CommitStatusState) bool {
28-
switch css {
29-
case CommitStatusError:
30-
return true
31-
case CommitStatusFailure:
32-
return css2 != CommitStatusError
33-
case CommitStatusWarning:
34-
return css2 != CommitStatusError && css2 != CommitStatusFailure
35-
case CommitStatusSuccess:
36-
return css2 != CommitStatusError && css2 != CommitStatusFailure && css2 != CommitStatusWarning
37-
default:
38-
return css2 != CommitStatusError && css2 != CommitStatusFailure && css2 != CommitStatusWarning && css2 != CommitStatusSuccess
39-
}
40-
}
41-
42-
const (
43-
// CommitStatusPending is for when the Status is Pending
44-
CommitStatusPending CommitStatusState = "pending"
45-
// CommitStatusSuccess is for when the Status is Success
46-
CommitStatusSuccess CommitStatusState = "success"
47-
// CommitStatusError is for when the Status is Error
48-
CommitStatusError CommitStatusState = "error"
49-
// CommitStatusFailure is for when the Status is Failure
50-
CommitStatusFailure CommitStatusState = "failure"
51-
// CommitStatusWarning is for when the Status is Warning
52-
CommitStatusWarning CommitStatusState = "warning"
53-
)
54-
5522
// CommitStatus holds a single Status of a single Commit
5623
type CommitStatus struct {
57-
ID int64 `xorm:"pk autoincr"`
58-
Index int64 `xorm:"INDEX UNIQUE(repo_sha_index)"`
59-
RepoID int64 `xorm:"INDEX UNIQUE(repo_sha_index)"`
60-
Repo *Repository `xorm:"-"`
61-
State CommitStatusState `xorm:"VARCHAR(7) NOT NULL"`
62-
SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_sha_index)"`
63-
TargetURL string `xorm:"TEXT"`
64-
Description string `xorm:"TEXT"`
65-
ContextHash string `xorm:"char(40) index"`
66-
Context string `xorm:"TEXT"`
67-
Creator *User `xorm:"-"`
24+
ID int64 `xorm:"pk autoincr"`
25+
Index int64 `xorm:"INDEX UNIQUE(repo_sha_index)"`
26+
RepoID int64 `xorm:"INDEX UNIQUE(repo_sha_index)"`
27+
Repo *Repository `xorm:"-"`
28+
State api.CommitStatusState `xorm:"VARCHAR(7) NOT NULL"`
29+
SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_sha_index)"`
30+
TargetURL string `xorm:"TEXT"`
31+
Description string `xorm:"TEXT"`
32+
ContextHash string `xorm:"char(40) index"`
33+
Context string `xorm:"TEXT"`
34+
Creator *User `xorm:"-"`
6835
CreatorID int64
6936

7037
CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
@@ -118,9 +85,9 @@ func (status *CommitStatus) APIFormat() *api.Status {
11885
// CalcCommitStatus returns commit status state via some status, the commit statues should order by id desc
11986
func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus {
12087
var lastStatus *CommitStatus
121-
var state CommitStatusState
88+
var state api.CommitStatusState
12289
for _, status := range statuses {
123-
if status.State.IsWorseThan(state) {
90+
if status.State.NoBetterThan(state) {
12491
state = status.State
12592
lastStatus = status
12693
}

‎models/commit_status_test.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package models
77
import (
88
"testing"
99

10+
"code.gitea.io/gitea/modules/structs"
1011
"github.com/stretchr/testify/assert"
1112
)
1213

@@ -23,22 +24,22 @@ func TestGetCommitStatuses(t *testing.T) {
2324
assert.Len(t, statuses, 5)
2425

2526
assert.Equal(t, "ci/awesomeness", statuses[0].Context)
26-
assert.Equal(t, CommitStatusPending, statuses[0].State)
27+
assert.Equal(t, structs.CommitStatusPending, statuses[0].State)
2728
assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[0].APIURL())
2829

2930
assert.Equal(t, "cov/awesomeness", statuses[1].Context)
30-
assert.Equal(t, CommitStatusWarning, statuses[1].State)
31+
assert.Equal(t, structs.CommitStatusWarning, statuses[1].State)
3132
assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[1].APIURL())
3233

3334
assert.Equal(t, "cov/awesomeness", statuses[2].Context)
34-
assert.Equal(t, CommitStatusSuccess, statuses[2].State)
35+
assert.Equal(t, structs.CommitStatusSuccess, statuses[2].State)
3536
assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[2].APIURL())
3637

3738
assert.Equal(t, "ci/awesomeness", statuses[3].Context)
38-
assert.Equal(t, CommitStatusFailure, statuses[3].State)
39+
assert.Equal(t, structs.CommitStatusFailure, statuses[3].State)
3940
assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[3].APIURL())
4041

4142
assert.Equal(t, "deploy/awesomeness", statuses[4].Context)
42-
assert.Equal(t, CommitStatusError, statuses[4].State)
43+
assert.Equal(t, structs.CommitStatusError, statuses[4].State)
4344
assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[4].APIURL())
4445
}

‎modules/structs/attachment.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// license that can be found in the LICENSE file.
44

55
package structs // import "code.gitea.io/gitea/modules/structs"
6+
67
import (
78
"time"
89
)

‎modules/structs/commit_status.go‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
// Copyright 2020 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package structs
6+
7+
// CommitStatusState holds the state of a Status
8+
// It can be "pending", "success", "error", "failure", and "warning"
9+
type CommitStatusState string
10+
11+
const (
12+
// CommitStatusPending is for when the Status is Pending
13+
CommitStatusPending CommitStatusState = "pending"
14+
// CommitStatusSuccess is for when the Status is Success
15+
CommitStatusSuccess CommitStatusState = "success"
16+
// CommitStatusError is for when the Status is Error
17+
CommitStatusError CommitStatusState = "error"
18+
// CommitStatusFailure is for when the Status is Failure
19+
CommitStatusFailure CommitStatusState = "failure"
20+
// CommitStatusWarning is for when the Status is Warning
21+
CommitStatusWarning CommitStatusState = "warning"
22+
)
23+
24+
// NoBetterThan returns true if this State is no better than the given State
25+
func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool {
26+
switch css {
27+
case CommitStatusError:
28+
return true
29+
case CommitStatusFailure:
30+
return css2 != CommitStatusError
31+
case CommitStatusWarning:
32+
return css2 != CommitStatusError && css2 != CommitStatusFailure
33+
case CommitStatusPending:
34+
return css2 != CommitStatusError && css2 != CommitStatusFailure && css2 != CommitStatusWarning
35+
default:
36+
return css2 != CommitStatusError && css2 != CommitStatusFailure && css2 != CommitStatusWarning && css2 != CommitStatusPending
37+
}
38+
}
39+
40+
// IsPending represents if commit status state is pending
41+
func (css CommitStatusState) IsPending() bool {
42+
return css == CommitStatusPending
43+
}
44+
45+
// IsSuccess represents if commit status state is success
46+
func (css CommitStatusState) IsSuccess() bool {
47+
return css == CommitStatusSuccess
48+
}
49+
50+
// IsError represents if commit status state is error
51+
func (css CommitStatusState) IsError() bool {
52+
return css == CommitStatusError
53+
}
54+
55+
// IsFailure represents if commit status state is failure
56+
func (css CommitStatusState) IsFailure() bool {
57+
return css == CommitStatusFailure
58+
}
59+
60+
// IsWarning represents if commit status state is warning
61+
func (css CommitStatusState) IsWarning() bool {
62+
return css == CommitStatusWarning
63+
}

‎routers/api/v1/repo/status.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ func NewCommitStatus(ctx *context.APIContext, form api.CreateStatusOption) {
5353
return
5454
}
5555
status := &models.CommitStatus{
56-
State: models.CommitStatusState(form.State),
56+
State: api.CommitStatusState(form.State),
5757
TargetURL: form.TargetURL,
5858
Description: form.Description,
5959
Context: form.Context,
@@ -220,13 +220,13 @@ func getCommitStatuses(ctx *context.APIContext, sha string) {
220220
}
221221

222222
type combinedCommitStatus struct {
223-
State models.CommitStatusState `json:"state"`
224-
SHA string `json:"sha"`
225-
TotalCount int `json:"total_count"`
226-
Statuses []*api.Status `json:"statuses"`
227-
Repo *api.Repository `json:"repository"`
228-
CommitURL string `json:"commit_url"`
229-
URL string `json:"url"`
223+
State api.CommitStatusState `json:"state"`
224+
SHA string `json:"sha"`
225+
TotalCount int `json:"total_count"`
226+
Statuses []*api.Status `json:"statuses"`
227+
Repo *api.Repository `json:"repository"`
228+
CommitURL string `json:"commit_url"`
229+
URL string `json:"url"`
230230
}
231231

232232
// GetCombinedCommitStatusByRef returns the combined status for any given commit hash
@@ -293,7 +293,7 @@ func GetCombinedCommitStatusByRef(ctx *context.APIContext) {
293293
retStatus.Statuses = make([]*api.Status, 0, len(statuses))
294294
for _, status := range statuses {
295295
retStatus.Statuses = append(retStatus.Statuses, status.APIFormat())
296-
if status.State.IsWorseThan(retStatus.State) {
296+
if status.State.NoBetterThan(retStatus.State) {
297297
retStatus.State = status.State
298298
}
299299
}

‎routers/repo/pull.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,9 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
417417
}
418418
return false
419419
}
420-
ctx.Data["IsRequiredStatusCheckSuccess"] = pull_service.IsCommitStatusContextSuccess(commitStatuses, pull.ProtectedBranch.StatusCheckContexts)
420+
state := pull_service.MergeRequiredContextsCommitStatus(commitStatuses, pull.ProtectedBranch.StatusCheckContexts)
421+
ctx.Data["RequiredStatusCheckState"] = state
422+
ctx.Data["IsRequiredStatusCheckSuccess"] = state.IsSuccess()
421423
}
422424

423425
ctx.Data["HeadBranchMovedOn"] = headBranchSha != sha

‎services/pull/commit_status.go‎

Lines changed: 49 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,47 @@ package pull
88
import (
99
"code.gitea.io/gitea/models"
1010
"code.gitea.io/gitea/modules/git"
11+
"code.gitea.io/gitea/modules/structs"
12+
1113
"github.com/pkg/errors"
1214
)
1315

16+
// MergeRequiredContextsCommitStatus returns a commit status state for given required contexts
17+
func MergeRequiredContextsCommitStatus(commitStatuses []*models.CommitStatus, requiredContexts []string) structs.CommitStatusState {
18+
if len(requiredContexts) == 0 {
19+
status := models.CalcCommitStatus(commitStatuses)
20+
if status != nil {
21+
return status.State
22+
}
23+
return structs.CommitStatusSuccess
24+
}
25+
26+
var returnedStatus = structs.CommitStatusPending
27+
for _, ctx := range requiredContexts {
28+
var targetStatus structs.CommitStatusState
29+
for _, commitStatus := range commitStatuses {
30+
if commitStatus.Context == ctx {
31+
targetStatus = commitStatus.State
32+
break
33+
}
34+
}
35+
36+
if targetStatus == "" {
37+
targetStatus = structs.CommitStatusPending
38+
}
39+
if targetStatus.NoBetterThan(returnedStatus) {
40+
returnedStatus = targetStatus
41+
}
42+
}
43+
return returnedStatus
44+
}
45+
1446
// IsCommitStatusContextSuccess returns true if all required status check contexts succeed.
1547
func IsCommitStatusContextSuccess(commitStatuses []*models.CommitStatus, requiredContexts []string) bool {
1648
// If no specific context is required, require that last commit status is a success
1749
if len(requiredContexts) == 0 {
1850
status := models.CalcCommitStatus(commitStatuses)
19-
if status == nil || status.State != models.CommitStatusSuccess {
51+
if status == nil || status.State != structs.CommitStatusSuccess {
2052
return false
2153
}
2254
return true
@@ -26,7 +58,7 @@ func IsCommitStatusContextSuccess(commitStatuses []*models.CommitStatus, require
2658
var found bool
2759
for _, commitStatus := range commitStatuses {
2860
if commitStatus.Context == ctx {
29-
if commitStatus.State != models.CommitStatusSuccess {
61+
if commitStatus.State != structs.CommitStatusSuccess {
3062
return false
3163
}
3264

@@ -50,30 +82,39 @@ func IsPullCommitStatusPass(pr *models.PullRequest) (bool, error) {
5082
return true, nil
5183
}
5284

85+
state, err := GetPullRequestCommitStatusState(pr)
86+
if err != nil {
87+
return false, err
88+
}
89+
return state.IsSuccess(), nil
90+
}
91+
92+
// GetPullRequestCommitStatusState returns pull request merged commit status state
93+
func GetPullRequestCommitStatusState(pr *models.PullRequest) (structs.CommitStatusState, error) {
5394
// check if all required status checks are successful
5495
headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath())
5596
if err != nil {
56-
return false, errors.Wrap(err, "OpenRepository")
97+
return "", errors.Wrap(err, "OpenRepository")
5798
}
5899
defer headGitRepo.Close()
59100

60101
if !headGitRepo.IsBranchExist(pr.HeadBranch) {
61-
return false, errors.New("Head branch does not exist, can not merge")
102+
return "", errors.New("Head branch does not exist, can not merge")
62103
}
63104

64105
sha, err := headGitRepo.GetBranchCommitID(pr.HeadBranch)
65106
if err != nil {
66-
return false, errors.Wrap(err, "GetBranchCommitID")
107+
return "", errors.Wrap(err, "GetBranchCommitID")
67108
}
68109

69110
if err := pr.LoadBaseRepo(); err != nil {
70-
return false, errors.Wrap(err, "LoadBaseRepo")
111+
return "", errors.Wrap(err, "LoadBaseRepo")
71112
}
72113

73114
commitStatuses, err := models.GetLatestCommitStatus(pr.BaseRepo, sha, 0)
74115
if err != nil {
75-
return false, errors.Wrap(err, "GetLatestCommitStatus")
116+
return "", errors.Wrap(err, "GetLatestCommitStatus")
76117
}
77118

78-
return IsCommitStatusContextSuccess(commitStatuses, pr.ProtectedBranch.StatusCheckContexts), nil
119+
return MergeRequiredContextsCommitStatus(commitStatuses, pr.ProtectedBranch.StatusCheckContexts), nil
79120
}

‎templates/repo/issue/view_content/pull.tmpl‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,8 @@
4747
{{else if .IsPullRequestBroken}}red
4848
{{else if .IsBlockedByApprovals}}red
4949
{{else if .IsBlockedByRejection}}red
50-
{{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}red
50+
{{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsFailure .RequiredStatusCheckState.IsError)}}red
51+
{{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsPending .RequiredStatusCheckState.IsWarning)}}yellow
5152
{{else if and .RequireSigned (not .WillSign)}}}red
5253
{{else if .Issue.PullRequest.IsChecking}}yellow
5354
{{else if .Issue.PullRequest.CanAutoMerge}}green
@@ -118,7 +119,7 @@
118119
<i class="icon icon-octicon"><span class="octicon octicon-x"></span></i>
119120
{{$.i18n.Tr "repo.pulls.blocked_by_rejection"}}
120121
</div>
121-
{{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}
122+
{{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsError .RequiredStatusCheckState.IsFailure)}}
122123
<div class="item text red">
123124
<i class="icon icon-octicon"><span class="octicon octicon-x"></span></i>
124125
{{$.i18n.Tr "repo.pulls.required_status_check_failed"}}

0 commit comments

Comments
 (0)