From 449920b40fc8a5015d1578ca10a301aa385a1914 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 22 Jul 2025 10:47:24 -0600 Subject: [PATCH 1/5] Add TeamReviewers flag to RepoMetadataInput Introduces a TeamReviewers boolean to RepoMetadataInput to control whether team reviewers are fetched. Updates RepoMetadata logic to only fetch teams if both Reviewers and TeamReviewers are true. Adds tests to verify correct behavior when TeamReviewers is false. --- api/queries_repo.go | 3 ++- api/queries_repo_test.go | 49 +++++++++++++++++++++++++++++++++++----- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 6552aa230..324200afd 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -919,6 +919,7 @@ type RepoMetadataInput struct { Assignees bool ActorAssignees bool Reviewers bool + TeamReviewers bool Labels bool ProjectsV1 bool ProjectsV2 bool @@ -964,7 +965,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput } } - if input.Reviewers { + if input.Reviewers && input.TeamReviewers { g.Go(func() error { teams, err := OrganizationTeams(client, repo) // TODO: better detection of non-org repos diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index c885b9968..2f7a335ca 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -42,12 +42,13 @@ func Test_RepoMetadata(t *testing.T) { repo, _ := ghrepo.FromFullName("OWNER/REPO") input := RepoMetadataInput{ - Assignees: true, - Reviewers: true, - Labels: true, - ProjectsV1: true, - ProjectsV2: true, - Milestones: true, + Assignees: true, + Reviewers: true, + TeamReviewers: true, + Labels: true, + ProjectsV1: true, + ProjectsV2: true, + Milestones: true, } http.Register( @@ -213,6 +214,42 @@ func Test_RepoMetadata(t *testing.T) { } } +func Test_RepoMetadataTeams(t *testing.T) { + // Test that RepoMetadata only fetches teams if the input specifies it + http := &httpmock.Registry{} + client := newTestClient(http) + repo, _ := ghrepo.FromFullName("OWNER/REPO") + input := RepoMetadataInput{ + Reviewers: true, + TeamReviewers: false, + } + + http.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "MonaLisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + + http.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "login": "monalisa" } } } + `)) + + _, err := RepoMetadata(client, repo, input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + require.NoError(t, err) +} + func Test_ProjectNamesToPaths(t *testing.T) { t.Run("when projectsV1 is supported, requests them", func(t *testing.T) { http := &httpmock.Registry{} From df317d4a052e5337fe0b6d1000758ca0d68f0e93 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 22 Jul 2025 11:00:47 -0600 Subject: [PATCH 2/5] FIX: conditionally fetching team reviewers Updated the logic for fetching team reviewers in PR edit and create flows. In `pr edit`, team reviewers are always fetched for consistency with existing behavior, with a note to potentially align with `pr create` logic in the future. In `pr create`, team reviewers are now only fetched if a reviewer contains a slash, aligning with behavior before the regression. --- pkg/cmd/pr/create/create_test.go | 8 -------- pkg/cmd/pr/shared/editable.go | 11 ++++++++++- pkg/cmd/pr/shared/params.go | 6 +++++- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 41fd10ab0..0760e72fe 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1314,14 +1314,6 @@ func Test_createRun(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [], - "pageInfo": { "hasNextPage": false } - } } } } - `)) reg.Register( httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), httpmock.GraphQLMutation(` diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index ffe1642da..9adbeb47c 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -429,7 +429,16 @@ func FieldsToEditSurvey(p EditPrompter, editable *Editable) error { func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) error { input := api.RepoMetadataInput{ - Reviewers: editable.Reviewers.Edited, + Reviewers: editable.Reviewers.Edited, + // TeamReviewers is always true if Reviewers is true because + // this is the existing `pr edit` behavior. This means + // always fetch teams. + // TODO: evaluate whether this can follow the same logic as + // `pr create` to conditionally fetch teams if a reviewer contains + // a slash. + // See https://github.com/cli/cli/blob/449920b40fc8a5015d1578ca10a301aa385a1914/pkg/cmd/pr/shared/params.go#L67-L71 + // See https://github.com/cli/cli/issues/11360 + TeamReviewers: editable.Reviewers.Edited, Assignees: editable.Assignees.Edited, ActorAssignees: editable.Assignees.ActorAssignees, Labels: editable.Labels.Edited, diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index d94f338c5..c3315aeae 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -3,6 +3,7 @@ package shared import ( "fmt" "net/url" + "slices" "strings" "github.com/cli/cli/v2/api" @@ -63,7 +64,10 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par // 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, + Reviewers: len(tb.Reviewers) > 0, + TeamReviewers: len(tb.Reviewers) > 0 && slices.ContainsFunc(tb.Reviewers, func(r string) bool { + return strings.ContainsRune(r, '/') + }), Assignees: len(tb.Assignees) > 0, ActorAssignees: tb.ActorAssignees, Labels: len(tb.Labels) > 0, From e5feda353f41b9be7b20af00bb80ba2032cc9bc7 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 22 Jul 2025 11:06:50 -0600 Subject: [PATCH 3/5] Refactor error assertion in Test_RepoMetadataTeams Replaces manual error check with require.NoError for consistency and improved readability in the test. --- api/queries_repo_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 2f7a335ca..35ddceb3d 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -243,10 +243,6 @@ func Test_RepoMetadataTeams(t *testing.T) { `)) _, err := RepoMetadata(client, repo, input) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - require.NoError(t, err) } From addee16531fb834e7459bc9da105b0f551e5f71d Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 23 Jul 2025 09:59:20 -0600 Subject: [PATCH 4/5] Refactor and improve RepoMetadata teams test Renamed the test to clarify its purpose and added an explicit exclusion for the OrganizationTeamList GraphQL query to ensure teams are only fetched when specified. This improves test accuracy and readability. --- api/queries_repo_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 35ddceb3d..ed6c7a7fd 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -214,14 +214,14 @@ func Test_RepoMetadata(t *testing.T) { } } -func Test_RepoMetadataTeams(t *testing.T) { - // Test that RepoMetadata only fetches teams if the input specifies it +// Test that RepoMetadata only fetches teams if the input specifies it +func Test_RepoMetadata_TeamsAreConditionallyFetched(t *testing.T) { http := &httpmock.Registry{} client := newTestClient(http) repo, _ := ghrepo.FromFullName("OWNER/REPO") input := RepoMetadataInput{ Reviewers: true, - TeamReviewers: false, + TeamReviewers: false, // Do not fetch teams } http.Register( @@ -242,6 +242,11 @@ func Test_RepoMetadataTeams(t *testing.T) { { "data": { "viewer": { "login": "monalisa" } } } `)) + http.Exclude( + t, + httpmock.GraphQL(`query OrganizationTeamList\b`), + ) + _, err := RepoMetadata(client, repo, input) require.NoError(t, err) } From 5a6cac3643df856d56d980b50cd3bb4352b64156 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 23 Jul 2025 10:15:30 -0600 Subject: [PATCH 5/5] Add tests for reviewer team handling in PR creation Added test cases to verify that teams are fetched when reviewers include teams and not fetched when only users are specified. This ensures correct behavior when requesting reviews from users and teams during pull request creation. --- pkg/cmd/pr/create/create_test.go | 119 +++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 0760e72fe..469428670 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1654,6 +1654,125 @@ func Test_createRun(t *testing.T) { }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", }, + { + name: "if reviewer contains any team, fetch teams", + 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(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "MonaLisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "login": "monalisa" } } } + `)) + 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 PullRequestCreateRequestReviews\b`), + httpmock.GraphQLMutation(` + { "data": { "requestReviews": { + "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, true, inputs["union"]) + })) + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedErrOut: "", + }, + { + name: "if reviewer does NOT contain any team, do NOT fetch teams", + 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"} + 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(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "MonaLisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "login": "monalisa" } } } + `)) + reg.Exclude( + t, + httpmock.GraphQL(`query OrganizationTeamList\b`), + ) + 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{}{"HUBOTID", "MONAID"}, inputs["userIds"]) + assert.NotEqual(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + 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) {