Skip to content

Commit 0045b5f

Browse files
tphoneyactions-user
authored andcommitted
fix, cli limit get-changes status enum (#2755)
<!-- CURSOR_SUMMARY --> > [!NOTE] > Restricts get-change --status to allowed enums with validation and adds unit tests. > > - **CLI**: > - `GetChange`: Validate `--status` via new `validateChangeStatus` and pass the parsed `sdp.ChangeStatus` to `getChangeUUIDAndCheckStatus`. > - `validateChangeStatus`: Accepts only `CHANGE_STATUS_DEFINING`, `CHANGE_STATUS_HAPPENING`, `CHANGE_STATUS_DONE`; returns `flagError` for invalid values. > - **Tests**: > - Add `changes_get_change_test.go` covering valid/invalid `--status` inputs and asserting `flagError` on failures. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit dd7cb9061f2af5f9810cba4d024b9f0bd5efff90. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> check the linear ticket to see this issue in honeycomb. but our customers are filtering on statuses we do not use, therefore they will be getting confusing messages about not finding changes that are in ChangeStatus_CHANGE_STATUS_UNSPECIFIED(which is no longer used) or CHANGE_STATUS_PROCESSING(not used anywhere). <img width="2841" height="1817" alt="image" src="https://github.com/user-attachments/assets/0e646e4b-493f-46cc-bc83-18d873f3f515" /> GitOrigin-RevId: 686ee76106f8ff308ece2f50072f3a294c1dafb3
1 parent 508232f commit 0045b5f

File tree

2 files changed

+122
-1
lines changed

2 files changed

+122
-1
lines changed

cmd/changes_get_change.go

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@ func GetChange(cmd *cobra.Command, args []string) error {
2727

2828
app := viper.GetString("app")
2929

30+
// Validate status flag
31+
status, err := validateChangeStatus(viper.GetString("status"))
32+
if err != nil {
33+
return err
34+
}
35+
3036
riskLevels := []sdp.Risk_Severity{}
3137
for _, level := range viper.GetStringSlice("risk-levels") {
3238
switch level {
@@ -52,7 +58,7 @@ func GetChange(cmd *cobra.Command, args []string) error {
5258
return err
5359
}
5460

55-
changeUuid, err := getChangeUUIDAndCheckStatus(ctx, oi, sdp.ChangeStatus(sdp.ChangeStatus_value[viper.GetString("status")]), viper.GetString("ticket-link"), true)
61+
changeUuid, err := getChangeUUIDAndCheckStatus(ctx, oi, status, viper.GetString("ticket-link"), true)
5662
if err != nil {
5763
return loggedError{
5864
err: err,
@@ -148,6 +154,30 @@ fetch:
148154
return nil
149155
}
150156

157+
// validateChangeStatus validates that the provided status string is a valid ChangeStatus
158+
func validateChangeStatus(statusStr string) (sdp.ChangeStatus, error) {
159+
// Define valid status values (excluding UNSPECIFIED and PROCESSING as they are not typically used)
160+
validStatuses := map[string]sdp.ChangeStatus{
161+
"CHANGE_STATUS_DEFINING": sdp.ChangeStatus_CHANGE_STATUS_DEFINING,
162+
"CHANGE_STATUS_HAPPENING": sdp.ChangeStatus_CHANGE_STATUS_HAPPENING,
163+
"CHANGE_STATUS_DONE": sdp.ChangeStatus_CHANGE_STATUS_DONE,
164+
}
165+
166+
if status, exists := validStatuses[statusStr]; exists {
167+
return status, nil
168+
}
169+
170+
// Build list of valid status names for error message
171+
validNames := make([]string, 0, len(validStatuses))
172+
for name := range validStatuses {
173+
validNames = append(validNames, name)
174+
}
175+
176+
return sdp.ChangeStatus_CHANGE_STATUS_UNSPECIFIED, flagError{
177+
fmt.Sprintf("invalid --status value '%s', allowed values are: %s", statusStr, strings.Join(validNames, ", ")),
178+
}
179+
}
180+
151181
func init() {
152182
changesCmd.AddCommand(getChangeCmd)
153183
addAPIFlags(getChangeCmd)

cmd/changes_get_change_test.go

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
package cmd
2+
3+
import (
4+
"errors"
5+
"testing"
6+
7+
"github.com/overmindtech/cli/sdp-go"
8+
)
9+
10+
func TestValidateChangeStatus(t *testing.T) {
11+
tests := []struct {
12+
name string
13+
statusStr string
14+
expected sdp.ChangeStatus
15+
expectError bool
16+
}{
17+
{
18+
name: "valid defining status",
19+
statusStr: "CHANGE_STATUS_DEFINING",
20+
expected: sdp.ChangeStatus_CHANGE_STATUS_DEFINING,
21+
expectError: false,
22+
},
23+
{
24+
name: "valid happening status",
25+
statusStr: "CHANGE_STATUS_HAPPENING",
26+
expected: sdp.ChangeStatus_CHANGE_STATUS_HAPPENING,
27+
expectError: false,
28+
},
29+
{
30+
name: "valid done status",
31+
statusStr: "CHANGE_STATUS_DONE",
32+
expected: sdp.ChangeStatus_CHANGE_STATUS_DONE,
33+
expectError: false,
34+
},
35+
{
36+
name: "invalid status - empty string",
37+
statusStr: "",
38+
expected: sdp.ChangeStatus_CHANGE_STATUS_UNSPECIFIED,
39+
expectError: true,
40+
},
41+
{
42+
name: "invalid status - random string",
43+
statusStr: "INVALID_STATUS",
44+
expected: sdp.ChangeStatus_CHANGE_STATUS_UNSPECIFIED,
45+
expectError: true,
46+
},
47+
{
48+
name: "invalid status - unspecified",
49+
statusStr: "CHANGE_STATUS_UNSPECIFIED",
50+
expected: sdp.ChangeStatus_CHANGE_STATUS_UNSPECIFIED,
51+
expectError: true,
52+
},
53+
{
54+
name: "invalid status - processing",
55+
statusStr: "CHANGE_STATUS_PROCESSING",
56+
expected: sdp.ChangeStatus_CHANGE_STATUS_UNSPECIFIED,
57+
expectError: true,
58+
},
59+
{
60+
name: "invalid status - lowercase",
61+
statusStr: "change_status_defining",
62+
expected: sdp.ChangeStatus_CHANGE_STATUS_UNSPECIFIED,
63+
expectError: true,
64+
},
65+
}
66+
67+
for _, tt := range tests {
68+
t.Run(tt.name, func(t *testing.T) {
69+
result, err := validateChangeStatus(tt.statusStr)
70+
71+
if tt.expectError {
72+
if err == nil {
73+
t.Errorf("validateChangeStatus() expected error but got none")
74+
}
75+
// Check that it returns a flagError
76+
var fError flagError
77+
if !errors.As(err, &fError) {
78+
t.Errorf("validateChangeStatus() expected flagError but got %T", err)
79+
}
80+
} else {
81+
if err != nil {
82+
t.Errorf("validateChangeStatus() unexpected error: %v", err)
83+
}
84+
}
85+
86+
if result != tt.expected {
87+
t.Errorf("validateChangeStatus() = %v, expected %v", result, tt.expected)
88+
}
89+
})
90+
}
91+
}

0 commit comments

Comments
 (0)