diff --git a/api/queries_issue.go b/api/queries_issue.go index f09360152..24e0b4f4c 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 AssignedActors Labels Labels ProjectCards ProjectCards ProjectItems ProjectItems @@ -91,6 +92,61 @@ func (a Assignees) Logins() []string { return logins } +type AssignedActors struct { + Nodes []Actor + 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 + // from the AssignedActors type. But, this is only one piece of the puzzle + // as later, other queries will fetch the full list of possible assignable + // actors from the repository, and the two lists will be reconciled. + // + // It's important that the display names are the same between the defaults + // (the values returned here) and the full list (the values returned by + // other repository queries). Any discrepancy would result in an + // "invalid default", which means an assigned actor will not be matched + // to an assignable actor and not presented as a "default" selection. + // Not being presented as a default would cause the actor to be potentially + // unassigned if the edits were submitted. + // + // To prevent this, we need shared logic to look up an actor's display name. + // However, our API types between assignedActors and the full list of + // assignableActors are different. So, as an attempt to maintain + // consistency we convert the assignedActors to the same types as the + // repository's assignableActors, treating the assignableActors DisplayName + // methods as the sources of truth. + // TODO KW: make this comment less of a wall of text if needed. + var displayNames []string + for _, a := range a.Nodes { + if a.TypeName == "User" { + u := NewAssignableUser( + a.ID, + a.Login, + a.Name, + ) + displayNames = append(displayNames, u.DisplayName()) + } else if a.TypeName == "Bot" { + b := NewAssignableBot( + a.ID, + a.Login, + ) + displayNames = append(displayNames, b.DisplayName()) + } + } + return displayNames +} + type Labels struct { Nodes []IssueLabel TotalCount int diff --git a/api/queries_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/api/queries_repo.go b/api/queries_repo.go index 93a32d80c..efbcfcb19 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -146,6 +146,18 @@ type GitHubUser struct { Name string `json:"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"` + Name string `json:"name"` + TypeName string `json:"__typename"` +} + // BranchRef is the branch name in a GitHub repository type BranchRef struct { Name string `json:"name"` @@ -674,13 +686,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 []AssignableUser + AssignableActors []AssignableActor + Labels []RepoLabel + Projects []RepoProject + ProjectsV2 []ProjectV2 + Milestones []RepoMilestone + Teams []OrgTeam } func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { @@ -688,12 +701,27 @@ 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 } } + + // 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()) + found = true + break + } + if strings.EqualFold(assigneeLogin, a.DisplayName()) { + ids = append(ids, a.ID()) + found = true + break + } + } + if !found { return nil, fmt.Errorf("'%s' not found", assigneeLogin) } @@ -885,12 +913,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 @@ -899,14 +928,37 @@ 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) - } - result.AssignableUsers = users - return err - }) + if input.ActorAssignees { + g.Go(func() error { + actors, err := RepoAssignableActors(client, repo) + if err != nil { + return fmt.Errorf("error fetching assignable actors: %w", err) + } + result.AssignableActors = actors + + // 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 { + continue + } + users = append(users, a.(AssignableUser)) + } + result.AssignableUsers = users + return nil + }) + } 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 assignable users: %w", err) + } + result.AssignableUsers = users + return err + }) + } } if input.Reviewers { @@ -1070,12 +1122,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)) } } @@ -1127,26 +1183,99 @@ func RepoProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) return projects, nil } -type RepoAssignee struct { - ID string - Login string - Name string +// 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. +const CopilotActorLogin = "copilot-swe-agent" + +type AssignableActor interface { + DisplayName() string + ID() string + Login() string + + sealedAssignableActor() +} + +// Always a user +type AssignableUser struct { + id string + login string + name string +} + +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 NewAssignableBot(id, login string) AssignableBot { + return AssignableBot{ + id: id, + login: login, + } +} + +func (b AssignableBot) DisplayName() string { + if b.login == CopilotActorLogin { + return "Copilot (AI)" + } + 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 @@ -1161,7 +1290,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) @@ -1169,7 +1298,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 } @@ -1179,6 +1316,72 @@ 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) ([]AssignableActor, error) { + type responseData struct { + Repository struct { + SuggestedActors struct { + Nodes []struct { + 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 + 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 []AssignableActor + 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 { + if node.User.TypeName == "User" { + 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 := AssignableBot{ + id: node.Bot.ID, + login: node.Bot.Login, + } + actors = append(actors, actor) + } + } + + 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/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/api/query_builder.go b/api/query_builder.go index 47fb4c225..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 { @@ -366,6 +385,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) case "labels": q = append(q, `labels(first:100){nodes{id,name,description,color},totalCount}`) case "projectCards": 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 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/internal/featuredetection/feature_detection.go b/internal/featuredetection/feature_detection.go index fba317f58..a2f34a60b 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, // replaceActorsForAssignable GraphQL mutation unavailable on GHES } 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, }, diff --git a/pkg/cmd/config/config.go b/pkg/cmd/config/config.go index 66051f83a..5f8242d2a 100644 --- a/pkg/cmd/config/config.go +++ b/pkg/cmd/config/config.go @@ -16,14 +16,14 @@ 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 { - 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') } diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 8386cbcfa..e959cde2b 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 (not supported on GitHub Enterprise Server) `, "`"), 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 @@ -197,9 +203,24 @@ 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 { + editable.Assignees.ActorAssignees = true + lookupFields = append(lookupFields, "assignedActors") + } else { + lookupFields = append(lookupFields, "assignees") + } } if editable.Labels.Edited { lookupFields = append(lookupFields, "labels") @@ -207,11 +228,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 +270,14 @@ 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 editable.Assignees.ActorAssignees { + editable.Assignees.Default = issue.AssignedActors.DisplayNames() + editable.Assignees.DefaultLogins = 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..4840cbf7a 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"}, @@ -388,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) }, @@ -399,10 +406,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"}, @@ -433,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) @@ -449,10 +460,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 +507,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 +524,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" } + { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, + { "login": "MonaLisa", "id": "MONAID", "__typename": "User" } ], - "pageInfo": { "hasNextPage": false } + "pageInfo": { "hasNextPage": false, "endCursor": "Mg" } } } } } `)) reg.Register( @@ -534,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" @@ -544,8 +567,8 @@ func Test_editRun(t *testing.T) { ) // Updating 456 should fail. reg.Register( - httpmock.GraphQLMutationMatcher(`mutation IssueUpdate\b`, func(m map[string]interface{}) bool { - return m["id"] == "456" + httpmock.GraphQLMutationMatcher(`mutation ReplaceActorsForAssignable\b`, func(m map[string]interface{}) bool { + return m["assignableId"] == "456" }), httpmock.GraphQLMutation(` { "errors": [ { "message": "test error" } ] }`, @@ -591,11 +614,129 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockRepoMetadata(t, reg) mockIssueUpdate(t, reg) + mockIssueUpdateActorAssignees(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) }, 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) { @@ -654,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`), @@ -670,16 +839,17 @@ func mockIssueProjectItemsGet(_ *testing.T, reg *httpmock.Registry) { func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) { 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" } + { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } ], "pageInfo": { "hasNextPage": false } } } } } `)) + reg.Register( httpmock.GraphQL(`query RepositoryLabelList\b`), httpmock.StringResponse(` @@ -767,6 +937,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`), @@ -791,6 +970,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) { diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index cc5eefb9e..c30c4f9bb 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -3,6 +3,7 @@ package edit import ( "fmt" "net/http" + "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" @@ -20,15 +21,14 @@ 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 Fetcher EditableOptionsFetcher EditorRetriever EditorRetriever Prompter shared.EditPrompter + Detector fd.Detector + BaseRepo func() (ghrepo.Interface, error) SelectorArg string Interactive bool @@ -60,12 +60,21 @@ 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 (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" $ 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 @@ -73,6 +82,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] @@ -196,9 +206,36 @@ 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"}, Detector: opts.Detector, } + + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + if opts.Detector == nil { + 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 @@ -210,7 +247,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.DisplayNames() + } 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{} @@ -229,10 +271,6 @@ func editRun(opts *EditOptions) error { } } - httpClient, err := opts.HttpClient() - if err != nil { - return err - } apiClient := api.NewClientFromHTTP(httpClient) opts.IO.StartProgressIndicator() diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 63e380486..3a1e8a548 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -166,9 +166,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, + }, }, }, }, @@ -180,9 +182,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, + }, }, }, }, @@ -360,10 +364,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"}, @@ -387,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) @@ -414,10 +421,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"}, @@ -441,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) }, @@ -471,10 +481,12 @@ func Test_editRun(t *testing.T) { Remove: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot", "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"}, @@ -500,6 +512,7 @@ func Test_editRun(t *testing.T) { mockPullRequestUpdate(reg) mockPullRequestReviewersUpdate(reg) mockPullRequestUpdateLabels(reg) + mockPullRequestUpdateActorAssignees(reg) mockProjectV2ItemUpdate(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", @@ -519,6 +532,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) @@ -540,6 +554,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { mockRepoMetadata(reg, true) mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -561,11 +576,50 @@ func Test_editRun(t *testing.T) { mockRepoMetadata(reg, false) mockPullRequestUpdate(reg) mockPullRequestReviewersUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, + { + name: "Legacy assignee users 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) { @@ -579,9 +633,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) @@ -593,16 +649,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(` @@ -705,6 +761,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`), 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 e73b3c294..2f51f2ae8 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,14 @@ 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 + DefaultLogins []string // For disambiguating actors from display names +} + // ProjectsV2 mutations require a mapping of an item ID to a project ID. // Keep that map along with standard EditableSlice data. type EditableProjects struct { @@ -105,21 +113,56 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str if !e.Assignees.Edited { return nil, nil } + + // If assignees came in from command line flags, we need to + // curate the final list of assignees from the default list. if len(e.Assignees.Add) != 0 || len(e.Assignees.Remove) != 0 { meReplacer := NewMeReplacer(client, repo.RepoHost()) - s := set.NewStringSet() - s.AddValues(e.Assignees.Default) - add, err := meReplacer.ReplaceSlice(e.Assignees.Add) + copilotReplacer := NewCopilotReplacer() + + 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 = copilotReplacer.ReplaceSlice(replaced) + } + + 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 + // 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 { + assigneeSet.AddValues(e.Assignees.DefaultLogins) + } else { + assigneeSet.AddValues(e.Assignees.Default) + } + + 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 @@ -245,6 +288,14 @@ func (es *EditableSlice) clone() EditableSlice { return cpy } +func (ea *EditableAssignees) clone() EditableAssignees { + return EditableAssignees{ + EditableSlice: ea.EditableSlice.clone(), + ActorAssignees: ea.ActorAssignees, + DefaultLogins: ea.DefaultLogins, + } +} + func (ep *EditableProjects) clone() EditableProjects { return EditableProjects{ EditableSlice: ep.EditableSlice.clone(), @@ -378,12 +429,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 { @@ -392,7 +444,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.DisplayName()) } var teams []string for _, t := range metadata.Teams { @@ -416,7 +472,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 diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index fcc30095a..8cd51c349 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -60,25 +60,78 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR if dirtyExcludingLabels(options) { wg.Go(func() error { - return replaceIssueFields(httpClient, repo, id, isPR, options) + // 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) + 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 nil }) } return wg.Wait() } -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) +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) + 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 diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 08968939d..1fa45652a 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -312,3 +312,26 @@ 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 { + if strings.EqualFold(handle, "@copilot") { + return api.CopilotActorLogin + } + return handle +} + +// 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 { + res[i] = r.replace(h) + } + return res +} diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index 15f00ca4f..53eb6328f 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -187,6 +187,67 @@ 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 + }{ + { + name: "replaces @copilot with copilot-swe-agent", + args: args{ + handles: []string{"monalisa", "@copilot", "hubot"}, + }, + want: []string{"monalisa", "copilot-swe-agent", "hubot"}, + }, + { + name: "handles no @copilot mentions", + args: args{ + handles: []string{"monalisa", "user", "hubot"}, + }, + 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"}, + }, + { + name: "handles @copilot case-insensitively", + args: args{ + handles: []string{"@Copilot", "user", "@CoPiLoT"}, + }, + 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 { + t.Run(tt.name, func(t *testing.T) { + r := NewCopilotReplacer() + got := r.ReplaceSlice(tt.args.handles) + require.Equal(t, tt.want, got) + }) + } +} + func Test_QueryHasStateClause(t *testing.T) { tests := []struct { searchQuery string 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"}, diff --git a/pkg/cmd/preview/preview.go b/pkg/cmd/preview/preview.go new file mode 100644 index 000000000..791c100b6 --- /dev/null +++ b/pkg/cmd/preview/preview.go @@ -0,0 +1,25 @@ +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" +) + +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) + + 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..5b44a5cbf --- /dev/null +++ b/pkg/cmd/preview/prompter/prompter.go @@ -0,0 +1,236 @@ +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, *iostreams.IOStreams) 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, *iostreams.IOStreams) error{ + selectPrompt: runSelect, + multiSelectPrompt: runMultiSelect, + inputPrompt: runInput, + passwordPrompt: runPassword, + confirmPrompt: runConfirm, + authTokenPrompt: runAuthToken, + confirmDeletionPrompt: runConfirmDeletion, + inputHostnamePrompt: runInputHostname, + 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", + 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, in a fixed order + for _, promptType := range allPromptsOrder { + f := prompterTypeFuncMap[promptType] + 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) + + for _, f := range opts.PromptsToRun { + if err := f(p, opts.IO); err != nil { + return err + } + } + + return nil +} + +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.Fprintf(io.Out, "Favorite cuisine: %s\n", cuisines[favorite]) + return nil +} + +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.Fprintf(io.Out, "Favorite cuisine: %s\n", cuisines[f]) + } + return nil +} + +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.Fprintf(io.Out, "You typed: %s\n", text) + return nil +} + +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.Fprintf(io.Out, "Safe word: %s\n", safeword) + return nil +} + +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.Fprintf(io.Out, "Confirmation: %t\n", confirmation) + return nil +} + +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.Fprintf(io.Out, "Auth token: %s\n", token) + return nil +} + +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.Fprintln(io.Out, "Item deleted") + return nil +} + +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.Fprintf(io.Out, "Hostname: %s\n", hostname) + return nil +} + +func runMarkdownEditor(p prompter.Prompter, io *iostreams.IOStreams) error { + defaultText := "default text value" + + 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.Fprintf(io.Out, "Returned text: %s\n\n", editorText) + + 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.Fprintf(io.Out, "Returned text: %s\n\n", editorText2) + + 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.Fprintf(io.Out, "Returned text: %s\n", editorText3) + return nil +} diff --git a/pkg/cmd/root/help_test.go b/pkg/cmd/root/help_test.go index d24a84a0d..40f333159 100644 --- a/pkg/cmd/root/help_test.go +++ b/pkg/cmd/root/help_test.go @@ -1,7 +1,20 @@ package root import ( + "fmt" "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 +57,68 @@ 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) { + 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) + } + } + + walk(cmd) +} + +// 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 assertPipesAreInCodeBlocks(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) +} diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 8cf30db1b..6a709c336 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -26,6 +26,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" @@ -141,6 +142,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 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 {