diff --git a/api/queries_issue.go b/api/queries_issue.go index f719e52f9..bff84029d 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -311,7 +311,7 @@ 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). + // Assign users using login-based mutation when ApiActorsSupported is true (github.com). if assigneeLogins, ok := params["assigneeLogins"].([]string); ok && len(assigneeLogins) > 0 { err := ReplaceActorsForAssignableByLogin(client, repo, issue.ID, assigneeLogins) if err != nil { diff --git a/api/queries_pr.go b/api/queries_pr.go index 01846a9e6..29342521f 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -524,7 +524,7 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter } } - // Assign users using login-based mutation when ActorAssignees is true (github.com). + // Assign users using login-based mutation when ApiActorsSupported is true (github.com). if assigneeLogins, ok := params["assigneeLogins"].([]string); ok && len(assigneeLogins) > 0 { err := ReplaceActorsForAssignableByLogin(client, repo, pr.ID, assigneeLogins) if err != nil { @@ -532,7 +532,7 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter } } - // TODO requestReviewsByLoginCleanup + // TODO ApiActorsSupported // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation. // The ID-based path can be removed once GHES supports requestReviewsByLogin. userLogins, hasUserLogins := params["userReviewerLogins"].([]string) @@ -599,6 +599,12 @@ func ReplaceActorsForAssignableByLogin(client *Client, repo ghrepo.Interface, as actorLogins := make([]githubv4.String, len(logins)) for i, l := range logins { + // The replaceActorsForAssignable mutation requires the [bot] suffix + // for bot actor logins (e.g. "copilot-swe-agent[bot]"), unlike + // requestReviewsByLogin which has a separate botLogins field. + if l == CopilotAssigneeLogin { + l = l + "[bot]" + } actorLogins[i] = githubv4.String(l) } diff --git a/api/queries_repo.go b/api/queries_repo.go index 6754a1542..d1bc2df1e 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -963,14 +963,15 @@ func (m *RepoMetadataResult) Merge(m2 *RepoMetadataResult) { } type RepoMetadataInput struct { - Assignees bool - ActorAssignees bool - Reviewers bool - TeamReviewers bool - Labels bool - ProjectsV1 bool - ProjectsV2 bool - Milestones bool + Assignees bool + Reviewers bool + TeamReviewers bool + // TODO ApiActorsSupported + ApiActorsSupported bool + Labels bool + ProjectsV1 bool + ProjectsV2 bool + Milestones bool } // RepoMetadata pre-fetches the metadata for attaching to issues and pull requests @@ -979,7 +980,8 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput var g errgroup.Group if input.Assignees || input.Reviewers { - if input.ActorAssignees { + // TODO ApiActorsSupported + if input.ApiActorsSupported { g.Go(func() error { actors, err := RepoAssignableActors(client, repo) if err != nil { diff --git a/internal/featuredetection/feature_detection.go b/internal/featuredetection/feature_detection.go index 9af4c5aec..d4ef62070 100644 --- a/internal/featuredetection/feature_detection.go +++ b/internal/featuredetection/feature_detection.go @@ -23,11 +23,35 @@ type Detector interface { } type IssueFeatures struct { - ActorIsAssignable bool + // TODO ApiActorsSupported + // ApiActorsSupported indicates the host supports actor-based APIs. True for + // github.com and ghe.com, false for GHES. + // + // The GitHub API has two generations of assignee/reviewer types: + // + // Legacy (GHES): Uses AssignableUser (users only) and node-ID-based mutations. + // - assignableUsers query returns []AssignableUser + // - Mutations take node IDs (assigneeIds, userReviewerIds, teamReviewerIds) + // + // Actor-based (github.com): Uses AssignableActor (User + Bot union) and + // login-based mutations, enabling assignment of non-user actors like Copilot. + // - suggestedActors query returns []AssignableActor (User | Bot) + // - suggestedReviewerActors returns []ReviewerCandidate (User | Bot | Team) + // - Mutations take logins (replaceActorsForAssignable, requestReviewsByLogin) + // + // When GHES adds support for the actor-based types and mutations, this flag + // can be removed and all // TODO ApiActorsSupported sites collapsed to the + // actor-only path. To verify GHES support, check whether the GHES GraphQL + // schema includes: + // - The suggestedActors field on Repository (assignee search) + // - The suggestedReviewerActors field on PullRequest (reviewer search) + // - The replaceActorsForAssignable mutation + // - The requestReviewsByLogin mutation + ApiActorsSupported bool } var allIssueFeatures = IssueFeatures{ - ActorIsAssignable: true, + ApiActorsSupported: true, } type PullRequestFeatures struct { @@ -136,7 +160,7 @@ func (d *detector) IssueFeatures() (IssueFeatures, error) { } return IssueFeatures{ - ActorIsAssignable: false, // replaceActorsForAssignable GraphQL mutation unavailable on GHES + ApiActorsSupported: false, // TODO ApiActorsSupported — actor-based mutations unavailable on GHES }, nil } diff --git a/internal/featuredetection/feature_detection_test.go b/internal/featuredetection/feature_detection_test.go index 82132ab83..f24e31f4c 100644 --- a/internal/featuredetection/feature_detection_test.go +++ b/internal/featuredetection/feature_detection_test.go @@ -23,7 +23,7 @@ func TestIssueFeatures(t *testing.T) { name: "github.com", hostname: "github.com", wantFeatures: IssueFeatures{ - ActorIsAssignable: true, + ApiActorsSupported: true, }, wantErr: false, }, @@ -31,7 +31,7 @@ func TestIssueFeatures(t *testing.T) { name: "ghec data residency (ghe.com)", hostname: "stampname.ghe.com", wantFeatures: IssueFeatures{ - ActorIsAssignable: true, + ApiActorsSupported: true, }, wantErr: false, }, @@ -39,7 +39,7 @@ func TestIssueFeatures(t *testing.T) { name: "GHE", hostname: "git.my.org", wantFeatures: IssueFeatures{ - ActorIsAssignable: false, + ApiActorsSupported: false, }, wantErr: false, }, diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 4346bd5cd..8e6c6255f 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -179,7 +179,7 @@ func createRun(opts *CreateOptions) (err error) { // Replace special values in assignees // For web mode, @copilot should be replaced by name; otherwise, login. - assigneeReplacer := prShared.NewSpecialAssigneeReplacer(apiClient, baseRepo.RepoHost(), issueFeatures.ActorIsAssignable, !opts.WebMode) + assigneeReplacer := prShared.NewSpecialAssigneeReplacer(apiClient, baseRepo.RepoHost(), issueFeatures.ApiActorsSupported, !opts.WebMode) assignees, err := assigneeReplacer.ReplaceSlice(opts.Assignees) if err != nil { return err @@ -188,14 +188,14 @@ func createRun(opts *CreateOptions) (err error) { assigneeSet.AddValues(assignees) tb := prShared.IssueMetadataState{ - Type: prShared.IssueMetadata, - ActorAssignees: issueFeatures.ActorIsAssignable, - Assignees: assigneeSet.ToSlice(), - Labels: opts.Labels, - ProjectTitles: opts.Projects, - Milestones: milestones, - Title: opts.Title, - Body: opts.Body, + Type: prShared.IssueMetadata, + ApiActorsSupported: issueFeatures.ApiActorsSupported, // TODO ApiActorsSupported + Assignees: assigneeSet.ToSlice(), + Labels: opts.Labels, + ProjectTitles: opts.Projects, + Milestones: milestones, + Title: opts.Title, + Body: opts.Body, } if opts.RecoverFile != "" { @@ -309,7 +309,7 @@ func createRun(opts *CreateOptions) (err error) { State: &tb, } var assigneeSearchFunc func(string) prompter.MultiSelectSearchResult - if issueFeatures.ActorIsAssignable { + if issueFeatures.ApiActorsSupported { assigneeSearchFunc = prShared.RepoAssigneeSearchFunc(apiClient, baseRepo) } err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support, nil, assigneeSearchFunc) diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index d595f34cd..4a55a3326 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -548,7 +548,7 @@ func Test_createRun(t *testing.T) { { "data": { "replaceActorsForAssignable": { "__typename": "" } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "ISSUEID", inputs["assignableId"]) - assert.Equal(t, []interface{}{"copilot-swe-agent", "MonaLisa"}, inputs["actorLogins"]) + assert.Equal(t, []interface{}{"copilot-swe-agent[bot]", "MonaLisa"}, inputs["actorLogins"]) })) }, wantsStdout: "https://github.com/OWNER/REPO/issues/12\n", @@ -1161,7 +1161,7 @@ func TestIssueCreate_AtCopilotAssignee(t *testing.T) { { "data": { "replaceActorsForAssignable": { "__typename": "" } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWISSUEID", inputs["assignableId"]) - assert.Equal(t, []interface{}{"copilot-swe-agent"}, inputs["actorLogins"]) + assert.Equal(t, []interface{}{"copilot-swe-agent[bot]"}, inputs["actorLogins"]) })) output, err := runCommand(http, true, `-a @copilot -t hello -b "cash rules everything around me"`, nil) diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 2fd632b5e..1d5455504 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -215,9 +215,9 @@ func editRun(opts *EditOptions) error { lookupFields := []string{"id", "number", "title", "body", "url"} if editable.Assignees.Edited { - // TODO actorIsAssignableCleanup - if issueFeatures.ActorIsAssignable { - editable.Assignees.ActorAssignees = true + // TODO ApiActorsSupported + if issueFeatures.ApiActorsSupported { + editable.ApiActorsSupported = true lookupFields = append(lookupFields, "assignedActors") } else { lookupFields = append(lookupFields, "assignees") @@ -249,9 +249,9 @@ func editRun(opts *EditOptions) error { // Fetch editable shared fields once for all issues. apiClient := api.NewClientFromHTTP(httpClient) - // Wire up search function for assignees when ActorIsAssignable is available. + // Wire up search function for assignees when ApiActorsSupported 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 { + if issueFeatures.ApiActorsSupported && opts.Interactive && len(issues) == 1 { editable.AssigneeSearchFunc = prShared.AssigneeSearchFunc(apiClient, baseRepo, issues[0].ID) } @@ -280,7 +280,8 @@ func editRun(opts *EditOptions) error { editable.Body.Default = issue.Body // We use Actors as the default assignees if Actors are assignable // on this GitHub host. - if editable.Assignees.ActorAssignees { + // TODO ApiActorsSupported + if editable.ApiActorsSupported { editable.Assignees.Default = issue.AssignedActors.DisplayNames() editable.Assignees.DefaultLogins = issue.AssignedActors.Logins() } else { diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index ef496d1de..626c28162 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -395,7 +395,7 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockRepoMetadata(t, reg) mockIssueUpdate(t, reg) - mockIssueUpdateActorAssignees(t, reg) + mockIssueUpdateApiActors(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) }, @@ -444,8 +444,8 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockIssueUpdate(t, reg) mockIssueUpdate(t, reg) - mockIssueUpdateActorAssignees(t, reg) - mockIssueUpdateActorAssignees(t, reg) + mockIssueUpdateApiActors(t, reg) + mockIssueUpdateApiActors(t, reg) mockIssueUpdateLabels(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) @@ -608,7 +608,7 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockRepoMetadata(t, reg) mockIssueUpdate(t, reg) - mockIssueUpdateActorAssignees(t, reg) + mockIssueUpdateApiActors(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) }, @@ -902,7 +902,7 @@ func mockIssueUpdate(t *testing.T, reg *httpmock.Registry) { ) } -func mockIssueUpdateActorAssignees(t *testing.T, reg *httpmock.Registry) { +func mockIssueUpdateApiActors(t *testing.T, reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), httpmock.GraphQLMutation(` @@ -935,7 +935,7 @@ func mockProjectV2ItemUpdate(t *testing.T, reg *httpmock.Registry) { ) } -func TestActorIsAssignable(t *testing.T) { +func TestApiActorsSupported(t *testing.T) { t.Run("when actors are assignable, query includes assignedActors", func(t *testing.T) { ios, _, _, _ := iostreams.Test() diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index d8230dc3c..1a3eba7d8 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -399,7 +399,7 @@ func createRun(opts *CreateOptions) error { client := ctx.Client - // Detect ActorIsAssignable feature to determine if we can use search-based + // Detect ApiActorsSupported feature to determine if we can use search-based // reviewer selection (github.com) or need to use legacy ID-based selection (GHES) issueFeatures, err := opts.Detector.IssueFeatures() if err != nil { @@ -407,7 +407,7 @@ func createRun(opts *CreateOptions) error { } var reviewerSearchFunc func(string) prompter.MultiSelectSearchResult var assigneeSearchFunc func(string) prompter.MultiSelectSearchResult - if issueFeatures.ActorIsAssignable { + if issueFeatures.ApiActorsSupported { reviewerSearchFunc = func(query string) prompter.MultiSelectSearchResult { candidates, moreResults, err := api.SuggestedReviewerActorsForRepo(client, ctx.PRRefs.BaseRepo(), query) if err != nil { @@ -424,14 +424,14 @@ func createRun(opts *CreateOptions) error { assigneeSearchFunc = shared.RepoAssigneeSearchFunc(client, ctx.PRRefs.BaseRepo()) } - state, err := NewIssueState(*ctx, *opts) + state, err := NewIssueState(*ctx, *opts, issueFeatures.ApiActorsSupported) if err != nil { return err } - if issueFeatures.ActorIsAssignable { - state.ActorReviewers = true - state.ActorAssignees = true + // TODO ApiActorsSupported + if issueFeatures.ApiActorsSupported { + state.ApiActorsSupported = true } var openURL string @@ -672,14 +672,14 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } -func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadataState, error) { +func NewIssueState(ctx CreateContext, opts CreateOptions, apiActorsSupported bool) (*shared.IssueMetadataState, error) { var milestoneTitles []string if opts.Milestone != "" { milestoneTitles = []string{opts.Milestone} } - meReplacer := shared.NewMeReplacer(ctx.Client, ctx.PRRefs.BaseRepo().RepoHost()) - assignees, err := meReplacer.ReplaceSlice(opts.Assignees) + assigneeReplacer := shared.NewSpecialAssigneeReplacer(ctx.Client, ctx.PRRefs.BaseRepo().RepoHost(), apiActorsSupported, !opts.WebMode) + assignees, err := assigneeReplacer.ReplaceSlice(opts.Assignees) if err != nil { return nil, err } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index fd53a92fc..5bad889b6 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -965,7 +965,7 @@ func Test_createRun(t *testing.T) { `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) if _, ok := inputs["assigneeIds"]; ok { - t.Error("did not expect assigneeIds in updatePullRequest when ActorAssignees is true") + t.Error("did not expect assigneeIds in updatePullRequest when ApiActorsSupported is true") } assert.Equal(t, []interface{}{"BUGID", "TODOID"}, inputs["labelIds"]) assert.Equal(t, []interface{}{"ROADMAPID"}, inputs["projectIds"]) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 12c197b5a..33a71154a 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -270,8 +270,8 @@ func editRun(opts *EditOptions) error { return err } - // TODO actorIsAssignableCleanup - if issueFeatures.ActorIsAssignable { + // TODO ApiActorsSupported + if issueFeatures.ApiActorsSupported { findOptions.Fields = append(findOptions.Fields, "assignedActors") } else { findOptions.Fields = append(findOptions.Fields, "assignees") @@ -289,9 +289,9 @@ func editRun(opts *EditOptions) error { editable.Base.Default = pr.BaseRefName editable.Reviewers.Default = pr.ReviewRequests.DisplayNames() editable.Reviewers.DefaultLogins = pr.ReviewRequests.Logins() - // TODO actorIsAssignableCleanup - if issueFeatures.ActorIsAssignable { - editable.Assignees.ActorAssignees = true + // TODO ApiActorsSupported + if issueFeatures.ApiActorsSupported { + editable.ApiActorsSupported = true editable.Assignees.Default = pr.AssignedActors.DisplayNames() editable.Assignees.DefaultLogins = pr.AssignedActors.Logins() } else { @@ -320,8 +320,8 @@ func editRun(opts *EditOptions) error { // Wire up search functions for assignees and reviewers. // When these aren't wired up, it triggers a downstream fallback // to legacy reviewer/assignee fetching. - // TODO actorIsAssignableCleanup - if issueFeatures.ActorIsAssignable { + // TODO ApiActorsSupported + if issueFeatures.ApiActorsSupported { editable.AssigneeSearchFunc = shared.AssigneeSearchFunc(apiClient, repo, pr.ID) editable.ReviewerSearchFunc = reviewerSearchFunc(apiClient, repo, &editable, pr.ID) } @@ -438,7 +438,8 @@ func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, pr // Replace @copilot with the Copilot reviewer login (only on github.com). // Also use DefaultLogins (not Default display names) for computing the set. var defaultLogins []string - if editable.Assignees.ActorAssignees { + // TODO ApiActorsSupported + if editable.ApiActorsSupported { copilotReplacer := shared.NewCopilotReviewerReplacer() add = copilotReplacer.ReplaceSlice(add) remove = copilotReplacer.ReplaceSlice(remove) @@ -457,7 +458,8 @@ func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, pr // On github.com, use the new GraphQL mutation which supports bots. // On GHES, fall back to REST API. - if editable.Assignees.ActorAssignees { + // TODO ApiActorsSupported + if editable.ApiActorsSupported { return updatePullRequestReviewsGraphQL(client, repo, prID, editable) } return updatePullRequestReviewsREST(client, repo, number, editable) diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 01acb7e0f..abe764158 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -417,7 +417,7 @@ func Test_editRun(t *testing.T) { // REST API accepts logins and team slugs directly mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) - mockPullRequestUpdateActorAssignees(reg) + mockPullRequestUpdateApiActors(reg) mockRequestReviewsByLogin(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) @@ -475,7 +475,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) - mockPullRequestUpdateActorAssignees(reg) + mockPullRequestUpdateApiActors(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -551,7 +551,7 @@ func Test_editRun(t *testing.T) { mockPullRequestUpdate(reg) mockRequestReviewsByLogin(reg) mockPullRequestUpdateLabels(reg) - mockPullRequestUpdateActorAssignees(reg) + mockPullRequestUpdateApiActors(reg) mockProjectV2ItemUpdate(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", @@ -756,7 +756,7 @@ func Test_editRun(t *testing.T) { // (searchFunc handles dynamic fetching, metadata populated in test mock) mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) - mockPullRequestUpdateActorAssignees(reg) + mockPullRequestUpdateApiActors(reg) mockRequestReviewsByLogin(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) @@ -785,7 +785,7 @@ func Test_editRun(t *testing.T) { editFields: func(e *shared.Editable, _ string) error { e.Title.Value = "new title" e.Body.Value = "new body" - // When ActorAssignees is enabled, the interactive flow returns display names (or logins for non-users) + // When ApiActorsSupported is enabled, the interactive flow returns display names (or logins for non-users) e.Assignees.Value = []string{"monalisa (Mona Display Name)", "hubot"} // Populate metadata to simulate what searchFunc would do during prompting e.Metadata.AssignableActors = []api.AssignableActor{ @@ -808,7 +808,7 @@ func Test_editRun(t *testing.T) { // assignees: false because searchFunc handles dynamic fetching (metadata populated in test mock) mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) - mockPullRequestUpdateActorAssignees(reg) + mockPullRequestUpdateApiActors(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -876,7 +876,7 @@ func Test_editRun(t *testing.T) { mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockRequestReviewsByLogin(reg) - mockPullRequestUpdateActorAssignees(reg) + mockPullRequestUpdateApiActors(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -990,7 +990,7 @@ func Test_editRun(t *testing.T) { return nil }, editFields: func(e *shared.Editable, _ string) error { - require.False(t, e.Assignees.ActorAssignees) + require.False(t, e.ApiActorsSupported) require.Nil(t, e.AssigneeSearchFunc) require.Contains(t, e.Assignees.Options, "monalisa") require.Contains(t, e.Assignees.Options, "hubot") @@ -1190,7 +1190,7 @@ type mockRepoMetadataOptions struct { } func mockRepoMetadata(reg *httpmock.Registry, opt mockRepoMetadataOptions) { - // Assignable actors (users/bots) are fetched when reviewers OR assignees edited with ActorAssignees enabled. + // Assignable actors (users/bots) are fetched when reviewers OR assignees edited with ApiActorsSupported enabled. if opt.reviewers || opt.assignees { reg.Register( httpmock.GraphQL(`query RepositoryAssignableActors\b`), @@ -1314,7 +1314,7 @@ func mockPullRequestUpdate(reg *httpmock.Registry) { httpmock.StringResponse(`{}`)) } -func mockPullRequestUpdateActorAssignees(reg *httpmock.Registry) { +func mockPullRequestUpdateApiActors(reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), httpmock.GraphQLMutation(` @@ -1336,7 +1336,7 @@ func mockPullRequestRemoveReviewers(reg *httpmock.Registry) { } // mockRequestReviewsByLogin mocks the RequestReviewsByLogin GraphQL mutation -// used on github.com when ActorAssignees is enabled. +// used on github.com when ApiActorsSupported is enabled. func mockRequestReviewsByLogin(reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 02fe5d33c..d29b6d4c4 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -22,6 +22,13 @@ type Editable struct { Projects EditableProjects Milestone EditableString Metadata api.RepoMetadataResult + + // TODO ApiActorsSupported + // ApiActorsSupported indicates the host supports actor-based APIs (github.com, ghe.com). + // When true, mutations use logins directly instead of resolving node IDs. + // Remove this flag (and collapse to actor-only paths) once GHES supports + // replaceActorsForAssignable and requestReviewsByLogin mutations. + ApiActorsSupported bool } type EditableString struct { @@ -42,11 +49,9 @@ type EditableSlice struct { } // EditableAssignees is a special case of EditableSlice. -// It contains a flag to indicate whether the assignees are actors or not. type EditableAssignees struct { EditableSlice - ActorAssignees bool - DefaultLogins []string // For disambiguating actors from display names + DefaultLogins []string // For disambiguating actors from display names } // EditableReviewers is a special case of EditableSlice. @@ -95,7 +100,8 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str // 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 { - replacer := NewSpecialAssigneeReplacer(client, repo.RepoHost(), e.Assignees.ActorAssignees, true) + // TODO ApiActorsSupported + replacer := NewSpecialAssigneeReplacer(client, repo.RepoHost(), e.ApiActorsSupported, true) assigneeSet := set.NewStringSet() @@ -107,7 +113,8 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str // 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 { + // TODO ApiActorsSupported + if e.ApiActorsSupported { assigneeSet.AddValues(e.Assignees.DefaultLogins) } else { assigneeSet.AddValues(e.Assignees.Default) @@ -283,6 +290,7 @@ func (e *Editable) Clone() Editable { Labels: e.Labels.clone(), Projects: e.Projects.clone(), Milestone: e.Milestone.clone(), + ApiActorsSupported: e.ApiActorsSupported, // Shallow copy since no mutation. Metadata: e.Metadata, } @@ -319,9 +327,8 @@ func (es *EditableSlice) clone() EditableSlice { func (ea *EditableAssignees) clone() EditableAssignees { return EditableAssignees{ - EditableSlice: ea.EditableSlice.clone(), - ActorAssignees: ea.ActorAssignees, - DefaultLogins: ea.DefaultLogins, + EditableSlice: ea.EditableSlice.clone(), + DefaultLogins: ea.DefaultLogins, } } @@ -522,22 +529,23 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, fetchAssignees = true } // For non-interactive Add/Remove operations, we only need to fetch assignees - // on GHES where ID resolution is required. On github.com (ActorAssignees), + // on GHES where ID resolution is required. On github.com (ApiActorsSupported), // logins are passed directly to the mutation. - if (len(editable.Assignees.Add) > 0 || len(editable.Assignees.Remove) > 0) && !editable.Assignees.ActorAssignees { + // TODO ApiActorsSupported + if (len(editable.Assignees.Add) > 0 || len(editable.Assignees.Remove) > 0) && !editable.ApiActorsSupported { fetchAssignees = true } } input := api.RepoMetadataInput{ - Reviewers: fetchReviewers, - TeamReviewers: teamReviewers, - Assignees: fetchAssignees, - ActorAssignees: editable.Assignees.ActorAssignees, - Labels: editable.Labels.Edited, - ProjectsV1: editable.Projects.Edited && projectV1Support == gh.ProjectsV1Supported, - ProjectsV2: editable.Projects.Edited, - Milestones: editable.Milestone.Edited, + Reviewers: fetchReviewers, + TeamReviewers: teamReviewers, + Assignees: fetchAssignees, + ApiActorsSupported: editable.ApiActorsSupported, + Labels: editable.Labels.Edited, + ProjectsV1: editable.Projects.Edited && projectV1Support == gh.ProjectsV1Supported, + ProjectsV2: editable.Projects.Edited, + Milestones: editable.Milestone.Edited, } metadata, err := api.RepoMetadata(client, repo, input) if err != nil { @@ -574,7 +582,8 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, editable.Metadata = *metadata editable.Reviewers.Options = append(users, teams...) - if editable.Assignees.ActorAssignees { + // TODO ApiActorsSupported + if editable.ApiActorsSupported { editable.Assignees.Options = actors } else { editable.Assignees.Options = users diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index bb63ceb0f..39140eefd 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -66,7 +66,8 @@ 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 - if options.Assignees.Edited && options.Assignees.ActorAssignees { + // TODO ApiActorsSupported + if options.Assignees.Edited && options.ApiActorsSupported { apiClient := api.NewClientFromHTTP(httpClient) logins, err := options.AssigneeLogins(apiClient, repo) if err != nil { @@ -99,7 +100,8 @@ func replaceIssueFields(httpClient *http.Client, repo ghrepo.Interface, id strin } var assigneeIds *[]string - if !options.Assignees.ActorAssignees { + // TODO ApiActorsSupported + if !options.ApiActorsSupported { assigneeIds, err = options.AssigneeIds(apiClient, repo) if err != nil { return err diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 17d40d639..76f096efd 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -61,11 +61,13 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par return nil } - // When ActorReviewers is true, we use login-based mutation and don't need to resolve reviewer IDs. - needReviewerIDs := len(tb.Reviewers) > 0 && !tb.ActorReviewers + // TODO ApiActorsSupported + // When ApiActorsSupported is true, we use login-based mutation and don't need to resolve reviewer IDs. + needReviewerIDs := len(tb.Reviewers) > 0 && !tb.ApiActorsSupported - // When ActorAssignees is true, we use login-based mutation and don't need to resolve assignee IDs. - needAssigneeIDs := len(tb.Assignees) > 0 && !tb.ActorAssignees + // TODO ApiActorsSupported + // When ApiActorsSupported is true, we use login-based mutation and don't need to resolve assignee IDs. + needAssigneeIDs := len(tb.Assignees) > 0 && !tb.ApiActorsSupported // Retrieve minimal information needed to resolve metadata if this was not previously cached from additional metadata survey. if tb.MetadataResult == nil { @@ -88,9 +90,10 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par tb.MetadataResult = metadataResult } - // When ActorAssignees is true (github.com), pass logins directly for use with + // TODO ApiActorsSupported + // When ApiActorsSupported is true (github.com), pass logins directly for use with // ReplaceActorsForAssignable mutation. The ID-based else branch is for GHES compatibility. - if tb.ActorAssignees { + if tb.ApiActorsSupported { params["assigneeLogins"] = tb.Assignees } else { assigneeIDs, err := tb.MetadataResult.MembersToIDs(tb.Assignees) @@ -138,11 +141,11 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par } } - // TODO requestReviewsByLoginCleanup - // When ActorReviewers is true (github.com), pass logins directly for use with + // TODO ApiActorsSupported + // When ApiActorsSupported is true (github.com), pass logins directly for use with // RequestReviewsByLogin mutation. The ID-based else branch can be removed once // GHES supports requestReviewsByLogin. - if tb.ActorReviewers { + if tb.ApiActorsSupported { params["userReviewerLogins"] = userReviewers if len(botReviewers) > 0 { params["botReviewerLogins"] = botReviewers diff --git a/pkg/cmd/pr/shared/state.go b/pkg/cmd/pr/shared/state.go index 0e5c31cdd..32807ae4d 100644 --- a/pkg/cmd/pr/shared/state.go +++ b/pkg/cmd/pr/shared/state.go @@ -18,9 +18,14 @@ const ( type IssueMetadataState struct { Type metadataStateType - Draft bool - ActorAssignees bool - ActorReviewers bool + Draft bool + + // TODO ApiActorsSupported + // ApiActorsSupported indicates the host supports actor-based APIs (github.com, ghe.com). + // When true, mutations use logins directly instead of resolving node IDs. + // Remove this flag (and collapse to actor-only paths) once GHES supports + // replaceActorsForAssignable and requestReviewsByLogin mutations. + ApiActorsSupported bool Body string Title string diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 71cfcd063..05b41d79b 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -181,19 +181,20 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface } // Retrieve and process data for survey prompts based on the extra fields selected. - // When search-based reviewer selection is available, skip the expensive assignable-users - // and teams fetch since reviewers are found dynamically via the search function. - useReviewerSearch := state.ActorReviewers && reviewerSearchFunc != nil - useAssigneeSearch := state.ActorAssignees && assigneeSearchFunc != nil + // When search-based selection is available, skip the expensive assignable-users + // and teams fetch since they are found dynamically via the search function. + // TODO ApiActorsSupported + useReviewerSearch := state.ApiActorsSupported && reviewerSearchFunc != nil + useAssigneeSearch := state.ApiActorsSupported && assigneeSearchFunc != nil metadataInput := api.RepoMetadataInput{ - Reviewers: isChosen("Reviewers") && !useReviewerSearch, - TeamReviewers: isChosen("Reviewers") && !useReviewerSearch, - Assignees: isChosen("Assignees") && !useAssigneeSearch, - ActorAssignees: isChosen("Assignees") && !useAssigneeSearch && state.ActorAssignees, - Labels: isChosen("Labels"), - ProjectsV1: isChosen("Projects") && projectsV1Support == gh.ProjectsV1Supported, - ProjectsV2: isChosen("Projects"), - Milestones: isChosen("Milestone"), + Reviewers: isChosen("Reviewers") && !useReviewerSearch, + TeamReviewers: isChosen("Reviewers") && !useReviewerSearch, + Assignees: isChosen("Assignees") && !useAssigneeSearch, + ApiActorsSupported: state.ApiActorsSupported, + Labels: isChosen("Labels"), + ProjectsV1: isChosen("Projects") && projectsV1Support == gh.ProjectsV1Supported, + ProjectsV2: isChosen("Projects"), + Milestones: isChosen("Milestone"), } metadataResult, err := fetcher.RepoMetadataFetch(metadataInput) if err != nil { @@ -217,7 +218,8 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface var assignees []string var assigneesDefault []string if !useAssigneeSearch { - if state.ActorAssignees { + // TODO ApiActorsSupported + if state.ApiActorsSupported { for _, u := range metadataResult.AssignableActors { assignees = append(assignees, u.DisplayName()) @@ -261,7 +263,7 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface }{} if isChosen("Reviewers") { - if reviewerSearchFunc != nil { + if useReviewerSearch { selectedReviewers, err := p.MultiSelectWithSearch( "Reviewers", "Search reviewers", @@ -273,7 +275,7 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface } values.Reviewers = selectedReviewers } else if len(reviewers) > 0 { - // TODO requestReviewsByLoginCleanup + // TODO ApiActorsSupported // The static MultiSelect path can be removed once GHES supports // requestReviewsByLogin and search-based selection is always used. selected, err := p.MultiSelect("Reviewers", state.Reviewers, reviewers) @@ -305,7 +307,8 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface return err } for _, i := range selected { - if state.ActorAssignees { + // TODO ApiActorsSupported + if state.ApiActorsSupported { values.Assignees = append(values.Assignees, metadataResult.AssignableActors[i].Login()) } else { values.Assignees = append(values.Assignees, metadataResult.AssignableUsers[i].Login())