From 33783748f3d77ec149cf5b09fd54fe0fd45b2c0d Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Mon, 23 Mar 2026 17:36:52 -0600 Subject: [PATCH] review: address code review feedback - Fix tests: assert logins (not display names) in actorLogins - Remove dead ReplaceActorsForAssignableByID (no callers) - Extract shared AssigneeSearchFunc to pkg/cmd/pr/shared/editable.go - Remove duplicate assigneeSearchFunc from pr/edit and issue/edit Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/queries_pr.go | 39 +++++----------------------- pkg/cmd/issue/edit/edit.go | 36 +------------------------- pkg/cmd/issue/edit/edit_test.go | 10 +++---- pkg/cmd/pr/edit/edit.go | 46 +-------------------------------- pkg/cmd/pr/edit/edit_test.go | 13 +++------- pkg/cmd/pr/shared/editable.go | 32 +++++++++++++++++++++++ 6 files changed, 47 insertions(+), 129 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 8b0d6b179..01846a9e6 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -602,6 +602,12 @@ func ReplaceActorsForAssignableByLogin(client *Client, repo ghrepo.Interface, as actorLogins[i] = githubv4.String(l) } + var mutation struct { + ReplaceActorsForAssignable struct { + TypeName string `graphql:"__typename"` + } `graphql:"replaceActorsForAssignable(input: $input)"` + } + variables := map[string]interface{}{ "input": ReplaceActorsForAssignableInput{ AssignableID: githubv4.ID(assignableID), @@ -609,39 +615,6 @@ func ReplaceActorsForAssignableByLogin(client *Client, repo ghrepo.Interface, as }, } - return replaceActorsForAssignable(client, repo, variables) -} - -// ReplaceActorsForAssignableByID calls the replaceActorsForAssignable mutation -// using actor node IDs. Used for GHES and edit flows that resolve IDs from search results. -func ReplaceActorsForAssignableByID(client *Client, repo ghrepo.Interface, assignableID string, actorIDs []string) error { - type ReplaceActorsForAssignableInput struct { - AssignableID githubv4.ID `json:"assignableId"` - ActorIDs []githubv4.ID `json:"actorIds"` - } - - ids := make([]githubv4.ID, len(actorIDs)) - for i, id := range actorIDs { - ids[i] = githubv4.ID(id) - } - - variables := map[string]interface{}{ - "input": ReplaceActorsForAssignableInput{ - AssignableID: githubv4.ID(assignableID), - ActorIDs: ids, - }, - } - - return replaceActorsForAssignable(client, repo, variables) -} - -func replaceActorsForAssignable(client *Client, repo ghrepo.Interface, variables map[string]interface{}) error { - var mutation struct { - ReplaceActorsForAssignable struct { - TypeName string `graphql:"__typename"` - } `graphql:"replaceActorsForAssignable(input: $input)"` - } - return client.Mutate(repo.RepoHost(), "ReplaceActorsForAssignable", &mutation, variables) } diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index c0cd0c89c..2fd632b5e 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -12,7 +12,6 @@ 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" shared "github.com/cli/cli/v2/pkg/cmd/issue/shared" prShared "github.com/cli/cli/v2/pkg/cmd/pr/shared" @@ -253,7 +252,7 @@ func editRun(opts *EditOptions) error { // Wire up search function for assignees when ActorIsAssignable is available. // Interactive mode only supports a single issue, so we use its ID for the search query. if issueFeatures.ActorIsAssignable && opts.Interactive && len(issues) == 1 { - editable.AssigneeSearchFunc = assigneeSearchFunc(apiClient, baseRepo, issues[0].ID) + editable.AssigneeSearchFunc = prShared.AssigneeSearchFunc(apiClient, baseRepo, issues[0].ID) } opts.IO.StartProgressIndicatorWithLabel("Fetching repository information") @@ -359,36 +358,3 @@ func editRun(opts *EditOptions) error { return nil } - -func assigneeSearchFunc(apiClient *api.Client, repo ghrepo.Interface, assignableID string) func(string) prompter.MultiSelectSearchResult { - return func(input string) prompter.MultiSelectSearchResult { - actors, availableAssigneesCount, err := api.SuggestedAssignableActors( - apiClient, - repo, - assignableID, - input) - if err != nil { - return prompter.MultiSelectSearchResult{Err: err} - } - - logins := make([]string, 0, len(actors)) - displayNames := make([]string, 0, len(actors)) - - for _, a := range actors { - if a.Login() == "" { - continue - } - logins = append(logins, a.Login()) - if a.DisplayName() != "" { - displayNames = append(displayNames, a.DisplayName()) - } else { - displayNames = append(displayNames, a.Login()) - } - } - return prompter.MultiSelectSearchResult{ - Keys: logins, - Labels: displayNames, - MoreResults: availableAssigneesCount, - } - } -} diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index d4dcce399..ef496d1de 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -629,8 +629,9 @@ func Test_editRun(t *testing.T) { require.Equal(t, []string{"hubot"}, eo.Assignees.Default) require.Equal(t, []string{"hubot"}, eo.Assignees.DefaultLogins) - // Adding MonaLisa as PR assignee, should preserve hubot. - eo.Assignees.Value = []string{"hubot", "MonaLisa (Mona Display Name)"} + // Adding MonaLisa as issue assignee, should preserve hubot. + // MultiSelectWithSearch returns Keys (logins), not display names. + eo.Assignees.Value = []string{"hubot", "MonaLisa"} return nil }, FetchOptions: prShared.FetchOptions, @@ -644,10 +645,7 @@ func Test_editRun(t *testing.T) { httpmock.GraphQLMutation(` { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, func(inputs map[string]interface{}) { - // Checking that despite the display name being returned - // from the EditFieldsSurvey, the login is still - // used in the mutation. - require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "MonaLisa (Mona Display Name)"}) + require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "MonaLisa"}) }), ) }, diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index ea0712491..0937c7533 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -303,7 +303,7 @@ func editRun(opts *EditOptions) error { // to legacy reviewer/assignee fetching. // TODO actorIsAssignableCleanup if issueFeatures.ActorIsAssignable { - editable.AssigneeSearchFunc = assigneeSearchFunc(apiClient, repo, pr.ID) + editable.AssigneeSearchFunc = shared.AssigneeSearchFunc(apiClient, repo, pr.ID) editable.ReviewerSearchFunc = reviewerSearchFunc(apiClient, repo, &editable, pr.ID) } @@ -346,50 +346,6 @@ func editRun(opts *EditOptions) error { return nil } -// assigneeSearchFunc is intended to be an arg for MultiSelectWithSearch -// to return potential assignee actors. -func assigneeSearchFunc(apiClient *api.Client, repo ghrepo.Interface, assignableID string) func(string) prompter.MultiSelectSearchResult { - searchFunc := func(input string) prompter.MultiSelectSearchResult { - actors, availableAssigneesCount, err := api.SuggestedAssignableActors( - apiClient, - repo, - assignableID, - input) - if err != nil { - return prompter.MultiSelectSearchResult{ - Keys: nil, - Labels: nil, - MoreResults: 0, - Err: err, - } - } - - logins := make([]string, 0, len(actors)) - displayNames := make([]string, 0, len(actors)) - - for _, a := range actors { - if a.Login() != "" { - logins = append(logins, a.Login()) - } else { - continue - } - - if a.DisplayName() != "" { - displayNames = append(displayNames, a.DisplayName()) - } else { - displayNames = append(displayNames, a.Login()) - } - } - return prompter.MultiSelectSearchResult{ - Keys: logins, - Labels: displayNames, - MoreResults: availableAssigneesCount, - Err: nil, - } - } - return searchFunc -} - // reviewerSearchFunc is intended to be an arg for MultiSelectWithSearch // to return potential reviewer candidates (users, bots, and teams). // It also updates the editable's metadata for later ID resolution. diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 2582c500b..01acb7e0f 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -912,12 +912,8 @@ func Test_editRun(t *testing.T) { require.Equal(t, []string{"hubot"}, e.Assignees.DefaultLogins) // Adding monalisa as PR assignee, should preserve hubot. - e.Assignees.Value = []string{"hubot", "monalisa (Mona Display Name)"} - // Populate metadata to simulate what searchFunc would do during prompting - e.Metadata.AssignableActors = []api.AssignableActor{ - api.NewAssignableBot("HUBOTID", "hubot"), - api.NewAssignableUser("MONAID", "monalisa", "Mona Display Name"), - } + // MultiSelectWithSearch returns Keys (logins), not display names. + e.Assignees.Value = []string{"hubot", "monalisa"} return nil }, }, @@ -931,10 +927,7 @@ func Test_editRun(t *testing.T) { httpmock.GraphQLMutation(` { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, func(inputs map[string]interface{}) { - // Checking that despite the display name being returned - // from the EditFieldsSurvey, the login is still - // used in the mutation. - require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "monalisa (Mona Display Name)"}) + require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "monalisa"}) }), ) }, diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 115d765f1..566c9939f 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -611,3 +611,35 @@ func milestoneSurvey(p EditPrompter, title string, opts []string) (result string result = opts[selected] return } + +// AssigneeSearchFunc returns a search function for MultiSelectWithSearch that +// dynamically fetches assignable actors for the given assignable (Issue/PR) node ID. +func AssigneeSearchFunc(apiClient *api.Client, repo ghrepo.Interface, assignableID string) func(string) prompter.MultiSelectSearchResult { + return func(input string) prompter.MultiSelectSearchResult { + actors, availableAssigneesCount, err := api.SuggestedAssignableActors( + apiClient, repo, assignableID, input) + if err != nil { + return prompter.MultiSelectSearchResult{Err: err} + } + + logins := make([]string, 0, len(actors)) + displayNames := make([]string, 0, len(actors)) + + for _, a := range actors { + if a.Login() == "" { + continue + } + logins = append(logins, a.Login()) + if a.DisplayName() != "" { + displayNames = append(displayNames, a.DisplayName()) + } else { + displayNames = append(displayNames, a.Login()) + } + } + return prompter.MultiSelectSearchResult{ + Keys: logins, + Labels: displayNames, + MoreResults: availableAssigneesCount, + } + } +}