Skip to content

Commit 93c4340

Browse files
BagToadBagToad
authored
Merge pull request cli#12627 from cli/kw/pr-create-multi-select-with-search-ccr
`gh pr create`: login-based reviewer requests and search-based interactive selection
2 parents c0cd4c1 + 7382b86 commit 93c4340

11 files changed

Lines changed: 1421 additions & 831 deletions

File tree

‎api/queries_pr.go‎

Lines changed: 34 additions & 467 deletions
Large diffs are not rendered by default.

‎api/queries_pr_review.go‎

Lines changed: 619 additions & 27 deletions
Large diffs are not rendered by default.

‎api/queries_pr_test.go‎

Lines changed: 231 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -160,18 +160,16 @@ func mockReviewerResponse(suggestions, collabs, teams, totalCollabs, totalTeams
160160

161161
return fmt.Sprintf(`{
162162
"data": {
163-
"node": {"suggestedReviewerActors": {"nodes": [%s]}},
163+
"node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [%s]}},
164164
"repository": {
165+
"owner": {"__typename": "Organization", "teams": {"nodes": [%s]}, "teamsTotalCount": {"totalCount": %d}},
165166
"collaborators": {"nodes": [%s]},
166167
"collaboratorsTotalCount": {"totalCount": %d}
167-
},
168-
"organization": {
169-
"teams": {"nodes": [%s]},
170-
"teamsTotalCount": {"totalCount": %d}
171168
}
172169
}
173-
}`, strings.Join(suggestionNodes, ","), strings.Join(collabNodes, ","), totalCollabs,
174-
strings.Join(teamNodes, ","), totalTeams)
170+
}`, strings.Join(suggestionNodes, ","),
171+
strings.Join(teamNodes, ","), totalTeams,
172+
strings.Join(collabNodes, ","), totalCollabs)
175173
}
176174

177175
func TestSuggestedReviewerActors(t *testing.T) {
@@ -235,18 +233,15 @@ func TestSuggestedReviewerActors(t *testing.T) {
235233
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
236234
httpmock.StringResponse(`{
237235
"data": {
238-
"node": {"suggestedReviewerActors": {"nodes": [
236+
"node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [
239237
{"isAuthor": true, "reviewer": {"__typename": "User", "login": "author", "name": "Author"}},
240238
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}},
241239
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s2", "name": "S2"}}
242240
]}},
243241
"repository": {
242+
"owner": {"__typename": "Organization", "teams": {"nodes": [{"slug": "team1"}]}, "teamsTotalCount": {"totalCount": 3}},
244243
"collaborators": {"nodes": [{"login": "c1", "name": "C1"}]},
245244
"collaboratorsTotalCount": {"totalCount": 5}
246-
},
247-
"organization": {
248-
"teams": {"nodes": [{"slug": "team1"}]},
249-
"teamsTotalCount": {"totalCount": 3}
250245
}
251246
}
252247
}`))
@@ -255,6 +250,30 @@ func TestSuggestedReviewerActors(t *testing.T) {
255250
expectedLogins: []string{"s1", "s2", "c1", "OWNER/team1"},
256251
expectedMore: 8,
257252
},
253+
{
254+
name: "author excluded from collaborators",
255+
httpStubs: func(reg *httpmock.Registry) {
256+
reg.Register(
257+
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
258+
httpmock.StringResponse(`{
259+
"data": {
260+
"node": {"author": {"login": "theauthor"}, "suggestedReviewerActors": {"nodes": [
261+
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}}
262+
]}},
263+
"repository": {
264+
"collaborators": {"nodes": [
265+
{"login": "theauthor", "name": "The Author"},
266+
{"login": "c1", "name": "C1"}
267+
]},
268+
"collaboratorsTotalCount": {"totalCount": 5}
269+
}
270+
}
271+
}`))
272+
},
273+
expectedCount: 2,
274+
expectedLogins: []string{"s1", "c1"},
275+
expectedMore: 5,
276+
},
258277
{
259278
name: "deduplication across sources",
260279
httpStubs: func(reg *httpmock.Registry) {
@@ -263,19 +282,16 @@ func TestSuggestedReviewerActors(t *testing.T) {
263282
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
264283
httpmock.StringResponse(`{
265284
"data": {
266-
"node": {"suggestedReviewerActors": {"nodes": [
285+
"node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [
267286
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "shareduser", "name": "Shared"}}
268287
]}},
269288
"repository": {
289+
"owner": {"__typename": "Organization", "teams": {"nodes": [{"slug": "team1"}]}, "teamsTotalCount": {"totalCount": 5}},
270290
"collaborators": {"nodes": [
271291
{"login": "shareduser", "name": "Shared"},
272292
{"login": "c1", "name": "C1"}
273293
]},
274294
"collaboratorsTotalCount": {"totalCount": 10}
275-
},
276-
"organization": {
277-
"teams": {"nodes": [{"slug": "team1"}]},
278-
"teamsTotalCount": {"totalCount": 5}
279295
}
280296
}
281297
}`))
@@ -291,16 +307,15 @@ func TestSuggestedReviewerActors(t *testing.T) {
291307
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
292308
httpmock.StringResponse(`{
293309
"data": {
294-
"node": {"suggestedReviewerActors": {"nodes": [
310+
"node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [
295311
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}}
296312
]}},
297313
"repository": {
314+
"owner": {"__typename": "User"},
298315
"collaborators": {"nodes": [{"login": "c1", "name": "C1"}]},
299316
"collaboratorsTotalCount": {"totalCount": 3}
300-
},
301-
"organization": null
302-
},
303-
"errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}]
317+
}
318+
}
304319
}`))
305320
},
306321
expectedCount: 2,
@@ -314,17 +329,14 @@ func TestSuggestedReviewerActors(t *testing.T) {
314329
httpmock.GraphQL(`query SuggestedReviewerActors\b`),
315330
httpmock.StringResponse(`{
316331
"data": {
317-
"node": {"suggestedReviewerActors": {"nodes": [
332+
"node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [
318333
{"isAuthor": false, "reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}},
319334
{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}}
320335
]}},
321336
"repository": {
337+
"owner": {"__typename": "Organization", "teams": {"nodes": []}, "teamsTotalCount": {"totalCount": 0}},
322338
"collaborators": {"nodes": []},
323339
"collaboratorsTotalCount": {"totalCount": 5}
324-
},
325-
"organization": {
326-
"teams": {"nodes": []},
327-
"teamsTotalCount": {"totalCount": 0}
328340
}
329341
}
330342
}`))
@@ -362,3 +374,195 @@ func TestSuggestedReviewerActors(t *testing.T) {
362374
})
363375
}
364376
}
377+
378+
// mockReviewerResponseForRepo generates a GraphQL response for SuggestedReviewerActorsForRepo tests.
379+
// It creates collaborators (c1, c2...) and teams (team1, team2...).
380+
func mockReviewerResponseForRepo(collabs, teams, totalCollabs, totalTeams int) string {
381+
return mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalTeams, false)
382+
}
383+
384+
// mockReviewerResponseForRepoWithCopilot generates a GraphQL response for SuggestedReviewerActorsForRepo tests.
385+
// If copilotAvailable is true, includes Copilot in the first open PR's suggested reviewers.
386+
func mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalTeams int, copilotAvailable bool) string {
387+
var collabNodes, teamNodes []string
388+
389+
for i := 1; i <= collabs; i++ {
390+
collabNodes = append(collabNodes,
391+
fmt.Sprintf(`{"login": "c%d", "name": "C%d"}`, i, i))
392+
}
393+
for i := 1; i <= teams; i++ {
394+
teamNodes = append(teamNodes,
395+
fmt.Sprintf(`{"slug": "team%d"}`, i))
396+
}
397+
398+
pullRequestsJSON := `"pullRequests": {"nodes": []}`
399+
if copilotAvailable {
400+
pullRequestsJSON = `"pullRequests": {"nodes": [{"suggestedReviewerActors": {"nodes": [{"reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}}]}}]}`
401+
}
402+
403+
return fmt.Sprintf(`{
404+
"data": {
405+
"viewer": {"login": "testuser"},
406+
"repository": {
407+
%s,
408+
"owner": {"__typename": "Organization", "teams": {"nodes": [%s]}, "teamsTotalCount": {"totalCount": %d}},
409+
"collaborators": {"nodes": [%s]},
410+
"collaboratorsTotalCount": {"totalCount": %d}
411+
}
412+
}
413+
}`, pullRequestsJSON,
414+
strings.Join(teamNodes, ","), totalTeams,
415+
strings.Join(collabNodes, ","), totalCollabs)
416+
}
417+
418+
func TestSuggestedReviewerActorsForRepo(t *testing.T) {
419+
tests := []struct {
420+
name string
421+
httpStubs func(*httpmock.Registry)
422+
expectedCount int
423+
expectedLogins []string
424+
expectedMore int
425+
expectError bool
426+
}{
427+
{
428+
name: "both sources plentiful - 5 each from cascading quota",
429+
httpStubs: func(reg *httpmock.Registry) {
430+
reg.Register(
431+
httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`),
432+
httpmock.StringResponse(mockReviewerResponseForRepo(6, 6, 20, 10)))
433+
},
434+
expectedCount: 10,
435+
expectedLogins: []string{"c1", "c2", "c3", "c4", "c5", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"},
436+
expectedMore: 30,
437+
},
438+
{
439+
name: "few collaborators - teams fill gap",
440+
httpStubs: func(reg *httpmock.Registry) {
441+
reg.Register(
442+
httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`),
443+
httpmock.StringResponse(mockReviewerResponseForRepo(2, 10, 2, 15)))
444+
},
445+
expectedCount: 10,
446+
expectedLogins: []string{"c1", "c2", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8"},
447+
expectedMore: 17,
448+
},
449+
{
450+
name: "no collaborators - teams only",
451+
httpStubs: func(reg *httpmock.Registry) {
452+
reg.Register(
453+
httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`),
454+
httpmock.StringResponse(mockReviewerResponseForRepo(0, 10, 0, 20)))
455+
},
456+
expectedCount: 10,
457+
expectedLogins: []string{"OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"},
458+
expectedMore: 20,
459+
},
460+
{
461+
name: "personal repo - no organization teams",
462+
httpStubs: func(reg *httpmock.Registry) {
463+
reg.Register(
464+
httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`),
465+
httpmock.StringResponse(`{
466+
"data": {
467+
"repository": {
468+
"pullRequests": {"nodes": []},
469+
"owner": {"__typename": "User"},
470+
"collaborators": {"nodes": [{"login": "c1", "name": "C1"}]},
471+
"collaboratorsTotalCount": {"totalCount": 3}
472+
}
473+
}
474+
}`))
475+
},
476+
expectedCount: 1,
477+
expectedLogins: []string{"c1"},
478+
expectedMore: 3,
479+
},
480+
{
481+
name: "empty repo",
482+
httpStubs: func(reg *httpmock.Registry) {
483+
reg.Register(
484+
httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`),
485+
httpmock.StringResponse(mockReviewerResponseForRepo(0, 0, 0, 0)))
486+
},
487+
expectedCount: 0,
488+
expectedLogins: []string{},
489+
expectedMore: 0,
490+
},
491+
{
492+
name: "copilot available - prepended to candidates",
493+
httpStubs: func(reg *httpmock.Registry) {
494+
reg.Register(
495+
httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`),
496+
httpmock.StringResponse(mockReviewerResponseForRepoWithCopilot(3, 2, 5, 5, true)))
497+
},
498+
expectedCount: 6,
499+
expectedLogins: []string{"copilot-pull-request-reviewer", "c1", "c2", "c3", "OWNER/team1", "OWNER/team2"},
500+
expectedMore: 10,
501+
},
502+
{
503+
name: "copilot not available - not included",
504+
httpStubs: func(reg *httpmock.Registry) {
505+
reg.Register(
506+
httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`),
507+
httpmock.StringResponse(mockReviewerResponseForRepoWithCopilot(3, 2, 5, 5, false)))
508+
},
509+
expectedCount: 5,
510+
expectedLogins: []string{"c1", "c2", "c3", "OWNER/team1", "OWNER/team2"},
511+
expectedMore: 10,
512+
},
513+
{
514+
name: "viewer excluded from collaborators",
515+
httpStubs: func(reg *httpmock.Registry) {
516+
// c1 matches the viewer login "testuser" won't be in this fixture,
517+
// but we can craft a response where the viewer login matches a collaborator.
518+
reg.Register(
519+
httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`),
520+
httpmock.StringResponse(`{
521+
"data": {
522+
"viewer": {"login": "c2"},
523+
"repository": {
524+
"pullRequests": {"nodes": []},
525+
"owner": {"__typename": "Organization", "teams": {"nodes": []}, "teamsTotalCount": {"totalCount": 0}},
526+
"collaborators": {"nodes": [
527+
{"login": "c1", "name": "C1"},
528+
{"login": "c2", "name": "C2"},
529+
{"login": "c3", "name": "C3"}
530+
]},
531+
"collaboratorsTotalCount": {"totalCount": 3}
532+
}
533+
}
534+
}`))
535+
},
536+
expectedCount: 2,
537+
expectedLogins: []string{"c1", "c3"},
538+
expectedMore: 3,
539+
},
540+
}
541+
542+
for _, tt := range tests {
543+
t.Run(tt.name, func(t *testing.T) {
544+
reg := &httpmock.Registry{}
545+
if tt.httpStubs != nil {
546+
tt.httpStubs(reg)
547+
}
548+
549+
client := newTestClient(reg)
550+
repo, _ := ghrepo.FromFullName("OWNER/REPO")
551+
552+
candidates, moreResults, err := SuggestedReviewerActorsForRepo(client, repo, "")
553+
if tt.expectError {
554+
assert.Error(t, err)
555+
return
556+
}
557+
assert.NoError(t, err)
558+
assert.Equal(t, tt.expectedCount, len(candidates), "candidate count mismatch")
559+
assert.Equal(t, tt.expectedMore, moreResults, "moreResults mismatch")
560+
561+
logins := make([]string, len(candidates))
562+
for i, c := range candidates {
563+
logins[i] = c.Login()
564+
}
565+
assert.Equal(t, tt.expectedLogins, logins)
566+
})
567+
}
568+
}

‎pkg/cmd/issue/create/create.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ func createRun(opts *CreateOptions) (err error) {
313313
Repo: baseRepo,
314314
State: &tb,
315315
}
316-
err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support)
316+
err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support, nil)
317317
if err != nil {
318318
return
319319
}

0 commit comments

Comments
 (0)