From 9904f7d1b9070bc22a1bccca0aa2c7ed150f2ec0 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 30 Jan 2026 15:50:56 -0700 Subject: [PATCH] gh pr create: CCR and multiselectwithsearch --- api/queries_pr.go | 170 ++++++++-- api/queries_pr_test.go | 167 ++++++++++ pkg/cmd/issue/create/create.go | 2 +- pkg/cmd/pr/create/create.go | 28 +- pkg/cmd/pr/create/create_test.go | 548 +++++++++++++++++++------------ pkg/cmd/pr/shared/params.go | 42 ++- pkg/cmd/pr/shared/state.go | 1 + pkg/cmd/pr/shared/survey.go | 16 +- pkg/cmd/pr/shared/survey_test.go | 8 +- 9 files changed, 731 insertions(+), 251 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index bb5438fb3..632e7dd75 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -615,29 +615,41 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter } } - // reviewers are requested in yet another additional mutation - reviewParams := make(map[string]interface{}) - if ids, ok := params["userReviewerIds"]; ok && !isBlank(ids) { - reviewParams["userIds"] = ids - } - if ids, ok := params["teamReviewerIds"]; ok && !isBlank(ids) { - reviewParams["teamIds"] = ids - } + // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation + userLogins, hasUserLogins := params["userReviewerLogins"].([]string) + teamSlugs, hasTeamSlugs := params["teamReviewerSlugs"].([]string) - //TODO: How much work to extract this into own method and use for create and edit? - if len(reviewParams) > 0 { - reviewQuery := ` + if hasUserLogins || hasTeamSlugs { + // Use login-based mutation (RequestReviewsByLogin) for github.com + err := RequestReviewsByLogin(client, repo, pr.ID, userLogins, nil, teamSlugs, true) + if err != nil { + return pr, err + } + } else { + // Use ID-based mutation (requestReviews) for GHES compatibility + reviewParams := make(map[string]interface{}) + if ids, ok := params["userReviewerIds"]; ok && !isBlank(ids) { + reviewParams["userIds"] = ids + } + if ids, ok := params["teamReviewerIds"]; ok && !isBlank(ids) { + reviewParams["teamIds"] = ids + } + + //TODO: How much work to extract this into own method and use for create and edit? + if len(reviewParams) > 0 { + reviewQuery := ` mutation PullRequestCreateRequestReviews($input: RequestReviewsInput!) { requestReviews(input: $input) { clientMutationId } }` - reviewParams["pullRequestId"] = pr.ID - reviewParams["union"] = true - variables := map[string]interface{}{ - "input": reviewParams, - } - err := client.GraphQL(repo.RepoHost(), reviewQuery, variables, &result) - if err != nil { - return pr, err + reviewParams["pullRequestId"] = pr.ID + reviewParams["union"] = true + variables := map[string]interface{}{ + "input": reviewParams, + } + err := client.GraphQL(repo.RepoHost(), reviewQuery, variables, &result) + if err != nil { + return pr, err + } } } @@ -1109,6 +1121,126 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, return candidates, moreResults, nil } +// SuggestedReviewerActorsForRepo fetches potential reviewers for a repository. +// Unlike SuggestedReviewerActors, this doesn't require an existing PR - used for gh pr create. +// It combines results from two sources using a cascading quota system: +// - repository collaborators (base quota: 5) +// - organization teams (base quota: 5 + unfilled from collaborators) +// +// This ensures we show up to 10 total candidates, with each source filling any +// unfilled quota from the previous source. Results are deduplicated. +// Returns the candidates, a MoreResults count, and an error. +func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query string) ([]ReviewerCandidate, int, error) { + type responseData struct { + Repository struct { + // Check for Copilot availability by looking at any open PR's suggested reviewers + PullRequests struct { + Nodes []struct { + SuggestedActors struct { + Nodes []struct { + Reviewer struct { + TypeName string `graphql:"__typename"` + Bot struct { + Login string + } `graphql:"... on Bot"` + } + } + } `graphql:"suggestedReviewerActors(first: 10)"` + } + } `graphql:"pullRequests(first: 1, states: [OPEN])"` + Collaborators struct { + Nodes []struct { + Login string + Name string + } + } `graphql:"collaborators(first: 10, query: $query)"` + CollaboratorsTotalCount struct { + TotalCount int + } `graphql:"collaboratorsTotalCount: collaborators(first: 0)"` + } `graphql:"repository(owner: $owner, name: $name)"` + Organization struct { + Teams struct { + Nodes []struct { + Slug string + } + } `graphql:"teams(first: 10, query: $query)"` + TeamsTotalCount struct { + TotalCount int + } `graphql:"teamsTotalCount: teams(first: 0)"` + } `graphql:"organization(login: $owner)"` + } + + variables := map[string]interface{}{ + "query": githubv4.String(query), + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + } + + var result responseData + err := client.Query(repo.RepoHost(), "SuggestedReviewerActorsForRepo", &result, variables) + // Handle the case where the owner is not an organization - the query still returns + // partial data (repository), so we can continue processing. + if err != nil && !strings.Contains(err.Error(), errorResolvingOrganization) { + return nil, 0, err + } + + // Build candidates using cascading quota logic + seen := make(map[string]bool) + var candidates []ReviewerCandidate + const baseQuota = 5 + + // Check for Copilot availability from open PR's suggested reviewers + for _, pr := range result.Repository.PullRequests.Nodes { + for _, actor := range pr.SuggestedActors.Nodes { + if actor.Reviewer.TypeName == "Bot" && actor.Reviewer.Bot.Login == CopilotReviewerLogin { + candidates = append(candidates, NewReviewerBot(CopilotReviewerLogin)) + seen[CopilotReviewerLogin] = true + break + } + } + } + + // Collaborators + collaboratorsAdded := 0 + for _, c := range result.Repository.Collaborators.Nodes { + if collaboratorsAdded >= baseQuota { + break + } + if c.Login == "" { + continue + } + if !seen[c.Login] { + seen[c.Login] = true + candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) + collaboratorsAdded++ + } + } + + // Teams: quota = base + unfilled from collaborators + teamsQuota := baseQuota + (baseQuota - collaboratorsAdded) + teamsAdded := 0 + ownerName := repo.RepoOwner() + for _, t := range result.Organization.Teams.Nodes { + if teamsAdded >= teamsQuota { + break + } + if t.Slug == "" { + continue + } + teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) + if !seen[teamLogin] { + seen[teamLogin] = true + candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) + teamsAdded++ + } + } + + // MoreResults uses unfiltered total counts (teams will be 0 for personal repos) + moreResults := result.Repository.CollaboratorsTotalCount.TotalCount + result.Organization.TeamsTotalCount.TotalCount + + return candidates, moreResults, nil +} + func UpdatePullRequestBranch(client *Client, repo ghrepo.Interface, params githubv4.UpdatePullRequestBranchInput) error { var mutation struct { UpdatePullRequestBranch struct { diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 69dc505ca..4ee312b5d 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -362,3 +362,170 @@ func TestSuggestedReviewerActors(t *testing.T) { }) } } + +// mockReviewerResponseForRepo generates a GraphQL response for SuggestedReviewerActorsForRepo tests. +// It creates collaborators (c1, c2...) and teams (team1, team2...). +func mockReviewerResponseForRepo(collabs, teams, totalCollabs, totalTeams int) string { + return mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalTeams, false) +} + +// mockReviewerResponseForRepoWithCopilot generates a GraphQL response for SuggestedReviewerActorsForRepo tests. +// If copilotAvailable is true, includes Copilot in the first open PR's suggested reviewers. +func mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalTeams int, copilotAvailable bool) string { + var collabNodes, teamNodes []string + + for i := 1; i <= collabs; i++ { + collabNodes = append(collabNodes, + fmt.Sprintf(`{"login": "c%d", "name": "C%d"}`, i, i)) + } + for i := 1; i <= teams; i++ { + teamNodes = append(teamNodes, + fmt.Sprintf(`{"slug": "team%d"}`, i)) + } + + pullRequestsJSON := `"pullRequests": {"nodes": []}` + if copilotAvailable { + pullRequestsJSON = `"pullRequests": {"nodes": [{"suggestedReviewerActors": {"nodes": [{"reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}}]}}]}` + } + + return fmt.Sprintf(`{ + "data": { + "repository": { + %s, + "collaborators": {"nodes": [%s]}, + "collaboratorsTotalCount": {"totalCount": %d} + }, + "organization": { + "teams": {"nodes": [%s]}, + "teamsTotalCount": {"totalCount": %d} + } + } + }`, pullRequestsJSON, strings.Join(collabNodes, ","), totalCollabs, + strings.Join(teamNodes, ","), totalTeams) +} + +func TestSuggestedReviewerActorsForRepo(t *testing.T) { + tests := []struct { + name string + httpStubs func(*httpmock.Registry) + expectedCount int + expectedLogins []string + expectedMore int + expectError bool + }{ + { + name: "both sources plentiful - 5 each from cascading quota", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(6, 6, 20, 10))) + }, + expectedCount: 10, + expectedLogins: []string{"c1", "c2", "c3", "c4", "c5", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"}, + expectedMore: 30, + }, + { + name: "few collaborators - teams fill gap", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(2, 10, 2, 15))) + }, + expectedCount: 10, + expectedLogins: []string{"c1", "c2", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8"}, + expectedMore: 17, + }, + { + name: "no collaborators - teams only", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(0, 10, 0, 20))) + }, + expectedCount: 10, + expectedLogins: []string{"OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"}, + expectedMore: 20, + }, + { + name: "personal repo - no organization teams", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(`{ + "data": { + "repository": { + "pullRequests": {"nodes": []}, + "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, + "collaboratorsTotalCount": {"totalCount": 3} + }, + "organization": null + }, + "errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}] + }`)) + }, + expectedCount: 1, + expectedLogins: []string{"c1"}, + expectedMore: 3, + }, + { + name: "empty repo", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(0, 0, 0, 0))) + }, + expectedCount: 0, + expectedLogins: []string{}, + expectedMore: 0, + }, + { + name: "copilot available - prepended to candidates", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepoWithCopilot(3, 2, 5, 5, true))) + }, + expectedCount: 6, + expectedLogins: []string{"copilot-pull-request-reviewer", "c1", "c2", "c3", "OWNER/team1", "OWNER/team2"}, + expectedMore: 10, + }, + { + name: "copilot not available - not included", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepoWithCopilot(3, 2, 5, 5, false))) + }, + expectedCount: 5, + expectedLogins: []string{"c1", "c2", "c3", "OWNER/team1", "OWNER/team2"}, + expectedMore: 10, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + if tt.httpStubs != nil { + tt.httpStubs(reg) + } + + client := newTestClient(reg) + repo, _ := ghrepo.FromFullName("OWNER/REPO") + + candidates, moreResults, err := SuggestedReviewerActorsForRepo(client, repo, "") + if tt.expectError { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.expectedCount, len(candidates), "candidate count mismatch") + assert.Equal(t, tt.expectedMore, moreResults, "moreResults mismatch") + + logins := make([]string, len(candidates)) + for i, c := range candidates { + logins[i] = c.Login() + } + assert.Equal(t, tt.expectedLogins, logins) + }) + } +} diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index bc38c52b3..e3f2bd5e9 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -312,7 +312,7 @@ func createRun(opts *CreateOptions) (err error) { Repo: baseRepo, State: &tb, } - err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support) + err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support, nil) if err != nil { return } diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 5b5d45f9c..d29240fe5 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -21,6 +21,7 @@ import ( fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -397,11 +398,36 @@ func createRun(opts *CreateOptions) error { client := ctx.Client + // Detect ActorIsAssignable feature to determine if we can use search-based + // reviewer selection (github.com) or need to use traditional ID-based selection (GHES) + issueFeatures, _ := opts.Detector.IssueFeatures() + var reviewerSearchFunc func(string) prompter.MultiSelectSearchResult + if issueFeatures.ActorIsAssignable { + // Create search function for reviewer selection using login-based API + reviewerSearchFunc = func(query string) prompter.MultiSelectSearchResult { + candidates, moreResults, err := api.SuggestedReviewerActorsForRepo(client, ctx.PRRefs.BaseRepo(), query) + if err != nil { + return prompter.MultiSelectSearchResult{Err: err} + } + keys := make([]string, len(candidates)) + labels := make([]string, len(candidates)) + for i, c := range candidates { + keys[i] = c.Login() + labels[i] = c.DisplayName() + } + return prompter.MultiSelectSearchResult{Keys: keys, Labels: labels, MoreResults: moreResults} + } + } + state, err := NewIssueState(*ctx, *opts) if err != nil { return err } + if issueFeatures.ActorIsAssignable { + state.ActorReviewers = true + } + var openURL string if opts.WebMode { @@ -568,7 +594,7 @@ func createRun(opts *CreateOptions) error { Repo: ctx.PRRefs.BaseRepo(), State: state, } - err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, projectsV1Support) + err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, projectsV1Support, reviewerSearchFunc) if err != nil { return err } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 5ccffd7a8..254b54528 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -434,92 +434,6 @@ func Test_createRun(t *testing.T) { }, expectedErrOut: "", }, - { - name: "dry-run-nontty-with-all-opts", - tty: false, - setup: func(opts *CreateOptions, t *testing.T) func() { - opts.TitleProvided = true - opts.BodyProvided = true - opts.Title = "TITLE" - opts.Body = "BODY" - opts.BaseBranch = "trunk" - opts.HeadBranch = "feature" - opts.Assignees = []string{"monalisa"} - opts.Labels = []string{"bug", "todo"} - opts.Projects = []string{"roadmap"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} - opts.Milestone = "big one.oh" - opts.DryRun = true - return func() {} - }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "name": "" }, - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryLabelList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryMilestoneList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "milestones": { - "nodes": [ - { "title": "GA", "id": "GAID" }, - { "title": "Big One.oh", "id": "BIGONEID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - mockRetrieveProjects(t, reg) - }, - expectedOutputs: []string{ - "Would have created a Pull Request with:", - `title: TITLE`, - `draft: false`, - `base: trunk`, - `head: feature`, - `labels: bug, todo`, - `reviewers: hubot, monalisa, /core, /robots`, - `assignees: monalisa`, - `milestones: big one.oh`, - `projects: roadmap`, - `maintainerCanModify: false`, - `body:`, - `BODY`, - ``, - }, - expectedErrOut: "", - }, { name: "dry-run-tty-with-default-base", tty: true, @@ -549,98 +463,6 @@ func Test_createRun(t *testing.T) { Dry Running pull request for feature into master in OWNER/REPO - `), - }, - { - name: "dry-run-tty-with-all-opts", - tty: true, - setup: func(opts *CreateOptions, t *testing.T) func() { - opts.TitleProvided = true - opts.BodyProvided = true - opts.Title = "TITLE" - opts.Body = "BODY" - opts.BaseBranch = "trunk" - opts.HeadBranch = "feature" - opts.Assignees = []string{"monalisa"} - opts.Labels = []string{"bug", "todo"} - opts.Projects = []string{"roadmap"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} - opts.Milestone = "big one.oh" - opts.DryRun = true - return func() {} - }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "name": "" }, - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryLabelList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryMilestoneList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "milestones": { - "nodes": [ - { "title": "GA", "id": "GAID" }, - { "title": "Big One.oh", "id": "BIGONEID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - mockRetrieveProjects(t, reg) - }, - expectedOutputs: []string{ - `Would have created a Pull Request with:`, - `Title: TITLE`, - `Draft: false`, - `Base: trunk`, - `Head: feature`, - `Labels: bug, todo`, - `Reviewers: hubot, monalisa, /core, /robots`, - `Assignees: monalisa`, - `Milestones: big one.oh`, - `Projects: roadmap`, - `MaintainerCanModify: false`, - `Body:`, - ``, - ` BODY `, - ``, - ``, - }, - expectedErrOut: heredoc.Doc(` - - Dry Running pull request for feature into trunk in OWNER/REPO - `), }, { @@ -1092,9 +914,6 @@ func Test_createRun(t *testing.T) { return func() {} }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) reg.Register( httpmock.GraphQL(`query RepositoryAssignableUsers\b`), httpmock.StringResponse(` @@ -1128,17 +947,6 @@ func Test_createRun(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) mockRetrieveProjects(t, reg) reg.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), @@ -1171,15 +979,15 @@ func Test_createRun(t *testing.T) { assert.Equal(t, "BIGONEID", inputs["milestoneId"]) })) reg.Register( - httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), httpmock.GraphQLMutation(` - { "data": { "requestReviews": { + { "data": { "requestReviewsByLogin": { "clientMutationId": "" } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) - assert.Equal(t, []interface{}{"HUBOTID", "MONAID"}, inputs["userIds"]) - assert.Equal(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + assert.Equal(t, []interface{}{"hubot", "monalisa"}, inputs["userLogins"]) + assert.Equal(t, []interface{}{"core", "robots"}, inputs["teamSlugs"]) assert.Equal(t, true, inputs["union"]) })) }, @@ -1679,6 +1487,327 @@ func Test_createRun(t *testing.T) { }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", }, + { + name: "request reviewers by login", + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "my title" + opts.Body = "my body" + opts.Reviewers = []string{"hubot", "monalisa", "org/core", "org/robots"} + opts.HeadBranch = "feature" + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreate\b`), + httpmock.GraphQLMutation(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12", + "id": "NEWPULLID" + } } } }`, + func(input map[string]interface{}) {})) + reg.Register( + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), + httpmock.GraphQLMutation(` + { "data": { "requestReviewsByLogin": { + "clientMutationId": "" + } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) + assert.Equal(t, []interface{}{"hubot", "monalisa"}, inputs["userLogins"]) + assert.Equal(t, []interface{}{"core", "robots"}, inputs["teamSlugs"]) + assert.Equal(t, true, inputs["union"]) + })) + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedErrOut: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + branch := "feature" + reg := &httpmock.Registry{} + reg.StubRepoInfoResponse("OWNER", "REPO", "master") + defer reg.Verify(t) + if tt.httpStubs != nil { + tt.httpStubs(reg, t) + } + + pm := &prompter.PrompterMock{} + + if tt.promptStubs != nil { + tt.promptStubs(pm) + } + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + if !tt.customBranchConfig { + cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "") + } + + if tt.cmdStubs != nil { + tt.cmdStubs(cs) + } + + opts := CreateOptions{} + opts.Prompter = pm + + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(tt.tty) + ios.SetStdinTTY(tt.tty) + ios.SetStderrTTY(tt.tty) + + browser := &browser.Stub{} + opts.IO = ios + opts.Browser = browser + opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + opts.Config = func() (gh.Config, error) { + return config.NewBlankConfig(), nil + } + opts.Remotes = func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "origin", + Resolved: "base", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + } + opts.Branch = func() (string, error) { + return branch, nil + } + opts.Finder = shared.NewMockFinder(branch, nil, nil) + opts.GitClient = &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } + cleanSetup := func() {} + if tt.setup != nil { + cleanSetup = tt.setup(&opts, t) + } + defer cleanSetup() + + // All tests in this function use github.com behavior + opts.Detector = &fd.EnabledDetectorMock{} + + if opts.HeadBranch == "" { + cs.Register(`git status --porcelain`, 0, "") + } + + err := createRun(&opts) + output := &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + BrowsedURL: browser.BrowsedURL(), + } + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + } else { + assert.NoError(t, err) + if tt.expectedOut != "" { + assert.Equal(t, tt.expectedOut, output.String()) + } + if len(tt.expectedOutputs) > 0 { + assert.Equal(t, tt.expectedOutputs, strings.Split(output.String(), "\n")) + } + assert.Equal(t, tt.expectedErrOut, output.Stderr()) + assert.Equal(t, tt.expectedBrowse, output.BrowsedURL) + } + }) + } +} + +func Test_createRun_GHES(t *testing.T) { + tests := []struct { + name string + setup func(*CreateOptions, *testing.T) func() + cmdStubs func(*run.CommandStubber) + promptStubs func(*prompter.PrompterMock) + httpStubs func(*httpmock.Registry, *testing.T) + expectedOutputs []string + expectedOut string + expectedErrOut string + tty bool + customBranchConfig bool + }{ + { + name: "dry-run-nontty-with-all-opts", + tty: false, + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "TITLE" + opts.Body = "BODY" + opts.BaseBranch = "trunk" + opts.HeadBranch = "feature" + opts.Assignees = []string{"monalisa"} + opts.Labels = []string{"bug", "todo"} + opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} + opts.Milestone = "big one.oh" + opts.DryRun = true + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "name": "" }, + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryLabelList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "labels": { + "nodes": [ + { "name": "TODO", "id": "TODOID" }, + { "name": "bug", "id": "BUGID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryMilestoneList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [ + { "title": "GA", "id": "GAID" }, + { "title": "Big One.oh", "id": "BIGONEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + expectedOutputs: []string{ + "Would have created a Pull Request with:", + `title: TITLE`, + `draft: false`, + `base: trunk`, + `head: feature`, + `labels: bug, todo`, + `reviewers: hubot, monalisa, /core, /robots`, + `assignees: monalisa`, + `milestones: big one.oh`, + `maintainerCanModify: false`, + `body:`, + `BODY`, + ``, + }, + expectedErrOut: "", + }, + { + name: "dry-run-tty-with-all-opts", + tty: true, + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "TITLE" + opts.Body = "BODY" + opts.BaseBranch = "trunk" + opts.HeadBranch = "feature" + opts.Assignees = []string{"monalisa"} + opts.Labels = []string{"bug", "todo"} + opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} + opts.Milestone = "big one.oh" + opts.DryRun = true + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "name": "" }, + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryLabelList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "labels": { + "nodes": [ + { "name": "TODO", "id": "TODOID" }, + { "name": "bug", "id": "BUGID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryMilestoneList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [ + { "title": "GA", "id": "GAID" }, + { "title": "Big One.oh", "id": "BIGONEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + expectedOutputs: []string{ + `Would have created a Pull Request with:`, + `Title: TITLE`, + `Draft: false`, + `Base: trunk`, + `Head: feature`, + `Labels: bug, todo`, + `Reviewers: hubot, monalisa, /core, /robots`, + `Assignees: monalisa`, + `Milestones: big one.oh`, + `MaintainerCanModify: false`, + `Body:`, + ``, + ` BODY `, + ``, + ``, + }, + expectedErrOut: heredoc.Doc(` + + Dry Running pull request for feature into trunk in OWNER/REPO + + `), + }, { name: "fetch org teams non-interactively if reviewer contains any team", setup: func(opts *CreateOptions, t *testing.T) func() { @@ -1951,11 +2080,10 @@ func Test_createRun(t *testing.T) { } opts := CreateOptions{} - opts.Detector = &fd.EnabledDetectorMock{} + opts.Detector = &fd.DisabledDetectorMock{} opts.Prompter = pm ios, _, stdout, stderr := iostreams.Test() - // TODO do i need to bother with this ios.SetStdoutTTY(tt.tty) ios.SetStdinTTY(tt.tty) ios.SetStderrTTY(tt.tty) @@ -1999,23 +2127,17 @@ func Test_createRun(t *testing.T) { err := createRun(&opts) output := &test.CmdOut{ - OutBuf: stdout, - ErrBuf: stderr, - BrowsedURL: browser.BrowsedURL(), + OutBuf: stdout, + ErrBuf: stderr, } - if tt.wantErr != "" { - assert.EqualError(t, err, tt.wantErr) - } else { - assert.NoError(t, err) - if tt.expectedOut != "" { - assert.Equal(t, tt.expectedOut, output.String()) - } - if len(tt.expectedOutputs) > 0 { - assert.Equal(t, tt.expectedOutputs, strings.Split(output.String(), "\n")) - } - assert.Equal(t, tt.expectedErrOut, output.Stderr()) - assert.Equal(t, tt.expectedBrowse, output.BrowsedURL) + assert.NoError(t, err) + if tt.expectedOut != "" { + assert.Equal(t, tt.expectedOut, output.String()) } + if len(tt.expectedOutputs) > 0 { + assert.Equal(t, tt.expectedOutputs, strings.Split(output.String(), "\n")) + } + assert.Equal(t, tt.expectedErrOut, output.Stderr()) }) } } diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 784b68cf9..70990f2bf 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -61,11 +61,14 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par return nil } + // When ActorReviewers is true, we use login-based mutation and don't need to resolve reviewer IDs. + needReviewerIDs := len(tb.Reviewers) > 0 && !tb.ActorReviewers + // Retrieve minimal information needed to resolve metadata if this was not previously cached from additional metadata survey. if tb.MetadataResult == nil { input := api.RepoMetadataInput{ - Reviewers: len(tb.Reviewers) > 0, - TeamReviewers: len(tb.Reviewers) > 0 && slices.ContainsFunc(tb.Reviewers, func(r string) bool { + Reviewers: needReviewerIDs, + TeamReviewers: needReviewerIDs && slices.ContainsFunc(tb.Reviewers, func(r string) bool { return strings.ContainsRune(r, '/') }), Assignees: len(tb.Assignees) > 0, @@ -124,17 +127,34 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par } } - userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) - } - params["userReviewerIds"] = userReviewerIDs + // When ActorReviewers is true (github.com), pass logins directly for use with + // RequestReviewsByLogin mutation. Otherwise, resolve to IDs for GHES compatibility. + if tb.ActorReviewers { + params["userReviewerLogins"] = userReviewers + // Extract team slugs from org/slug format + teamSlugs := make([]string, len(teamReviewers)) + for i, t := range teamReviewers { + parts := strings.SplitN(t, "/", 2) + if len(parts) == 2 { + teamSlugs[i] = parts[1] + } else { + teamSlugs[i] = t + } + } + params["teamReviewerSlugs"] = teamSlugs + } else { + userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["userReviewerIds"] = userReviewerIDs - teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) + teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["teamReviewerIds"] = teamReviewerIDs } - params["teamReviewerIds"] = teamReviewerIDs return nil } diff --git a/pkg/cmd/pr/shared/state.go b/pkg/cmd/pr/shared/state.go index b9f2c0293..0e5c31cdd 100644 --- a/pkg/cmd/pr/shared/state.go +++ b/pkg/cmd/pr/shared/state.go @@ -20,6 +20,7 @@ type IssueMetadataState struct { Draft bool ActorAssignees bool + ActorReviewers bool Body string Title string diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index e350671b9..7bcf3a4a7 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -154,7 +154,7 @@ type RepoMetadataFetcher interface { RepoMetadataFetch(api.RepoMetadataInput) (*api.RepoMetadataResult, error) } -func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState, projectsV1Support gh.ProjectsV1Support) error { +func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState, projectsV1Support gh.ProjectsV1Support, reviewerSearchFunc func(string) prompter.MultiSelectSearchResult) error { isChosen := func(m string) bool { for _, c := range state.Metadata { if m == c { @@ -254,7 +254,19 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface }{} if isChosen("Reviewers") { - if len(reviewers) > 0 { + if reviewerSearchFunc != nil { + // Use search-based selection (github.com with ActorIsAssignable) + selectedReviewers, err := p.MultiSelectWithSearch( + "Reviewers", + "Search reviewers", + state.Reviewers, + []string{}, + reviewerSearchFunc) + if err != nil { + return err + } + values.Reviewers = selectedReviewers + } else if len(reviewers) > 0 { selected, err := p.MultiSelect("Reviewers", state.Reviewers, reviewers) if err != nil { return err diff --git a/pkg/cmd/pr/shared/survey_test.go b/pkg/cmd/pr/shared/survey_test.go index 7097d0761..23ba96cef 100644 --- a/pkg/cmd/pr/shared/survey_test.go +++ b/pkg/cmd/pr/shared/survey_test.go @@ -71,7 +71,7 @@ func TestMetadataSurvey_selectAll(t *testing.T) { Assignees: []string{"hubot"}, Type: PRMetadata, } - err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported) + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -117,7 +117,7 @@ func TestMetadataSurvey_keepExisting(t *testing.T) { Assignees: []string{"hubot"}, } - err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported) + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -146,7 +146,7 @@ func TestMetadataSurveyProjectV1Deprecation(t *testing.T) { return []int{0}, nil }) - err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Supported) + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Supported, nil) require.ErrorContains(t, err, "expected test error") require.True(t, fetcher.projectsV1Requested, "expected projectsV1 to be requested") @@ -167,7 +167,7 @@ func TestMetadataSurveyProjectV1Deprecation(t *testing.T) { return []int{0}, nil }) - err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Unsupported) + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Unsupported, nil) require.ErrorContains(t, err, "expected test error") require.False(t, fetcher.projectsV1Requested, "expected projectsV1 not to be requested")