From 0dced15c1a7f984f5a363c10853f9348c9b39aa9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 22 Jan 2020 14:06:11 +0800 Subject: [PATCH] Fix wrong hint when status checking is running on pull request view (#9886) (#9928) * 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 --- integrations/pull_status_test.go | 25 ++++---- models/commit_status.go | 59 +++++-------------- models/commit_status_test.go | 11 ++-- modules/structs/attachment.go | 1 + modules/structs/commit_status.go | 63 +++++++++++++++++++++ routers/api/v1/repo/status.go | 18 +++--- routers/repo/pull.go | 4 +- services/pull/commit_status.go | 57 ++++++++++++++++--- templates/repo/issue/view_content/pull.tmpl | 5 +- 9 files changed, 159 insertions(+), 84 deletions(-) create mode 100644 modules/structs/commit_status.go diff --git a/integrations/pull_status_test.go b/integrations/pull_status_test.go index fde2d3cc9..95ed755fb 100644 --- a/integrations/pull_status_test.go +++ b/integrations/pull_status_test.go @@ -11,7 +11,6 @@ import ( "strings" "testing" - "code.gitea.io/gitea/models" api "code.gitea.io/gitea/modules/structs" "github.com/stretchr/testify/assert" @@ -48,20 +47,20 @@ func TestPullCreate_CommitStatus(t *testing.T) { commitID := path.Base(commitURL) - statusList := []models.CommitStatusState{ - models.CommitStatusPending, - models.CommitStatusError, - models.CommitStatusFailure, - models.CommitStatusWarning, - models.CommitStatusSuccess, + statusList := []api.CommitStatusState{ + api.CommitStatusPending, + api.CommitStatusError, + api.CommitStatusFailure, + api.CommitStatusWarning, + api.CommitStatusSuccess, } - 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", + statesIcons := map[api.CommitStatusState]string{ + api.CommitStatusPending: "circle icon yellow", + api.CommitStatusSuccess: "check icon green", + api.CommitStatusError: "warning icon red", + api.CommitStatusFailure: "remove icon red", + api.CommitStatusWarning: "warning sign icon yellow", } // Update commit status, and check if icon is updated as well diff --git a/models/commit_status.go b/models/commit_status.go index 4e0f8166f..4102e731e 100644 --- a/models/commit_status.go +++ b/models/commit_status.go @@ -19,52 +19,19 @@ import ( "xorm.io/xorm" ) -// CommitStatusState holds the state of a Status -// It can be "pending", "success", "error", "failure", and "warning" -type CommitStatusState string - -// IsWorseThan returns true if this State is worse than the given State -func (css CommitStatusState) IsWorseThan(css2 CommitStatusState) bool { - switch css { - case CommitStatusError: - return true - case CommitStatusFailure: - return css2 != CommitStatusError - case CommitStatusWarning: - return css2 != CommitStatusError && css2 != CommitStatusFailure - case CommitStatusSuccess: - return css2 != CommitStatusError && css2 != CommitStatusFailure && css2 != CommitStatusWarning - default: - return css2 != CommitStatusError && css2 != CommitStatusFailure && css2 != CommitStatusWarning && css2 != CommitStatusSuccess - } -} - -const ( - // CommitStatusPending is for when the Status is Pending - CommitStatusPending CommitStatusState = "pending" - // CommitStatusSuccess is for when the Status is Success - CommitStatusSuccess CommitStatusState = "success" - // CommitStatusError is for when the Status is Error - CommitStatusError CommitStatusState = "error" - // CommitStatusFailure is for when the Status is Failure - CommitStatusFailure CommitStatusState = "failure" - // CommitStatusWarning is for when the Status is Warning - CommitStatusWarning CommitStatusState = "warning" -) - // CommitStatus holds a single Status of a single Commit type CommitStatus struct { - ID int64 `xorm:"pk autoincr"` - Index int64 `xorm:"INDEX UNIQUE(repo_sha_index)"` - RepoID int64 `xorm:"INDEX UNIQUE(repo_sha_index)"` - Repo *Repository `xorm:"-"` - State CommitStatusState `xorm:"VARCHAR(7) NOT NULL"` - SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_sha_index)"` - TargetURL string `xorm:"TEXT"` - Description string `xorm:"TEXT"` - ContextHash string `xorm:"char(40) index"` - Context string `xorm:"TEXT"` - Creator *User `xorm:"-"` + ID int64 `xorm:"pk autoincr"` + Index int64 `xorm:"INDEX UNIQUE(repo_sha_index)"` + RepoID int64 `xorm:"INDEX UNIQUE(repo_sha_index)"` + Repo *Repository `xorm:"-"` + State api.CommitStatusState `xorm:"VARCHAR(7) NOT NULL"` + SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_sha_index)"` + TargetURL string `xorm:"TEXT"` + Description string `xorm:"TEXT"` + ContextHash string `xorm:"char(40) index"` + Context string `xorm:"TEXT"` + Creator *User `xorm:"-"` CreatorID int64 CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` @@ -118,9 +85,9 @@ func (status *CommitStatus) APIFormat() *api.Status { // CalcCommitStatus returns commit status state via some status, the commit statues should order by id desc func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus { var lastStatus *CommitStatus - var state CommitStatusState + var state api.CommitStatusState for _, status := range statuses { - if status.State.IsWorseThan(state) { + if status.State.NoBetterThan(state) { state = status.State lastStatus = status } diff --git a/models/commit_status_test.go b/models/commit_status_test.go index 97783ae6f..90d72cd74 100644 --- a/models/commit_status_test.go +++ b/models/commit_status_test.go @@ -7,6 +7,7 @@ package models import ( "testing" + "code.gitea.io/gitea/modules/structs" "github.com/stretchr/testify/assert" ) @@ -23,22 +24,22 @@ func TestGetCommitStatuses(t *testing.T) { assert.Len(t, statuses, 5) assert.Equal(t, "ci/awesomeness", statuses[0].Context) - assert.Equal(t, CommitStatusPending, statuses[0].State) + assert.Equal(t, structs.CommitStatusPending, statuses[0].State) assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[0].APIURL()) assert.Equal(t, "cov/awesomeness", statuses[1].Context) - assert.Equal(t, CommitStatusWarning, statuses[1].State) + assert.Equal(t, structs.CommitStatusWarning, statuses[1].State) assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[1].APIURL()) assert.Equal(t, "cov/awesomeness", statuses[2].Context) - assert.Equal(t, CommitStatusSuccess, statuses[2].State) + assert.Equal(t, structs.CommitStatusSuccess, statuses[2].State) assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[2].APIURL()) assert.Equal(t, "ci/awesomeness", statuses[3].Context) - assert.Equal(t, CommitStatusFailure, statuses[3].State) + assert.Equal(t, structs.CommitStatusFailure, statuses[3].State) assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[3].APIURL()) assert.Equal(t, "deploy/awesomeness", statuses[4].Context) - assert.Equal(t, CommitStatusError, statuses[4].State) + assert.Equal(t, structs.CommitStatusError, statuses[4].State) assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[4].APIURL()) } diff --git a/modules/structs/attachment.go b/modules/structs/attachment.go index 954956f32..7becd9433 100644 --- a/modules/structs/attachment.go +++ b/modules/structs/attachment.go @@ -3,6 +3,7 @@ // license that can be found in the LICENSE file. package structs // import "code.gitea.io/gitea/modules/structs" + import ( "time" ) diff --git a/modules/structs/commit_status.go b/modules/structs/commit_status.go new file mode 100644 index 000000000..397356b13 --- /dev/null +++ b/modules/structs/commit_status.go @@ -0,0 +1,63 @@ +// Copyright 2020 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 structs + +// CommitStatusState holds the state of a Status +// It can be "pending", "success", "error", "failure", and "warning" +type CommitStatusState string + +const ( + // CommitStatusPending is for when the Status is Pending + CommitStatusPending CommitStatusState = "pending" + // CommitStatusSuccess is for when the Status is Success + CommitStatusSuccess CommitStatusState = "success" + // CommitStatusError is for when the Status is Error + CommitStatusError CommitStatusState = "error" + // CommitStatusFailure is for when the Status is Failure + CommitStatusFailure CommitStatusState = "failure" + // CommitStatusWarning is for when the Status is Warning + CommitStatusWarning CommitStatusState = "warning" +) + +// NoBetterThan returns true if this State is no better than the given State +func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool { + switch css { + case CommitStatusError: + return true + case CommitStatusFailure: + return css2 != CommitStatusError + case CommitStatusWarning: + return css2 != CommitStatusError && css2 != CommitStatusFailure + case CommitStatusPending: + return css2 != CommitStatusError && css2 != CommitStatusFailure && css2 != CommitStatusWarning + default: + return css2 != CommitStatusError && css2 != CommitStatusFailure && css2 != CommitStatusWarning && css2 != CommitStatusPending + } +} + +// IsPending represents if commit status state is pending +func (css CommitStatusState) IsPending() bool { + return css == CommitStatusPending +} + +// IsSuccess represents if commit status state is success +func (css CommitStatusState) IsSuccess() bool { + return css == CommitStatusSuccess +} + +// IsError represents if commit status state is error +func (css CommitStatusState) IsError() bool { + return css == CommitStatusError +} + +// IsFailure represents if commit status state is failure +func (css CommitStatusState) IsFailure() bool { + return css == CommitStatusFailure +} + +// IsWarning represents if commit status state is warning +func (css CommitStatusState) IsWarning() bool { + return css == CommitStatusWarning +} diff --git a/routers/api/v1/repo/status.go b/routers/api/v1/repo/status.go index c137b64f5..b6b3d495c 100644 --- a/routers/api/v1/repo/status.go +++ b/routers/api/v1/repo/status.go @@ -53,7 +53,7 @@ func NewCommitStatus(ctx *context.APIContext, form api.CreateStatusOption) { return } status := &models.CommitStatus{ - State: models.CommitStatusState(form.State), + State: api.CommitStatusState(form.State), TargetURL: form.TargetURL, Description: form.Description, Context: form.Context, @@ -220,13 +220,13 @@ func getCommitStatuses(ctx *context.APIContext, sha string) { } type combinedCommitStatus struct { - State models.CommitStatusState `json:"state"` - SHA string `json:"sha"` - TotalCount int `json:"total_count"` - Statuses []*api.Status `json:"statuses"` - Repo *api.Repository `json:"repository"` - CommitURL string `json:"commit_url"` - URL string `json:"url"` + State api.CommitStatusState `json:"state"` + SHA string `json:"sha"` + TotalCount int `json:"total_count"` + Statuses []*api.Status `json:"statuses"` + Repo *api.Repository `json:"repository"` + CommitURL string `json:"commit_url"` + URL string `json:"url"` } // GetCombinedCommitStatusByRef returns the combined status for any given commit hash @@ -293,7 +293,7 @@ func GetCombinedCommitStatusByRef(ctx *context.APIContext) { retStatus.Statuses = make([]*api.Status, 0, len(statuses)) for _, status := range statuses { retStatus.Statuses = append(retStatus.Statuses, status.APIFormat()) - if status.State.IsWorseThan(retStatus.State) { + if status.State.NoBetterThan(retStatus.State) { retStatus.State = status.State } } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 1a5c4a036..df6a92fb4 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -403,7 +403,9 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare } return false } - ctx.Data["IsRequiredStatusCheckSuccess"] = pull_service.IsCommitStatusContextSuccess(commitStatuses, pull.ProtectedBranch.StatusCheckContexts) + state := pull_service.MergeRequiredContextsCommitStatus(commitStatuses, pull.ProtectedBranch.StatusCheckContexts) + ctx.Data["RequiredStatusCheckState"] = state + ctx.Data["IsRequiredStatusCheckSuccess"] = state.IsSuccess() } ctx.Data["HeadBranchMovedOn"] = headBranchSha != sha diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index ca00cdaad..3dccfb1f0 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -8,15 +8,47 @@ package pull import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/structs" + "github.com/pkg/errors" ) +// MergeRequiredContextsCommitStatus returns a commit status state for given required contexts +func MergeRequiredContextsCommitStatus(commitStatuses []*models.CommitStatus, requiredContexts []string) structs.CommitStatusState { + if len(requiredContexts) == 0 { + status := models.CalcCommitStatus(commitStatuses) + if status != nil { + return status.State + } + return structs.CommitStatusSuccess + } + + var returnedStatus = structs.CommitStatusPending + for _, ctx := range requiredContexts { + var targetStatus structs.CommitStatusState + for _, commitStatus := range commitStatuses { + if commitStatus.Context == ctx { + targetStatus = commitStatus.State + break + } + } + + if targetStatus == "" { + targetStatus = structs.CommitStatusPending + } + if targetStatus.NoBetterThan(returnedStatus) { + returnedStatus = targetStatus + } + } + return returnedStatus +} + // IsCommitStatusContextSuccess returns true if all required status check contexts succeed. func IsCommitStatusContextSuccess(commitStatuses []*models.CommitStatus, requiredContexts []string) bool { // If no specific context is required, require that last commit status is a success if len(requiredContexts) == 0 { status := models.CalcCommitStatus(commitStatuses) - if status == nil || status.State != models.CommitStatusSuccess { + if status == nil || status.State != structs.CommitStatusSuccess { return false } return true @@ -26,7 +58,7 @@ func IsCommitStatusContextSuccess(commitStatuses []*models.CommitStatus, require var found bool for _, commitStatus := range commitStatuses { if commitStatus.Context == ctx { - if commitStatus.State != models.CommitStatusSuccess { + if commitStatus.State != structs.CommitStatusSuccess { return false } @@ -50,30 +82,39 @@ func IsPullCommitStatusPass(pr *models.PullRequest) (bool, error) { return true, nil } + state, err := GetPullRequestCommitStatusState(pr) + if err != nil { + return false, err + } + return state.IsSuccess(), nil +} + +// GetPullRequestCommitStatusState returns pull request merged commit status state +func GetPullRequestCommitStatusState(pr *models.PullRequest) (structs.CommitStatusState, error) { // check if all required status checks are successful headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) if err != nil { - return false, errors.Wrap(err, "OpenRepository") + return "", errors.Wrap(err, "OpenRepository") } defer headGitRepo.Close() if !headGitRepo.IsBranchExist(pr.HeadBranch) { - return false, errors.New("Head branch does not exist, can not merge") + return "", errors.New("Head branch does not exist, can not merge") } sha, err := headGitRepo.GetBranchCommitID(pr.HeadBranch) if err != nil { - return false, errors.Wrap(err, "GetBranchCommitID") + return "", errors.Wrap(err, "GetBranchCommitID") } if err := pr.LoadBaseRepo(); err != nil { - return false, errors.Wrap(err, "LoadBaseRepo") + return "", errors.Wrap(err, "LoadBaseRepo") } commitStatuses, err := models.GetLatestCommitStatus(pr.BaseRepo, sha, 0) if err != nil { - return false, errors.Wrap(err, "GetLatestCommitStatus") + return "", errors.Wrap(err, "GetLatestCommitStatus") } - return IsCommitStatusContextSuccess(commitStatuses, pr.ProtectedBranch.StatusCheckContexts), nil + return MergeRequiredContextsCommitStatus(commitStatuses, pr.ProtectedBranch.StatusCheckContexts), nil } diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 2dc76dcf2..72ec47024 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -42,7 +42,8 @@ {{else if .IsPullRequestBroken}}red {{else if .IsBlockedByApprovals}}red {{else if .IsBlockedByRejection}}red - {{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}red + {{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsFailure .RequiredStatusCheckState.IsError)}}red + {{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsPending .RequiredStatusCheckState.IsWarning)}}yellow {{else if .Issue.PullRequest.IsChecking}}yellow {{else if .Issue.PullRequest.CanAutoMerge}}green {{else}}red{{end}}"> @@ -117,7 +118,7 @@ {{$.i18n.Tr "repo.pulls.required_status_check_failed"}} {{else if .Issue.PullRequest.CanAutoMerge}} - {{if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}} + {{if and .EnableStatusCheck (or .RequiredStatusCheckState.IsError .RequiredStatusCheckState.IsFailure)}}
{{$.i18n.Tr "repo.pulls.required_status_check_failed"}}