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

@@ -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

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
 		}

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())
 }

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"
 )

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
+}

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
 		}
 	}

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

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
 }

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}}"><span class="mega-octicon octicon-git-merge"></span></a>
@@ -117,7 +118,7 @@
 					{{$.i18n.Tr "repo.pulls.required_status_check_failed"}}
 				</div>
 			{{else if .Issue.PullRequest.CanAutoMerge}}
-				{{if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}
+				{{if and .EnableStatusCheck (or .RequiredStatusCheckState.IsError .RequiredStatusCheckState.IsFailure)}}
 				<div class="item text red">
 					<span class="octicon octicon-x"></span>
 					{{$.i18n.Tr "repo.pulls.required_status_check_failed"}}