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..ed6c7a7fd 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,43 @@ func Test_RepoMetadata(t *testing.T) { } } +// 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, // Do not fetch teams + } + + 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" } } } + `)) + + http.Exclude( + t, + httpmock.GraphQL(`query OrganizationTeamList\b`), + ) + + _, err := RepoMetadata(client, repo, input) + 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{} diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 41fd10ab0..469428670 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(` @@ -1662,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) { 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,