From 71f22d8843a8f5f862be19e7bd3f4cd53b82c118 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 13 May 2025 13:34:58 -0600 Subject: [PATCH 1/6] feat(pr edit): fetch assigned actors --- api/queries_pr.go | 1 + pkg/cmd/pr/edit/edit.go | 41 ++++++++++++++++++++++++++++++++++-- pkg/cmd/pr/edit/edit_test.go | 2 ++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 5b941bb42..525418a11 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -84,6 +84,7 @@ type PullRequest struct { } Assignees Assignees + AssignedActors AssignedActors Labels Labels ProjectCards ProjectCards ProjectItems ProjectItems diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 3c8d73ad3..23a75dd49 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -3,9 +3,11 @@ package edit import ( "fmt" "net/http" + "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" shared "github.com/cli/cli/v2/pkg/cmd/pr/shared" @@ -25,6 +27,8 @@ type EditOptions struct { Fetcher EditableOptionsFetcher EditorRetriever EditorRetriever Prompter shared.EditPrompter + Detector fd.Detector + BaseRepo func() (ghrepo.Interface, error) SelectorArg string Interactive bool @@ -69,6 +73,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Finder = shared.NewFinder(f) + opts.BaseRepo = f.BaseRepo if len(args) > 0 { opts.SelectorArg = args[0] @@ -192,8 +197,35 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman func editRun(opts *EditOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, - Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "assignees", "labels", "projectCards", "projectItems", "milestone"}, + Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"}, } + + if opts.Detector == nil { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost()) + } + + issueFeatures, err := opts.Detector.IssueFeatures() + if err != nil { + return err + } + + if issueFeatures.ActorIsAssignable { + findOptions.Fields = append(findOptions.Fields, "assignedActors") + } else { + findOptions.Fields = append(findOptions.Fields, "assignees") + } + pr, repo, err := opts.Finder.Find(findOptions) if err != nil { return err @@ -205,7 +237,12 @@ func editRun(opts *EditOptions) error { editable.Body.Default = pr.Body editable.Base.Default = pr.BaseRefName editable.Reviewers.Default = pr.ReviewRequests.Logins() - editable.Assignees.Default = pr.Assignees.Logins() + if issueFeatures.ActorIsAssignable { + // editable.Assignees.ActorAssignees = true + editable.Assignees.Default = pr.AssignedActors.Logins() + } else { + editable.Assignees.Default = pr.Assignees.Logins() + } editable.Labels.Default = pr.Labels.Names() editable.Projects.Default = append(pr.ProjectCards.ProjectNames(), pr.ProjectItems.ProjectTitles()...) projectItems := map[string]string{} diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index f45984c76..64231fa70 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -507,9 +507,11 @@ func Test_editRun(t *testing.T) { tt.httpStubs(reg) httpClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } + baseRepo := func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } tt.input.IO = ios tt.input.HttpClient = httpClient + tt.input.BaseRepo = baseRepo err := editRun(tt.input) assert.NoError(t, err) From d4832ba0159f6a564d55a93918cd038d9d8acab2 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 14 May 2025 11:04:19 -0600 Subject: [PATCH 2/6] feat(pr edit): fetch assignable actors --- api/queries_repo.go | 42 +++++++++------------- pkg/cmd/pr/edit/edit.go | 2 +- pkg/cmd/pr/edit/edit_test.go | 70 +++++++++++++++++++++++++++++++----- 3 files changed, 78 insertions(+), 36 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index e6286bb0b..8f2646159 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -918,39 +918,30 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput var g errgroup.Group if input.Assignees || input.Reviewers { - if input.ActorAssignees { g.Go(func() error { actors, err := RepoAssignableActors(client, repo) if err != nil { - err = fmt.Errorf("error fetching assignees: %w", err) + return fmt.Errorf("error fetching assignees: %w", err) } result.AssignableActors = actors - return err - }) - // If reviewers are also requested, we still need to fetch the assignable users - // since commands use assignable users for reviewers too, - // but Actors are not supported for requesting review (need to confirm this). - // TODO KW: find out how to do this in the above query so we don't need to - // run two potentially expensive queries. When we fetch Actors, this - // should still return Users - Users are distinguishable from other Actors - // by having a name property. Maybe we can use the Name to filter out - // non-user Actors and populate the users list for reviewers based on - // that. - // Note: this only matters for `gh pr` flows, which currently does not - // request actor assignees, so we probably won't hit this until - // `gh pr` reqeuests actor assignees. - if input.Reviewers { - g.Go(func() error { - users, err := RepoAssignableUsers(client, repo) - if err != nil { - err = fmt.Errorf("error fetching assignees: %w", err) + // Processing the bots out from the actors + // because requesting a reviewer leverages + // result.AssignableUsers. + // Note: this prevents us from needing to make another + // request to fetch assignable users when the user has + // selected to modify both reviewers and assignees. + var users []AssignableUser + for _, a := range actors { + if _, ok := a.(AssignableUser); !ok { + continue } - result.AssignableUsers = users - return err - }) - } + users = append(users, a.(AssignableUser)) + } + result.AssignableUsers = users + return nil + }) } else { // Not using Actors, fetch legacy assignable users. g.Go(func() error { @@ -962,7 +953,6 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput return err }) } - } if input.Reviewers { diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 23a75dd49..d484f74ed 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -238,7 +238,7 @@ func editRun(opts *EditOptions) error { editable.Base.Default = pr.BaseRefName editable.Reviewers.Default = pr.ReviewRequests.Logins() if issueFeatures.ActorIsAssignable { - // editable.Assignees.ActorAssignees = true + editable.Assignees.ActorAssignees = true editable.Assignees.Default = pr.AssignedActors.Logins() } else { editable.Assignees.Default = pr.Assignees.Logins() diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 64231fa70..5c2e65eba 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/cli/cli/v2/api" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" shared "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -392,6 +393,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { mockRepoMetadata(reg, false) mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestReviewersUpdate(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) @@ -448,6 +450,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { mockRepoMetadata(reg, true) mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -468,6 +471,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { mockRepoMetadata(reg, false) mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestReviewersUpdate(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) @@ -489,11 +493,50 @@ func Test_editRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { mockRepoMetadata(reg, true) mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, + { + name: "Legacy assignee user are fetched and updated on unsupported GitHub Hosts", + input: &EditOptions{ + Detector: &fd.DisabledDetectorMock{}, + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{ + URL: "https://github.com/OWNER/REPO/pull/123", + }, ghrepo.New("OWNER", "REPO")), + Interactive: false, + Editable: shared.Editable{ + Assignees: shared.EditableAssignees{ + EditableSlice: shared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Remove: []string{"octocat"}, + Edited: true, + }, + }, + }, + Fetcher: testFetcher{}, + }, + httpStubs: func(reg *httpmock.Registry) { + // Notice there is no call to mockReplaceActorsForAssignable() + // and no GraphQL call to RepositoryAssignableActors below. + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "MonaLisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + mockPullRequestUpdate(reg) + }, + stdout: "https://github.com/OWNER/REPO/pull/123\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -523,16 +566,16 @@ func Test_editRun(t *testing.T) { func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.GraphQL(`query RepositoryAssignableActors\b`), httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) + { "data": { "repository": { "suggestedActors": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, + { "login": "MonaLisa", "id": "MONAID", "__typename": "User" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) reg.Register( httpmock.GraphQL(`query RepositoryLabelList\b`), httpmock.StringResponse(` @@ -635,6 +678,15 @@ func mockPullRequestUpdate(reg *httpmock.Registry) { httpmock.StringResponse(`{}`)) } +func mockPullRequestUpdateActorAssignees(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, + func(inputs map[string]interface{}) {}), + ) +} + func mockPullRequestReviewersUpdate(reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation PullRequestUpdateRequestReviews\b`), From eace1e889ae41e7b17d8e3c3caf6922ad2c4633b Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 14 May 2025 20:26:53 -0600 Subject: [PATCH 3/6] feat(editable): update assigned actors to use display names - Refactored AssignedActors to return display names instead of logins. - Updated related functions to ensure consistency in display names. - Enhanced comments for clarity on display name logic and actor types. --- api/queries_issue.go | 49 ++++++++++++++++++++++++++++------- api/queries_repo.go | 28 +++++++++++++++++--- api/query_builder.go | 21 ++++++++++++++- pkg/cmd/issue/edit/edit.go | 2 +- pkg/cmd/pr/edit/edit.go | 2 +- pkg/cmd/pr/shared/editable.go | 2 +- 6 files changed, 87 insertions(+), 17 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 701b92039..bfdd223e5 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -93,19 +93,50 @@ func (a Assignees) Logins() []string { } type AssignedActors struct { - Edges []struct { - Node Actor - } + Nodes []Actor TotalCount int } -// TODO kw: Display names for actors with special display names. -func (a AssignedActors) Logins() []string { - logins := make([]string, len(a.Edges)) - for i, a := range a.Edges { - logins[i] = a.Node.Login +// DisplayNames returns a list of display names for the assigned actors. +func (a AssignedActors) DisplayNames() []string { + // These display names are used for populating the "default" assigned actors + // from the AssignedActors type. But, this is only one piece of the puzzle + // as later, other queries will fetch the full list of possible assignable + // actors from the repository, and the two lists will be reconciled. + // + // It's important that the display names are the same between the defaults + // (the values returned here) and the full list (the values returned by + // other repository queries). Any discrepancy would result in an + // "invalid default", which means an assigned actor will not be matched + // to an assignable actor and not presented as a "default" selection. + // Not being presented as a default would cause the actor to be potentially + // unassigned if the edits were submitted. + // + // To prevent this, we need shared logic to look up an actor's display name. + // However, our API types between assignedActors and the full list of + // assignableActors are different. So, as an attempt to maintain + // consistency we convert the assignedActors to the same types as the + // repository's assignableActors, treating the assignableActors DisplayName + // methods as the sources of truth. + // TODO KW: make this comment less of a wall of text if needed. + displayNames := make([]string, len(a.Nodes)) + for _, a := range a.Nodes { + if a.TypeName == "User" { + u := NewAssignableUser( + a.ID, + a.Login, + a.Name, + ) + displayNames = append(displayNames, u.DisplayName()) + } else if a.TypeName == "Bot" { + b := NewAssignableBot( + a.ID, + a.Login, + ) + displayNames = append(displayNames, b.DisplayName()) + } } - return logins + return displayNames } type Labels struct { diff --git a/api/queries_repo.go b/api/queries_repo.go index 8f2646159..dd9027ad4 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -146,11 +146,16 @@ type GitHubUser struct { Name string `json:"name"` } -// Actor is a superset of User and Bot -// At the time of writing, it does not support Name. +// Actor is a superset of User and Bot, among others. +// At the time of writing, some of these fields +// are not directly supported by the Actor type and +// instead are only available on the User or Bot types +// directly. type Actor struct { - ID string `json:"id"` - Login string `json:"login"` + ID string `json:"id"` + Login string `json:"login"` + Name string `json:"name"` + TypeName string `json:"__typename"` } // BranchRef is the branch name in a GitHub repository @@ -710,6 +715,11 @@ func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { found = true break } + if strings.EqualFold(assigneeLogin, a.DisplayName()) { + ids = append(ids, a.ID()) + found = true + break + } } if !found { @@ -1227,7 +1237,17 @@ type AssignableBot struct { login string } +func NewAssignableBot(id, login string) AssignableBot { + return AssignableBot{ + id: id, + login: login, + } +} + func (b AssignableBot) DisplayName() string { + if b.login == "copilot-swe-agent" { + return "Copilot (AI)" + } return b.Login() } diff --git a/api/query_builder.go b/api/query_builder.go index 0ef44a347..a2432673b 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -20,6 +20,25 @@ func shortenQuery(q string) string { return strings.Map(squeeze, q) } +var assignedActors = shortenQuery(` + assignedActors(first: 10) { + nodes { + ...on User { + id, + login, + name, + __typename + } + ...on Bot { + id, + login, + __typename + } + }, + totalCount + } +`) + var issueComments = shortenQuery(` comments(first: 100) { nodes { @@ -367,7 +386,7 @@ func IssueGraphQL(fields []string) string { case "assignees": q = append(q, `assignees(first:100){nodes{id,login,name},totalCount}`) case "assignedActors": - q = append(q, `assignedActors(first: 10){edges{node{...on Actor{login}}},totalCount}`) + q = append(q, assignedActors) case "labels": q = append(q, `labels(first:100){nodes{id,name,description,color},totalCount}`) case "projectCards": diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 5cb789543..f3c4e46ad 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -267,7 +267,7 @@ func editRun(opts *EditOptions) error { // We use Actors as the default assignees if Actors are assignable // on this GitHub host. if editable.Assignees.ActorAssignees { - editable.Assignees.Default = issue.AssignedActors.Logins() + editable.Assignees.Default = issue.AssignedActors.DisplayNames() } else { editable.Assignees.Default = issue.Assignees.Logins() } diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index d484f74ed..3002c8dbe 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -239,7 +239,7 @@ func editRun(opts *EditOptions) error { editable.Reviewers.Default = pr.ReviewRequests.Logins() if issueFeatures.ActorIsAssignable { editable.Assignees.ActorAssignees = true - editable.Assignees.Default = pr.AssignedActors.Logins() + editable.Assignees.Default = pr.AssignedActors.DisplayNames() } else { editable.Assignees.Default = pr.Assignees.Logins() } diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index d51405acd..cc11812ae 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -411,7 +411,7 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) } var actors []string for _, a := range metadata.AssignableActors { - actors = append(actors, a.Login()) + actors = append(actors, a.DisplayName()) } var teams []string for _, t := range metadata.Teams { From 9a5ea87d75bfb8ea2bb0c4d81f9237301cab2e3d Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 09:56:35 -0600 Subject: [PATCH 4/6] fix(issues): fix non-interactive assignee matching to logins&IDs --- api/queries_issue.go | 10 +++++++++- pkg/cmd/issue/edit/edit.go | 1 + pkg/cmd/pr/shared/editable.go | 7 ++++++- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index bfdd223e5..24e0b4f4c 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -97,6 +97,14 @@ type AssignedActors struct { TotalCount int } +func (a AssignedActors) Logins() []string { + logins := make([]string, len(a.Nodes)) + for i, a := range a.Nodes { + logins[i] = a.Login + } + return logins +} + // DisplayNames returns a list of display names for the assigned actors. func (a AssignedActors) DisplayNames() []string { // These display names are used for populating the "default" assigned actors @@ -119,7 +127,7 @@ func (a AssignedActors) DisplayNames() []string { // repository's assignableActors, treating the assignableActors DisplayName // methods as the sources of truth. // TODO KW: make this comment less of a wall of text if needed. - displayNames := make([]string, len(a.Nodes)) + var displayNames []string for _, a := range a.Nodes { if a.TypeName == "User" { u := NewAssignableUser( diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index f3c4e46ad..8fc8a2e41 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -268,6 +268,7 @@ func editRun(opts *EditOptions) error { // on this GitHub host. if editable.Assignees.ActorAssignees { editable.Assignees.Default = issue.AssignedActors.DisplayNames() + editable.Assignees.DefaultLogins = issue.AssignedActors.Logins() } else { editable.Assignees.Default = issue.Assignees.Logins() } diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index cc11812ae..cfe022c78 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -43,6 +43,7 @@ type EditableSlice struct { type EditableAssignees struct { EditableSlice ActorAssignees bool + DefaultLogins []string } // ProjectsV2 mutations require a mapping of an item ID to a project ID. @@ -115,7 +116,11 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str if len(e.Assignees.Add) != 0 || len(e.Assignees.Remove) != 0 { meReplacer := NewMeReplacer(client, repo.RepoHost()) s := set.NewStringSet() - s.AddValues(e.Assignees.Default) + if e.Assignees.ActorAssignees { + s.AddValues(e.Assignees.DefaultLogins) + } else { + s.AddValues(e.Assignees.Default) + } add, err := meReplacer.ReplaceSlice(e.Assignees.Add) if err != nil { return nil, err From 52b6ebff9cf1f4c9a5570cc0d105447cfc7c938c Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 10:36:57 -0600 Subject: [PATCH 5/6] doc(pr edit): Add comments describing the use of DefaultLogins --- pkg/cmd/pr/shared/editable.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index cfe022c78..77bab8440 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -43,7 +43,7 @@ type EditableSlice struct { type EditableAssignees struct { EditableSlice ActorAssignees bool - DefaultLogins []string + DefaultLogins []string // For disambiguating actors from display names } // ProjectsV2 mutations require a mapping of an item ID to a project ID. @@ -113,9 +113,20 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str if !e.Assignees.Edited { return nil, nil } + + // If assignees came in from command line flags, we need to + // curate the final list of assignees from the default list. if len(e.Assignees.Add) != 0 || len(e.Assignees.Remove) != 0 { meReplacer := NewMeReplacer(client, repo.RepoHost()) s := set.NewStringSet() + // This check below is required because in a non-interactive flow, + // the user gives us a login and not the DisplayName, and when + // we have actor assignees e.Assignees.Default will contain + // DisplayNames and not logins (this is to accommodate special actor + // display names in the interactive flow). + // So, we need to add the default logins here instead of the DisplayNames. + // Otherwise, the value the user provided won't be found in the + // set to be added or removed, causing unexpected behavior. if e.Assignees.ActorAssignees { s.AddValues(e.Assignees.DefaultLogins) } else { From a22a1bbde475be05875e3a2ac2524e2ae2a51bce Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 09:20:58 -0600 Subject: [PATCH 6/6] test(editable): prompts use assignee display names --- pkg/cmd/issue/edit/edit_test.go | 147 +++++++++++++++++++++++++++++++- 1 file changed, 146 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 0df6d5e9f..4840cbf7a 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -620,6 +620,123 @@ func Test_editRun(t *testing.T) { }, stdout: "https://github.com/OWNER/REPO/issue/123\n", }, + { + name: "interactive prompts with actor assignee display names when actors available", + input: &EditOptions{ + IssueNumbers: []int{123}, + Interactive: true, + FieldsToEditSurvey: func(p prShared.EditPrompter, eo *prShared.Editable) error { + eo.Assignees.Edited = true + return nil + }, + EditFieldsSurvey: func(p prShared.EditPrompter, eo *prShared.Editable, _ string) error { + // Checking that the display name is being used in the prompt. + require.Equal(t, eo.Assignees.Default, []string{"hubot", "MonaLisa (Mona Display Name)"}) + + // Mocking a selection of only MonaLisa in the prompt. + eo.Assignees.Value = []string{"MonaLisa (Mona Display Name)"} + return nil + }, + FetchOptions: prShared.FetchOptions, + DetermineEditor: func() (string, error) { return "vim", nil }, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + mockIsssueNumberGetWithAssignedActors(t, reg, 123) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableActors\b`), + httpmock.StringResponse(` + { "data": { "repository": { "suggestedActors": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + mockIssueUpdate(t, reg) + reg.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, + func(inputs map[string]interface{}) { + // Checking that despite the display name being returned + // from the EditFieldsSurvey, the ID is still + // used in the mutation. + require.Contains(t, inputs["actorIds"], "MONAID") + }), + ) + }, + stdout: "https://github.com/OWNER/REPO/issue/123\n", + }, + { + name: "interactive prompts with user assignee logins when actors unavailable", + input: &EditOptions{ + IssueNumbers: []int{123}, + Interactive: true, + FieldsToEditSurvey: func(p prShared.EditPrompter, eo *prShared.Editable) error { + eo.Assignees.Edited = true + return nil + }, + EditFieldsSurvey: func(p prShared.EditPrompter, eo *prShared.Editable, _ string) error { + // Checking that only the login is used in the prompt (no display name) + require.Equal(t, eo.Assignees.Default, []string{"hubot", "MonaLisa"}) + + // Mocking a selection of only MonaLisa in the prompt. + eo.Assignees.Value = []string{"MonaLisa"} + return nil + }, + FetchOptions: prShared.FetchOptions, + DetermineEditor: func() (string, error) { return "vim", nil }, + Detector: &fd.DisabledDetectorMock{}, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query IssueByNumber\b`), + httpmock.StringResponse(fmt.Sprintf(` + { "data": { "repository": { "hasIssuesEnabled": true, "issue": { + "id": "%[1]d", + "number": %[1]d, + "url": "https://github.com/OWNER/REPO/issue/123", + "assignees": { + "nodes": [ + { + "id": "HUBOTID", + "login": "hubot", + "name": "" + }, + { + "id": "MONAID", + "login": "MonaLisa", + "name": "Mona Display Name" + } + ], + "totalCount": 2 + } + } } } }`, 123)), + ) + 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(`mutation IssueUpdate\b`), + httpmock.GraphQLMutation(` + { "data": { "updateIssue": { "__typename": "" } } }`, + func(inputs map[string]interface{}) { + // Checking that we still assigned the expected ID. + require.Contains(t, inputs["assigneeIds"], "MONAID") + }), + ) + }, + stdout: "https://github.com/OWNER/REPO/issue/123\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -678,6 +795,34 @@ func mockIssueNumberGet(_ *testing.T, reg *httpmock.Registry, number int) { ) } +func mockIsssueNumberGetWithAssignedActors(_ *testing.T, reg *httpmock.Registry, number int) { + reg.Register( + httpmock.GraphQL(`query IssueByNumber\b`), + httpmock.StringResponse(fmt.Sprintf(` + { "data": { "repository": { "hasIssuesEnabled": true, "issue": { + "id": "%[1]d", + "number": %[1]d, + "url": "https://github.com/OWNER/REPO/issue/%[1]d", + "assignedActors": { + "nodes": [ + { + "id": "HUBOTID", + "login": "hubot", + "__typename": "Bot" + }, + { + "id": "MONAID", + "login": "MonaLisa", + "name": "Mona Display Name", + "__typename": "User" + } + ], + "totalCount": 2 + } + } } } }`, number)), + ) +} + func mockIssueProjectItemsGet(_ *testing.T, reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`query IssueProjectItems\b`), @@ -699,7 +844,7 @@ func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) { { "data": { "repository": { "suggestedActors": { "nodes": [ { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, - { "login": "MonaLisa", "id": "MONAID", "__typename": "User" } + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } ], "pageInfo": { "hasNextPage": false } } } } }