diff --git a/api/queries_issue.go b/api/queries_issue.go index 701b92039..24e0b4f4c 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -93,21 +93,60 @@ 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 + 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 + // 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. + var displayNames []string + 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 displayNames +} + type Labels struct { Nodes []IssueLabel TotalCount int diff --git a/api/queries_repo.go b/api/queries_repo.go index fd5ae4679..f72277fb2 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 { @@ -1223,7 +1233,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..8fc8a2e41 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -267,7 +267,8 @@ 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() + editable.Assignees.DefaultLogins = issue.AssignedActors.Logins() } else { editable.Assignees.Default = issue.Assignees.Logins() } 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 } } } } } diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index ff293a511..903cb81b3 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -240,7 +240,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..77bab8440 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 // For disambiguating actors from display names } // ProjectsV2 mutations require a mapping of an item ID to a project ID. @@ -112,10 +113,25 @@ 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() - s.AddValues(e.Assignees.Default) + // 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 { + s.AddValues(e.Assignees.Default) + } add, err := meReplacer.ReplaceSlice(e.Assignees.Add) if err != nil { return nil, err @@ -411,7 +427,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 {