diff --git a/api/queries_issue.go b/api/queries_issue.go index d545ef59f..f719e52f9 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -289,7 +289,8 @@ func IssueCreate(client *Client, repo *Repository, params map[string]interface{} switch key { case "assigneeIds", "body", "issueTemplate", "labelIds", "milestoneId", "projectIds", "repositoryId", "title": inputParams[key] = val - case "projectV2Ids": + case "projectV2Ids", "assigneeLogins": + // handled after issue creation default: return nil, fmt.Errorf("invalid IssueCreate mutation parameter %s", key) } @@ -310,6 +311,14 @@ func IssueCreate(client *Client, repo *Repository, params map[string]interface{} } issue := &result.CreateIssue.Issue + // Assign users using login-based mutation when ActorAssignees is true (github.com). + if assigneeLogins, ok := params["assigneeLogins"].([]string); ok && len(assigneeLogins) > 0 { + err := ReplaceActorsForAssignableByLogin(client, repo, issue.ID, assigneeLogins) + if err != nil { + return issue, err + } + } + // projectV2 parameters aren't supported in the `createIssue` mutation, // so add them after the issue has been created. projectV2Ids, ok := params["projectV2Ids"].([]string) diff --git a/api/queries_pr.go b/api/queries_pr.go index d0488c64b..8b0d6b179 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -524,6 +524,14 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter } } + // Assign users using login-based mutation when ActorAssignees is true (github.com). + if assigneeLogins, ok := params["assigneeLogins"].([]string); ok && len(assigneeLogins) > 0 { + err := ReplaceActorsForAssignableByLogin(client, repo, pr.ID, assigneeLogins) + if err != nil { + return pr, err + } + } + // TODO requestReviewsByLoginCleanup // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation. // The ID-based path can be removed once GHES supports requestReviewsByLogin. @@ -581,6 +589,62 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter return pr, nil } +// ReplaceActorsForAssignableByLogin calls the replaceActorsForAssignable mutation +// using actor logins. This avoids the need to resolve logins to node IDs. +func ReplaceActorsForAssignableByLogin(client *Client, repo ghrepo.Interface, assignableID string, logins []string) error { + type ReplaceActorsForAssignableInput struct { + AssignableID githubv4.ID `json:"assignableId"` + ActorLogins []githubv4.String `json:"actorLogins"` + } + + actorLogins := make([]githubv4.String, len(logins)) + for i, l := range logins { + actorLogins[i] = githubv4.String(l) + } + + variables := map[string]interface{}{ + "input": ReplaceActorsForAssignableInput{ + AssignableID: githubv4.ID(assignableID), + ActorLogins: actorLogins, + }, + } + + 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) +} + // SuggestedAssignableActors fetches up to 10 suggested actors for a specific assignable // (Issue or PullRequest) node ID. `assignableID` is the GraphQL node ID for the Issue/PR. // Returns the actors, the total count of available assignees in the repo, and an error. diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index 80c8f76d3..e02929a45 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -539,10 +539,21 @@ func Test_createRun(t *testing.T) { httpmock.GraphQL(`mutation IssueCreate\b`), httpmock.GraphQLMutation(` { "data": { "createIssue": { "issue": { + "id": "ISSUEID", "URL": "https://github.com/OWNER/REPO/issues/12" } } } } `, func(inputs map[string]interface{}) { - assert.Equal(t, []interface{}{"COPILOTID", "MONAID"}, inputs["assigneeIds"]) + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } + })) + r.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "ISSUEID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"copilot-swe-agent", "MonaLisa"}, inputs["actorLogins"]) })) }, wantsStdout: "https://github.com/OWNER/REPO/issues/12\n", @@ -948,16 +959,6 @@ func TestIssueCreate_metadata(t *testing.T) { defer http.Verify(t) http.StubRepoInfoResponse("OWNER", "REPO", "main") - http.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) http.Register( httpmock.GraphQL(`query RepositoryLabelList\b`), httpmock.StringResponse(` @@ -1030,12 +1031,15 @@ func TestIssueCreate_metadata(t *testing.T) { httpmock.GraphQL(`mutation IssueCreate\b`), httpmock.GraphQLMutation(` { "data": { "createIssue": { "issue": { + "id": "NEWISSUEID", "URL": "https://github.com/OWNER/REPO/issues/12" } } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "TITLE", inputs["title"]) assert.Equal(t, "BODY", inputs["body"]) - assert.Equal(t, []interface{}{"MONAID"}, inputs["assigneeIds"]) + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } assert.Equal(t, []interface{}{"BUGID", "TODOID"}, inputs["labelIds"]) assert.Equal(t, []interface{}{"ROADMAPID"}, inputs["projectIds"]) assert.Equal(t, "BIGONEID", inputs["milestoneId"]) @@ -1043,6 +1047,14 @@ func TestIssueCreate_metadata(t *testing.T) { assert.NotContains(t, inputs, "teamIds") assert.NotContains(t, inputs, "projectV2Ids") })) + http.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWISSUEID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"monalisa"}, inputs["actorLogins"]) + })) output, err := runCommand(http, true, `-t TITLE -b BODY -a monalisa -l bug -l todo -p roadmap -m 'big one.oh'`, nil) if err != nil { @@ -1091,27 +1103,27 @@ func TestIssueCreate_AtMeAssignee(t *testing.T) { "hasIssuesEnabled": true } } } `)) - http.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" }, - { "login": "SomeOneElse", "id": "SOMEID", "name": "Someone else", "__typename": "User" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) http.Register( httpmock.GraphQL(`mutation IssueCreate\b`), httpmock.GraphQLMutation(` { "data": { "createIssue": { "issue": { + "id": "NEWISSUEID", "URL": "https://github.com/OWNER/REPO/issues/12" } } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "hello", inputs["title"]) assert.Equal(t, "cash rules everything around me", inputs["body"]) - assert.Equal(t, []interface{}{"MONAID", "SOMEID"}, inputs["assigneeIds"]) + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } + })) + http.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWISSUEID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"MonaLisa", "someoneelse"}, inputs["actorLogins"]) })) output, err := runCommand(http, true, `-a @me -a someoneelse -t hello -b "cash rules everything around me"`, nil) @@ -1134,26 +1146,27 @@ func TestIssueCreate_AtCopilotAssignee(t *testing.T) { "hasIssuesEnabled": true } } } `)) - http.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "copilot-swe-agent", "id": "COPILOTID", "name": "Copilot (AI)", "__typename": "Bot" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) http.Register( httpmock.GraphQL(`mutation IssueCreate\b`), httpmock.GraphQLMutation(` { "data": { "createIssue": { "issue": { + "id": "NEWISSUEID", "URL": "https://github.com/OWNER/REPO/issues/12" } } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "hello", inputs["title"]) assert.Equal(t, "cash rules everything around me", inputs["body"]) - assert.Equal(t, []interface{}{"COPILOTID"}, inputs["assigneeIds"]) + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } + })) + http.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWISSUEID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"copilot-swe-agent"}, inputs["actorLogins"]) })) output, err := runCommand(http, true, `-a @copilot -t hello -b "cash rules everything around me"`, nil) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 0dbdb3987..76a30e24b 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -429,6 +429,7 @@ func createRun(opts *CreateOptions) error { if issueFeatures.ActorIsAssignable { state.ActorReviewers = true + state.ActorAssignees = true } var openURL string diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index ed2a9cf2e..fd53a92fc 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -916,17 +916,6 @@ func Test_createRun(t *testing.T) { return func() {} }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { - 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(`query RepositoryLabelList\b`), httpmock.StringResponse(` @@ -975,11 +964,21 @@ func Test_createRun(t *testing.T) { } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) - assert.Equal(t, []interface{}{"MONAID"}, inputs["assigneeIds"]) + if _, ok := inputs["assigneeIds"]; ok { + t.Error("did not expect assigneeIds in updatePullRequest when ActorAssignees is true") + } assert.Equal(t, []interface{}{"BUGID", "TODOID"}, inputs["labelIds"]) assert.Equal(t, []interface{}{"ROADMAPID"}, inputs["projectIds"]) assert.Equal(t, "BIGONEID", inputs["milestoneId"]) })) + reg.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWPULLID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"monalisa"}, inputs["actorLogins"]) + })) reg.Register( httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), httpmock.GraphQLMutation(` diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 1b7c42be3..a70cbf1ff 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -142,6 +142,10 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str e.Assignees.Value = assigneeSet.ToSlice() } + // TODO replaceActorsByLoginCleanup + // When ActorAssignees is true (github.com), this should compute the final + // assignee logins and return them directly without resolving to IDs, for use + // with ReplaceActorsForAssignableByLogin. a, err := e.Metadata.MembersToIDs(e.Assignees.Value) return &a, err } @@ -472,8 +476,9 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, } // However, if we have Add/Remove operations (non-interactive flow), // we do need to fetch the assignees. - // TODO: KW noninteractive assignees need to migrate to directly use - // new logins input with ReplaceActorsForAssignable to prevent fetching. + // 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 { fetchAssignees = true } diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index 8cd51c349..097252eaa 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -66,6 +66,11 @@ 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) @@ -73,7 +78,7 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR return err } - err = replaceActorAssigneesForEditable(apiClient, repo, id, assigneeIds) + err = api.ReplaceActorsForAssignableByID(apiClient, repo, id, *assigneeIds) if err != nil { return err } @@ -90,32 +95,6 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR return wg.Wait() } -func replaceActorAssigneesForEditable(apiClient *api.Client, repo ghrepo.Interface, id string, assigneeIds *[]string) error { - type ReplaceActorsForAssignableInput struct { - AssignableID githubv4.ID `json:"assignableId"` - ActorIDs []githubv4.ID `json:"actorIds"` - } - - params := ReplaceActorsForAssignableInput{ - AssignableID: githubv4.ID(id), - ActorIDs: *ghIds(assigneeIds), - } - - var mutation struct { - ReplaceActorsForAssignable struct { - TypeName string `graphql:"__typename"` - } `graphql:"replaceActorsForAssignable(input: $input)"` - } - - variables := map[string]interface{}{"input": params} - err := apiClient.Mutate(repo.RepoHost(), "ReplaceActorsForAssignable", &mutation, variables) - if err != nil { - return err - } - - return nil -} - func replaceIssueFields(httpClient *http.Client, repo ghrepo.Interface, id string, isPR bool, options Editable) error { apiClient := api.NewClientFromHTTP(httpClient) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index d5c168e5f..17d40d639 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -64,6 +64,9 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par // When ActorReviewers is true, we use login-based mutation and don't need to resolve reviewer IDs. needReviewerIDs := len(tb.Reviewers) > 0 && !tb.ActorReviewers + // When ActorAssignees is true, we use login-based mutation and don't need to resolve assignee IDs. + needAssigneeIDs := len(tb.Assignees) > 0 && !tb.ActorAssignees + // Retrieve minimal information needed to resolve metadata if this was not previously cached from additional metadata survey. if tb.MetadataResult == nil { input := api.RepoMetadataInput{ @@ -71,12 +74,11 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par TeamReviewers: needReviewerIDs && slices.ContainsFunc(tb.Reviewers, func(r string) bool { return strings.ContainsRune(r, '/') }), - Assignees: len(tb.Assignees) > 0, - ActorAssignees: tb.ActorAssignees, - Labels: len(tb.Labels) > 0, - ProjectsV1: len(tb.ProjectTitles) > 0 && projectV1Support == gh.ProjectsV1Supported, - ProjectsV2: len(tb.ProjectTitles) > 0, - Milestones: len(tb.Milestones) > 0, + Assignees: needAssigneeIDs, + Labels: len(tb.Labels) > 0, + ProjectsV1: len(tb.ProjectTitles) > 0 && projectV1Support == gh.ProjectsV1Supported, + ProjectsV2: len(tb.ProjectTitles) > 0, + Milestones: len(tb.Milestones) > 0, } metadataResult, err := api.RepoMetadata(client, baseRepo, input) @@ -86,11 +88,17 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par tb.MetadataResult = metadataResult } - assigneeIDs, err := tb.MetadataResult.MembersToIDs(tb.Assignees) - if err != nil { - return fmt.Errorf("could not assign user: %w", err) + // When ActorAssignees is true (github.com), pass logins directly for use with + // ReplaceActorsForAssignable mutation. The ID-based else branch is for GHES compatibility. + if tb.ActorAssignees { + params["assigneeLogins"] = tb.Assignees + } else { + assigneeIDs, err := tb.MetadataResult.MembersToIDs(tb.Assignees) + if err != nil { + return fmt.Errorf("could not assign user: %w", err) + } + params["assigneeIds"] = assigneeIDs } - params["assigneeIds"] = assigneeIDs labelIDs, err := tb.MetadataResult.LabelsToIDs(tb.Labels) if err != nil {