refactor improve NoBetterThan (#26126)

- The `NoBetterThan` function can only handle comparisons between
"pending," "success," "error," and "failure." For any other comparison,
we directly return false. This prevents logic errors like the one in
#26121.
- The callers of the `NoBetterThan` function should also avoid making
incomparable calls.

---------

Co-authored-by: yp05327 <576951401@qq.com>
Co-authored-by: puni9869 <80308335+puni9869@users.noreply.github.com>

4 files changed, 193 insertions, 8 deletions

diff --git a/modules/structs/commit_status.go b/modules/structs/commit_status.go
index ff31f2d2ac..de1d8fa566 100644
--- a/modules/structs/commit_status.go
+++ b/modules/structs/commit_status.go
@@ -4,7 +4,7 @@
 package structs
 
 // CommitStatusState holds the state of a CommitStatus
-// It can be "pending", "success", "error", "failure", and "warning"
+// It can be "pending", "success", "error" and "failure"
 type CommitStatusState string
 
 const (
@@ -18,14 +18,25 @@ const (
 	CommitStatusFailure CommitStatusState = "failure"
 )
 
+var commitStatusPriorities = map[CommitStatusState]int{
+	CommitStatusError:   0,
+	CommitStatusFailure: 1,
+	CommitStatusPending: 2,
+	CommitStatusSuccess: 3,
+}
+
 // NoBetterThan returns true if this State is no better than the given State
+// This function only handles the states defined in CommitStatusPriorities
 func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool {
-	commitStatusPriorities := map[CommitStatusState]int{
-		CommitStatusError:   0,
-		CommitStatusFailure: 1,
-		CommitStatusPending: 2,
-		CommitStatusSuccess: 3,
+	// NoBetterThan only handles the 4 states above
+	if _, exist := commitStatusPriorities[css]; !exist {
+		return false
 	}
+
+	if _, exist := commitStatusPriorities[css2]; !exist {
+		return false
+	}
+
 	return commitStatusPriorities[css] <= commitStatusPriorities[css2]
 }
 
diff --git a/modules/structs/commit_status_test.go b/modules/structs/commit_status_test.go
new file mode 100644
index 0000000000..f06808534c
--- /dev/null
+++ b/modules/structs/commit_status_test.go
@@ -0,0 +1,174 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package structs
+
+import (
+	"testing"
+)
+
+func TestNoBetterThan(t *testing.T) {
+	type args struct {
+		css  CommitStatusState
+		css2 CommitStatusState
+	}
+	var unExpectedState CommitStatusState
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "success is no better than success",
+			args: args{
+				css:  CommitStatusSuccess,
+				css2: CommitStatusSuccess,
+			},
+			want: true,
+		},
+		{
+			name: "success is no better than pending",
+			args: args{
+				css:  CommitStatusSuccess,
+				css2: CommitStatusPending,
+			},
+			want: false,
+		},
+		{
+			name: "success is no better than failure",
+			args: args{
+				css:  CommitStatusSuccess,
+				css2: CommitStatusFailure,
+			},
+			want: false,
+		},
+		{
+			name: "success is no better than error",
+			args: args{
+				css:  CommitStatusSuccess,
+				css2: CommitStatusError,
+			},
+			want: false,
+		},
+		{
+			name: "pending is no better than success",
+			args: args{
+				css:  CommitStatusPending,
+				css2: CommitStatusSuccess,
+			},
+			want: true,
+		},
+		{
+			name: "pending is no better than pending",
+			args: args{
+				css:  CommitStatusPending,
+				css2: CommitStatusPending,
+			},
+			want: true,
+		},
+		{
+			name: "pending is no better than failure",
+			args: args{
+				css:  CommitStatusPending,
+				css2: CommitStatusFailure,
+			},
+			want: false,
+		},
+		{
+			name: "pending is no better than error",
+			args: args{
+				css:  CommitStatusPending,
+				css2: CommitStatusError,
+			},
+			want: false,
+		},
+		{
+			name: "failure is no better than success",
+			args: args{
+				css:  CommitStatusFailure,
+				css2: CommitStatusSuccess,
+			},
+			want: true,
+		},
+		{
+			name: "failure is no better than pending",
+			args: args{
+				css:  CommitStatusFailure,
+				css2: CommitStatusPending,
+			},
+			want: true,
+		},
+		{
+			name: "failure is no better than failure",
+			args: args{
+				css:  CommitStatusFailure,
+				css2: CommitStatusFailure,
+			},
+			want: true,
+		},
+		{
+			name: "failure is no better than error",
+			args: args{
+				css:  CommitStatusFailure,
+				css2: CommitStatusError,
+			},
+			want: false,
+		},
+		{
+			name: "error is no better than success",
+			args: args{
+				css:  CommitStatusError,
+				css2: CommitStatusSuccess,
+			},
+			want: true,
+		},
+		{
+			name: "error is no better than pending",
+			args: args{
+				css:  CommitStatusError,
+				css2: CommitStatusPending,
+			},
+			want: true,
+		},
+		{
+			name: "error is no better than failure",
+			args: args{
+				css:  CommitStatusError,
+				css2: CommitStatusFailure,
+			},
+			want: true,
+		},
+		{
+			name: "error is no better than error",
+			args: args{
+				css:  CommitStatusError,
+				css2: CommitStatusError,
+			},
+			want: true,
+		},
+		{
+			name: "unExpectedState is no better than success",
+			args: args{
+				css:  unExpectedState,
+				css2: CommitStatusSuccess,
+			},
+			want: false,
+		},
+		{
+			name: "unExpectedState is no better than unExpectedState",
+			args: args{
+				css:  unExpectedState,
+				css2: unExpectedState,
+			},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := tt.args.css.NoBetterThan(tt.args.css2)
+			if result != tt.want {
+				t.Errorf("NoBetterThan() = %v, want %v", result, tt.want)
+			}
+		})
+	}
+}
diff --git a/services/convert/status.go b/services/convert/status.go
index c7b6450e27..6cef63c1cd 100644
--- a/services/convert/status.go
+++ b/services/convert/status.go
@@ -48,7 +48,7 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r
 	retStatus.Statuses = make([]*api.CommitStatus, 0, len(statuses))
 	for _, status := range statuses {
 		retStatus.Statuses = append(retStatus.Statuses, ToCommitStatus(ctx, status))
-		if status.State.NoBetterThan(retStatus.State) {
+		if retStatus.State == "" || status.State.NoBetterThan(retStatus.State) {
 			retStatus.State = status.State
 		}
 	}
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl
index 7cb9bac95d..10ad8f555c 100644
--- a/templates/swagger/v1_json.tmpl
+++ b/templates/swagger/v1_json.tmpl
@@ -16514,7 +16514,7 @@
       "x-go-package": "code.gitea.io/gitea/modules/structs"
     },
     "CommitStatusState": {
-      "description": "CommitStatusState holds the state of a CommitStatus\nIt can be \"pending\", \"success\", \"error\", \"failure\", and \"warning\"",
+      "description": "CommitStatusState holds the state of a CommitStatus\nIt can be \"pending\", \"success\", \"error\" and \"failure\"",
       "type": "string",
       "x-go-package": "code.gitea.io/gitea/modules/structs"
     },