diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 2fcb85c56..c529f5ae2 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -527,17 +527,6 @@ func Test_editRun(t *testing.T) { }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { // Should only be one fetch of metadata. - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, - { "login": "MonaLisa", "id": "MONAID", "__typename": "User" } - ], - "pageInfo": { "hasNextPage": false, "endCursor": "Mg" } - } } } } - `)) reg.Register( httpmock.GraphQL(`query RepositoryMilestoneList\b`), httpmock.StringResponse(` @@ -618,6 +607,17 @@ func Test_editRun(t *testing.T) { mockIssueGet(t, reg) mockIssueProjectItemsGet(t, reg) mockRepoMetadata(t, reg) + 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) mockIssueUpdateActorAssignees(t, reg) mockIssueUpdateLabels(t, reg) @@ -667,9 +667,9 @@ func Test_editRun(t *testing.T) { { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, func(inputs map[string]interface{}) { // Checking that despite the display name being returned - // from the EditFieldsSurvey, the ID is still + // from the EditFieldsSurvey, the login is still // used in the mutation. - require.Subset(t, inputs["actorIds"], []string{"MONAID", "HUBOTID"}) + require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "MonaLisa (Mona Display Name)"}) }), ) }, @@ -839,18 +839,6 @@ func mockIssueProjectItemsGet(_ *testing.T, reg *httpmock.Registry) { } func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) { - 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 } - } } } } - `)) - reg.Register( httpmock.GraphQL(`query RepositoryLabelList\b`), httpmock.StringResponse(` diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index e38fac3ca..2582c500b 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -415,7 +415,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { // Non-interactive with Add/Remove doesn't need reviewers/assignees metadata // REST API accepts logins and team slugs directly - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) mockRequestReviewsByLogin(reg) @@ -473,7 +473,7 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: true, labels: true, projects: true, milestones: true}) + mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) @@ -547,7 +547,7 @@ func Test_editRun(t *testing.T) { }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { // Non-interactive with Remove doesn't need reviewers metadata - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockRequestReviewsByLogin(reg) mockPullRequestUpdateLabels(reg) @@ -932,9 +932,9 @@ func Test_editRun(t *testing.T) { { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, func(inputs map[string]interface{}) { // Checking that despite the display name being returned - // from the EditFieldsSurvey, the ID is still + // from the EditFieldsSurvey, the login is still // used in the mutation. - require.Subset(t, inputs["actorIds"], []string{"MONAID", "HUBOTID"}) + require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "monalisa (Mona Display Name)"}) }), ) }, diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index a70cbf1ff..bb6228b4e 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -150,6 +150,49 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str return &a, err } +// AssigneeLogins computes the final list of assignee logins from the current +// defaults plus any Add/Remove operations. Unlike AssigneeIds, this does not +// resolve logins to node IDs, and is used on github.com where the +// ReplaceActorsForAssignable mutation accepts logins directly. +func (e Editable) AssigneeLogins(client *api.Client, repo ghrepo.Interface) ([]string, error) { + if !e.Assignees.Edited { + return nil, nil + } + + if len(e.Assignees.Add) != 0 || len(e.Assignees.Remove) != 0 { + meReplacer := NewMeReplacer(client, repo.RepoHost()) + copilotReplacer := NewCopilotReplacer(true) + + replaceSpecialAssigneeNames := func(value []string) ([]string, error) { + replaced, err := meReplacer.ReplaceSlice(value) + if err != nil { + return nil, err + } + replaced = copilotReplacer.ReplaceSlice(replaced) + return replaced, nil + } + + assigneeSet := set.NewStringSet() + assigneeSet.AddValues(e.Assignees.DefaultLogins) + + add, err := replaceSpecialAssigneeNames(e.Assignees.Add) + if err != nil { + return nil, err + } + assigneeSet.AddValues(add) + + remove, err := replaceSpecialAssigneeNames(e.Assignees.Remove) + if err != nil { + return nil, err + } + assigneeSet.RemoveValues(remove) + + e.Assignees.Value = assigneeSet.ToSlice() + } + + return e.Assignees.Value, nil +} + // ProjectIds returns a slice containing IDs of projects v1 that the issue or a PR has to be linked to. func (e Editable) ProjectIds() (*[]string, error) { if !e.Projects.Edited { @@ -474,12 +517,10 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, if len(editable.Assignees.Add) == 0 && len(editable.Assignees.Remove) == 0 && editable.AssigneeSearchFunc == nil { fetchAssignees = true } - // However, if we have Add/Remove operations (non-interactive flow), - // we do need to fetch the assignees. - // TODO replaceActorsByLoginCleanup - // When ActorAssignees is true, noninteractive assignees should use - // ReplaceActorsForAssignableByLogin to skip this fetch entirely. - if len(editable.Assignees.Add) > 0 || len(editable.Assignees.Remove) > 0 { + // For non-interactive Add/Remove operations, we only need to fetch assignees + // on GHES where ID resolution is required. On github.com (ActorAssignees), + // logins are passed directly to the mutation. + if (len(editable.Assignees.Add) > 0 || len(editable.Assignees.Remove) > 0) && !editable.Assignees.ActorAssignees { fetchAssignees = true } } diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index 097252eaa..bb63ceb0f 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -66,19 +66,14 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR // other issue fields to ensure consistency with how legacy // user assignees are handled. // https://github.com/cli/cli/pull/10960#discussion_r2086725348 - // TODO replaceActorsByLoginCleanup - // When ActorAssignees is true (github.com), this should pass logins directly - // to ReplaceActorsForAssignableByLogin instead of resolving IDs. The ID-based - // path can be removed once both interactive and non-interactive edit flows - // are migrated to use logins. if options.Assignees.Edited && options.Assignees.ActorAssignees { apiClient := api.NewClientFromHTTP(httpClient) - assigneeIds, err := options.AssigneeIds(apiClient, repo) + logins, err := options.AssigneeLogins(apiClient, repo) if err != nil { return err } - err = api.ReplaceActorsForAssignableByID(apiClient, repo, id, *assigneeIds) + err = api.ReplaceActorsForAssignableByLogin(apiClient, repo, id, logins) if err != nil { return err }