refactor: streamline UpdatePullRequest logic and enhance test cases for reviewer updates

Author: MayorFaj

pkg/github/discussions_test.go

@@ -87,7 +87,6 @@ var (
 			"url":       "https://github.com/owner/.github/discussions/4",
 			"category":  map[string]any{"name": "General"},
 		},
-
 	}
 
 	// Ordered mock responses
@@ -190,7 +189,7 @@ var (
 					"startCursor":     "",
 					"endCursor":       "",
 				},
-				"totalCount": 4, 
+				"totalCount": 4,
 			},
 		},
 	})

pkg/github/pullrequests.go

@@ -315,33 +315,38 @@ func UpdatePullRequest(getClient GetClientFn, getGQLClient GetGQLClientFn, t tra
 				return mcp.NewToolResultError(err.Error()), nil
 			}
 
-			// If no updates and no reviewers, return error early
-			if !restUpdateNeeded && len(reviewers) == 0 && !draftProvided {
-				return mcp.NewToolResultError("No update parameters provided"), nil
+			// If no updates, no draft change, and no reviewers, return error early
+			if !restUpdateNeeded && !draftProvided && len(reviewers) == 0 {
+				return mcp.NewToolResultError("No update parameters provided."), nil
 			}
 
-			client, err := getClient(ctx)
-			if err != nil {
-				return nil, fmt.Errorf("failed to get GitHub client: %w", err)
-			}
-			pr, resp, err := client.PullRequests.Edit(ctx, owner, repo, pullNumber, update)
-			if err != nil {
-				return ghErrors.NewGitHubAPIErrorResponse(ctx,
-					"failed to update pull request",
-					resp,
-					err,
-				), nil
-			}
-			defer func() { _ = resp.Body.Close() }()
+			// Handle REST API updates
+			if restUpdateNeeded {
+				client, err := getClient(ctx)
+				if err != nil {
+					return nil, fmt.Errorf("failed to get GitHub client: %w", err)
+				}
 
-			if resp.StatusCode != http.StatusOK {
-				body, err := io.ReadAll(resp.Body)
+				_, resp, err := client.PullRequests.Edit(ctx, owner, repo, pullNumber, update)
 				if err != nil {
-					return nil, fmt.Errorf("failed to read response body: %w", err)
+					return ghErrors.NewGitHubAPIErrorResponse(ctx,
+						"failed to update pull request",
+						resp,
+						err,
+					), nil
+				}
+				defer func() { _ = resp.Body.Close() }()
+
+				if resp.StatusCode != http.StatusOK {
+					body, err := io.ReadAll(resp.Body)
+					if err != nil {
+						return nil, fmt.Errorf("failed to read response body: %w", err)
+					}
+					return mcp.NewToolResultError(fmt.Sprintf("failed to update pull request: %s", string(body))), nil
 				}
-				return mcp.NewToolResultError(fmt.Sprintf("failed to update pull request: %s", string(body))), nil
 			}
 
+			// Handle draft status changes using GraphQL
 			if draftProvided {
 				gqlClient, err := getGQLClient(ctx)
 				if err != nil {
@@ -407,6 +412,41 @@ func UpdatePullRequest(getClient GetClientFn, getGQLClient GetGQLClientFn, t tra
 				}
 			}
 
+			// Handle reviewer requests
+			if len(reviewers) > 0 {
+				client, err := getClient(ctx)
+				if err != nil {
+					return nil, fmt.Errorf("failed to get GitHub client: %w", err)
+				}
+
+				reviewersRequest := github.ReviewersRequest{
+					Reviewers: reviewers,
+				}
+
+				_, resp, err := client.PullRequests.RequestReviewers(ctx, owner, repo, pullNumber, reviewersRequest)
+				if err != nil {
+					return ghErrors.NewGitHubAPIErrorResponse(ctx,
+						"failed to request reviewers",
+						resp,
+						err,
+					), nil
+				}
+				defer func() {
+					if resp != nil && resp.Body != nil {
+						_ = resp.Body.Close()
+					}
+				}()
+
+				if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK {
+					body, err := io.ReadAll(resp.Body)
+					if err != nil {
+						return nil, fmt.Errorf("failed to read response body: %w", err)
+					}
+					return mcp.NewToolResultError(fmt.Sprintf("failed to request reviewers: %s", string(body))), nil
+				}
+			}
+
+			// Get the final state of the PR to return
 			client, err := getClient(ctx)
 			if err != nil {
 				return nil, err

pkg/github/pullrequests_test.go

@@ -257,58 +257,47 @@ func Test_UpdatePullRequest(t *testing.T) {
 			expectedPR:  mockClosedPR,
 		},
 		{
-<<<<<<< HEAD
 			name: "successful PR update with reviewers",
 			mockedClient: mock.NewMockedHTTPClient(
-				mock.WithRequestMatch(
-					mock.GetReposPullsByOwnerByRepoByPullNumber,
-					&github.PullRequest{
-						Number: github.Ptr(42),
-						Title:  github.Ptr("Test PR"),
-						State:  github.Ptr("open"),
-					},
-				),
 				// Mock for RequestReviewers call, returning the PR with reviewers
 				mock.WithRequestMatch(
 					mock.PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber,
 					mockPRWithReviewers,
-=======
-			name: "successful PR update (title only)",
-			mockedClient: mock.NewMockedHTTPClient(
-				mock.WithRequestMatchHandler(
-					mock.PatchReposPullsByOwnerByRepoByPullNumber,
-					expectRequestBody(t, map[string]interface{}{
-						"title": "Updated Test PR Title",
-					}).andThen(
-						mockResponse(t, http.StatusOK, mockUpdatedPR),
-					),
 				),
 				mock.WithRequestMatch(
 					mock.GetReposPullsByOwnerByRepoByPullNumber,
-					mockUpdatedPR,
->>>>>>> d5e1f48 (Add updating draft state to `update_pull_request` tool (#774))
+					mockPRWithReviewers,
 				),
 			),
 			requestArgs: map[string]interface{}{
 				"owner":      "owner",
 				"repo":       "repo",
 				"pullNumber": float64(42),
-<<<<<<< HEAD
 				"reviewers":  []interface{}{"reviewer1", "reviewer2"},
 			},
 			expectError: false,
 			expectedPR:  mockPRWithReviewers,
 		},
 		{
-			name: "no update parameters provided",
+			name: "successful PR update (title only)",
 			mockedClient: mock.NewMockedHTTPClient(
-				// Mock a response for the GET PR request in case of no updates
+				mock.WithRequestMatchHandler(
+					mock.PatchReposPullsByOwnerByRepoByPullNumber,
+					expectRequestBody(t, map[string]interface{}{
+						"title": "Updated Test PR Title",
+					}).andThen(
+						mockResponse(t, http.StatusOK, mockUpdatedPR),
+					),
+				),
 				mock.WithRequestMatch(
 					mock.GetReposPullsByOwnerByRepoByPullNumber,
-					mockNoUpdatePR,
+					mockUpdatedPR,
 				),
 			),
-=======
+			requestArgs: map[string]interface{}{
+				"owner":      "owner",
+				"repo":       "repo",
+				"pullNumber": float64(42),
 				"title":      "Updated Test PR Title",
 			},
 			expectError: false,
@@ -317,7 +306,6 @@ func Test_UpdatePullRequest(t *testing.T) {
 		{
 			name:         "no update parameters provided",
 			mockedClient: mock.NewMockedHTTPClient(), // No API call expected
->>>>>>> d5e1f48 (Add updating draft state to `update_pull_request` tool (#774))
 			requestArgs: map[string]interface{}{
 				"owner":      "owner",
 				"repo":       "repo",