Keeping consistent between UI and API about combined commit status state and fix some bugs

models/git/commit_status.go

@@ -17,10 +17,10 @@ import (
 	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/commitstatus"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
-	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/translation"
 
@@ -30,17 +30,17 @@ import (
 
 // 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        *repo_model.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:"VARCHAR(64) index"`
-	Context     string                 `xorm:"TEXT"`
-	Creator     *user_model.User       `xorm:"-"`
+	ID          int64                          `xorm:"pk autoincr"`
+	Index       int64                          `xorm:"INDEX UNIQUE(repo_sha_index)"`
+	RepoID      int64                          `xorm:"INDEX UNIQUE(repo_sha_index)"`
+	Repo        *repo_model.Repository         `xorm:"-"`
+	State       commitstatus.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:"VARCHAR(64) index"`
+	Context     string                         `xorm:"TEXT"`
+	Creator     *user_model.User               `xorm:"-"`
 	CreatorID   int64
 
 	CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
@@ -230,28 +230,25 @@ func (status *CommitStatus) HideActionsURL(ctx context.Context) {
 
 // CalcCommitStatus returns commit status state via some status, the commit statues should order by id desc
 func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus {
-	// This function is widely used, but it is not quite right.
-	// Ideally it should return something like "CommitStatusSummary" with properly aggregated state.
-	// GitHub's behavior: if all statuses are "skipped", GitHub will return "success" as the combined status.
-	var lastStatus *CommitStatus
-	state := api.CommitStatusSuccess
+	if len(statuses) == 0 {
+		return nil
+	}
+
+	states := make(commitstatus.CommitStatusStates, 0, len(statuses))
+	targetURL := ""
 	for _, status := range statuses {
-		if state == status.State || status.State.HasHigherPriorityThan(state) {
-			state = status.State
-			lastStatus = status
+		states = append(states, status.State)
+		if status.TargetURL != "" {
+			targetURL = status.TargetURL
 		}
 	}
-	if lastStatus == nil {
-		if len(statuses) > 0 {
-			// FIXME: a bad case: Gitea just returns the first commit status, its status is "skipped" in this case.
-			lastStatus = statuses[0]
-		} else {
-			// FIXME: another bad case: if the "statuses" slice is empty, the returned value is an invalid CommitStatus, all its fields are empty.
-			// Frontend code (tmpl&vue) sometimes depend on the empty fields to skip rendering commit status elements (need to double check in the future)
-			lastStatus = &CommitStatus{}
-		}
+
+	return &CommitStatus{
+		RepoID:    statuses[0].RepoID,
+		SHA:       statuses[0].SHA,
+		State:     states.Combine(),
+		TargetURL: targetURL,
 	}
-	return lastStatus
 }
 
 // CommitStatusOptions holds the options for query commit statuses

models/git/commit_status_summary.go

@@ -7,19 +7,19 @@ import (
 	"context"
 
 	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/commitstatus"
 	"code.gitea.io/gitea/modules/setting"
-	api "code.gitea.io/gitea/modules/structs"
 
 	"xorm.io/builder"
 )
 
 // CommitStatusSummary holds the latest commit Status of a single Commit
 type CommitStatusSummary struct {
-	ID        int64                 `xorm:"pk autoincr"`
-	RepoID    int64                 `xorm:"INDEX UNIQUE(repo_id_sha)"`
-	SHA       string                `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_id_sha)"`
-	State     api.CommitStatusState `xorm:"VARCHAR(7) NOT NULL"`
-	TargetURL string                `xorm:"TEXT"`
+	ID        int64                          `xorm:"pk autoincr"`
+	RepoID    int64                          `xorm:"INDEX UNIQUE(repo_id_sha)"`
+	SHA       string                         `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_id_sha)"`
+	State     commitstatus.CommitStatusState `xorm:"VARCHAR(7) NOT NULL"`
+	TargetURL string                         `xorm:"TEXT"`
 }
 
 func init() {

models/git/commit_status_test.go

@@ -14,9 +14,9 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/commitstatus"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
-	"code.gitea.io/gitea/modules/structs"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -38,23 +38,23 @@ func TestGetCommitStatuses(t *testing.T) {
 	assert.Len(t, statuses, 5)
 
 	assert.Equal(t, "ci/awesomeness", statuses[0].Context)
-	assert.Equal(t, structs.CommitStatusPending, statuses[0].State)
+	assert.Equal(t, commitstatus.CommitStatusPending, statuses[0].State)
 	assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[0].APIURL(db.DefaultContext))
 
 	assert.Equal(t, "cov/awesomeness", statuses[1].Context)
-	assert.Equal(t, structs.CommitStatusWarning, statuses[1].State)
+	assert.Equal(t, commitstatus.CommitStatusWarning, statuses[1].State)
 	assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[1].APIURL(db.DefaultContext))
 
 	assert.Equal(t, "cov/awesomeness", statuses[2].Context)
-	assert.Equal(t, structs.CommitStatusSuccess, statuses[2].State)
+	assert.Equal(t, commitstatus.CommitStatusSuccess, statuses[2].State)
 	assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[2].APIURL(db.DefaultContext))
 
 	assert.Equal(t, "ci/awesomeness", statuses[3].Context)
-	assert.Equal(t, structs.CommitStatusFailure, statuses[3].State)
+	assert.Equal(t, commitstatus.CommitStatusFailure, statuses[3].State)
 	assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[3].APIURL(db.DefaultContext))
 
 	assert.Equal(t, "deploy/awesomeness", statuses[4].Context)
-	assert.Equal(t, structs.CommitStatusError, statuses[4].State)
+	assert.Equal(t, commitstatus.CommitStatusError, statuses[4].State)
 	assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[4].APIURL(db.DefaultContext))
 
 	statuses, maxResults, err = db.FindAndCount[git_model.CommitStatus](db.DefaultContext, &git_model.CommitStatusOptions{
@@ -75,110 +75,110 @@ func Test_CalcCommitStatus(t *testing.T) {
 		{
 			statuses: []*git_model.CommitStatus{
 				{
-					State: structs.CommitStatusPending,
+					State: commitstatus.CommitStatusPending,
 				},
 			},
 			expected: &git_model.CommitStatus{
-				State: structs.CommitStatusPending,
+				State: commitstatus.CommitStatusPending,
 			},
 		},
 		{
 			statuses: []*git_model.CommitStatus{
 				{
-					State: structs.CommitStatusSuccess,
+					State: commitstatus.CommitStatusSuccess,
 				},
 				{
-					State: structs.CommitStatusPending,
+					State: commitstatus.CommitStatusPending,
 				},
 			},
 			expected: &git_model.CommitStatus{
-				State: structs.CommitStatusPending,
+				State: commitstatus.CommitStatusPending,
 			},
 		},
 		{
 			statuses: []*git_model.CommitStatus{
 				{
-					State: structs.CommitStatusSuccess,
+					State: commitstatus.CommitStatusSuccess,
 				},
 				{
-					State: structs.CommitStatusPending,
+					State: commitstatus.CommitStatusPending,
 				},
 				{
-					State: structs.CommitStatusSuccess,
+					State: commitstatus.CommitStatusSuccess,
 				},
 			},
 			expected: &git_model.CommitStatus{
-				State: structs.CommitStatusPending,
+				State: commitstatus.CommitStatusPending,
 			},
 		},
 		{
 			statuses: []*git_model.CommitStatus{
 				{
-					State: structs.CommitStatusError,
+					State: commitstatus.CommitStatusError,
 				},
 				{
-					State: structs.CommitStatusPending,
+					State: commitstatus.CommitStatusPending,
 				},
 				{
-					State: structs.CommitStatusSuccess,
+					State: commitstatus.CommitStatusSuccess,
 				},
 			},
 			expected: &git_model.CommitStatus{
-				State: structs.CommitStatusError,
+				State: commitstatus.CommitStatusFailure,
 			},
 		},
 		{
 			statuses: []*git_model.CommitStatus{
 				{
-					State: structs.CommitStatusWarning,
+					State: commitstatus.CommitStatusWarning,
 				},
 				{
-					State: structs.CommitStatusPending,
+					State: commitstatus.CommitStatusPending,
 				},
 				{
-					State: structs.CommitStatusSuccess,
+					State: commitstatus.CommitStatusSuccess,
 				},
 			},
 			expected: &git_model.CommitStatus{
-				State: structs.CommitStatusWarning,
+				State: commitstatus.CommitStatusPending,
 			},
 		},
 		{
 			statuses: []*git_model.CommitStatus{
 				{
-					State: structs.CommitStatusSuccess,
+					State: commitstatus.CommitStatusSuccess,
 				},
 				{
-					State: structs.CommitStatusSuccess,
+					State: commitstatus.CommitStatusSuccess,
 				},
 				{
-					State: structs.CommitStatusSuccess,
+					State: commitstatus.CommitStatusSuccess,
 				},
 			},
 			expected: &git_model.CommitStatus{
-				State: structs.CommitStatusSuccess,
+				State: commitstatus.CommitStatusSuccess,
 			},
 		},
 		{
 			statuses: []*git_model.CommitStatus{
 				{
-					State: structs.CommitStatusFailure,
+					State: commitstatus.CommitStatusFailure,
 				},
 				{
-					State: structs.CommitStatusError,
+					State: commitstatus.CommitStatusError,
 				},
 				{
-					State: structs.CommitStatusWarning,
+					State: commitstatus.CommitStatusWarning,
 				},
 			},
 			expected: &git_model.CommitStatus{
-				State: structs.CommitStatusError,
+				State: commitstatus.CommitStatusFailure,
 			},
 		},
 	}
 
 	for _, kase := range kases {
-		assert.Equal(t, kase.expected, git_model.CalcCommitStatus(kase.statuses))
+		assert.Equal(t, kase.expected, git_model.CalcCommitStatus(kase.statuses), "statuses: %v", kase.statuses)
 	}
 }
 
@@ -208,7 +208,7 @@ func TestFindRepoRecentCommitStatusContexts(t *testing.T) {
 		Creator: user2,
 		SHA:     commit.ID,
 		CommitStatus: &git_model.CommitStatus{
-			State:     structs.CommitStatusFailure,
+			State:     commitstatus.CommitStatusFailure,
 			TargetURL: "https://example.com/tests/",
 			Context:   "compliance/lint-backend",
 		},
@@ -220,7 +220,7 @@ func TestFindRepoRecentCommitStatusContexts(t *testing.T) {
 		Creator: user2,
 		SHA:     commit.ID,
 		CommitStatus: &git_model.CommitStatus{
-			State:     structs.CommitStatusSuccess,
+			State:     commitstatus.CommitStatusSuccess,
 			TargetURL: "https://example.com/tests/",
 			Context:   "compliance/lint-backend",
 		},
@@ -270,9 +270,9 @@ func TestGetCountLatestCommitStatus(t *testing.T) {
 	})
 	assert.NoError(t, err)
 	assert.Len(t, commitStatuses, 2)
-	assert.Equal(t, structs.CommitStatusFailure, commitStatuses[0].State)
+	assert.Equal(t, commitstatus.CommitStatusFailure, commitStatuses[0].State)
 	assert.Equal(t, "ci/awesomeness", commitStatuses[0].Context)
-	assert.Equal(t, structs.CommitStatusError, commitStatuses[1].State)
+	assert.Equal(t, commitstatus.CommitStatusError, commitStatuses[1].State)
 	assert.Equal(t, "deploy/awesomeness", commitStatuses[1].Context)
 
 	count, err := git_model.CountLatestCommitStatus(db.DefaultContext, repo1.ID, sha1)

modules/structs/commit_status.go → modules/commitstatus/commit_status.go

@@ -1,11 +1,11 @@
 // Copyright 2020 The Gitea Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
-package structs
+package commitstatus
 
 // CommitStatusState holds the state of a CommitStatus
-// It can be "pending", "success", "error" and "failure"
-type CommitStatusState string
+// swagger:enum CommitStatusState
+type CommitStatusState string //nolint
 
 const (
 	// CommitStatusPending is for when the CommitStatus is Pending
@@ -22,25 +22,10 @@ const (
 	CommitStatusSkipped CommitStatusState = "skipped"
 )
 
-var commitStatusPriorities = map[CommitStatusState]int{
-	CommitStatusError:   0,
-	CommitStatusFailure: 1,
-	CommitStatusWarning: 2,
-	CommitStatusPending: 3,
-	CommitStatusSuccess: 4,
-	CommitStatusSkipped: 5,
-}
-
 func (css CommitStatusState) String() string {
 	return string(css)
 }
 
-// HasHigherPriorityThan returns true if this state has higher priority than the other
-// Undefined states are considered to have the highest priority like CommitStatusError(0)
-func (css CommitStatusState) HasHigherPriorityThan(other CommitStatusState) bool {
-	return commitStatusPriorities[css] < commitStatusPriorities[other]
-}
-
 // IsPending represents if commit status state is pending
 func (css CommitStatusState) IsPending() bool {
 	return css == CommitStatusPending
@@ -65,3 +50,32 @@ func (css CommitStatusState) IsFailure() bool {
 func (css CommitStatusState) IsWarning() bool {
 	return css == CommitStatusWarning
 }
+
+// IsSkipped represents if commit status state is skipped
+func (css CommitStatusState) IsSkipped() bool {
+	return css == CommitStatusSkipped
+}
+
+type CommitStatusStates []CommitStatusState //nolint
+
+// According to https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#get-the-combined-status-for-a-specific-reference
+// > Additionally, a combined state is returned. The state is one of:
+// > failure if any of the contexts report as error or failure
+// > pending if there are no statuses or a context is pending
+// > success if the latest status for all contexts is success
+func (css CommitStatusStates) Combine() CommitStatusState {
+	successCnt := 0
+	for _, state := range css {
+		switch {
+		case state.IsError() || state.IsFailure():
+			return CommitStatusFailure
+		case state.IsPending():
+		case state.IsSuccess() || state.IsWarning() || state.IsSkipped():
+			successCnt++
+		}
+	}
+	if successCnt > 0 && successCnt == len(css) {
+		return CommitStatusSuccess
+	}
+	return CommitStatusPending
+}