Skip to content

Commit d66d3ac

Browse files
kerobbikerobbi
committed
address latest feedback
1 parent 0c9bc56 commit d66d3ac

File tree

2 files changed

+60
-106
lines changed

2 files changed

+60
-106
lines changed

‎pkg/github/issues.go‎

Lines changed: 25 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -88,17 +88,14 @@ func fetchIssueIDs(ctx context.Context, gqlClient *githubv4.Client, owner, repo
8888
}
8989

9090
// getCloseStateReason converts a string state reason to the appropriate enum value
91-
func getCloseStateReason(stateReason string) *IssueClosedStateReason {
91+
func getCloseStateReason(stateReason string) IssueClosedStateReason {
9292
switch stateReason {
9393
case "not_planned":
94-
reason := IssueClosedStateReasonNotPlanned
95-
return &reason
94+
return IssueClosedStateReasonNotPlanned
9695
case "duplicate":
97-
reason := IssueClosedStateReasonDuplicate
98-
return &reason
96+
return IssueClosedStateReasonDuplicate
9997
default: // Default to "completed" for empty or "completed" values
100-
reason := IssueClosedStateReasonCompleted
101-
return &reason
98+
return IssueClosedStateReasonCompleted
10299
}
103100
}
104101

@@ -1259,7 +1256,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
12591256

12601257
// Create the issue request with only provided fields
12611258
issueRequest := &github.IssueRequest{}
1262-
hasNonStateUpdates := false
12631259

12641260
// Set optional parameters if provided
12651261
title, err := OptionalParam[string](request, "title")
@@ -1268,7 +1264,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
12681264
}
12691265
if title != "" {
12701266
issueRequest.Title = github.Ptr(title)
1271-
hasNonStateUpdates = true
12721267
}
12731268

12741269
body, err := OptionalParam[string](request, "body")
@@ -1277,7 +1272,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
12771272
}
12781273
if body != "" {
12791274
issueRequest.Body = github.Ptr(body)
1280-
hasNonStateUpdates = true
12811275
}
12821276

12831277
// Get labels
@@ -1287,7 +1281,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
12871281
}
12881282
if len(labels) > 0 {
12891283
issueRequest.Labels = &labels
1290-
hasNonStateUpdates = true
12911284
}
12921285

12931286
// Get assignees
@@ -1297,7 +1290,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
12971290
}
12981291
if len(assignees) > 0 {
12991292
issueRequest.Assignees = &assignees
1300-
hasNonStateUpdates = true
13011293
}
13021294

13031295
milestone, err := OptionalIntParam(request, "milestone")
@@ -1307,7 +1299,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
13071299
if milestone != 0 {
13081300
milestoneNum := milestone
13091301
issueRequest.Milestone = &milestoneNum
1310-
hasNonStateUpdates = true
13111302
}
13121303

13131304
// Get issue type
@@ -1317,7 +1308,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
13171308
}
13181309
if issueType != "" {
13191310
issueRequest.Type = github.Ptr(issueType)
1320-
hasNonStateUpdates = true
13211311
}
13221312

13231313
// Handle state, state_reason and duplicateOf parameters
@@ -1330,9 +1320,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
13301320
if err != nil {
13311321
return mcp.NewToolResultError(err.Error()), nil
13321322
}
1333-
if stateReason != "" && state == "" {
1334-
return mcp.NewToolResultError("state_reason can only be used when state is also provided"), nil
1335-
}
13361323

13371324
duplicateOf, err := OptionalIntParam(request, "duplicate_of")
13381325
if err != nil {
@@ -1342,35 +1329,28 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
13421329
return mcp.NewToolResultError("duplicate_of can only be used when state_reason is 'duplicate'"), nil
13431330
}
13441331

1345-
// Determine if there were any updates at all
1346-
if !hasNonStateUpdates && state == "" {
1347-
return mcp.NewToolResultError("No update parameters provided."), nil
1332+
// Use REST API for non-state updates
1333+
client, err := getClient(ctx)
1334+
if err != nil {
1335+
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
13481336
}
13491337

1350-
// Use REST API for non-state updates
1351-
if hasNonStateUpdates {
1352-
client, err := getClient(ctx)
1353-
if err != nil {
1354-
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
1355-
}
1338+
updatedIssue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest)
1339+
if err != nil {
1340+
return ghErrors.NewGitHubAPIErrorResponse(ctx,
1341+
"failed to update issue",
1342+
resp,
1343+
err,
1344+
), nil
1345+
}
1346+
defer func() { _ = resp.Body.Close() }()
13561347

1357-
_, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest)
1348+
if resp.StatusCode != http.StatusOK {
1349+
body, err := io.ReadAll(resp.Body)
13581350
if err != nil {
1359-
return ghErrors.NewGitHubAPIErrorResponse(ctx,
1360-
"failed to update issue",
1361-
resp,
1362-
err,
1363-
), nil
1364-
}
1365-
defer func() { _ = resp.Body.Close() }()
1366-
1367-
if resp.StatusCode != http.StatusOK {
1368-
body, err := io.ReadAll(resp.Body)
1369-
if err != nil {
1370-
return nil, fmt.Errorf("failed to read response body: %w", err)
1371-
}
1372-
return mcp.NewToolResultError(fmt.Sprintf("failed to update issue: %s", string(body))), nil
1351+
return nil, fmt.Errorf("failed to read response body: %w", err)
13731352
}
1353+
return mcp.NewToolResultError(fmt.Sprintf("failed to update issue: %s", string(body))), nil
13741354
}
13751355

13761356
// Use GraphQL API for state updates
@@ -1391,7 +1371,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
13911371
return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to find issues", err), nil
13921372
}
13931373

1394-
// Do all logic
13951374
switch state {
13961375
case "open":
13971376
// Use ReopenIssue mutation for opening
@@ -1425,9 +1404,10 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
14251404
} `graphql:"closeIssue(input: $input)"`
14261405
}
14271406

1407+
stateReasonValue := getCloseStateReason(stateReason)
14281408
closeInput := CloseIssueInput{
14291409
IssueID: issueID,
1430-
StateReason: getCloseStateReason(stateReason),
1410+
StateReason: &stateReasonValue,
14311411
}
14321412

14331413
// Set duplicate issue ID if needed
@@ -1442,26 +1422,10 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati
14421422
}
14431423
}
14441424

1445-
// Get the final state of the issue to return
1446-
client, err := getClient(ctx)
1447-
if err != nil {
1448-
return nil, err
1449-
}
1450-
1451-
finalIssue, resp, err := client.Issues.Get(ctx, owner, repo, issueNumber)
1452-
if err != nil {
1453-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "Failed to get issue", resp, err), nil
1454-
}
1455-
defer func() {
1456-
if resp != nil && resp.Body != nil {
1457-
_ = resp.Body.Close()
1458-
}
1459-
}()
1460-
14611425
// Return minimal response with just essential information
14621426
minimalResponse := MinimalResponse{
1463-
ID: fmt.Sprintf("%d", finalIssue.GetID()),
1464-
URL: finalIssue.GetHTMLURL(),
1427+
ID: fmt.Sprintf("%d", updatedIssue.GetID()),
1428+
URL: updatedIssue.GetHTMLURL(),
14651429
}
14661430

14671431
r, err := json.Marshal(minimalResponse)

‎pkg/github/issues_test.go‎

Lines changed: 35 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1054,6 +1054,18 @@ func Test_UpdateIssue(t *testing.T) {
10541054
assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "issue_number"})
10551055

10561056
// Mock issues for reuse across test cases
1057+
mockBaseIssue := &github.Issue{
1058+
Number: github.Ptr(123),
1059+
Title: github.Ptr("Title"),
1060+
Body: github.Ptr("Description"),
1061+
State: github.Ptr("open"),
1062+
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"),
1063+
Assignees: []*github.User{{Login: github.Ptr("assignee1")}, {Login: github.Ptr("assignee2")}},
1064+
Labels: []*github.Label{{Name: github.Ptr("bug")}, {Name: github.Ptr("priority")}},
1065+
Milestone: &github.Milestone{Number: github.Ptr(5)},
1066+
Type: &github.IssueType{Name: github.Ptr("Bug")},
1067+
}
1068+
10571069
mockUpdatedIssue := &github.Issue{
10581070
Number: github.Ptr(123),
10591071
Title: github.Ptr("Updated Title"),
@@ -1068,10 +1080,11 @@ func Test_UpdateIssue(t *testing.T) {
10681080
}
10691081

10701082
mockReopenedIssue := &github.Issue{
1071-
Number: github.Ptr(123),
1072-
Title: github.Ptr("Issue Title"),
1073-
State: github.Ptr("open"),
1074-
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"),
1083+
Number: github.Ptr(123),
1084+
Title: github.Ptr("Title"),
1085+
State: github.Ptr("open"),
1086+
StateReason: github.Ptr("reopened"),
1087+
HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"),
10751088
}
10761089

10771090
// Mock GraphQL responses for reuse across test cases
@@ -1139,10 +1152,6 @@ func Test_UpdateIssue(t *testing.T) {
11391152
mockResponse(t, http.StatusOK, mockUpdatedIssue),
11401153
),
11411154
),
1142-
mock.WithRequestMatch(
1143-
mock.GetReposIssuesByOwnerByRepoByIssueNumber,
1144-
mockUpdatedIssue,
1145-
),
11461155
),
11471156
mockedGQLClient: githubv4mock.NewMockedHTTPClient(),
11481157
requestArgs: map[string]interface{}{
@@ -1180,8 +1189,8 @@ func Test_UpdateIssue(t *testing.T) {
11801189
name: "close issue as duplicate",
11811190
mockedRESTClient: mock.NewMockedHTTPClient(
11821191
mock.WithRequestMatch(
1183-
mock.GetReposIssuesByOwnerByRepoByIssueNumber,
1184-
mockUpdatedIssue,
1192+
mock.PatchReposIssuesByOwnerByRepoByIssueNumber,
1193+
mockBaseIssue,
11851194
),
11861195
),
11871196
mockedGQLClient: githubv4mock.NewMockedHTTPClient(
@@ -1239,8 +1248,8 @@ func Test_UpdateIssue(t *testing.T) {
12391248
name: "reopen issue",
12401249
mockedRESTClient: mock.NewMockedHTTPClient(
12411250
mock.WithRequestMatch(
1242-
mock.GetReposIssuesByOwnerByRepoByIssueNumber,
1243-
mockReopenedIssue,
1251+
mock.PatchReposIssuesByOwnerByRepoByIssueNumber,
1252+
mockBaseIssue,
12441253
),
12451254
),
12461255
mockedGQLClient: githubv4mock.NewMockedHTTPClient(
@@ -1287,8 +1296,13 @@ func Test_UpdateIssue(t *testing.T) {
12871296
expectedIssue: mockReopenedIssue,
12881297
},
12891298
{
1290-
name: "main issue not found when trying to close it",
1291-
mockedRESTClient: mock.NewMockedHTTPClient(),
1299+
name: "main issue not found when trying to close it",
1300+
mockedRESTClient: mock.NewMockedHTTPClient(
1301+
mock.WithRequestMatch(
1302+
mock.PatchReposIssuesByOwnerByRepoByIssueNumber,
1303+
mockBaseIssue,
1304+
),
1305+
),
12921306
mockedGQLClient: githubv4mock.NewMockedHTTPClient(
12931307
githubv4mock.NewQueryMatcher(
12941308
struct {
@@ -1317,8 +1331,13 @@ func Test_UpdateIssue(t *testing.T) {
13171331
expectedErrMsg: "Failed to find issues",
13181332
},
13191333
{
1320-
name: "duplicate issue not found when closing as duplicate",
1321-
mockedRESTClient: mock.NewMockedHTTPClient(),
1334+
name: "duplicate issue not found when closing as duplicate",
1335+
mockedRESTClient: mock.NewMockedHTTPClient(
1336+
mock.WithRequestMatch(
1337+
mock.PatchReposIssuesByOwnerByRepoByIssueNumber,
1338+
mockBaseIssue,
1339+
),
1340+
),
13221341
mockedGQLClient: githubv4mock.NewMockedHTTPClient(
13231342
githubv4mock.NewQueryMatcher(
13241343
struct {
@@ -1377,10 +1396,6 @@ func Test_UpdateIssue(t *testing.T) {
13771396
}),
13781397
),
13791398
),
1380-
mock.WithRequestMatch(
1381-
mock.GetReposIssuesByOwnerByRepoByIssueNumber,
1382-
mockUpdatedIssue,
1383-
),
13841399
),
13851400
mockedGQLClient: githubv4mock.NewMockedHTTPClient(
13861401
githubv4mock.NewQueryMatcher(
@@ -1439,31 +1454,6 @@ func Test_UpdateIssue(t *testing.T) {
14391454
expectError: false,
14401455
expectedIssue: mockUpdatedIssue,
14411456
},
1442-
{
1443-
name: "no update params provided",
1444-
mockedRESTClient: mock.NewMockedHTTPClient(),
1445-
mockedGQLClient: githubv4mock.NewMockedHTTPClient(),
1446-
requestArgs: map[string]interface{}{
1447-
"owner": "owner",
1448-
"repo": "repo",
1449-
"issue_number": float64(123),
1450-
},
1451-
expectError: true,
1452-
expectedErrMsg: "No update parameters provided.",
1453-
},
1454-
{
1455-
name: "state_reason without state should fail",
1456-
mockedRESTClient: mock.NewMockedHTTPClient(),
1457-
mockedGQLClient: githubv4mock.NewMockedHTTPClient(),
1458-
requestArgs: map[string]interface{}{
1459-
"owner": "owner",
1460-
"repo": "repo",
1461-
"issue_number": float64(123),
1462-
"state_reason": "not_planned",
1463-
},
1464-
expectError: true,
1465-
expectedErrMsg: "state_reason can only be used when state is also provided",
1466-
},
14671457
{
14681458
name: "duplicate_of without duplicate state_reason should fail",
14691459
mockedRESTClient: mock.NewMockedHTTPClient(),

0 commit comments

Comments
 (0)