From abd573ac66e8e4abd18cdc7abaef7deb60502fc6 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Mon, 7 Apr 2025 14:40:03 -0600 Subject: [PATCH 01/59] feat(preview): add `preview prompter` command --- pkg/cmd/preview/preview.go | 20 +++ pkg/cmd/preview/prompter/prompter.go | 225 +++++++++++++++++++++++++++ pkg/cmd/root/root.go | 2 + 3 files changed, 247 insertions(+) create mode 100644 pkg/cmd/preview/preview.go create mode 100644 pkg/cmd/preview/prompter/prompter.go diff --git a/pkg/cmd/preview/preview.go b/pkg/cmd/preview/preview.go new file mode 100644 index 000000000..5c3209722 --- /dev/null +++ b/pkg/cmd/preview/preview.go @@ -0,0 +1,20 @@ +package preview + +import ( + cmdPrompter "github.com/cli/cli/v2/pkg/cmd/preview/prompter" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/spf13/cobra" +) + +func NewCmdPreview(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "preview ", + Short: "Execute previews for gh features", + } + + cmdutil.DisableAuthCheck(cmd) + + cmd.AddCommand(cmdPrompter.NewCmdPrompter(f, nil)) + + return cmd +} diff --git a/pkg/cmd/preview/prompter/prompter.go b/pkg/cmd/preview/prompter/prompter.go new file mode 100644 index 000000000..97fffb23a --- /dev/null +++ b/pkg/cmd/preview/prompter/prompter.go @@ -0,0 +1,225 @@ +package prompter + +import ( + "fmt" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +type prompterOptions struct { + IO *iostreams.IOStreams + Config func() (gh.Config, error) + + PromptsToRun []func(prompter.Prompter) error +} + +func NewCmdPrompter(f *cmdutil.Factory, runF func(*prompterOptions) error) *cobra.Command { + opts := &prompterOptions{ + IO: f.IOStreams, + Config: f.Config, + } + + const ( + selectPrompt = "select" + multiSelectPrompt = "multi-select" + inputPrompt = "input" + passwordPrompt = "password" + confirmPrompt = "confirm" + authTokenPrompt = "auth-token" + confirmDeletionPrompt = "confirm-deletion" + inputHostnamePrompt = "input-hostname" + markdownEditorPrompt = "markdown-editor" + ) + + prompterTypeFuncMap := map[string]func(prompter.Prompter) error{ + selectPrompt: runSelect, + multiSelectPrompt: runMultiSelect, + inputPrompt: runInput, + passwordPrompt: runPassword, + confirmPrompt: runConfirm, + authTokenPrompt: runAuthToken, + confirmDeletionPrompt: runConfirmDeletion, + inputHostnamePrompt: runInputHostname, + markdownEditorPrompt: runMarkdownEditor, + } + + cmd := &cobra.Command{ + Use: "prompter [prompt type]", + Short: "Execute a test program to preview the prompter", + Long: heredoc.Doc(` + Execute a test program to preview the prompter. + Without an argument, all prompts will be run. + + Available prompt types: + - select + - multi-select + - input + - password + - confirm + - auth-token + - confirm-deletion + - input-hostname + - markdown-editor + `), + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if runF != nil { + return runF(opts) + } + + if len(args) == 0 { + // All prompts + for _, f := range prompterTypeFuncMap { + opts.PromptsToRun = append(opts.PromptsToRun, f) + } + } else { + // Only the one specified + for _, arg := range args { + f, ok := prompterTypeFuncMap[arg] + if !ok { + return fmt.Errorf("unknown prompter type: %q", arg) + } + opts.PromptsToRun = append(opts.PromptsToRun, f) + } + } + + return prompterRun(opts) + }, + } + + return cmd +} + +func prompterRun(opts *prompterOptions) error { + editor, err := cmdutil.DetermineEditor(opts.Config) + if err != nil { + return err + } + + p := prompter.New(editor, opts.IO.In, opts.IO.Out, opts.IO.ErrOut) + + for _, f := range opts.PromptsToRun { + if err := f(p); err != nil { + return err + } + // Newline for readability + fmt.Println() + } + + return nil +} + +func runSelect(p prompter.Prompter) error { + fmt.Println("Demonstrating Single Select") + cuisines := []string{"Italian", "Greek", "Indian", "Japanese", "American"} + favorite, err := p.Select("Favorite cuisine?", "Italian", cuisines) + if err != nil { + return err + } + fmt.Printf("Favorite cuisine: %s\n", cuisines[favorite]) + return nil +} + +func runMultiSelect(p prompter.Prompter) error { + fmt.Println("Demonstrating Multi Select") + cuisines := []string{"Italian", "Greek", "Indian", "Japanese", "American"} + favorites, err := p.MultiSelect("Favorite cuisines?", []string{}, cuisines) + if err != nil { + return err + } + for _, f := range favorites { + fmt.Printf("Favorite cuisine: %s\n", cuisines[f]) + } + return nil +} + +func runInput(p prompter.Prompter) error { + fmt.Println("Demonstrating Text Input") + text, err := p.Input("Favorite meal?", "Breakfast") + if err != nil { + return err + } + fmt.Printf("You typed: %s\n", text) + return nil +} + +func runPassword(p prompter.Prompter) error { + fmt.Println("Demonstrating Password Input") + safeword, err := p.Password("Safe word?") + if err != nil { + return err + } + fmt.Printf("Safe word: %s\n", safeword) + return nil +} + +func runConfirm(p prompter.Prompter) error { + fmt.Println("Demonstrating Confirmation") + confirmation, err := p.Confirm("Are you sure?", true) + if err != nil { + return err + } + fmt.Printf("Confirmation: %t\n", confirmation) + return nil +} + +func runAuthToken(p prompter.Prompter) error { + fmt.Println("Demonstrating Auth Token (can't be blank)") + token, err := p.AuthToken() + if err != nil { + return err + } + fmt.Printf("Auth token: %s\n", token) + return nil +} + +func runConfirmDeletion(p prompter.Prompter) error { + fmt.Println("Demonstrating Deletion Confirmation") + err := p.ConfirmDeletion("delete-me") + if err != nil { + return err + } + fmt.Println("Item deleted") + return nil +} + +func runInputHostname(p prompter.Prompter) error { + fmt.Println("Demonstrating Hostname") + hostname, err := p.InputHostname() + if err != nil { + return err + } + fmt.Printf("Hostname: %s\n", hostname) + return nil +} + +func runMarkdownEditor(p prompter.Prompter) error { + defaultText := "default text value" + + fmt.Println("Demonstrating Markdown Editor with blanks allowed and default text") + editorText, err := p.MarkdownEditor("Edit your text:", defaultText, true) + if err != nil { + return err + } + fmt.Printf("Returned text: %s\n", editorText) + + fmt.Println("Demonstrating Markdown Editor with blanks disallowed and default text") + editorText2, err := p.MarkdownEditor("Edit your text:", defaultText, false) + if err != nil { + return err + } + fmt.Printf("Returned text: %s\n", editorText2) + + fmt.Println("Demonstrating Markdown Editor with blanks disallowed and no default text") + editorText3, err := p.MarkdownEditor("Edit your text:", "", false) + if err != nil { + return err + } + fmt.Printf("Returned text: %s\n", editorText3) + return nil +} diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index c0dad93ec..ce142b32d 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -25,6 +25,7 @@ import ( labelCmd "github.com/cli/cli/v2/pkg/cmd/label" orgCmd "github.com/cli/cli/v2/pkg/cmd/org" prCmd "github.com/cli/cli/v2/pkg/cmd/pr" + previewCmd "github.com/cli/cli/v2/pkg/cmd/preview" projectCmd "github.com/cli/cli/v2/pkg/cmd/project" releaseCmd "github.com/cli/cli/v2/pkg/cmd/release" repoCmd "github.com/cli/cli/v2/pkg/cmd/repo" @@ -139,6 +140,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, cmd.AddCommand(statusCmd.NewCmdStatus(f, nil)) cmd.AddCommand(codespaceCmd.NewCmdCodespace(f)) cmd.AddCommand(projectCmd.NewCmdProject(f)) + cmd.AddCommand(previewCmd.NewCmdPreview(f)) // below here at the commands that require the "intelligent" BaseRepo resolver repoResolvingCmdFactory := *f From 54a929dcd9684dc11dc23b68f09e4ed04821faa1 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 16 Apr 2025 15:49:30 -0600 Subject: [PATCH 02/59] feat(preview): enforce fixed order for prompts --- pkg/cmd/preview/prompter/prompter.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/preview/prompter/prompter.go b/pkg/cmd/preview/prompter/prompter.go index 97fffb23a..5b9b91f23 100644 --- a/pkg/cmd/preview/prompter/prompter.go +++ b/pkg/cmd/preview/prompter/prompter.go @@ -48,6 +48,18 @@ func NewCmdPrompter(f *cmdutil.Factory, runF func(*prompterOptions) error) *cobr markdownEditorPrompt: runMarkdownEditor, } + allPromptsOrder := []string{ + selectPrompt, + multiSelectPrompt, + inputPrompt, + passwordPrompt, + confirmPrompt, + authTokenPrompt, + confirmDeletionPrompt, + inputHostnamePrompt, + markdownEditorPrompt, + } + cmd := &cobra.Command{ Use: "prompter [prompt type]", Short: "Execute a test program to preview the prompter", @@ -73,8 +85,9 @@ func NewCmdPrompter(f *cmdutil.Factory, runF func(*prompterOptions) error) *cobr } if len(args) == 0 { - // All prompts - for _, f := range prompterTypeFuncMap { + // All prompts, in a fixed order + for _, promptType := range allPromptsOrder { + f := prompterTypeFuncMap[promptType] opts.PromptsToRun = append(opts.PromptsToRun, f) } } else { @@ -206,14 +219,14 @@ func runMarkdownEditor(p prompter.Prompter) error { if err != nil { return err } - fmt.Printf("Returned text: %s\n", editorText) + fmt.Printf("Returned text: %s\n\n", editorText) fmt.Println("Demonstrating Markdown Editor with blanks disallowed and default text") editorText2, err := p.MarkdownEditor("Edit your text:", defaultText, false) if err != nil { return err } - fmt.Printf("Returned text: %s\n", editorText2) + fmt.Printf("Returned text: %s\n\n", editorText2) fmt.Println("Demonstrating Markdown Editor with blanks disallowed and no default text") editorText3, err := p.MarkdownEditor("Edit your text:", "", false) From 8ebbd1d4bf0ad67c420d41bbe04fdf4ccb46fa15 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 7 May 2025 12:19:16 -0600 Subject: [PATCH 03/59] feat(fd): add ActorIsAssignable to IssueFeatures --- internal/featuredetection/feature_detection.go | 9 ++++++--- internal/featuredetection/feature_detection_test.go | 9 ++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/featuredetection/feature_detection.go b/internal/featuredetection/feature_detection.go index fba317f58..5ff08a573 100644 --- a/internal/featuredetection/feature_detection.go +++ b/internal/featuredetection/feature_detection.go @@ -18,11 +18,13 @@ type Detector interface { } type IssueFeatures struct { - StateReason bool + StateReason bool + ActorIsAssignable bool } var allIssueFeatures = IssueFeatures{ - StateReason: true, + StateReason: true, + ActorIsAssignable: true, } type PullRequestFeatures struct { @@ -70,7 +72,8 @@ func (d *detector) IssueFeatures() (IssueFeatures, error) { } features := IssueFeatures{ - StateReason: false, + StateReason: false, + ActorIsAssignable: false, } var featureDetection struct { diff --git a/internal/featuredetection/feature_detection_test.go b/internal/featuredetection/feature_detection_test.go index f1152da2c..2c7d19071 100644 --- a/internal/featuredetection/feature_detection_test.go +++ b/internal/featuredetection/feature_detection_test.go @@ -23,7 +23,8 @@ func TestIssueFeatures(t *testing.T) { name: "github.com", hostname: "github.com", wantFeatures: IssueFeatures{ - StateReason: true, + StateReason: true, + ActorIsAssignable: true, }, wantErr: false, }, @@ -31,7 +32,8 @@ func TestIssueFeatures(t *testing.T) { name: "ghec data residency (ghe.com)", hostname: "stampname.ghe.com", wantFeatures: IssueFeatures{ - StateReason: true, + StateReason: true, + ActorIsAssignable: true, }, wantErr: false, }, @@ -42,7 +44,8 @@ func TestIssueFeatures(t *testing.T) { `query Issue_fields\b`: `{"data": {}}`, }, wantFeatures: IssueFeatures{ - StateReason: false, + StateReason: false, + ActorIsAssignable: false, }, wantErr: false, }, From 38e52db3779e9a6a199b6799283a452bcbe0802b Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 8 May 2025 11:06:34 -0600 Subject: [PATCH 04/59] feat(issue edit): fetch currently assigned actors --- api/queries_issue.go | 16 +++++++ api/queries_repo.go | 7 +++ pkg/cmd/issue/edit/edit.go | 41 +++++++++++++++--- pkg/cmd/issue/edit/edit_test.go | 75 +++++++++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 7 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index f09360152..2b15a41b9 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -38,6 +38,7 @@ type Issue struct { Comments Comments Author Author Assignees Assignees + AssignedActors ActorAssignees Labels Labels ProjectCards ProjectCards ProjectItems ProjectItems @@ -91,6 +92,21 @@ func (a Assignees) Logins() []string { return logins } +type ActorAssignees struct { + Edges []struct { + Node Actor + } + TotalCount int +} + +func (a ActorAssignees) Logins() []string { + logins := make([]string, len(a.Edges)) + for i, a := range a.Edges { + logins[i] = a.Node.Login + } + return logins +} + type Labels struct { Nodes []IssueLabel TotalCount int diff --git a/api/queries_repo.go b/api/queries_repo.go index 93a32d80c..06ae48b12 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -146,6 +146,13 @@ 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. +type Actor struct { + ID string `json:"id"` + Login string `json:"login"` +} + // BranchRef is the branch name in a GitHub repository type BranchRef struct { Name string `json:"name"` diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 8386cbcfa..a5694e6c9 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -197,9 +197,35 @@ func editRun(opts *EditOptions) error { } } + if opts.Detector == nil { + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost()) + } + + issueFeatures, err := opts.Detector.IssueFeatures() + if err != nil { + return err + } + lookupFields := []string{"id", "number", "title", "body", "url"} if editable.Assignees.Edited { - lookupFields = append(lookupFields, "assignees") + if issueFeatures.ActorIsAssignable { + // At the time of writing, only 10 Actors can be assigned to an issue. + assignedActors := heredoc.Doc(` + assignedActors(first: 10) { + edges { + node { + ... on Actor { + login + } + } + } + } + `) + lookupFields = append(lookupFields, assignedActors) + } else { + lookupFields = append(lookupFields, "assignees") + } } if editable.Labels.Edited { lookupFields = append(lookupFields, "labels") @@ -207,11 +233,6 @@ func editRun(opts *EditOptions) error { if editable.Projects.Edited { // TODO projectsV1Deprecation // Remove this section as we should no longer add projectCards - if opts.Detector == nil { - cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) - opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost()) - } - projectsV1Support := opts.Detector.ProjectsV1() if projectsV1Support == gh.ProjectsV1Supported { lookupFields = append(lookupFields, "projectCards") @@ -254,7 +275,13 @@ func editRun(opts *EditOptions) error { editable.Title.Default = issue.Title editable.Body.Default = issue.Body - editable.Assignees.Default = issue.Assignees.Logins() + // We use Actors as the default assignees if Actors are assignable + // on this GitHub host. + if issueFeatures.ActorIsAssignable { + editable.Assignees.Default = issue.AssignedActors.Logins() + } else { + editable.Assignees.Default = issue.Assignees.Logins() + } editable.Labels.Default = issue.Labels.Names() editable.Projects.Default = append(issue.ProjectCards.ProjectNames(), issue.ProjectItems.ProjectTitles()...) projectItems := map[string]string{} diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index c9aa4c409..d0115ab4e 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -875,3 +875,78 @@ func TestProjectsV1Deprecation(t *testing.T) { reg.Verify(t) }) } + +func TestActorIsAssignable(t *testing.T) { + t.Run("when actors are assignable, query includes assignedActors", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`assignedActors`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we don't care. + _ = editRun(&EditOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Detector: &fd.EnabledDetectorMock{}, + IssueNumbers: []int{123}, + Editable: prShared.Editable{ + Assignees: prShared.EditableSlice{ + Add: []string{"monalisa", "octocat"}, + Edited: true, + }, + }, + }) + + reg.Verify(t) + }) + + t.Run("when actors are not assignable, query includes assignees instead", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + // This test should NOT include assignedActors in the query + reg.Exclude(t, httpmock.GraphQL(`assignedActors`)) + // It should include the regular assignees field + reg.Register( + httpmock.GraphQL(`assignees`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we're not really interested in it. + _ = editRun(&EditOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Detector: &fd.DisabledDetectorMock{}, + IssueNumbers: []int{123}, + Editable: prShared.Editable{ + Assignees: prShared.EditableSlice{ + Add: []string{"monalisa", "octocat"}, + Edited: true, + }, + }, + }) + + reg.Verify(t) + }) +} From ee9d16920425caf41dc9f1f1ca4bd7fc388acbd4 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 9 May 2025 13:37:15 -0600 Subject: [PATCH 05/59] feat(issue edit): fetch assignable actors --- api/queries_repo.go | 141 +++++++++++++++++++++++++++----- pkg/cmd/issue/edit/edit.go | 4 +- pkg/cmd/issue/edit/edit_test.go | 97 ++++++++++++++-------- pkg/cmd/pr/edit/edit_test.go | 36 ++++---- pkg/cmd/pr/shared/editable.go | 39 +++++++-- 5 files changed, 240 insertions(+), 77 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 06ae48b12..99041e49f 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -681,13 +681,14 @@ func RepoFindForks(client *Client, repo ghrepo.Interface, limit int) ([]*Reposit } type RepoMetadataResult struct { - CurrentLogin string - AssignableUsers []RepoAssignee - Labels []RepoLabel - Projects []RepoProject - ProjectsV2 []ProjectV2 - Milestones []RepoMilestone - Teams []OrgTeam + CurrentLogin string + AssignableUsers []RepoAssignee + AssignableActors []RepoAssignee + Labels []RepoLabel + Projects []RepoProject + ProjectsV2 []ProjectV2 + Milestones []RepoMilestone + Teams []OrgTeam } func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { @@ -701,6 +702,16 @@ func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { break } } + + // Look for ID in assignable actors if not found in assignable users + for _, u := range m.AssignableActors { + if strings.EqualFold(assigneeLogin, u.Login) { + ids = append(ids, u.ID) + found = true + break + } + } + if !found { return nil, fmt.Errorf("'%s' not found", assigneeLogin) } @@ -892,12 +903,13 @@ func (m *RepoMetadataResult) Merge(m2 *RepoMetadataResult) { } type RepoMetadataInput struct { - Assignees bool - Reviewers bool - Labels bool - ProjectsV1 bool - ProjectsV2 bool - Milestones bool + Assignees bool + ActorAssignees bool + Reviewers bool + Labels bool + ProjectsV1 bool + ProjectsV2 bool + Milestones bool } // RepoMetadata pre-fetches the metadata for attaching to issues and pull requests @@ -906,14 +918,48 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput var g errgroup.Group if input.Assignees || input.Reviewers { - g.Go(func() error { - users, err := RepoAssignableUsers(client, repo) - if err != nil { - err = fmt.Errorf("error fetching assignees: %w", err) + + if input.ActorAssignees { + g.Go(func() error { + actors, err := RepoAssignableActors(client, repo) + if err != nil { + err = fmt.Errorf("error fetching assignees: %w", err) + } + result.AssignableActors = actors + return err + }) + + // If reviewers are 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. + if input.Reviewers { + g.Go(func() error { + users, err := RepoAssignableUsers(client, repo) + if err != nil { + err = fmt.Errorf("error fetching assignees: %w", err) + } + result.AssignableUsers = users + return err + }) } - result.AssignableUsers = users - return err - }) + } else { + // Not using Actors, fetch legacy assignable users. + g.Go(func() error { + users, err := RepoAssignableUsers(client, repo) + if err != nil { + err = fmt.Errorf("error fetching assignees: %w", err) + } + result.AssignableUsers = users + return err + }) + } + } if input.Reviewers { @@ -1186,6 +1232,61 @@ func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]RepoAssignee, return users, nil } +// RepoAssignableActors fetches all the assignable actors for a repository on +// GitHub hosts that support Actor assignees. +func RepoAssignableActors(client *Client, repo ghrepo.Interface) ([]RepoAssignee, error) { + type repoActorAssignee struct { + ID string + Login string + } + + type responseData struct { + Repository struct { + SuggestedActors struct { + Nodes []struct { + User RepoAssignee `graphql:"... on User"` + Bot repoActorAssignee `graphql:"... on Bot"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"suggestedActors(first: 100, after: $endCursor, capabilities: CAN_BE_ASSIGNED)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + "endCursor": (*githubv4.String)(nil), + } + + var actors []RepoAssignee + for { + var query responseData + err := client.Query(repo.RepoHost(), "RepositoryAssignableActors", &query, variables) + if err != nil { + return nil, err + } + + for _, node := range query.Repository.SuggestedActors.Nodes { + // Edge case if the Actor is not a Bot or a User, + // it won't be unmarshalled properly, and we'll have an + // zero value node. + if node.User.ID == "" || node.User.Login == "" { + continue + } + actors = append(actors, node.User) + } + + if !query.Repository.SuggestedActors.PageInfo.HasNextPage { + break + } + variables["endCursor"] = githubv4.String(query.Repository.SuggestedActors.PageInfo.EndCursor) + } + return actors, nil +} + type RepoLabel struct { ID string Name string diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index a5694e6c9..9177f60ad 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -210,6 +210,8 @@ func editRun(opts *EditOptions) error { lookupFields := []string{"id", "number", "title", "body", "url"} if editable.Assignees.Edited { if issueFeatures.ActorIsAssignable { + editable.Assignees.ActorAssignees = true + // At the time of writing, only 10 Actors can be assigned to an issue. assignedActors := heredoc.Doc(` assignedActors(first: 10) { @@ -277,7 +279,7 @@ 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 issueFeatures.ActorIsAssignable { + if editable.Assignees.ActorAssignees { editable.Assignees.Default = 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 d0115ab4e..01cd74900 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -118,9 +118,11 @@ func TestNewCmdEdit(t *testing.T) { output: EditOptions{ IssueNumbers: []int{23}, Editable: prShared.Editable{ - Assignees: prShared.EditableSlice{ - Add: []string{"monalisa", "hubot"}, - Edited: true, + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Edited: true, + }, }, }, }, @@ -132,9 +134,11 @@ func TestNewCmdEdit(t *testing.T) { output: EditOptions{ IssueNumbers: []int{23}, Editable: prShared.Editable{ - Assignees: prShared.EditableSlice{ - Remove: []string{"monalisa", "hubot"}, - Edited: true, + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Remove: []string{"monalisa", "hubot"}, + Edited: true, + }, }, }, }, @@ -354,10 +358,12 @@ func Test_editRun(t *testing.T) { Value: "new body", Edited: true, }, - Assignees: prShared.EditableSlice{ - Add: []string{"monalisa", "hubot"}, - Remove: []string{"octocat"}, - Edited: true, + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Remove: []string{"octocat"}, + Edited: true, + }, }, Labels: prShared.EditableSlice{ Add: []string{"feature", "TODO", "bug"}, @@ -399,10 +405,12 @@ func Test_editRun(t *testing.T) { IssueNumbers: []int{456, 123}, Interactive: false, Editable: prShared.Editable{ - Assignees: prShared.EditableSlice{ - Add: []string{"monalisa", "hubot"}, - Remove: []string{"octocat"}, - Edited: true, + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Remove: []string{"octocat"}, + Edited: true, + }, }, Labels: prShared.EditableSlice{ Add: []string{"feature", "TODO", "bug"}, @@ -449,10 +457,12 @@ func Test_editRun(t *testing.T) { IssueNumbers: []int{123, 9999}, Interactive: false, Editable: prShared.Editable{ - Assignees: prShared.EditableSlice{ - Add: []string{"monalisa", "hubot"}, - Remove: []string{"octocat"}, - Edited: true, + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Remove: []string{"octocat"}, + Edited: true, + }, }, Labels: prShared.EditableSlice{ Add: []string{"feature", "TODO", "bug"}, @@ -494,10 +504,12 @@ func Test_editRun(t *testing.T) { IssueNumbers: []int{123, 456}, Interactive: false, Editable: prShared.Editable{ - Assignees: prShared.EditableSlice{ - Add: []string{"monalisa", "hubot"}, - Remove: []string{"octocat"}, - Edited: true, + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Remove: []string{"octocat"}, + Edited: true, + }, }, Milestone: prShared.EditableString{ Value: "GA", @@ -509,14 +521,14 @@ 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 RepositoryAssignableUsers\b`), + httpmock.GraphQL(`query RepositoryAssignableActors\b`), httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { + { "data": { "repository": { "suggestedActors": { "nodes": [ { "login": "hubot", "id": "HUBOTID" }, { "login": "MonaLisa", "id": "MONAID" } ], - "pageInfo": { "hasNextPage": false } + "pageInfo": { "hasNextPage": false, "endCursor": "Mg" } } } } } `)) reg.Register( @@ -669,17 +681,30 @@ func mockIssueProjectItemsGet(_ *testing.T, reg *httpmock.Registry) { } func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) { + // reg.Register( + // httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + // httpmock.StringResponse(` + // { "data": { "repository": { "assignableUsers": { + // "nodes": [ + // { "login": "hubot", "id": "HUBOTID" }, + // { "login": "MonaLisa", "id": "MONAID" } + // ], + // "pageInfo": { "hasNextPage": false } + // } } } } + // `)) + reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.GraphQL(`query RepositoryAssignableActors\b`), httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { + { "data": { "repository": { "suggestedActors": { "nodes": [ { "login": "hubot", "id": "HUBOTID" }, { "login": "MonaLisa", "id": "MONAID" } ], - "pageInfo": { "hasNextPage": false } + "pageInfo": { "hasNextPage": false, "endCursor": "Mg" } } } } } `)) + reg.Register( httpmock.GraphQL(`query RepositoryLabelList\b`), httpmock.StringResponse(` @@ -902,9 +927,11 @@ func TestActorIsAssignable(t *testing.T) { Detector: &fd.EnabledDetectorMock{}, IssueNumbers: []int{123}, Editable: prShared.Editable{ - Assignees: prShared.EditableSlice{ - Add: []string{"monalisa", "octocat"}, - Edited: true, + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "octocat"}, + Edited: true, + }, }, }, }) @@ -940,9 +967,11 @@ func TestActorIsAssignable(t *testing.T) { Detector: &fd.DisabledDetectorMock{}, IssueNumbers: []int{123}, Editable: prShared.Editable{ - Assignees: prShared.EditableSlice{ - Add: []string{"monalisa", "octocat"}, - Edited: true, + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "octocat"}, + Edited: true, + }, }, }, }) diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 3c4882961..f45984c76 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -165,9 +165,11 @@ func TestNewCmdEdit(t *testing.T) { output: EditOptions{ SelectorArg: "23", Editable: shared.Editable{ - Assignees: shared.EditableSlice{ - Add: []string{"monalisa", "hubot"}, - Edited: true, + Assignees: shared.EditableAssignees{ + EditableSlice: shared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Edited: true, + }, }, }, }, @@ -179,9 +181,11 @@ func TestNewCmdEdit(t *testing.T) { output: EditOptions{ SelectorArg: "23", Editable: shared.Editable{ - Assignees: shared.EditableSlice{ - Remove: []string{"monalisa", "hubot"}, - Edited: true, + Assignees: shared.EditableAssignees{ + EditableSlice: shared.EditableSlice{ + Remove: []string{"monalisa", "hubot"}, + Edited: true, + }, }, }, }, @@ -359,10 +363,12 @@ func Test_editRun(t *testing.T) { Remove: []string{"dependabot"}, Edited: true, }, - Assignees: shared.EditableSlice{ - Add: []string{"monalisa", "hubot"}, - Remove: []string{"octocat"}, - Edited: true, + Assignees: shared.EditableAssignees{ + EditableSlice: shared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Remove: []string{"octocat"}, + Edited: true, + }, }, Labels: shared.EditableSlice{ Add: []string{"feature", "TODO", "bug"}, @@ -413,10 +419,12 @@ func Test_editRun(t *testing.T) { Value: "base-branch-name", Edited: true, }, - Assignees: shared.EditableSlice{ - Add: []string{"monalisa", "hubot"}, - Remove: []string{"octocat"}, - Edited: true, + Assignees: shared.EditableAssignees{ + EditableSlice: shared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Remove: []string{"octocat"}, + Edited: true, + }, }, Labels: shared.EditableSlice{ Add: []string{"feature", "TODO", "bug"}, diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index e73b3c294..238939efe 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -14,7 +14,7 @@ type Editable struct { Body EditableString Base EditableString Reviewers EditableSlice - Assignees EditableSlice + Assignees EditableAssignees Labels EditableSlice Projects EditableProjects Milestone EditableString @@ -38,6 +38,13 @@ type EditableSlice struct { Allowed bool } +// 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 +} + // ProjectsV2 mutations require a mapping of an item ID to a project ID. // Keep that map along with standard EditableSlice data. type EditableProjects struct { @@ -245,6 +252,13 @@ func (es *EditableSlice) clone() EditableSlice { return cpy } +func (ea *EditableAssignees) clone() EditableAssignees { + return EditableAssignees{ + EditableSlice: ea.EditableSlice.clone(), + ActorAssignees: ea.ActorAssignees, + } +} + func (ep *EditableProjects) clone() EditableProjects { return EditableProjects{ EditableSlice: ep.EditableSlice.clone(), @@ -378,12 +392,13 @@ func FieldsToEditSurvey(p EditPrompter, editable *Editable) error { func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) error { input := api.RepoMetadataInput{ - Reviewers: editable.Reviewers.Edited, - Assignees: editable.Assignees.Edited, - Labels: editable.Labels.Edited, - ProjectsV1: editable.Projects.Edited, - ProjectsV2: editable.Projects.Edited, - Milestones: editable.Milestone.Edited, + Reviewers: editable.Reviewers.Edited, + Assignees: editable.Assignees.Edited, + ActorAssignees: editable.Assignees.ActorAssignees, + Labels: editable.Labels.Edited, + ProjectsV1: editable.Projects.Edited, + ProjectsV2: editable.Projects.Edited, + Milestones: editable.Milestone.Edited, } metadata, err := api.RepoMetadata(client, repo, input) if err != nil { @@ -394,6 +409,10 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) for _, u := range metadata.AssignableUsers { users = append(users, u.Login) } + var actors []string + for _, a := range metadata.AssignableActors { + actors = append(actors, a.Login) + } var teams []string for _, t := range metadata.Teams { teams = append(teams, fmt.Sprintf("%s/%s", repo.RepoOwner(), t.Slug)) @@ -416,7 +435,11 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) editable.Metadata = *metadata editable.Reviewers.Options = append(users, teams...) - editable.Assignees.Options = users + if editable.Assignees.ActorAssignees { + editable.Assignees.Options = actors + } else { + editable.Assignees.Options = users + } editable.Labels.Options = labels editable.Projects.Options = projects editable.Milestone.Options = milestones From 0efdfed0680c5afc8daf32da5c44928139684ca5 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 9 May 2025 22:56:09 -0600 Subject: [PATCH 06/59] feat(issue edit): support assigning actors to issues --- pkg/cmd/issue/edit/edit_test.go | 187 +++++++++++++++++------------ pkg/cmd/pr/shared/editable_http.go | 46 +++++++ 2 files changed, 154 insertions(+), 79 deletions(-) diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 01cd74900..d27bbdee6 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -394,6 +394,7 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockRepoMetadata(t, reg) mockIssueUpdate(t, reg) + mockIssueUpdateActorAssignees(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) }, @@ -441,6 +442,8 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockIssueUpdate(t, reg) mockIssueUpdate(t, reg) + mockIssueUpdateActorAssignees(t, reg) + mockIssueUpdateActorAssignees(t, reg) mockIssueUpdateLabels(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) @@ -546,6 +549,14 @@ func Test_editRun(t *testing.T) { mockIssueNumberGet(t, reg, 123) mockIssueNumberGet(t, reg, 456) // Updating 123 should succeed. + reg.Register( + httpmock.GraphQLMutationMatcher(`mutation ReplaceActorsForAssignable\b`, func(m map[string]interface{}) bool { + return m["assignableId"] == "123" + }), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, + func(inputs map[string]interface{}) {}), + ) reg.Register( httpmock.GraphQLMutationMatcher(`mutation IssueUpdate\b`, func(m map[string]interface{}) bool { return m["id"] == "123" @@ -555,6 +566,14 @@ func Test_editRun(t *testing.T) { func(inputs map[string]interface{}) {}), ) // Updating 456 should fail. + reg.Register( + httpmock.GraphQLMutationMatcher(`mutation ReplaceActorsForAssignable\b`, func(m map[string]interface{}) bool { + return m["assignableId"] == "456" + }), + httpmock.GraphQLMutation(` + { "errors": [ { "message": "test error" } ] }`, + func(inputs map[string]interface{}) {}), + ) reg.Register( httpmock.GraphQLMutationMatcher(`mutation IssueUpdate\b`, func(m map[string]interface{}) bool { return m["id"] == "456" @@ -603,6 +622,7 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockRepoMetadata(t, reg) mockIssueUpdate(t, reg) + mockIssueUpdateActorAssignees(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) }, @@ -792,6 +812,15 @@ func mockIssueUpdate(t *testing.T, reg *httpmock.Registry) { ) } +func mockIssueUpdateActorAssignees(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, + func(inputs map[string]interface{}) {}), + ) +} + func mockIssueUpdateLabels(t *testing.T, reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation LabelAdd\b`), @@ -816,6 +845,85 @@ func mockProjectV2ItemUpdate(t *testing.T, reg *httpmock.Registry) { ) } +func TestActorIsAssignable(t *testing.T) { + t.Run("when actors are assignable, query includes assignedActors", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`assignedActors`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we don't care. + _ = editRun(&EditOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Detector: &fd.EnabledDetectorMock{}, + IssueNumbers: []int{123}, + Editable: prShared.Editable{ + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "octocat"}, + Edited: true, + }, + }, + }, + }) + + reg.Verify(t) + }) + + t.Run("when actors are not assignable, query includes assignees instead", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + // This test should NOT include assignedActors in the query + reg.Exclude(t, httpmock.GraphQL(`assignedActors`)) + // It should include the regular assignees field + reg.Register( + httpmock.GraphQL(`assignees`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we're not really interested in it. + _ = editRun(&EditOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Detector: &fd.DisabledDetectorMock{}, + IssueNumbers: []int{123}, + Editable: prShared.Editable{ + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "octocat"}, + Edited: true, + }, + }, + }, + }) + + reg.Verify(t) + }) +} + // TODO projectsV1Deprecation // Remove this test. func TestProjectsV1Deprecation(t *testing.T) { @@ -900,82 +1008,3 @@ func TestProjectsV1Deprecation(t *testing.T) { reg.Verify(t) }) } - -func TestActorIsAssignable(t *testing.T) { - t.Run("when actors are assignable, query includes assignedActors", func(t *testing.T) { - ios, _, _, _ := iostreams.Test() - - reg := &httpmock.Registry{} - reg.Register( - httpmock.GraphQL(`assignedActors`), - // Simulate a GraphQL error to early exit the test. - httpmock.StatusStringResponse(500, ""), - ) - - _, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - // Ignore the error because we don't care. - _ = editRun(&EditOptions{ - IO: ios, - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - }, - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil - }, - Detector: &fd.EnabledDetectorMock{}, - IssueNumbers: []int{123}, - Editable: prShared.Editable{ - Assignees: prShared.EditableAssignees{ - EditableSlice: prShared.EditableSlice{ - Add: []string{"monalisa", "octocat"}, - Edited: true, - }, - }, - }, - }) - - reg.Verify(t) - }) - - t.Run("when actors are not assignable, query includes assignees instead", func(t *testing.T) { - ios, _, _, _ := iostreams.Test() - - reg := &httpmock.Registry{} - // This test should NOT include assignedActors in the query - reg.Exclude(t, httpmock.GraphQL(`assignedActors`)) - // It should include the regular assignees field - reg.Register( - httpmock.GraphQL(`assignees`), - // Simulate a GraphQL error to early exit the test. - httpmock.StatusStringResponse(500, ""), - ) - - _, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - // Ignore the error because we're not really interested in it. - _ = editRun(&EditOptions{ - IO: ios, - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - }, - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil - }, - Detector: &fd.DisabledDetectorMock{}, - IssueNumbers: []int{123}, - Editable: prShared.Editable{ - Assignees: prShared.EditableAssignees{ - EditableSlice: prShared.EditableSlice{ - Add: []string{"monalisa", "octocat"}, - Edited: true, - }, - }, - }, - }) - - reg.Verify(t) - }) -} diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index fcc30095a..2c09f2e69 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -58,6 +58,26 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR }) } + // updateIssue mutation does not support Actors so assignment needs to + // be in a separate request when our assignees are Actors. + if options.Assignees.Edited && options.Assignees.ActorAssignees { + apiClient := api.NewClientFromHTTP(httpClient) + assigneeIds, err := options.AssigneeIds(apiClient, repo) + if err != nil { + return err + } + + // Disable the edited flag for assignees so that it doesn't + // trigger a second update in the replaceIssueFields function. + // It's important that this is done AFTER the call to + // options.AssigneeIds() above because otherwise that just exits. + options.Assignees.Edited = false + + wg.Go(func() error { + return replaceActorAssigneesForEditable(apiClient, repo, id, assigneeIds) + }) + } + if dirtyExcludingLabels(options) { wg.Go(func() error { return replaceIssueFields(httpClient, repo, id, isPR, options) @@ -67,6 +87,32 @@ 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) assigneeIds, err := options.AssigneeIds(apiClient, repo) From e0afc91fef1c3fbfe81d34c3054dbbd147e4b231 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 9 May 2025 23:02:15 -0600 Subject: [PATCH 07/59] chore(issue edit): comments cleanup --- api/queries_issue.go | 1 + pkg/cmd/issue/edit/edit_test.go | 14 +------------- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 2b15a41b9..833faaa47 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -99,6 +99,7 @@ type ActorAssignees struct { TotalCount int } +// TODO kw: Display names for actors with special display names. func (a ActorAssignees) Logins() []string { logins := make([]string, len(a.Edges)) for i, a := range a.Edges { diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index d27bbdee6..5a06614bc 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -701,18 +701,6 @@ func mockIssueProjectItemsGet(_ *testing.T, reg *httpmock.Registry) { } func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) { - // reg.Register( - // httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - // httpmock.StringResponse(` - // { "data": { "repository": { "assignableUsers": { - // "nodes": [ - // { "login": "hubot", "id": "HUBOTID" }, - // { "login": "MonaLisa", "id": "MONAID" } - // ], - // "pageInfo": { "hasNextPage": false } - // } } } } - // `)) - reg.Register( httpmock.GraphQL(`query RepositoryAssignableActors\b`), httpmock.StringResponse(` @@ -721,7 +709,7 @@ func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) { { "login": "hubot", "id": "HUBOTID" }, { "login": "MonaLisa", "id": "MONAID" } ], - "pageInfo": { "hasNextPage": false, "endCursor": "Mg" } + "pageInfo": { "hasNextPage": false } } } } } `)) From 218152f7c5d454bffd019eb63e58645cd1f54639 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 9 May 2025 23:26:06 -0600 Subject: [PATCH 08/59] fix(issue edit): resolve race condition in actor assignment --- pkg/cmd/pr/shared/editable_http.go | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index 2c09f2e69..1475f978c 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -61,19 +61,13 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR // updateIssue mutation does not support Actors so assignment needs to // be in a separate request when our assignees are Actors. if options.Assignees.Edited && options.Assignees.ActorAssignees { - apiClient := api.NewClientFromHTTP(httpClient) - assigneeIds, err := options.AssigneeIds(apiClient, repo) - if err != nil { - return err - } - - // Disable the edited flag for assignees so that it doesn't - // trigger a second update in the replaceIssueFields function. - // It's important that this is done AFTER the call to - // options.AssigneeIds() above because otherwise that just exits. - options.Assignees.Edited = false - wg.Go(func() error { + apiClient := api.NewClientFromHTTP(httpClient) + assigneeIds, err := options.AssigneeIds(apiClient, repo) + if err != nil { + return err + } + return replaceActorAssigneesForEditable(apiClient, repo, id, assigneeIds) }) } @@ -115,16 +109,20 @@ func replaceActorAssigneesForEditable(apiClient *api.Client, repo ghrepo.Interfa func replaceIssueFields(httpClient *http.Client, repo ghrepo.Interface, id string, isPR bool, options Editable) error { apiClient := api.NewClientFromHTTP(httpClient) - assigneeIds, err := options.AssigneeIds(apiClient, repo) - if err != nil { - return err - } projectIds, err := options.ProjectIds() if err != nil { return err } + var assigneeIds *[]string + if !options.Assignees.ActorAssignees { + assigneeIds, err = options.AssigneeIds(apiClient, repo) + if err != nil { + return err + } + } + milestoneId, err := options.MilestoneId() if err != nil { return err From 3af32fb07026c08b6c7ba4d48f818ab1365a9e3c Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Sat, 10 May 2025 10:46:35 -0600 Subject: [PATCH 09/59] fix(preview): remove needless newlines --- pkg/cmd/preview/prompter/prompter.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/cmd/preview/prompter/prompter.go b/pkg/cmd/preview/prompter/prompter.go index 5b9b91f23..e07ec61d8 100644 --- a/pkg/cmd/preview/prompter/prompter.go +++ b/pkg/cmd/preview/prompter/prompter.go @@ -120,8 +120,6 @@ func prompterRun(opts *prompterOptions) error { if err := f(p); err != nil { return err } - // Newline for readability - fmt.Println() } return nil From c4ab455ebfbcc23b1f7e8483b6bdeb17d52525ba Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Sat, 10 May 2025 11:34:14 -0600 Subject: [PATCH 10/59] fix(prompter): update prompter create for changes in trunk --- pkg/cmd/preview/prompter/prompter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/preview/prompter/prompter.go b/pkg/cmd/preview/prompter/prompter.go index e07ec61d8..b63ac2d04 100644 --- a/pkg/cmd/preview/prompter/prompter.go +++ b/pkg/cmd/preview/prompter/prompter.go @@ -114,7 +114,7 @@ func prompterRun(opts *prompterOptions) error { return err } - p := prompter.New(editor, opts.IO.In, opts.IO.Out, opts.IO.ErrOut) + p := prompter.New(editor, opts.IO) for _, f := range opts.PromptsToRun { if err := f(p); err != nil { From f305cb13de20cfeb205735a2a1b315358505db1c Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Sat, 10 May 2025 11:43:44 -0600 Subject: [PATCH 11/59] fix(prompter): print to iostreams stdout --- pkg/cmd/preview/prompter/prompter.go | 68 ++++++++++++++-------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/pkg/cmd/preview/prompter/prompter.go b/pkg/cmd/preview/prompter/prompter.go index b63ac2d04..5b44a5cbf 100644 --- a/pkg/cmd/preview/prompter/prompter.go +++ b/pkg/cmd/preview/prompter/prompter.go @@ -15,7 +15,7 @@ type prompterOptions struct { IO *iostreams.IOStreams Config func() (gh.Config, error) - PromptsToRun []func(prompter.Prompter) error + PromptsToRun []func(prompter.Prompter, *iostreams.IOStreams) error } func NewCmdPrompter(f *cmdutil.Factory, runF func(*prompterOptions) error) *cobra.Command { @@ -36,7 +36,7 @@ func NewCmdPrompter(f *cmdutil.Factory, runF func(*prompterOptions) error) *cobr markdownEditorPrompt = "markdown-editor" ) - prompterTypeFuncMap := map[string]func(prompter.Prompter) error{ + prompterTypeFuncMap := map[string]func(prompter.Prompter, *iostreams.IOStreams) error{ selectPrompt: runSelect, multiSelectPrompt: runMultiSelect, inputPrompt: runInput, @@ -117,7 +117,7 @@ func prompterRun(opts *prompterOptions) error { p := prompter.New(editor, opts.IO) for _, f := range opts.PromptsToRun { - if err := f(p); err != nil { + if err := f(p, opts.IO); err != nil { return err } } @@ -125,112 +125,112 @@ func prompterRun(opts *prompterOptions) error { return nil } -func runSelect(p prompter.Prompter) error { - fmt.Println("Demonstrating Single Select") +func runSelect(p prompter.Prompter, io *iostreams.IOStreams) error { + fmt.Fprintln(io.Out, "Demonstrating Single Select") cuisines := []string{"Italian", "Greek", "Indian", "Japanese", "American"} favorite, err := p.Select("Favorite cuisine?", "Italian", cuisines) if err != nil { return err } - fmt.Printf("Favorite cuisine: %s\n", cuisines[favorite]) + fmt.Fprintf(io.Out, "Favorite cuisine: %s\n", cuisines[favorite]) return nil } -func runMultiSelect(p prompter.Prompter) error { - fmt.Println("Demonstrating Multi Select") +func runMultiSelect(p prompter.Prompter, io *iostreams.IOStreams) error { + fmt.Fprintln(io.Out, "Demonstrating Multi Select") cuisines := []string{"Italian", "Greek", "Indian", "Japanese", "American"} favorites, err := p.MultiSelect("Favorite cuisines?", []string{}, cuisines) if err != nil { return err } for _, f := range favorites { - fmt.Printf("Favorite cuisine: %s\n", cuisines[f]) + fmt.Fprintf(io.Out, "Favorite cuisine: %s\n", cuisines[f]) } return nil } -func runInput(p prompter.Prompter) error { - fmt.Println("Demonstrating Text Input") +func runInput(p prompter.Prompter, io *iostreams.IOStreams) error { + fmt.Fprintln(io.Out, "Demonstrating Text Input") text, err := p.Input("Favorite meal?", "Breakfast") if err != nil { return err } - fmt.Printf("You typed: %s\n", text) + fmt.Fprintf(io.Out, "You typed: %s\n", text) return nil } -func runPassword(p prompter.Prompter) error { - fmt.Println("Demonstrating Password Input") +func runPassword(p prompter.Prompter, io *iostreams.IOStreams) error { + fmt.Fprintln(io.Out, "Demonstrating Password Input") safeword, err := p.Password("Safe word?") if err != nil { return err } - fmt.Printf("Safe word: %s\n", safeword) + fmt.Fprintf(io.Out, "Safe word: %s\n", safeword) return nil } -func runConfirm(p prompter.Prompter) error { - fmt.Println("Demonstrating Confirmation") +func runConfirm(p prompter.Prompter, io *iostreams.IOStreams) error { + fmt.Fprintln(io.Out, "Demonstrating Confirmation") confirmation, err := p.Confirm("Are you sure?", true) if err != nil { return err } - fmt.Printf("Confirmation: %t\n", confirmation) + fmt.Fprintf(io.Out, "Confirmation: %t\n", confirmation) return nil } -func runAuthToken(p prompter.Prompter) error { - fmt.Println("Demonstrating Auth Token (can't be blank)") +func runAuthToken(p prompter.Prompter, io *iostreams.IOStreams) error { + fmt.Fprintln(io.Out, "Demonstrating Auth Token (can't be blank)") token, err := p.AuthToken() if err != nil { return err } - fmt.Printf("Auth token: %s\n", token) + fmt.Fprintf(io.Out, "Auth token: %s\n", token) return nil } -func runConfirmDeletion(p prompter.Prompter) error { - fmt.Println("Demonstrating Deletion Confirmation") +func runConfirmDeletion(p prompter.Prompter, io *iostreams.IOStreams) error { + fmt.Fprintln(io.Out, "Demonstrating Deletion Confirmation") err := p.ConfirmDeletion("delete-me") if err != nil { return err } - fmt.Println("Item deleted") + fmt.Fprintln(io.Out, "Item deleted") return nil } -func runInputHostname(p prompter.Prompter) error { - fmt.Println("Demonstrating Hostname") +func runInputHostname(p prompter.Prompter, io *iostreams.IOStreams) error { + fmt.Fprintln(io.Out, "Demonstrating Hostname") hostname, err := p.InputHostname() if err != nil { return err } - fmt.Printf("Hostname: %s\n", hostname) + fmt.Fprintf(io.Out, "Hostname: %s\n", hostname) return nil } -func runMarkdownEditor(p prompter.Prompter) error { +func runMarkdownEditor(p prompter.Prompter, io *iostreams.IOStreams) error { defaultText := "default text value" - fmt.Println("Demonstrating Markdown Editor with blanks allowed and default text") + fmt.Fprintln(io.Out, "Demonstrating Markdown Editor with blanks allowed and default text") editorText, err := p.MarkdownEditor("Edit your text:", defaultText, true) if err != nil { return err } - fmt.Printf("Returned text: %s\n\n", editorText) + fmt.Fprintf(io.Out, "Returned text: %s\n\n", editorText) - fmt.Println("Demonstrating Markdown Editor with blanks disallowed and default text") + fmt.Fprintln(io.Out, "Demonstrating Markdown Editor with blanks disallowed and default text") editorText2, err := p.MarkdownEditor("Edit your text:", defaultText, false) if err != nil { return err } - fmt.Printf("Returned text: %s\n\n", editorText2) + fmt.Fprintf(io.Out, "Returned text: %s\n\n", editorText2) - fmt.Println("Demonstrating Markdown Editor with blanks disallowed and no default text") + fmt.Fprintln(io.Out, "Demonstrating Markdown Editor with blanks disallowed and no default text") editorText3, err := p.MarkdownEditor("Edit your text:", "", false) if err != nil { return err } - fmt.Printf("Returned text: %s\n", editorText3) + fmt.Fprintf(io.Out, "Returned text: %s\n", editorText3) return nil } From 175767cc59db46e2ac235859af6c12a57248bf58 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Sat, 10 May 2025 11:54:32 -0600 Subject: [PATCH 12/59] doc(preview): add long description --- pkg/cmd/preview/preview.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/cmd/preview/preview.go b/pkg/cmd/preview/preview.go index 5c3209722..791c100b6 100644 --- a/pkg/cmd/preview/preview.go +++ b/pkg/cmd/preview/preview.go @@ -1,6 +1,7 @@ package preview import ( + "github.com/MakeNowJust/heredoc" cmdPrompter "github.com/cli/cli/v2/pkg/cmd/preview/prompter" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" @@ -10,6 +11,10 @@ func NewCmdPreview(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "preview ", Short: "Execute previews for gh features", + Long: heredoc.Doc(` + Preview commands are for testing, demonstrative, and development purposes only. + They should be considered unstable and can change at any time. + `), } cmdutil.DisableAuthCheck(cmd) From 29241cb7a51a8d672bfe00c706cb9ea09f84b6f4 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Mon, 12 May 2025 11:34:31 -0600 Subject: [PATCH 13/59] refactor(issue edit): improve actor type handling This improves actor type handling while fetching repository assignable actors. --- api/queries_repo.go | 37 +++++++++++++++++++++++---------- pkg/cmd/issue/edit/edit_test.go | 8 +++---- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 99041e49f..a5b926fc0 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1235,17 +1235,25 @@ func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]RepoAssignee, // RepoAssignableActors fetches all the assignable actors for a repository on // GitHub hosts that support Actor assignees. func RepoAssignableActors(client *Client, repo ghrepo.Interface) ([]RepoAssignee, error) { - type repoActorAssignee struct { - ID string - Login string + type repoBotAssignee struct { + ID string + Login string + TypeName string `graphql:"__typename"` + } + + type repoUserAssignee struct { + ID string + Login string + Name string + TypeName string `graphql:"__typename"` } type responseData struct { Repository struct { SuggestedActors struct { Nodes []struct { - User RepoAssignee `graphql:"... on User"` - Bot repoActorAssignee `graphql:"... on Bot"` + User repoUserAssignee `graphql:"... on User"` + Bot repoBotAssignee `graphql:"... on Bot"` } PageInfo struct { HasNextPage bool @@ -1270,13 +1278,20 @@ func RepoAssignableActors(client *Client, repo ghrepo.Interface) ([]RepoAssignee } for _, node := range query.Repository.SuggestedActors.Nodes { - // Edge case if the Actor is not a Bot or a User, - // it won't be unmarshalled properly, and we'll have an - // zero value node. - if node.User.ID == "" || node.User.Login == "" { - continue + if node.User.TypeName == "User" { + actor := RepoAssignee{ + ID: node.User.ID, + Login: node.User.Login, + Name: node.User.Name, + } + actors = append(actors, actor) + } else if node.Bot.TypeName == "Bot" { + actor := RepoAssignee{ + ID: node.Bot.ID, + Login: node.Bot.Login, + } + actors = append(actors, actor) } - actors = append(actors, node.User) } if !query.Repository.SuggestedActors.PageInfo.HasNextPage { diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 5a06614bc..bf2a1c417 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -528,8 +528,8 @@ func Test_editRun(t *testing.T) { httpmock.StringResponse(` { "data": { "repository": { "suggestedActors": { "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } + { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, + { "login": "MonaLisa", "id": "MONAID", "__typename": "User" } ], "pageInfo": { "hasNextPage": false, "endCursor": "Mg" } } } } } @@ -706,8 +706,8 @@ func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) { httpmock.StringResponse(` { "data": { "repository": { "suggestedActors": { "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } + { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, + { "login": "MonaLisa", "id": "MONAID", "__typename": "User" } ], "pageInfo": { "hasNextPage": false } } } } } From 73e5589059eebefedbc9116bae7adaad1c8f8768 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Mon, 12 May 2025 11:37:37 -0600 Subject: [PATCH 14/59] doc(issue): comment why assignable actors disabled --- internal/featuredetection/feature_detection.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/featuredetection/feature_detection.go b/internal/featuredetection/feature_detection.go index 5ff08a573..a2f34a60b 100644 --- a/internal/featuredetection/feature_detection.go +++ b/internal/featuredetection/feature_detection.go @@ -73,7 +73,7 @@ func (d *detector) IssueFeatures() (IssueFeatures, error) { features := IssueFeatures{ StateReason: false, - ActorIsAssignable: false, + ActorIsAssignable: false, // replaceActorsForAssignable GraphQL mutation unavailable on GHES } var featureDetection struct { From cff2fa71e2b044befe0ea865b174cd08e9ab91d6 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Mon, 12 May 2025 11:49:44 -0600 Subject: [PATCH 15/59] doc(repo queries): clarify reviewer actor fetching logic --- api/queries_repo.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index a5b926fc0..6b9f690eb 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -929,7 +929,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput return err }) - // If reviewers are requested, we still need to fetch the assignable users + // 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 @@ -938,6 +938,9 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput // 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) From 35792827ad3e0bd142bf91d2dc38fac9195fa526 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 13 May 2025 07:09:04 -0600 Subject: [PATCH 16/59] refactor(issue edit): rename AssignedActors to ActorAssignees --- api/queries_issue.go | 2 +- pkg/cmd/issue/edit/edit.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 833faaa47..69e93c974 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -38,7 +38,7 @@ type Issue struct { Comments Comments Author Author Assignees Assignees - AssignedActors ActorAssignees + ActorAssignees ActorAssignees Labels Labels ProjectCards ProjectCards ProjectItems ProjectItems diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 9177f60ad..b86f20550 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -280,7 +280,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.ActorAssignees.Logins() } else { editable.Assignees.Default = issue.Assignees.Logins() } From 3bed77836a5ff2f95dab3d01f5c819d19c729bd3 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 13 May 2025 07:11:24 -0600 Subject: [PATCH 17/59] refactor(issue edit): rename loop variable for clarity --- api/queries_repo.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 6b9f690eb..ea60319a3 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -704,9 +704,9 @@ func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { } // Look for ID in assignable actors if not found in assignable users - for _, u := range m.AssignableActors { - if strings.EqualFold(assigneeLogin, u.Login) { - ids = append(ids, u.ID) + for _, a := range m.AssignableActors { + if strings.EqualFold(assigneeLogin, a.Login) { + ids = append(ids, a.ID) found = true break } From 261297f0a2ccb0322c4dce8bb2fc99ae3e5c0c49 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 13 May 2025 07:28:42 -0600 Subject: [PATCH 18/59] refactor(issue edit): add assignedActors to lookupFields --- api/query_builder.go | 2 ++ pkg/cmd/issue/edit/edit.go | 15 +-------------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/api/query_builder.go b/api/query_builder.go index 47fb4c225..0ef44a347 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -366,6 +366,8 @@ func IssueGraphQL(fields []string) string { q = append(q, `headRepository{id,name}`) 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}`) 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 b86f20550..a2e824ebf 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -211,20 +211,7 @@ func editRun(opts *EditOptions) error { if editable.Assignees.Edited { if issueFeatures.ActorIsAssignable { editable.Assignees.ActorAssignees = true - - // At the time of writing, only 10 Actors can be assigned to an issue. - assignedActors := heredoc.Doc(` - assignedActors(first: 10) { - edges { - node { - ... on Actor { - login - } - } - } - } - `) - lookupFields = append(lookupFields, assignedActors) + lookupFields = append(lookupFields, `assignedActors`) } else { lookupFields = append(lookupFields, "assignees") } From 21bd797c6c31859d65edc37cdb6b0fa82bbb1f68 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 13 May 2025 11:45:40 -0600 Subject: [PATCH 19/59] fix(issue edit): use double quotes for assignedActors --- pkg/cmd/issue/edit/edit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index a2e824ebf..c9c76746b 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -211,7 +211,7 @@ func editRun(opts *EditOptions) error { if editable.Assignees.Edited { if issueFeatures.ActorIsAssignable { editable.Assignees.ActorAssignees = true - lookupFields = append(lookupFields, `assignedActors`) + lookupFields = append(lookupFields, "assignedActors") } else { lookupFields = append(lookupFields, "assignees") } From 1e5c3c7426144987f7dece2913c799ad7ca9a454 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 13 May 2025 13:02:20 -0600 Subject: [PATCH 20/59] fix(issue edit): revert rename of ActorAssignees --- api/queries_issue.go | 6 +++--- pkg/cmd/issue/edit/edit.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 69e93c974..701b92039 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -38,7 +38,7 @@ type Issue struct { Comments Comments Author Author Assignees Assignees - ActorAssignees ActorAssignees + AssignedActors AssignedActors Labels Labels ProjectCards ProjectCards ProjectItems ProjectItems @@ -92,7 +92,7 @@ func (a Assignees) Logins() []string { return logins } -type ActorAssignees struct { +type AssignedActors struct { Edges []struct { Node Actor } @@ -100,7 +100,7 @@ type ActorAssignees struct { } // TODO kw: Display names for actors with special display names. -func (a ActorAssignees) Logins() []string { +func (a AssignedActors) Logins() []string { logins := make([]string, len(a.Edges)) for i, a := range a.Edges { logins[i] = a.Node.Login diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index c9c76746b..5cb789543 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.ActorAssignees.Logins() + editable.Assignees.Default = issue.AssignedActors.Logins() } else { editable.Assignees.Default = issue.Assignees.Logins() } From dfd600713f001e3c1385ee9ea21597d78eaca535 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 14 May 2025 08:53:47 -0600 Subject: [PATCH 21/59] refactor(api): rename assignable user types and methods --- api/queries_repo.go | 148 +++++++++++++++++++++++-------- api/queries_repo_test.go | 6 +- pkg/cmd/pr/shared/completion.go | 8 +- pkg/cmd/pr/shared/editable.go | 4 +- pkg/cmd/pr/shared/survey.go | 2 +- pkg/cmd/pr/shared/survey_test.go | 6 +- 6 files changed, 123 insertions(+), 51 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index ea60319a3..9e822767d 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -682,8 +682,8 @@ func RepoFindForks(client *Client, repo ghrepo.Interface, limit int) ([]*Reposit type RepoMetadataResult struct { CurrentLogin string - AssignableUsers []RepoAssignee - AssignableActors []RepoAssignee + AssignableUsers []AssignableUser + AssignableActors []AssignableActor Labels []RepoLabel Projects []RepoProject ProjectsV2 []ProjectV2 @@ -696,8 +696,8 @@ func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { for _, assigneeLogin := range names { found := false for _, u := range m.AssignableUsers { - if strings.EqualFold(assigneeLogin, u.Login) { - ids = append(ids, u.ID) + if strings.EqualFold(assigneeLogin, (u.Login())) { + ids = append(ids, u.ID()) found = true break } @@ -705,8 +705,8 @@ func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { // Look for ID in assignable actors if not found in assignable users for _, a := range m.AssignableActors { - if strings.EqualFold(assigneeLogin, a.Login) { - ids = append(ids, a.ID) + if strings.EqualFold(assigneeLogin, a.Login()) { + ids = append(ids, a.ID()) found = true break } @@ -1126,12 +1126,16 @@ func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoRes result.Teams = append(result.Teams, t) } default: - user := RepoAssignee{} + user := struct { + Id string + Login string + Name string + }{} err := json.Unmarshal(v, &user) if err != nil { return result, err } - result.AssignableUsers = append(result.AssignableUsers, user) + result.AssignableUsers = append(result.AssignableUsers, NewAssignableUser(user.Id, user.Login, user.Name)) } } @@ -1183,26 +1187,86 @@ func RepoProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) return projects, nil } -type RepoAssignee struct { - ID string - Login string - Name string +type AssignableActor interface { + DisplayName() string + ID() string + Login() string + + sealedAssignableActor() +} + +// Always a user +type AssignableUser struct { + id string + login string + name string +} + +// NewAssignableUser is a test helper to create a new AssignableUser +// since the ID and Login are private fields +func NewAssignableUser(id, login, name string) AssignableUser { + return AssignableUser{ + id: id, + login: login, + name: name, + } } // DisplayName returns a formatted string that uses Login and Name to be displayed e.g. 'Login (Name)' or 'Login' -func (ra RepoAssignee) DisplayName() string { - if ra.Name != "" { - return fmt.Sprintf("%s (%s)", ra.Login, ra.Name) +func (u AssignableUser) DisplayName() string { + if u.name != "" { + return fmt.Sprintf("%s (%s)", u.login, u.name) } - return ra.Login + return u.login } +func (u AssignableUser) ID() string { + return u.id +} + +func (u AssignableUser) Login() string { + return u.login +} + +func (u AssignableUser) Name() string { + return u.name +} + +func (u AssignableUser) sealedAssignableActor() {} + +type AssignableBot struct { + id string + login string +} + +func (b AssignableBot) DisplayName() string { + return b.Login() +} + +func (b AssignableBot) ID() string { + return b.id +} + +func (b AssignableBot) Login() string { + return b.login +} + +func (b AssignableBot) Name() string { + return "" +} + +func (b AssignableBot) sealedAssignableActor() {} + // RepoAssignableUsers fetches all the assignable users for a repository -func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]RepoAssignee, error) { +func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]AssignableUser, error) { type responseData struct { Repository struct { AssignableUsers struct { - Nodes []RepoAssignee + Nodes []struct { + ID string + Login string + Name string + } PageInfo struct { HasNextPage bool EndCursor string @@ -1217,7 +1281,7 @@ func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]RepoAssignee, "endCursor": (*githubv4.String)(nil), } - var users []RepoAssignee + var users []AssignableUser for { var query responseData err := client.Query(repo.RepoHost(), "RepositoryAssignableUsers", &query, variables) @@ -1225,7 +1289,15 @@ func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]RepoAssignee, return nil, err } - users = append(users, query.Repository.AssignableUsers.Nodes...) + for _, node := range query.Repository.AssignableUsers.Nodes { + user := AssignableUser{ + id: node.ID, + login: node.Login, + name: node.Name, + } + + users = append(users, user) + } if !query.Repository.AssignableUsers.PageInfo.HasNextPage { break } @@ -1237,26 +1309,26 @@ func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]RepoAssignee, // RepoAssignableActors fetches all the assignable actors for a repository on // GitHub hosts that support Actor assignees. -func RepoAssignableActors(client *Client, repo ghrepo.Interface) ([]RepoAssignee, error) { - type repoBotAssignee struct { - ID string - Login string - TypeName string `graphql:"__typename"` - } - - type repoUserAssignee struct { +func RepoAssignableActors(client *Client, repo ghrepo.Interface) ([]AssignableActor, error) { + type assignableUser struct { ID string Login string Name string TypeName string `graphql:"__typename"` } + type assignableBot struct { + ID string + Login string + TypeName string `graphql:"__typename"` + } + type responseData struct { Repository struct { SuggestedActors struct { Nodes []struct { - User repoUserAssignee `graphql:"... on User"` - Bot repoBotAssignee `graphql:"... on Bot"` + User assignableUser `graphql:"... on User"` + Bot assignableBot `graphql:"... on Bot"` } PageInfo struct { HasNextPage bool @@ -1272,7 +1344,7 @@ func RepoAssignableActors(client *Client, repo ghrepo.Interface) ([]RepoAssignee "endCursor": (*githubv4.String)(nil), } - var actors []RepoAssignee + var actors []AssignableActor for { var query responseData err := client.Query(repo.RepoHost(), "RepositoryAssignableActors", &query, variables) @@ -1282,16 +1354,16 @@ func RepoAssignableActors(client *Client, repo ghrepo.Interface) ([]RepoAssignee for _, node := range query.Repository.SuggestedActors.Nodes { if node.User.TypeName == "User" { - actor := RepoAssignee{ - ID: node.User.ID, - Login: node.User.Login, - Name: node.User.Name, + actor := AssignableUser{ + id: node.User.ID, + login: node.User.Login, + name: node.User.Name, } actors = append(actors, actor) } else if node.Bot.TypeName == "Bot" { - actor := RepoAssignee{ - ID: node.Bot.ID, - Login: node.Bot.Login, + actor := AssignableBot{ + id: node.Bot.ID, + login: node.Bot.Login, } actors = append(actors, actor) } diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 01fc7a4c7..9040a0018 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -526,17 +526,17 @@ func Test_RepoMilestones(t *testing.T) { func TestDisplayName(t *testing.T) { tests := []struct { name string - assignee RepoAssignee + assignee AssignableUser want string }{ { name: "assignee with name", - assignee: RepoAssignee{"123", "octocat123", "Octavious Cath"}, + assignee: AssignableUser{"123", "octocat123", "Octavious Cath"}, want: "octocat123 (Octavious Cath)", }, { name: "assignee without name", - assignee: RepoAssignee{"123", "octocat123", ""}, + assignee: AssignableUser{"123", "octocat123", ""}, want: "octocat123", }, } diff --git a/pkg/cmd/pr/shared/completion.go b/pkg/cmd/pr/shared/completion.go index e07abc5a7..c1296be71 100644 --- a/pkg/cmd/pr/shared/completion.go +++ b/pkg/cmd/pr/shared/completion.go @@ -21,13 +21,13 @@ func RequestableReviewersForCompletion(httpClient *http.Client, repo ghrepo.Inte results := []string{} for _, user := range metadata.AssignableUsers { - if strings.EqualFold(user.Login, metadata.CurrentLogin) { + if strings.EqualFold(user.Login(), metadata.CurrentLogin) { continue } - if user.Name != "" { - results = append(results, fmt.Sprintf("%s\t%s", user.Login, user.Name)) + if user.Name() != "" { + results = append(results, fmt.Sprintf("%s\t%s", user.Login(), user.Name())) } else { - results = append(results, user.Login) + results = append(results, user.Login()) } } for _, team := range metadata.Teams { diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 238939efe..d51405acd 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -407,11 +407,11 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) var users []string for _, u := range metadata.AssignableUsers { - users = append(users, u.Login) + users = append(users, u.Login()) } var actors []string for _, a := range metadata.AssignableActors { - actors = append(actors, a.Login) + actors = append(actors, a.Login()) } var teams []string for _, t := range metadata.Teams { diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index bf4476ca1..b6c927a2d 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -192,7 +192,7 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface var reviewers []string for _, u := range metadataResult.AssignableUsers { - if u.Login != metadataResult.CurrentLogin { + if u.Login() != metadataResult.CurrentLogin { reviewers = append(reviewers, u.DisplayName()) } } diff --git a/pkg/cmd/pr/shared/survey_test.go b/pkg/cmd/pr/shared/survey_test.go index 6895b52ac..7097d0761 100644 --- a/pkg/cmd/pr/shared/survey_test.go +++ b/pkg/cmd/pr/shared/survey_test.go @@ -28,9 +28,9 @@ func TestMetadataSurvey_selectAll(t *testing.T) { fetcher := &metadataFetcher{ metadataResult: &api.RepoMetadataResult{ - AssignableUsers: []api.RepoAssignee{ - {Login: "hubot"}, - {Login: "monalisa"}, + AssignableUsers: []api.AssignableUser{ + api.NewAssignableUser("", "hubot", ""), + api.NewAssignableUser("", "monalisa", ""), }, Labels: []api.RepoLabel{ {Name: "help wanted"}, From 712eeabd4acbef9af03a297c38eec4e476cc993f Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 14 May 2025 08:55:55 -0600 Subject: [PATCH 22/59] doc(api): remove needless comment --- api/queries_repo.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 9e822767d..e6286bb0b 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1202,8 +1202,6 @@ type AssignableUser struct { name string } -// NewAssignableUser is a test helper to create a new AssignableUser -// since the ID and Login are private fields func NewAssignableUser(id, login, name string) AssignableUser { return AssignableUser{ id: id, From 08cd1dc7db690477fcfc32aec3f8c171fc53bb80 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 14 May 2025 11:43:29 -0600 Subject: [PATCH 23/59] feat(issue edit): replacing actor assignee is done synchronously with updateIssue --- pkg/cmd/issue/edit/edit_test.go | 8 -------- pkg/cmd/pr/shared/editable_http.go | 29 +++++++++++++++++------------ 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index bf2a1c417..0df6d5e9f 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -574,14 +574,6 @@ func Test_editRun(t *testing.T) { { "errors": [ { "message": "test error" } ] }`, func(inputs map[string]interface{}) {}), ) - reg.Register( - httpmock.GraphQLMutationMatcher(`mutation IssueUpdate\b`, func(m map[string]interface{}) bool { - return m["id"] == "456" - }), - httpmock.GraphQLMutation(` - { "errors": [ { "message": "test error" } ] }`, - func(inputs map[string]interface{}) {}), - ) }, stdout: heredoc.Doc(` https://github.com/OWNER/REPO/issue/123 diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index 1475f978c..465bfda6c 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -58,23 +58,28 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR }) } - // updateIssue mutation does not support Actors so assignment needs to - // be in a separate request when our assignees are Actors. - if options.Assignees.Edited && options.Assignees.ActorAssignees { + if dirtyExcludingLabels(options) { wg.Go(func() error { - apiClient := api.NewClientFromHTTP(httpClient) - assigneeIds, err := options.AssigneeIds(apiClient, repo) + // updateIssue mutation does not support Actors so assignment needs to + // be in a separate request when our assignees are Actors. + if options.Assignees.Edited && options.Assignees.ActorAssignees { + apiClient := api.NewClientFromHTTP(httpClient) + assigneeIds, err := options.AssigneeIds(apiClient, repo) + if err != nil { + return err + } + + err = replaceActorAssigneesForEditable(apiClient, repo, id, assigneeIds) + if err != nil { + return err + } + } + err := replaceIssueFields(httpClient, repo, id, isPR, options) if err != nil { return err } - return replaceActorAssigneesForEditable(apiClient, repo, id, assigneeIds) - }) - } - - if dirtyExcludingLabels(options) { - wg.Go(func() error { - return replaceIssueFields(httpClient, repo, id, isPR, options) + return nil }) } From 5dc854c75e1735fb906640963dcaea462000f4c9 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 14 May 2025 11:50:11 -0600 Subject: [PATCH 24/59] doc(issue edit): clarify synchronous handling of assignees --- pkg/cmd/pr/shared/editable_http.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index 465bfda6c..cd183e565 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -62,6 +62,10 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR wg.Go(func() error { // updateIssue mutation does not support Actors so assignment needs to // be in a separate request when our assignees are Actors. + // Note: this is intentionally done synchronously with updating + // 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 { apiClient := api.NewClientFromHTTP(httpClient) assigneeIds, err := options.AssigneeIds(apiClient, repo) 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 25/59] 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 26/59] 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 27/59] 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 da40e087460c0d01f741fba8200b1c5d825b2bee Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 08:33:30 -0600 Subject: [PATCH 28/59] doc(api): code comment typo Co-authored-by: Andy Feller --- api/queries_repo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index e6286bb0b..aa180a015 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -940,7 +940,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput // 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. + // `gh pr` requests actor assignees. if input.Reviewers { g.Go(func() error { users, err := RepoAssignableUsers(client, repo) From ea85b92b35dccaae5a4b359a33c74d5a83c21707 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 09:09:36 -0600 Subject: [PATCH 29/59] refactor(api): remove needless parenthesis Co-authored-by: Andy Feller --- api/queries_repo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index aa180a015..978e844c4 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -696,7 +696,7 @@ func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { for _, assigneeLogin := range names { found := false for _, u := range m.AssignableUsers { - if strings.EqualFold(assigneeLogin, (u.Login())) { + if strings.EqualFold(assigneeLogin, u.Login()) { ids = append(ids, u.ID()) found = true break From bcd47f1fb178c60ad7e87f75d3cfbe00f00545d1 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 09:13:46 -0600 Subject: [PATCH 30/59] fix(api): correct var name capitalization --- pkg/cmd/pr/shared/editable_http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index cd183e565..8cd51c349 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -103,7 +103,7 @@ func replaceActorAssigneesForEditable(apiClient *api.Client, repo ghrepo.Interfa var mutation struct { ReplaceActorsForAssignable struct { - Typename string `graphql:"__typename"` + TypeName string `graphql:"__typename"` } `graphql:"replaceActorsForAssignable(input: $input)"` } From 0db7ae872f71ec7926dc96c34430a60aa75fc5bf Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 09:09:36 -0600 Subject: [PATCH 31/59] refactor(api): remove needless parenthesis Co-authored-by: Andy Feller --- api/queries_repo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index e6286bb0b..732d89c8c 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -696,7 +696,7 @@ func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { for _, assigneeLogin := range names { found := false for _, u := range m.AssignableUsers { - if strings.EqualFold(assigneeLogin, (u.Login())) { + if strings.EqualFold(assigneeLogin, u.Login()) { ids = append(ids, u.ID()) found = true break From 6162a7c524d4507412e312e275ebff2d0a59da33 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 09:13:46 -0600 Subject: [PATCH 32/59] fix(api): correct var name capitalization --- pkg/cmd/pr/shared/editable_http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index cd183e565..8cd51c349 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -103,7 +103,7 @@ func replaceActorAssigneesForEditable(apiClient *api.Client, repo ghrepo.Interfa var mutation struct { ReplaceActorsForAssignable struct { - Typename string `graphql:"__typename"` + TypeName string `graphql:"__typename"` } `graphql:"replaceActorsForAssignable(input: $input)"` } From f55906810645e4a22eeb227d4b189ae9a9a0acaa 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 33/59] 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 d72018d3120dde1fb783afa3f612cd9255e1eb32 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 34/59] 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 732d89c8c..437261c21 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 54f48cfe46e231a7f1d45bdab0706ed999690f60 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 09:47:30 -0600 Subject: [PATCH 35/59] fix(pr edit): remove merge conflict artifact, extra detector --- pkg/cmd/pr/edit/edit.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 75b8c4e09..8d2ff8ddb 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -21,9 +21,6 @@ import ( type EditOptions struct { HttpClient func() (*http.Client, error) IO *iostreams.IOStreams - // TODO projectsV1Deprecation - // Remove this detector since it is only used for test validation. - Detector fd.Detector Finder shared.PRFinder Surveyor Surveyor 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 36/59] 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 37/59] 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 375c6cd28f7ded11fe8b53974735ec10404ea865 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 12:50:03 -0600 Subject: [PATCH 38/59] feat(issue/pr edit): support @copilot in assignee flags - Introduced CopilotReplacer to handle `@copilot` mentions in assignee lists. --- pkg/cmd/pr/shared/editable.go | 40 +++++++++++++++++++----- pkg/cmd/pr/shared/params.go | 27 ++++++++++++++++ pkg/cmd/pr/shared/params_test.go | 53 ++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 77bab8440..1bbcd3113 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -118,7 +118,28 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str // 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() + copilotReplacer := NewCopilotReplacer() + + // A closure to replace special assignee names with the actual logins. + replaceSpecialAssigneeNames := func(value []string) ([]string, error) { + replaced, err := meReplacer.ReplaceSlice(value) + if err != nil { + return nil, err + } + + // Only suppported for actor assignees. + if e.Assignees.ActorAssignees { + replaced, err = copilotReplacer.ReplaceSlice(replaced) + if err != nil { + return nil, err + } + } + + return replaced, nil + } + + assigneeSet := 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 @@ -128,21 +149,24 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str // 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) + assigneeSet.AddValues(e.Assignees.DefaultLogins) } else { - s.AddValues(e.Assignees.Default) + assigneeSet.AddValues(e.Assignees.Default) } - add, err := meReplacer.ReplaceSlice(e.Assignees.Add) + + add, err := replaceSpecialAssigneeNames(e.Assignees.Add) if err != nil { return nil, err } - s.AddValues(add) - remove, err := meReplacer.ReplaceSlice(e.Assignees.Remove) + assigneeSet.AddValues(add) + + remove, err := replaceSpecialAssigneeNames(e.Assignees.Remove) if err != nil { return nil, err } - s.RemoveValues(remove) - e.Assignees.Value = s.ToSlice() + assigneeSet.RemoveValues(remove) + + e.Assignees.Value = assigneeSet.ToSlice() } a, err := e.Metadata.MembersToIDs(e.Assignees.Value) return &a, err diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 08968939d..7ea364707 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -312,3 +312,30 @@ func (r *MeReplacer) ReplaceSlice(handles []string) ([]string, error) { } return res, nil } + +// CopilotReplacer resolves usages of `@copilot` to Copilot's login. +type CopilotReplacer struct{} + +func NewCopilotReplacer() *CopilotReplacer { + return &CopilotReplacer{} +} + +func (r *CopilotReplacer) replace(handle string) (string, error) { + if strings.EqualFold(handle, "@copilot") { + return "copilot-swe-agent", nil + } + return handle, nil +} + +// Replace replaces usages of `@copilot` in a slice with Copilot's login. +func (r *CopilotReplacer) ReplaceSlice(handles []string) ([]string, error) { + res := make([]string, len(handles)) + for i, h := range handles { + var err error + res[i], err = r.replace(h) + if err != nil { + return nil, err + } + } + return res, nil +} diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index 15f00ca4f..b1e3d32d6 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -187,6 +187,59 @@ func TestMeReplacer_Replace(t *testing.T) { } } +func TestCopilotReplacer_ReplaceSlice(t *testing.T) { + type args struct { + handles []string + } + tests := []struct { + name string + args args + want []string + wantErr bool + }{ + { + name: "replaces @copilot with copilot-swe-agent", + args: args{ + handles: []string{"monalisa", "@copilot", "hubot"}, + }, + want: []string{"monalisa", "copilot-swe-agent", "hubot"}, + wantErr: false, + }, + { + name: "handles no @copilot mentions", + args: args{ + handles: []string{"monalisa", "user", "hubot"}, + }, + want: []string{"monalisa", "user", "hubot"}, + wantErr: false, + }, + { + name: "replaces multiple @copilot mentions", + args: args{ + handles: []string{"@copilot", "user", "@copilot"}, + }, + want: []string{"copilot-swe-agent", "user", "copilot-swe-agent"}, + wantErr: false, + }, + { + name: "handles @copilot case-insensitively", + args: args{ + handles: []string{"@Copilot", "user", "@CoPiLoT"}, + }, + want: []string{"copilot-swe-agent", "user", "copilot-swe-agent"}, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := NewCopilotReplacer() + got, err := r.ReplaceSlice(tt.args.handles) + require.NoError(t, err) + require.Equal(t, tt.want, got) + }) + } +} + func Test_QueryHasStateClause(t *testing.T) { tests := []struct { searchQuery string From f6bb1ca75653975d42a7d43c7c7ee24784a79d8a Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 13:40:23 -0600 Subject: [PATCH 39/59] refactor(pr edit): move httpclient initialization --- pkg/cmd/pr/edit/edit.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 8d2ff8ddb..ff293a511 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -201,12 +201,12 @@ func editRun(opts *EditOptions) error { Detector: opts.Detector, } - if opts.Detector == nil { - httpClient, err := opts.HttpClient() - if err != nil { - return err - } + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + if opts.Detector == nil { baseRepo, err := opts.BaseRepo() if err != nil { return err @@ -262,10 +262,6 @@ func editRun(opts *EditOptions) error { } } - httpClient, err := opts.HttpClient() - if err != nil { - return err - } apiClient := api.NewClientFromHTTP(httpClient) opts.IO.StartProgressIndicator() From ec7c8ed844ec1532b6026af5d0b57bb0d238ed58 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 13:41:38 -0600 Subject: [PATCH 40/59] test(pr edit): fix typo in test name Co-authored-by: Andy Feller --- pkg/cmd/pr/edit/edit_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 0f15d6bd1..76fb40f88 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -500,7 +500,7 @@ func Test_editRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123\n", }, { - name: "Legacy assignee user are fetched and updated on unsupported GitHub Hosts", + name: "Legacy assignee users are fetched and updated on unsupported GitHub Hosts", input: &EditOptions{ Detector: &fd.DisabledDetectorMock{}, SelectorArg: "123", From 981da867018817f7ea08dae2259e022f2f231fe0 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 13:47:51 -0600 Subject: [PATCH 41/59] doc(pr edit): condense comment for reviewer/user filtering --- api/queries_repo.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 437261c21..1bd814746 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -926,12 +926,8 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput } result.AssignableActors = actors - // 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. + // Filter actors for users to use for pull request reviewers, + // skip retrieving the same info through RepoAssignableUsers(). var users []AssignableUser for _, a := range actors { if _, ok := a.(AssignableUser); !ok { From 8bd77c0a956680b16cfc0610e61f803bbe05b7ca Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 14:01:10 -0600 Subject: [PATCH 42/59] fix(pr edit): clarify error messages for assignee actors and users --- api/queries_repo.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 1bd814746..fd5ae4679 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -922,7 +922,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput g.Go(func() error { actors, err := RepoAssignableActors(client, repo) if err != nil { - return fmt.Errorf("error fetching assignees: %w", err) + return fmt.Errorf("error fetching assignable actors: %w", err) } result.AssignableActors = actors @@ -943,7 +943,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput g.Go(func() error { users, err := RepoAssignableUsers(client, repo) if err != nil { - err = fmt.Errorf("error fetching assignees: %w", err) + err = fmt.Errorf("error fetching assignable users: %w", err) } result.AssignableUsers = users return err 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 43/59] 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 } } } } } From c5f2d7bde70115a15135578a026b765f5c4c3429 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 09:37:20 -0600 Subject: [PATCH 44/59] doc(editable): remove needless comment --- pkg/cmd/pr/shared/editable.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 1bbcd3113..4b190cae8 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -120,7 +120,6 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str meReplacer := NewMeReplacer(client, repo.RepoHost()) copilotReplacer := NewCopilotReplacer() - // A closure to replace special assignee names with the actual logins. replaceSpecialAssigneeNames := func(value []string) ([]string, error) { replaced, err := meReplacer.ReplaceSlice(value) if err != nil { From 871671be526e7b3e40f90a5a562a5eb75136f0a6 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 09:40:29 -0600 Subject: [PATCH 45/59] fix(params): remove needless err return --- pkg/cmd/pr/shared/editable.go | 5 +---- pkg/cmd/pr/shared/params.go | 16 ++++++---------- pkg/cmd/pr/shared/params_test.go | 3 +-- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 4b190cae8..c71f89b52 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -128,10 +128,7 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str // Only suppported for actor assignees. if e.Assignees.ActorAssignees { - replaced, err = copilotReplacer.ReplaceSlice(replaced) - if err != nil { - return nil, err - } + replaced = copilotReplacer.ReplaceSlice(replaced) } return replaced, nil diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 7ea364707..e53ad7a06 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -320,22 +320,18 @@ func NewCopilotReplacer() *CopilotReplacer { return &CopilotReplacer{} } -func (r *CopilotReplacer) replace(handle string) (string, error) { +func (r *CopilotReplacer) replace(handle string) string { if strings.EqualFold(handle, "@copilot") { - return "copilot-swe-agent", nil + return "copilot-swe-agent" } - return handle, nil + return handle } // Replace replaces usages of `@copilot` in a slice with Copilot's login. -func (r *CopilotReplacer) ReplaceSlice(handles []string) ([]string, error) { +func (r *CopilotReplacer) ReplaceSlice(handles []string) []string { res := make([]string, len(handles)) for i, h := range handles { - var err error - res[i], err = r.replace(h) - if err != nil { - return nil, err - } + res[i] = r.replace(h) } - return res, nil + return res } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index b1e3d32d6..4a353e97e 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -233,8 +233,7 @@ func TestCopilotReplacer_ReplaceSlice(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { r := NewCopilotReplacer() - got, err := r.ReplaceSlice(tt.args.handles) - require.NoError(t, err) + got := r.ReplaceSlice(tt.args.handles) require.Equal(t, tt.want, got) }) } From debf0bbaf22b7293da97b10d0ad01f1c64e05904 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 09:50:24 -0600 Subject: [PATCH 46/59] test(params): enhance Copilot replacer tests for edge cases - Added tests for handling nil and empty slices in the Copilot replacer. - Simplified test structure by removing unnecessary wantErr field. --- pkg/cmd/pr/shared/params_test.go | 33 ++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index 4a353e97e..53eb6328f 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -192,42 +192,51 @@ func TestCopilotReplacer_ReplaceSlice(t *testing.T) { handles []string } tests := []struct { - name string - args args - want []string - wantErr bool + name string + args args + want []string }{ { name: "replaces @copilot with copilot-swe-agent", args: args{ handles: []string{"monalisa", "@copilot", "hubot"}, }, - want: []string{"monalisa", "copilot-swe-agent", "hubot"}, - wantErr: false, + want: []string{"monalisa", "copilot-swe-agent", "hubot"}, }, { name: "handles no @copilot mentions", args: args{ handles: []string{"monalisa", "user", "hubot"}, }, - want: []string{"monalisa", "user", "hubot"}, - wantErr: false, + want: []string{"monalisa", "user", "hubot"}, }, { name: "replaces multiple @copilot mentions", args: args{ handles: []string{"@copilot", "user", "@copilot"}, }, - want: []string{"copilot-swe-agent", "user", "copilot-swe-agent"}, - wantErr: false, + want: []string{"copilot-swe-agent", "user", "copilot-swe-agent"}, }, { name: "handles @copilot case-insensitively", args: args{ handles: []string{"@Copilot", "user", "@CoPiLoT"}, }, - want: []string{"copilot-swe-agent", "user", "copilot-swe-agent"}, - wantErr: false, + want: []string{"copilot-swe-agent", "user", "copilot-swe-agent"}, + }, + { + name: "handles nil slice", + args: args{ + handles: nil, + }, + want: []string{}, + }, + { + name: "handles empty slice", + args: args{ + handles: []string{}, + }, + want: []string{}, }, } for _, tt := range tests { From 532aa1c32a8602653f2e12e665e3b50f66d57714 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 09:55:17 -0600 Subject: [PATCH 47/59] doc(issue/pr edit): doc `@copilot` assignee --- pkg/cmd/issue/edit/edit.go | 6 ++++++ pkg/cmd/pr/edit/edit.go | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 8fc8a2e41..36260cb1b 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -60,11 +60,17 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman Editing issues' projects requires authorization with the %[1]sproject%[1]s scope. To authorize, run %[1]sgh auth refresh -s project%[1]s. + + The %[1]s--add-assignee%[1]s and %[1]s--remove-assignee%[1]s flags both support + the following special values: + - %[1]s@me%[1]s: assign or unassign yourself + - %[1]s@copilot%[1]s: assign or unassign Copilot `, "`"), Example: heredoc.Doc(` $ gh issue edit 23 --title "I found a bug" --body "Nothing works" $ gh issue edit 23 --add-label "bug,help wanted" --remove-label "core" $ gh issue edit 23 --add-assignee "@me" --remove-assignee monalisa,hubot + $ gh issue edit 23 --add-assignee "@copilot" $ gh issue edit 23 --add-project "Roadmap" --remove-project v1,v2 $ gh issue edit 23 --milestone "Version 1" $ gh issue edit 23 --remove-milestone diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 155f08897..92f40645e 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -60,12 +60,18 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman Editing a pull request's projects requires authorization with the %[1]sproject%[1]s scope. To authorize, run %[1]sgh auth refresh -s project%[1]s. + + The %[1]s--add-assignee%[1]s and %[1]s--remove-assignee%[1]s flags both support + the following special values: + - %[1]s@me%[1]s: assign or unassign yourself + - %[1]s@copilot%[1]s: assign or unassign Copilot `, "`"), Example: heredoc.Doc(` $ gh pr edit 23 --title "I found a bug" --body "Nothing works" $ gh pr edit 23 --add-label "bug,help wanted" --remove-label "core" $ gh pr edit 23 --add-reviewer monalisa,hubot --remove-reviewer myorg/team-name $ gh pr edit 23 --add-assignee "@me" --remove-assignee monalisa,hubot + $ gh pr edit 23 --add-assignee "@copilot" $ gh pr edit 23 --add-project "Roadmap" --remove-project v1,v2 $ gh pr edit 23 --milestone "Version 1" $ gh pr edit 23 --remove-milestone From 748afb6e85cf56bd6ad4ae6f348dcfd7d105192f Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 12:18:23 -0600 Subject: [PATCH 48/59] doc(issue/pr edit): clarify @copilot usage --- pkg/cmd/issue/edit/edit.go | 2 +- pkg/cmd/pr/edit/edit.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 36260cb1b..e959cde2b 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -64,7 +64,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman The %[1]s--add-assignee%[1]s and %[1]s--remove-assignee%[1]s flags both support the following special values: - %[1]s@me%[1]s: assign or unassign yourself - - %[1]s@copilot%[1]s: assign or unassign Copilot + - %[1]s@copilot%[1]s: assign or unassign Copilot (not supported on GitHub Enterprise Server) `, "`"), Example: heredoc.Doc(` $ gh issue edit 23 --title "I found a bug" --body "Nothing works" diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 92f40645e..c0dc61199 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -64,7 +64,10 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman The %[1]s--add-assignee%[1]s and %[1]s--remove-assignee%[1]s flags both support the following special values: - %[1]s@me%[1]s: assign or unassign yourself - - %[1]s@copilot%[1]s: assign or unassign Copilot + - %[1]s@copilot%[1]s: assign or unassign Copilot (not supported on GitHub Enterprise Server) + + The %[1]s--add-reviewer%[1]s and %[1]s--remove-reviewer%[1]s flags do not support + these special values. `, "`"), Example: heredoc.Doc(` $ gh pr edit 23 --title "I found a bug" --body "Nothing works" From a28e2d87840f275a2c4e1d216bce06aeabb7b011 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 12:28:46 -0600 Subject: [PATCH 49/59] refactor(api): use constant for Copilot login --- api/queries_repo.go | 7 ++++++- pkg/cmd/pr/shared/params.go | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index f72277fb2..28d4374b7 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1183,6 +1183,11 @@ func RepoProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) return projects, nil } +// Expected login for Copilot when retrieved as an Actor +// This is returned from assignable actors and issue/pr assigned actors. +// We use this to check if the actor is Copilot. +var CopilotActorLogin = "copilot-swe-agent" + type AssignableActor interface { DisplayName() string ID() string @@ -1241,7 +1246,7 @@ func NewAssignableBot(id, login string) AssignableBot { } func (b AssignableBot) DisplayName() string { - if b.login == "copilot-swe-agent" { + if b.login == CopilotActorLogin { return "Copilot (AI)" } return b.Login() diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index e53ad7a06..ba9901f77 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -322,7 +322,7 @@ func NewCopilotReplacer() *CopilotReplacer { func (r *CopilotReplacer) replace(handle string) string { if strings.EqualFold(handle, "@copilot") { - return "copilot-swe-agent" + return api.CopilotActorLogin } return handle } From 6f1906073f2281ae1a0ce63566d9c5911f5d19b5 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 12:51:09 -0600 Subject: [PATCH 50/59] doc(params): incorrect func name in comment Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/cmd/pr/shared/params.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index ba9901f77..1fa45652a 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -327,7 +327,7 @@ func (r *CopilotReplacer) replace(handle string) string { return handle } -// Replace replaces usages of `@copilot` in a slice with Copilot's login. +// ReplaceSlice replaces usages of `@copilot` in a slice with Copilot's login. func (r *CopilotReplacer) ReplaceSlice(handles []string) []string { res := make([]string, len(handles)) for i, h := range handles { From 53d0d5d192dcb3f0edf16b70bc1586b6ea71faa1 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 12:53:56 -0600 Subject: [PATCH 51/59] fix(editable): include DefaultLogins in EditableAssignees clone --- pkg/cmd/pr/shared/editable.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index c71f89b52..2f51f2ae8 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -292,6 +292,7 @@ func (ea *EditableAssignees) clone() EditableAssignees { return EditableAssignees{ EditableSlice: ea.EditableSlice.clone(), ActorAssignees: ea.ActorAssignees, + DefaultLogins: ea.DefaultLogins, } } From e0f3f0f2fde2f0ae0695d53f0b00e8f15006dd47 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 14:57:49 -0600 Subject: [PATCH 52/59] refactor(api): change CopilotActorLogin to constant --- api/queries_repo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 28d4374b7..2b2a1eb96 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1186,7 +1186,7 @@ func RepoProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) // Expected login for Copilot when retrieved as an Actor // This is returned from assignable actors and issue/pr assigned actors. // We use this to check if the actor is Copilot. -var CopilotActorLogin = "copilot-swe-agent" +const CopilotActorLogin = "copilot-swe-agent" type AssignableActor interface { DisplayName() string From 0788a015176a73adbc5db76a82b6f854a0480742 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 16 May 2025 15:17:39 -0600 Subject: [PATCH 53/59] refactor(api): inline struct definitions in RepoAssignableActors --- api/queries_repo.go | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 2b2a1eb96..efbcfcb19 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1319,25 +1319,21 @@ func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]AssignableUse // RepoAssignableActors fetches all the assignable actors for a repository on // GitHub hosts that support Actor assignees. func RepoAssignableActors(client *Client, repo ghrepo.Interface) ([]AssignableActor, error) { - type assignableUser struct { - ID string - Login string - Name string - TypeName string `graphql:"__typename"` - } - - type assignableBot struct { - ID string - Login string - TypeName string `graphql:"__typename"` - } - type responseData struct { Repository struct { SuggestedActors struct { Nodes []struct { - User assignableUser `graphql:"... on User"` - Bot assignableBot `graphql:"... on Bot"` + User struct { + ID string + Login string + Name string + TypeName string `graphql:"__typename"` + } `graphql:"... on User"` + Bot struct { + ID string + Login string + TypeName string `graphql:"__typename"` + } `graphql:"... on Bot"` } PageInfo struct { HasNextPage bool From b639d6d5149c9f1fc61837b92c26e0bc7dff0ad7 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 20 May 2025 08:37:01 -0600 Subject: [PATCH 54/59] doc(pr): format allowed values and defaults in help --- pkg/cmd/config/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/config/config.go b/pkg/cmd/config/config.go index 66051f83a..92d0ca1ba 100644 --- a/pkg/cmd/config/config.go +++ b/pkg/cmd/config/config.go @@ -20,10 +20,10 @@ func NewCmdConfig(f *cmdutil.Factory) *cobra.Command { for _, co := range config.Options { longDoc.WriteString(fmt.Sprintf("- `%s`: %s", co.Key, co.Description)) if len(co.AllowedValues) > 0 { - longDoc.WriteString(fmt.Sprintf(" {%s}", strings.Join(co.AllowedValues, "|"))) + longDoc.WriteString(fmt.Sprintf(" `{%s}`", strings.Join(co.AllowedValues, " | "))) } if co.DefaultValue != "" { - longDoc.WriteString(fmt.Sprintf(" (default %s)", co.DefaultValue)) + longDoc.WriteString(fmt.Sprintf(" (default `%s`)", co.DefaultValue)) } longDoc.WriteRune('\n') } From 944543863af83332ffaea84692eaec130a89a9cb Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 20 May 2025 08:54:30 -0600 Subject: [PATCH 55/59] Revert "[gh config] Escape pipe symbol in Long desc for website manual" --- internal/docs/markdown.go | 3 +-- pkg/cmd/config/config.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/docs/markdown.go b/internal/docs/markdown.go index 05d8686b8..7ae8c6862 100644 --- a/internal/docs/markdown.go +++ b/internal/docs/markdown.go @@ -142,8 +142,7 @@ func genMarkdownCustom(cmd *cobra.Command, w io.Writer, linkHandler func(string) fmt.Fprintf(w, "```\n%s\n```\n\n", cmd.UseLine()) } if hasLong { - longWithEscapedPipe := strings.ReplaceAll(cmd.Long, "|", "|") - fmt.Fprintf(w, "%s\n\n", longWithEscapedPipe) + fmt.Fprintf(w, "%s\n\n", cmd.Long) } for _, g := range root.GroupedCommands(cmd) { diff --git a/pkg/cmd/config/config.go b/pkg/cmd/config/config.go index 66051f83a..2661c3369 100644 --- a/pkg/cmd/config/config.go +++ b/pkg/cmd/config/config.go @@ -16,7 +16,7 @@ import ( func NewCmdConfig(f *cmdutil.Factory) *cobra.Command { longDoc := strings.Builder{} longDoc.WriteString("Display or change configuration settings for gh.\n\n") - longDoc.WriteString("Current respected settings:\n\n") + longDoc.WriteString("Current respected settings:\n") for _, co := range config.Options { longDoc.WriteString(fmt.Sprintf("- `%s`: %s", co.Key, co.Description)) if len(co.AllowedValues) > 0 { From 53d589223bed7f801d8b2fed67891d87acdebe18 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Wed, 21 May 2025 12:17:05 +0100 Subject: [PATCH 56/59] test: ensure proper usage of pipes in docs Signed-off-by: Babak K. Shandiz --- pkg/cmd/root/help_test.go | 76 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/pkg/cmd/root/help_test.go b/pkg/cmd/root/help_test.go index d24a84a0d..de82479a8 100644 --- a/pkg/cmd/root/help_test.go +++ b/pkg/cmd/root/help_test.go @@ -2,6 +2,18 @@ package root import ( "testing" + + "github.com/cli/cli/v2/internal/browser" + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/extensions" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + "github.com/yuin/goldmark" + "github.com/yuin/goldmark/ast" + "github.com/yuin/goldmark/text" ) func TestDedent(t *testing.T) { @@ -44,3 +56,67 @@ func TestDedent(t *testing.T) { } } } + +// Since our online docs website renders pages by using the kramdown (a superset +// of Markdown) engine, we have to check against some known quirks of the +// syntax. +func TestKramdownCompatibleDocs(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: ios, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + Browser: &browser.Stub{}, + ExtensionManager: &extensions.ExtensionManagerMock{ + ListFunc: func() []extensions.Extension { + return nil + }, + }, + } + + cmd, err := NewCmdRoot(f, "N/A", "") + require.NoError(t, err) + + var walk func(*cobra.Command) + walk = func(cmd *cobra.Command) { + t.Run(cmd.UseLine(), func(t *testing.T) { + assertProperUsageOfPipes(t, cmd) + }) + for _, child := range cmd.Commands() { + walk(child) + } + } + + walk(cmd) +} + +// If not in a code block, kramdown treats pipes ("|") as table column +// separators, even if there's no table header, or left/right table row borders +// (i.e. lines starting and ending with a pipe). +// +// We need to assert there's no pipe in the text unless it's in a code-block or +// code-span. +// +// (See https://github.com/cli/cli/issues/10348) +func assertProperUsageOfPipes(t *testing.T, cmd *cobra.Command) { + md := goldmark.New() + reader := text.NewReader([]byte(cmd.Long)) + doc := md.Parser().Parse(reader) + + var checkNode func(node ast.Node) + checkNode = func(node ast.Node) { + if node.Kind() == ast.KindCodeSpan || node.Kind() == ast.KindCodeBlock { + return + } + + if node.Kind() == ast.KindText { + text := string(node.(*ast.Text).Segment.Value(reader.Source())) + require.NotContains(t, text, "|", `found pipe ("|") in plain text in %q docs`, cmd.CommandPath()) + } + + for child := node.FirstChild(); child != nil; child = child.NextSibling() { + checkNode(child) + } + } + + checkNode(doc) +} From 34431ab07486a180bd85cfd6bc976f891deb1614 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Wed, 21 May 2025 12:20:08 +0100 Subject: [PATCH 57/59] chore: run `go mod tidy` Signed-off-by: Babak K. Shandiz --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index bf8b033fe..f95c8a7c2 100644 --- a/go.mod +++ b/go.mod @@ -48,6 +48,7 @@ require ( github.com/spf13/cobra v1.9.1 github.com/spf13/pflag v1.0.6 github.com/stretchr/testify v1.10.0 + github.com/yuin/goldmark v1.7.8 github.com/zalando/go-keyring v0.2.5 golang.org/x/crypto v0.37.0 golang.org/x/sync v0.13.0 @@ -171,7 +172,6 @@ require ( github.com/transparency-dev/merkle v0.0.2 // indirect github.com/vbatts/tar-split v0.11.6 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect - github.com/yuin/goldmark v1.7.8 // indirect github.com/yuin/goldmark-emoji v1.0.5 // indirect go.mongodb.org/mongo-driver v1.14.0 // indirect go.opentelemetry.io/auto/sdk v1.1.0 // indirect From 960aadf2b9a9e2005d599801ff15be27451d8c40 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Thu, 22 May 2025 17:04:04 +0100 Subject: [PATCH 58/59] test: improve test case naming Signed-off-by: Babak K. Shandiz --- pkg/cmd/root/help_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/root/help_test.go b/pkg/cmd/root/help_test.go index de82479a8..40f333159 100644 --- a/pkg/cmd/root/help_test.go +++ b/pkg/cmd/root/help_test.go @@ -1,6 +1,7 @@ package root import ( + "fmt" "testing" "github.com/cli/cli/v2/internal/browser" @@ -78,8 +79,9 @@ func TestKramdownCompatibleDocs(t *testing.T) { var walk func(*cobra.Command) walk = func(cmd *cobra.Command) { - t.Run(cmd.UseLine(), func(t *testing.T) { - assertProperUsageOfPipes(t, cmd) + name := fmt.Sprintf("%q: test pipes are in code blocks", cmd.UseLine()) + t.Run(name, func(t *testing.T) { + assertPipesAreInCodeBlocks(t, cmd) }) for _, child := range cmd.Commands() { walk(child) @@ -89,15 +91,15 @@ func TestKramdownCompatibleDocs(t *testing.T) { walk(cmd) } -// If not in a code block, kramdown treats pipes ("|") as table column -// separators, even if there's no table header, or left/right table row borders -// (i.e. lines starting and ending with a pipe). +// If not in a code block or a code span, kramdown treats pipes ("|") as table +// column separators, even if there's no table header, or left/right table row +// borders (i.e. lines starting and ending with a pipe). // // We need to assert there's no pipe in the text unless it's in a code-block or // code-span. // // (See https://github.com/cli/cli/issues/10348) -func assertProperUsageOfPipes(t *testing.T, cmd *cobra.Command) { +func assertPipesAreInCodeBlocks(t *testing.T, cmd *cobra.Command) { md := goldmark.New() reader := text.NewReader([]byte(cmd.Long)) doc := md.Parser().Parse(reader) From 1e6a2b1affb7d6a7850f4bc0e7a43cf14b80caf5 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Fri, 23 May 2025 14:55:41 +0500 Subject: [PATCH 59/59] Add `--compact` flag to `run watch` (#10629) * [gh run watch] Support `--compact` flag * [gh run watch] Support `--compact` flag * Add changes for updated AC * Incorporate review changes * docs(run watch): fill up the line to 80 chars Signed-off-by: Babak K. Shandiz --------- Signed-off-by: Babak K. Shandiz Co-authored-by: Babak K. Shandiz --- pkg/cmd/run/shared/presentation.go | 42 ++++++++++++++++++++++++++++++ pkg/cmd/run/watch/watch.go | 15 +++++++++-- pkg/cmd/run/watch/watch_test.go | 9 +++++++ 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/run/shared/presentation.go b/pkg/cmd/run/shared/presentation.go index a3556d743..983979d8a 100644 --- a/pkg/cmd/run/shared/presentation.go +++ b/pkg/cmd/run/shared/presentation.go @@ -47,6 +47,48 @@ func RenderJobs(cs *iostreams.ColorScheme, jobs []Job, verbose bool) string { return strings.Join(lines, "\n") } +func RenderJobsCompact(cs *iostreams.ColorScheme, jobs []Job) string { + lines := []string{} + for _, job := range jobs { + elapsed := job.CompletedAt.Sub(job.StartedAt) + elapsedStr := fmt.Sprintf(" in %s", elapsed) + if elapsed < 0 { + elapsedStr = "" + } + symbol, symbolColor := Symbol(cs, job.Status, job.Conclusion) + id := cs.Cyanf("%d", job.ID) + lines = append(lines, fmt.Sprintf("%s %s%s (ID %s)", symbolColor(symbol), cs.Bold(job.Name), elapsedStr, id)) + + if job.Status == Completed && job.Conclusion == Success { + continue + } + + var inProgressStepLine string + var failedStepLines []string + + for _, step := range job.Steps { + stepSymbol, stepSymColor := Symbol(cs, step.Status, step.Conclusion) + stepLine := fmt.Sprintf(" %s %s", stepSymColor(stepSymbol), step.Name) + + if IsFailureState(step.Conclusion) { + failedStepLines = append(failedStepLines, stepLine) + } + + if step.Status == InProgress { + inProgressStepLine = stepLine + } + } + + lines = append(lines, failedStepLines...) + + if inProgressStepLine != "" { + lines = append(lines, inProgressStepLine) + } + } + + return strings.Join(lines, "\n") +} + func RenderAnnotations(cs *iostreams.ColorScheme, annotations []Annotation) string { lines := []string{} diff --git a/pkg/cmd/run/watch/watch.go b/pkg/cmd/run/watch/watch.go index 57be01f64..a73a91e1a 100644 --- a/pkg/cmd/run/watch/watch.go +++ b/pkg/cmd/run/watch/watch.go @@ -28,6 +28,7 @@ type WatchOptions struct { RunID string Interval int ExitStatus bool + Compact bool Prompt bool @@ -48,6 +49,9 @@ func NewCmdWatch(f *cmdutil.Factory, runF func(*WatchOptions) error) *cobra.Comm Long: heredoc.Docf(` Watch a run until it completes, showing its progress. + By default, all steps are displayed. The %[1]s--compact%[1]s option can be used to only + show the relevant/failed steps. + This command does not support authenticating via fine grained PATs as it is not currently possible to create a PAT with the %[1]schecks:read%[1]s permission. `, "`"), @@ -55,6 +59,9 @@ func NewCmdWatch(f *cmdutil.Factory, runF func(*WatchOptions) error) *cobra.Comm # Watch a run until it's done $ gh run watch + # Watch a run in compact mode + $ gh run watch --compact + # Run some other command when the run is finished $ gh run watch && notify-send 'run is done!' `), @@ -78,6 +85,7 @@ func NewCmdWatch(f *cmdutil.Factory, runF func(*WatchOptions) error) *cobra.Comm }, } cmd.Flags().BoolVar(&opts.ExitStatus, "exit-status", false, "Exit with non-zero status if run fails") + cmd.Flags().BoolVar(&opts.Compact, "compact", false, "Show only relevant/failed steps") cmd.Flags().IntVarP(&opts.Interval, "interval", "i", defaultInterval, "Refresh interval in seconds") return cmd @@ -252,8 +260,11 @@ func renderRun(out io.Writer, opts WatchOptions, client *api.Client, repo ghrepo } fmt.Fprintln(out, cs.Bold("JOBS")) - - fmt.Fprintln(out, shared.RenderJobs(cs, jobs, true)) + if opts.Compact { + fmt.Fprintln(out, shared.RenderJobsCompact(cs, jobs)) + } else { + fmt.Fprintln(out, shared.RenderJobs(cs, jobs, true)) + } if missingAnnotationsPermissions { fmt.Fprintln(out) diff --git a/pkg/cmd/run/watch/watch_test.go b/pkg/cmd/run/watch/watch_test.go index 49e56217b..1471f64cf 100644 --- a/pkg/cmd/run/watch/watch_test.go +++ b/pkg/cmd/run/watch/watch_test.go @@ -57,6 +57,15 @@ func TestNewCmdWatch(t *testing.T) { ExitStatus: true, }, }, + { + name: "compact status", + cli: "1234 --compact", + wants: WatchOptions{ + Interval: defaultInterval, + RunID: "1234", + Compact: true, + }, + }, } for _, tt := range tests {