From 8f20f0ab53bb40e3b5a9f6153f8fa18276bc1f9b Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 29 Jul 2025 18:19:22 -0400 Subject: [PATCH 1/3] Include org teams for PR reviewers This commit enhances the following scenarios to include organization teams as pull request reviewers: 1. Interactive `gh pr create` during additional metadata 2. Tab completing `gh pr create --reviewer` 3. Tab completing `gh pr edit --add-reviewer` 4. Tab completing `gh pr edit --remove-reviewer` Additionally, a new `gh pr create` test case for ensuring that teams show up within interactive prompts has been added. --- pkg/cmd/pr/create/create_test.go | 128 ++++++++++++++++++++++++++++++- pkg/cmd/pr/shared/completion.go | 5 +- pkg/cmd/pr/shared/survey.go | 1 + 3 files changed, 131 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 469428670..9af351830 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1655,7 +1655,7 @@ func Test_createRun(t *testing.T) { expectedOut: "https://github.com/OWNER/REPO/pull/12\n", }, { - name: "if reviewer contains any team, fetch teams", + name: "fetch org teams non-interactively if reviewer contains any team", setup: func(opts *CreateOptions, t *testing.T) func() { opts.TitleProvided = true opts.BodyProvided = true @@ -1718,7 +1718,7 @@ func Test_createRun(t *testing.T) { expectedErrOut: "", }, { - name: "if reviewer does NOT contain any team, do NOT fetch teams", + name: "do not fetch org teams non-interactively if reviewer does not contain any team", setup: func(opts *CreateOptions, t *testing.T) func() { opts.TitleProvided = true opts.BodyProvided = true @@ -1773,6 +1773,130 @@ func Test_createRun(t *testing.T) { expectedOut: "https://github.com/OWNER/REPO/pull/12\n", expectedErrOut: "", }, + { + name: "fetch org teams interactively if reviewer metadata selected", + tty: true, + setup: func(opts *CreateOptions, t *testing.T) func() { + // In order to test additional metadata, title and body cannot be provided here. + opts.HeadBranch = "feature" + return func() {} + }, + cmdStubs: func(cs *run.CommandStubber) { + // Stub git commits for `initDefaultTitleBody` when initializing PR state. + cs.Register( + "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", + 0, + "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n", + ) + cs.Register(`git rev-parse --show-toplevel`, 0, "") + }, + promptStubs: func(pm *prompter.PrompterMock) { + firstConfirmSubmission := true + pm.InputFunc = func(message, defaultValue string) (string, error) { + switch message { + case "Title (required)": + return "TITLE", nil + default: + return "", fmt.Errorf("unexpected input prompt: %s", message) + } + } + pm.MarkdownEditorFunc = func(message, defaultValue string, allowEmpty bool) (string, error) { + switch message { + case "Body": + return "BODY", nil + default: + return "", fmt.Errorf("unexpected markdown editor prompt: %s", message) + } + } + pm.MultiSelectFunc = func(message string, defaults []string, options []string) ([]int, error) { + switch message { + case "What would you like to add?": + return prompter.IndexesFor(options, "Reviewers") + case "Reviewers": + return prompter.IndexesFor(options, "MonaLisa (Mona Display Name)", "OWNER/core") + default: + return nil, fmt.Errorf("unexpected multi-select prompt: %s", message) + } + } + pm.SelectFunc = func(message, defaultValue string, options []string) (int, error) { + switch message { + case "Where should we push the 'feature' branch?": + return 0, nil + case "What's next?": + if firstConfirmSubmission { + firstConfirmSubmission = false + return prompter.IndexFor(options, "Add metadata") + } + return prompter.IndexFor(options, "Submit") + default: + return 0, fmt.Errorf("unexpected select prompt: %s", message) + } + } + }, + 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 PullRequestTemplates\b`), + httpmock.StringResponse(`{ "data": { "repository": { "pullRequestTemplates": [] } } }`), + ) + 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 OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreate\b`), + httpmock.GraphQLMutation(` + { "data": { "createPullRequest": { "pullRequest": { + "id": "NEWPULLID", + "URL": "https://github.com/OWNER/REPO/pull/12" + } } } } + `, + func(inputs map[string]interface{}) { + assert.Equal(t, "TITLE", inputs["title"]) + assert.Equal(t, "BODY", inputs["body"]) + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } + if v, ok := inputs["userIds"]; ok { + t.Errorf("did not expect userIds: %v", v) + } + })) + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQLMutation(` + { "data": { "requestReviews": { + "clientMutationId": "" + } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) + assert.Equal(t, []interface{}{"COREID"}, inputs["teamIds"]) + assert.Equal(t, []interface{}{"MONAID"}, inputs["userIds"]) + assert.Equal(t, true, inputs["union"]) + })) + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedErrOut: "\nCreating pull request for feature into master in OWNER/REPO\n\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/cmd/pr/shared/completion.go b/pkg/cmd/pr/shared/completion.go index c1296be71..9f68c0bda 100644 --- a/pkg/cmd/pr/shared/completion.go +++ b/pkg/cmd/pr/shared/completion.go @@ -14,7 +14,10 @@ import ( func RequestableReviewersForCompletion(httpClient *http.Client, repo ghrepo.Interface) ([]string, error) { client := api.NewClientFromHTTP(api.NewCachedHTTPClient(httpClient, time.Minute*2)) - metadata, err := api.RepoMetadata(client, repo, api.RepoMetadataInput{Reviewers: true}) + metadata, err := api.RepoMetadata(client, repo, api.RepoMetadataInput{ + Reviewers: true, + TeamReviewers: true, + }) if err != nil { return nil, err } diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index af1fa871a..4b66bb0fa 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -181,6 +181,7 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface // Retrieve and process data for survey prompts based on the extra fields selected metadataInput := api.RepoMetadataInput{ Reviewers: isChosen("Reviewers"), + TeamReviewers: isChosen("Reviewers"), Assignees: isChosen("Assignees"), ActorAssignees: isChosen("Assignees") && state.ActorAssignees, Labels: isChosen("Labels"), From c9bc185209fd8f2f6624f1e61af9827ef842bfb6 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 30 Jul 2025 09:16:44 -0400 Subject: [PATCH 2/3] Test `gh pr create --reviewer` tab completion --- pkg/cmd/pr/create/create_test.go | 133 +++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 9af351830..eb39db0f9 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -2879,3 +2879,136 @@ func TestProjectsV1Deprecation(t *testing.T) { }) }) } + +func Test_requestableReviewersForCompletion(t *testing.T) { + tests := []struct { + name string + tty bool + expectedReviewers []string + httpStubs func(*httpmock.Registry, *testing.T) + }{ + { + name: "when users and teams are both available, both are listed", + expectedReviewers: []string{"MonaLisa\tMona Display Name", "OWNER/core", "OWNER/robots", "hubot"}, + 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 OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + }, + { + name: "when users are available but teams aren't, users are listed", + expectedReviewers: []string{"MonaLisa\tMona Display Name", "hubot"}, + 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 OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + }, + { + name: "when teams are available but users aren't, teams are listed", + expectedReviewers: []string{"OWNER/core", "OWNER/robots"}, + 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": [], + "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 } + } } } } + `)) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + if tt.httpStubs != nil { + tt.httpStubs(reg, t) + } + + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(tt.tty) + ios.SetStdinTTY(tt.tty) + ios.SetStderrTTY(tt.tty) + + opts := &CreateOptions{} + opts.IO = ios + opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + opts.Remotes = func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "origin", + Resolved: "base", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + } + + reviewers, err := requestableReviewersForCompletion(opts) + require.NoError(t, err) + require.Equal(t, tt.expectedReviewers, reviewers) + }) + } +} From bbc3d02cb394c882c9d3bd21bbcf20db509ce973 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 30 Jul 2025 09:25:33 -0400 Subject: [PATCH 3/3] Refactor tab completion test This commit moves the `gh pr create` tab completion test closer to the logic rather than the commands that use it. This should ensure that any command or flag that lists reviewers will present teams and users as expected. --- pkg/cmd/pr/create/create_test.go | 133 --------------------------- pkg/cmd/pr/shared/completion_test.go | 120 ++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 133 deletions(-) create mode 100644 pkg/cmd/pr/shared/completion_test.go diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index eb39db0f9..9af351830 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -2879,136 +2879,3 @@ func TestProjectsV1Deprecation(t *testing.T) { }) }) } - -func Test_requestableReviewersForCompletion(t *testing.T) { - tests := []struct { - name string - tty bool - expectedReviewers []string - httpStubs func(*httpmock.Registry, *testing.T) - }{ - { - name: "when users and teams are both available, both are listed", - expectedReviewers: []string{"MonaLisa\tMona Display Name", "OWNER/core", "OWNER/robots", "hubot"}, - 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 OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - }, - }, - { - name: "when users are available but teams aren't, users are listed", - expectedReviewers: []string{"MonaLisa\tMona Display Name", "hubot"}, - 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 OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - }, - }, - { - name: "when teams are available but users aren't, teams are listed", - expectedReviewers: []string{"OWNER/core", "OWNER/robots"}, - 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": [], - "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 } - } } } } - `)) - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - if tt.httpStubs != nil { - tt.httpStubs(reg, t) - } - - ios, _, _, _ := iostreams.Test() - ios.SetStdoutTTY(tt.tty) - ios.SetStdinTTY(tt.tty) - ios.SetStderrTTY(tt.tty) - - opts := &CreateOptions{} - opts.IO = ios - opts.HttpClient = func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - } - opts.Remotes = func() (context.Remotes, error) { - return context.Remotes{ - { - Remote: &git.Remote{ - Name: "origin", - Resolved: "base", - }, - Repo: ghrepo.New("OWNER", "REPO"), - }, - }, nil - } - - reviewers, err := requestableReviewersForCompletion(opts) - require.NoError(t, err) - require.Equal(t, tt.expectedReviewers, reviewers) - }) - } -} diff --git a/pkg/cmd/pr/shared/completion_test.go b/pkg/cmd/pr/shared/completion_test.go new file mode 100644 index 000000000..ca7c3ffa7 --- /dev/null +++ b/pkg/cmd/pr/shared/completion_test.go @@ -0,0 +1,120 @@ +package shared + +import ( + "net/http" + "testing" + + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/require" +) + +func TestRequestableReviewersForCompletion(t *testing.T) { + tests := []struct { + name string + expectedReviewers []string + httpStubs func(*httpmock.Registry, *testing.T) + }{ + { + name: "when users and teams are both available, both are listed", + expectedReviewers: []string{"MonaLisa\tMona Display Name", "OWNER/core", "OWNER/robots", "hubot"}, + 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 OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + }, + { + name: "when users are available but teams aren't, users are listed", + expectedReviewers: []string{"MonaLisa\tMona Display Name", "hubot"}, + 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 OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + }, + { + name: "when teams are available but users aren't, teams are listed", + expectedReviewers: []string{"OWNER/core", "OWNER/robots"}, + 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": [], + "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 } + } } } } + `)) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + if tt.httpStubs != nil { + tt.httpStubs(reg, t) + } + + reviewers, err := RequestableReviewersForCompletion(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO")) + require.NoError(t, err) + require.Equal(t, tt.expectedReviewers, reviewers) + }) + } +}