diff --git a/api/queries_pr.go b/api/queries_pr.go index c38018a68fe..d0488c64b4b 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -1,12 +1,9 @@ package api import ( - "bytes" - "encoding/json" "fmt" "net/http" "net/url" - "strings" "time" "github.com/cli/cli/v2/internal/ghrepo" @@ -302,65 +299,6 @@ type PullRequestFile struct { ChangeType string `json:"changeType"` } -type ReviewRequests struct { - Nodes []struct { - RequestedReviewer RequestedReviewer - } -} - -type RequestedReviewer struct { - TypeName string `json:"__typename"` - Login string `json:"login"` - Name string `json:"name"` - Slug string `json:"slug"` - Organization struct { - Login string `json:"login"` - } `json:"organization"` -} - -const teamTypeName = "Team" -const botTypeName = "Bot" - -func (r RequestedReviewer) LoginOrSlug() string { - if r.TypeName == teamTypeName { - return fmt.Sprintf("%s/%s", r.Organization.Login, r.Slug) - } - return r.Login -} - -// DisplayName returns a user-friendly name for the reviewer. -// For Copilot bot, returns "Copilot (AI)". For teams, returns "org/slug". -// For users, returns "login (Name)" if name is available, otherwise just login. -func (r RequestedReviewer) DisplayName() string { - if r.TypeName == teamTypeName { - return fmt.Sprintf("%s/%s", r.Organization.Login, r.Slug) - } - if r.TypeName == botTypeName && r.Login == CopilotReviewerLogin { - return "Copilot (AI)" - } - if r.Name != "" { - return fmt.Sprintf("%s (%s)", r.Login, r.Name) - } - return r.Login -} - -func (r ReviewRequests) Logins() []string { - logins := make([]string, len(r.Nodes)) - for i, r := range r.Nodes { - logins[i] = r.RequestedReviewer.LoginOrSlug() - } - return logins -} - -// DisplayNames returns user-friendly display names for all requested reviewers. -func (r ReviewRequests) DisplayNames() []string { - names := make([]string, len(r.Nodes)) - for i, r := range r.Nodes { - names[i] = r.RequestedReviewer.DisplayName() - } - return names -} - func (pr PullRequest) HeadLabel() string { if pr.IsCrossRepository { return fmt.Sprintf("%s:%s", pr.HeadRepositoryOwner.Login, pr.HeadRefName) @@ -384,25 +322,6 @@ func (pr PullRequest) IsOpen() bool { return pr.State == "OPEN" } -type PullRequestReviewStatus struct { - ChangesRequested bool - Approved bool - ReviewRequired bool -} - -func (pr *PullRequest) ReviewStatus() PullRequestReviewStatus { - var status PullRequestReviewStatus - switch pr.ReviewDecision { - case "CHANGES_REQUESTED": - status.ChangesRequested = true - case "APPROVED": - status.Approved = true - case "REVIEW_REQUIRED": - status.ReviewRequired = true - } - return status -} - type PullRequestChecksStatus struct { Pending int Failing int @@ -542,18 +461,6 @@ func parseCheckStatusFromCheckConclusionState(state CheckConclusionState) checkS } } -func (pr *PullRequest) DisplayableReviews() PullRequestReviews { - published := []PullRequestReview{} - for _, prr := range pr.Reviews.Nodes { - //Dont display pending reviews - //Dont display commenting reviews without top level comment body - if prr.State != "PENDING" && !(prr.State == "COMMENTED" && prr.Body == "") { - published = append(published, prr) - } - } - return PullRequestReviews{Nodes: published, TotalCount: len(published)} -} - // CreatePullRequest creates a pull request in a GitHub repository func CreatePullRequest(client *Client, repo *Repository, params map[string]interface{}) (*PullRequest, error) { query := ` @@ -617,29 +524,44 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter } } - // reviewers are requested in yet another additional mutation - reviewParams := make(map[string]interface{}) - if ids, ok := params["userReviewerIds"]; ok && !isBlank(ids) { - reviewParams["userIds"] = ids - } - if ids, ok := params["teamReviewerIds"]; ok && !isBlank(ids) { - reviewParams["teamIds"] = ids - } + // TODO requestReviewsByLoginCleanup + // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation. + // The ID-based path can be removed once GHES supports requestReviewsByLogin. + userLogins, hasUserLogins := params["userReviewerLogins"].([]string) + botLogins, hasBotLogins := params["botReviewerLogins"].([]string) + teamSlugs, hasTeamSlugs := params["teamReviewerSlugs"].([]string) + + if hasUserLogins || hasBotLogins || hasTeamSlugs { + // Use login-based mutation (RequestReviewsByLogin) for github.com + err := RequestReviewsByLogin(client, repo, pr.ID, userLogins, botLogins, teamSlugs, true) + if err != nil { + return pr, err + } + } else { + // Use ID-based mutation (requestReviews) for GHES compatibility + reviewParams := make(map[string]interface{}) + if ids, ok := params["userReviewerIds"]; ok && !isBlank(ids) { + reviewParams["userIds"] = ids + } + if ids, ok := params["teamReviewerIds"]; ok && !isBlank(ids) { + reviewParams["teamIds"] = ids + } - //TODO: How much work to extract this into own method and use for create and edit? - if len(reviewParams) > 0 { - reviewQuery := ` + //TODO: How much work to extract this into own method and use for create and edit? + if len(reviewParams) > 0 { + reviewQuery := ` mutation PullRequestCreateRequestReviews($input: RequestReviewsInput!) { requestReviews(input: $input) { clientMutationId } }` - reviewParams["pullRequestId"] = pr.ID - reviewParams["union"] = true - variables := map[string]interface{}{ - "input": reviewParams, - } - err := client.GraphQL(repo.RepoHost(), reviewQuery, variables, &result) - if err != nil { - return pr, err + reviewParams["pullRequestId"] = pr.ID + reviewParams["union"] = true + variables := map[string]interface{}{ + "input": reviewParams, + } + err := client.GraphQL(repo.RepoHost(), reviewQuery, variables, &result) + if err != nil { + return pr, err + } } } @@ -659,145 +581,6 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter return pr, nil } -// extractTeamSlugs extracts just the slug portion from team identifiers. -// Team identifiers can be in "org/slug" format; this returns just the slug. -func extractTeamSlugs(teams []string) []string { - slugs := make([]string, 0, len(teams)) - for _, t := range teams { - if t == "" { - continue - } - s := strings.SplitN(t, "/", 2) - slugs = append(slugs, s[len(s)-1]) - } - return slugs -} - -// toGitHubV4Strings converts a string slice to a githubv4.String slice, -// optionally appending a suffix to each element. -func toGitHubV4Strings(strs []string, suffix string) []githubv4.String { - result := make([]githubv4.String, len(strs)) - for i, s := range strs { - result[i] = githubv4.String(s + suffix) - } - return result -} - -// AddPullRequestReviews adds the given user and team reviewers to a pull request using the REST API. -// Team identifiers can be in "org/slug" format. -func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { - if len(users) == 0 && len(teams) == 0 { - return nil - } - - // The API requires empty arrays instead of null values - if users == nil { - users = []string{} - } - - path := fmt.Sprintf( - "repos/%s/%s/pulls/%d/requested_reviewers", - url.PathEscape(repo.RepoOwner()), - url.PathEscape(repo.RepoName()), - prNumber, - ) - body := struct { - Reviewers []string `json:"reviewers"` - TeamReviewers []string `json:"team_reviewers"` - }{ - Reviewers: users, - TeamReviewers: extractTeamSlugs(teams), - } - buf := &bytes.Buffer{} - if err := json.NewEncoder(buf).Encode(body); err != nil { - return err - } - // The endpoint responds with the updated pull request object; we don't need it here. - return client.REST(repo.RepoHost(), "POST", path, buf, nil) -} - -// RemovePullRequestReviews removes requested reviewers from a pull request using the REST API. -// Team identifiers can be in "org/slug" format. -func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { - if len(users) == 0 && len(teams) == 0 { - return nil - } - - // The API requires empty arrays instead of null values - if users == nil { - users = []string{} - } - - path := fmt.Sprintf( - "repos/%s/%s/pulls/%d/requested_reviewers", - url.PathEscape(repo.RepoOwner()), - url.PathEscape(repo.RepoName()), - prNumber, - ) - body := struct { - Reviewers []string `json:"reviewers"` - TeamReviewers []string `json:"team_reviewers"` - }{ - Reviewers: users, - TeamReviewers: extractTeamSlugs(teams), - } - buf := &bytes.Buffer{} - if err := json.NewEncoder(buf).Encode(body); err != nil { - return err - } - // The endpoint responds with the updated pull request object; we don't need it here. - return client.REST(repo.RepoHost(), "DELETE", path, buf, nil) -} - -// RequestReviewsByLogin sets requested reviewers on a pull request using the GraphQL mutation. -// This mutation replaces existing reviewers with the provided set unless union is true. -// Only available on github.com, not GHES. -// Bot logins should include the [bot] suffix (e.g., "copilot-pull-request-reviewer[bot]"). -// Team slugs should be in the format "org/team-slug". -// When union is false (replace mode), passing empty slices will remove all reviewers. -func RequestReviewsByLogin(client *Client, repo ghrepo.Interface, prID string, userLogins, botLogins, teamSlugs []string, union bool) error { - // In union mode (additive), nothing to do if all lists are empty. - // In replace mode, we may still need to call the mutation to clear reviewers. - if union && len(userLogins) == 0 && len(botLogins) == 0 && len(teamSlugs) == 0 { - return nil - } - - var mutation struct { - RequestReviewsByLogin struct { - ClientMutationId string `graphql:"clientMutationId"` - } `graphql:"requestReviewsByLogin(input: $input)"` - } - - type RequestReviewsByLoginInput struct { - PullRequestID githubv4.ID `json:"pullRequestId"` - UserLogins *[]githubv4.String `json:"userLogins,omitempty"` - BotLogins *[]githubv4.String `json:"botLogins,omitempty"` - TeamSlugs *[]githubv4.String `json:"teamSlugs,omitempty"` - Union githubv4.Boolean `json:"union"` - } - - input := RequestReviewsByLoginInput{ - PullRequestID: githubv4.ID(prID), - Union: githubv4.Boolean(union), - } - - userLoginValues := toGitHubV4Strings(userLogins, "") - input.UserLogins = &userLoginValues - - // Bot logins require the [bot] suffix for the mutation - botLoginValues := toGitHubV4Strings(botLogins, "[bot]") - input.BotLogins = &botLoginValues - - teamSlugValues := toGitHubV4Strings(teamSlugs, "") - input.TeamSlugs = &teamSlugValues - - variables := map[string]interface{}{ - "input": input, - } - - return client.Mutate(repo.RepoHost(), "RequestReviewsByLogin", &mutation, variables) -} - // SuggestedAssignableActors fetches up to 10 suggested actors for a specific assignable // (Issue or PullRequest) node ID. `assignableID` is the GraphQL node ID for the Issue/PR. // Returns the actors, the total count of available assignees in the repo, and an error. @@ -890,222 +673,6 @@ func SuggestedAssignableActors(client *Client, repo ghrepo.Interface, assignable return actors, availableAssigneesCount, nil } -// ReviewerCandidate represents a potential reviewer for a pull request. -// This can be a User, Bot, or Team. -type ReviewerCandidate interface { - DisplayName() string - Login() string - - sealedReviewerCandidate() -} - -// ReviewerUser is a user who can review a pull request. -type ReviewerUser struct { - AssignableUser -} - -func NewReviewerUser(login, name string) ReviewerUser { - return ReviewerUser{ - AssignableUser: NewAssignableUser("", login, name), - } -} - -func (r ReviewerUser) sealedReviewerCandidate() {} - -// ReviewerBot is a bot who can review a pull request. -type ReviewerBot struct { - AssignableBot -} - -func NewReviewerBot(login string) ReviewerBot { - return ReviewerBot{ - AssignableBot: NewAssignableBot("", login), - } -} - -func (b ReviewerBot) DisplayName() string { - if b.login == CopilotReviewerLogin { - return fmt.Sprintf("%s (AI)", CopilotActorName) - } - return b.Login() -} - -func (r ReviewerBot) sealedReviewerCandidate() {} - -// ReviewerTeam is a team that can review a pull request. -type ReviewerTeam struct { - org string - teamSlug string -} - -// NewReviewerTeam creates a new ReviewerTeam. -func NewReviewerTeam(orgName, teamSlug string) ReviewerTeam { - return ReviewerTeam{org: orgName, teamSlug: teamSlug} -} - -func (r ReviewerTeam) DisplayName() string { - return fmt.Sprintf("%s/%s", r.org, r.teamSlug) -} - -func (r ReviewerTeam) Login() string { - return fmt.Sprintf("%s/%s", r.org, r.teamSlug) -} - -func (r ReviewerTeam) Slug() string { - return r.teamSlug -} - -func (r ReviewerTeam) sealedReviewerCandidate() {} - -// SuggestedReviewerActors fetches suggested reviewers for a pull request. -// It combines results from three sources using a cascading quota system: -// - suggestedReviewerActors - suggested based on PR activity (base quota: 5) -// - repository collaborators - all collaborators (base quota: 5 + unfilled from suggestions) -// - organization teams - all teams for org repos (base quota: 5 + unfilled from collaborators) -// -// This ensures we show up to 15 total candidates, with each source filling any -// unfilled quota from the previous source. Results are deduplicated. -// Returns the candidates, a MoreResults count, and an error. -func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, query string) ([]ReviewerCandidate, int, error) { - // Fetch 10 from each source to allow cascading quota to fill from available results. - // Use a single query that includes organization.teams - if the owner is not an org, - // we'll get a "Could not resolve to an Organization" error which we handle gracefully. - // We also fetch unfiltered total counts via aliases for the "X more" display. - type responseData struct { - Node struct { - PullRequest struct { - SuggestedActors struct { - Nodes []struct { - IsAuthor bool - IsCommenter bool - Reviewer struct { - TypeName string `graphql:"__typename"` - User struct { - Login string - Name string - } `graphql:"... on User"` - Bot struct { - Login string - } `graphql:"... on Bot"` - } - } - } `graphql:"suggestedReviewerActors(first: 10, query: $query)"` - } `graphql:"... on PullRequest"` - } `graphql:"node(id: $id)"` - Repository struct { - Collaborators struct { - Nodes []struct { - Login string - Name string - } - } `graphql:"collaborators(first: 10, query: $query)"` - CollaboratorsTotalCount struct { - TotalCount int - } `graphql:"collaboratorsTotalCount: collaborators(first: 0)"` - } `graphql:"repository(owner: $owner, name: $name)"` - Organization struct { - Teams struct { - Nodes []struct { - Slug string - } - } `graphql:"teams(first: 10, query: $query)"` - TeamsTotalCount struct { - TotalCount int - } `graphql:"teamsTotalCount: teams(first: 0)"` - } `graphql:"organization(login: $owner)"` - } - - variables := map[string]interface{}{ - "id": githubv4.ID(prID), - "query": githubv4.String(query), - "owner": githubv4.String(repo.RepoOwner()), - "name": githubv4.String(repo.RepoName()), - } - - var result responseData - err := client.Query(repo.RepoHost(), "SuggestedReviewerActors", &result, variables) - // Handle the case where the owner is not an organization - the query still returns - // partial data (repository, node), so we can continue processing. - if err != nil && !strings.Contains(err.Error(), errorResolvingOrganization) { - return nil, 0, err - } - - // Build candidates using cascading quota logic: - // Each source has a base quota of 5, plus any unfilled quota from previous sources. - // This ensures we show up to 15 total candidates, filling gaps when earlier sources have fewer. - seen := make(map[string]bool) - var candidates []ReviewerCandidate - const baseQuota = 5 - - // Suggested reviewers (excluding author) - suggestionsAdded := 0 - for _, n := range result.Node.PullRequest.SuggestedActors.Nodes { - if suggestionsAdded >= baseQuota { - break - } - if n.IsAuthor { - continue - } - var candidate ReviewerCandidate - var login string - if n.Reviewer.TypeName == "User" && n.Reviewer.User.Login != "" { - login = n.Reviewer.User.Login - candidate = NewReviewerUser(login, n.Reviewer.User.Name) - } else if n.Reviewer.TypeName == "Bot" && n.Reviewer.Bot.Login != "" { - login = n.Reviewer.Bot.Login - candidate = NewReviewerBot(login) - } else { - continue - } - if !seen[login] { - seen[login] = true - candidates = append(candidates, candidate) - suggestionsAdded++ - } - } - - // Collaborators: quota = base + unfilled from suggestions - collaboratorsQuota := baseQuota + (baseQuota - suggestionsAdded) - collaboratorsAdded := 0 - for _, c := range result.Repository.Collaborators.Nodes { - if collaboratorsAdded >= collaboratorsQuota { - break - } - if c.Login == "" { - continue - } - if !seen[c.Login] { - seen[c.Login] = true - candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) - collaboratorsAdded++ - } - } - - // Teams: quota = base + unfilled from collaborators - teamsQuota := baseQuota + (collaboratorsQuota - collaboratorsAdded) - teamsAdded := 0 - ownerName := repo.RepoOwner() - for _, t := range result.Organization.Teams.Nodes { - if teamsAdded >= teamsQuota { - break - } - if t.Slug == "" { - continue - } - teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) - if !seen[teamLogin] { - seen[teamLogin] = true - candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) - teamsAdded++ - } - } - - // MoreResults uses unfiltered total counts (teams will be 0 for personal repos) - moreResults := result.Repository.CollaboratorsTotalCount.TotalCount + result.Organization.TeamsTotalCount.TotalCount - - return candidates, moreResults, nil -} - func UpdatePullRequestBranch(client *Client, repo ghrepo.Interface, params githubv4.UpdatePullRequestBranchInput) error { var mutation struct { UpdatePullRequestBranch struct { diff --git a/api/queries_pr_review.go b/api/queries_pr_review.go index d5565b54b23..a6fa34f9e88 100644 --- a/api/queries_pr_review.go +++ b/api/queries_pr_review.go @@ -1,6 +1,11 @@ package api import ( + "bytes" + "encoding/json" + "fmt" + "net/url" + "strings" "time" "github.com/cli/cli/v2/internal/ghrepo" @@ -42,33 +47,6 @@ type PullRequestReview struct { Commit Commit `json:"commit"` } -func AddReview(client *Client, repo ghrepo.Interface, pr *PullRequest, input *PullRequestReviewInput) error { - var mutation struct { - AddPullRequestReview struct { - ClientMutationID string - } `graphql:"addPullRequestReview(input:$input)"` - } - - state := githubv4.PullRequestReviewEventComment - switch input.State { - case ReviewApprove: - state = githubv4.PullRequestReviewEventApprove - case ReviewRequestChanges: - state = githubv4.PullRequestReviewEventRequestChanges - } - - body := githubv4.String(input.Body) - variables := map[string]interface{}{ - "input": githubv4.AddPullRequestReviewInput{ - PullRequestID: pr.ID, - Event: &state, - Body: &body, - }, - } - - return client.Mutate(repo.RepoHost(), "PullRequestReviewAdd", &mutation, variables) -} - func (prr PullRequestReview) Identifier() string { return prr.ID } @@ -115,3 +93,617 @@ func (prr PullRequestReview) Reactions() ReactionGroups { func (prr PullRequestReview) Status() string { return prr.State } + +type PullRequestReviewStatus struct { + ChangesRequested bool + Approved bool + ReviewRequired bool +} + +func (pr *PullRequest) ReviewStatus() PullRequestReviewStatus { + var status PullRequestReviewStatus + switch pr.ReviewDecision { + case "CHANGES_REQUESTED": + status.ChangesRequested = true + case "APPROVED": + status.Approved = true + case "REVIEW_REQUIRED": + status.ReviewRequired = true + } + return status +} + +func (pr *PullRequest) DisplayableReviews() PullRequestReviews { + published := []PullRequestReview{} + for _, prr := range pr.Reviews.Nodes { + //Dont display pending reviews + //Dont display commenting reviews without top level comment body + if prr.State != "PENDING" && !(prr.State == "COMMENTED" && prr.Body == "") { + published = append(published, prr) + } + } + return PullRequestReviews{Nodes: published, TotalCount: len(published)} +} + +type ReviewRequests struct { + Nodes []struct { + RequestedReviewer RequestedReviewer + } +} + +type RequestedReviewer struct { + TypeName string `json:"__typename"` + Login string `json:"login"` + Name string `json:"name"` + Slug string `json:"slug"` + Organization struct { + Login string `json:"login"` + } `json:"organization"` +} + +const teamTypeName = "Team" +const botTypeName = "Bot" + +func (r RequestedReviewer) LoginOrSlug() string { + if r.TypeName == teamTypeName { + return fmt.Sprintf("%s/%s", r.Organization.Login, r.Slug) + } + return r.Login +} + +// DisplayName returns a user-friendly name for the reviewer. +// For Copilot bot, returns "Copilot (AI)". For teams, returns "org/slug". +// For users, returns "login (Name)" if name is available, otherwise just login. +func (r RequestedReviewer) DisplayName() string { + if r.TypeName == teamTypeName { + return fmt.Sprintf("%s/%s", r.Organization.Login, r.Slug) + } + if r.TypeName == botTypeName && r.Login == CopilotReviewerLogin { + return "Copilot (AI)" + } + if r.Name != "" { + return fmt.Sprintf("%s (%s)", r.Login, r.Name) + } + return r.Login +} + +func (r ReviewRequests) Logins() []string { + logins := make([]string, len(r.Nodes)) + for i, r := range r.Nodes { + logins[i] = r.RequestedReviewer.LoginOrSlug() + } + return logins +} + +// DisplayNames returns user-friendly display names for all requested reviewers. +func (r ReviewRequests) DisplayNames() []string { + names := make([]string, len(r.Nodes)) + for i, r := range r.Nodes { + names[i] = r.RequestedReviewer.DisplayName() + } + return names +} + +// ReviewerCandidate represents a potential reviewer for a pull request. +// This can be a User, Bot, or Team. It mirrors AssignableActor but adds +// team support (teams can review but not be assigned) and drops the ID method. +// ReviewerUser and ReviewerBot are thin wrappers around AssignableUser and +// AssignableBot that satisfy this interface. +type ReviewerCandidate interface { + DisplayName() string + Login() string + + sealedReviewerCandidate() +} + +// ReviewerUser is a user who can review a pull request. +type ReviewerUser struct { + AssignableUser +} + +func NewReviewerUser(login, name string) ReviewerUser { + return ReviewerUser{ + AssignableUser: NewAssignableUser("", login, name), + } +} + +func (r ReviewerUser) sealedReviewerCandidate() {} + +// ReviewerBot is a bot who can review a pull request. +type ReviewerBot struct { + AssignableBot +} + +func NewReviewerBot(login string) ReviewerBot { + return ReviewerBot{ + AssignableBot: NewAssignableBot("", login), + } +} + +func (b ReviewerBot) DisplayName() string { + if b.login == CopilotReviewerLogin { + return fmt.Sprintf("%s (AI)", CopilotActorName) + } + return b.Login() +} + +func (r ReviewerBot) sealedReviewerCandidate() {} + +// ReviewerTeam is a team that can review a pull request. +type ReviewerTeam struct { + org string + teamSlug string +} + +// NewReviewerTeam creates a new ReviewerTeam. +func NewReviewerTeam(orgName, teamSlug string) ReviewerTeam { + return ReviewerTeam{org: orgName, teamSlug: teamSlug} +} + +func (r ReviewerTeam) DisplayName() string { + return fmt.Sprintf("%s/%s", r.org, r.teamSlug) +} + +func (r ReviewerTeam) Login() string { + return fmt.Sprintf("%s/%s", r.org, r.teamSlug) +} + +func (r ReviewerTeam) Slug() string { + return r.teamSlug +} + +func (r ReviewerTeam) sealedReviewerCandidate() {} + +func AddReview(client *Client, repo ghrepo.Interface, pr *PullRequest, input *PullRequestReviewInput) error { + var mutation struct { + AddPullRequestReview struct { + ClientMutationID string + } `graphql:"addPullRequestReview(input:$input)"` + } + + state := githubv4.PullRequestReviewEventComment + switch input.State { + case ReviewApprove: + state = githubv4.PullRequestReviewEventApprove + case ReviewRequestChanges: + state = githubv4.PullRequestReviewEventRequestChanges + } + + body := githubv4.String(input.Body) + variables := map[string]interface{}{ + "input": githubv4.AddPullRequestReviewInput{ + PullRequestID: pr.ID, + Event: &state, + Body: &body, + }, + } + + return client.Mutate(repo.RepoHost(), "PullRequestReviewAdd", &mutation, variables) +} + +// AddPullRequestReviews adds the given user and team reviewers to a pull request using the REST API. +// Team identifiers can be in "org/slug" format. +func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { + if len(users) == 0 && len(teams) == 0 { + return nil + } + + // The API requires empty arrays instead of null values + if users == nil { + users = []string{} + } + + path := fmt.Sprintf( + "repos/%s/%s/pulls/%d/requested_reviewers", + url.PathEscape(repo.RepoOwner()), + url.PathEscape(repo.RepoName()), + prNumber, + ) + body := struct { + Reviewers []string `json:"reviewers"` + TeamReviewers []string `json:"team_reviewers"` + }{ + Reviewers: users, + TeamReviewers: extractTeamSlugs(teams), + } + buf := &bytes.Buffer{} + if err := json.NewEncoder(buf).Encode(body); err != nil { + return err + } + // The endpoint responds with the updated pull request object; we don't need it here. + return client.REST(repo.RepoHost(), "POST", path, buf, nil) +} + +// RemovePullRequestReviews removes requested reviewers from a pull request using the REST API. +// Team identifiers can be in "org/slug" format. +func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { + if len(users) == 0 && len(teams) == 0 { + return nil + } + + // The API requires empty arrays instead of null values + if users == nil { + users = []string{} + } + + path := fmt.Sprintf( + "repos/%s/%s/pulls/%d/requested_reviewers", + url.PathEscape(repo.RepoOwner()), + url.PathEscape(repo.RepoName()), + prNumber, + ) + body := struct { + Reviewers []string `json:"reviewers"` + TeamReviewers []string `json:"team_reviewers"` + }{ + Reviewers: users, + TeamReviewers: extractTeamSlugs(teams), + } + buf := &bytes.Buffer{} + if err := json.NewEncoder(buf).Encode(body); err != nil { + return err + } + // The endpoint responds with the updated pull request object; we don't need it here. + return client.REST(repo.RepoHost(), "DELETE", path, buf, nil) +} + +// RequestReviewsByLogin sets requested reviewers on a pull request using the GraphQL mutation. +// This mutation replaces existing reviewers with the provided set unless union is true. +// Only available on github.com, not GHES. +// Bot logins should be passed without the [bot] suffix; it is appended automatically. +// Team slugs must be in the format "org/team-slug". +// When union is false (replace mode), passing empty slices will remove all reviewers. +func RequestReviewsByLogin(client *Client, repo ghrepo.Interface, prID string, userLogins, botLogins, teamSlugs []string, union bool) error { + // In union mode (additive), nothing to do if all lists are empty. + // In replace mode, we may still need to call the mutation to clear reviewers. + if union && len(userLogins) == 0 && len(botLogins) == 0 && len(teamSlugs) == 0 { + return nil + } + + var mutation struct { + RequestReviewsByLogin struct { + ClientMutationId string `graphql:"clientMutationId"` + } `graphql:"requestReviewsByLogin(input: $input)"` + } + + type RequestReviewsByLoginInput struct { + PullRequestID githubv4.ID `json:"pullRequestId"` + UserLogins *[]githubv4.String `json:"userLogins,omitempty"` + BotLogins *[]githubv4.String `json:"botLogins,omitempty"` + TeamSlugs *[]githubv4.String `json:"teamSlugs,omitempty"` + Union githubv4.Boolean `json:"union"` + } + + input := RequestReviewsByLoginInput{ + PullRequestID: githubv4.ID(prID), + Union: githubv4.Boolean(union), + } + + userLoginValues := toGitHubV4Strings(userLogins, "") + input.UserLogins = &userLoginValues + + // Bot logins require the [bot] suffix for the mutation + botLoginValues := toGitHubV4Strings(botLogins, "[bot]") + input.BotLogins = &botLoginValues + + teamSlugValues := toGitHubV4Strings(teamSlugs, "") + input.TeamSlugs = &teamSlugValues + + variables := map[string]interface{}{ + "input": input, + } + + return client.Mutate(repo.RepoHost(), "RequestReviewsByLogin", &mutation, variables) +} + +// SuggestedReviewerActors fetches suggested reviewers for a pull request. +// It combines results from three sources using a cascading quota system: +// - suggestedReviewerActors - suggested based on PR activity (base quota: 5) +// - repository collaborators - all collaborators (base quota: 5 + unfilled from suggestions) +// - organization teams - all teams for org repos (base quota: 5 + unfilled from collaborators) +// +// This ensures we show up to 15 total candidates, with each source filling any +// unfilled quota from the previous source. Results are deduplicated. +// Returns the candidates, a MoreResults count, and an error. +func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, query string) ([]ReviewerCandidate, int, error) { + // Fetch 10 from each source to allow cascading quota to fill from available results. + // Organization teams are fetched via repository.owner inline fragment, which + // gracefully returns empty data for personal (User-owned) repos. + // We also fetch unfiltered total counts via aliases for the "X more" display. + type responseData struct { + Node struct { + PullRequest struct { + Author struct { + Login string + } + SuggestedActors struct { + Nodes []struct { + IsAuthor bool + IsCommenter bool + Reviewer struct { + TypeName string `graphql:"__typename"` + User struct { + Login string + Name string + } `graphql:"... on User"` + Bot struct { + Login string + } `graphql:"... on Bot"` + } + } + } `graphql:"suggestedReviewerActors(first: 10, query: $query)"` + } `graphql:"... on PullRequest"` + } `graphql:"node(id: $id)"` + Repository struct { + Owner struct { + TypeName string `graphql:"__typename"` + Organization struct { + Teams struct { + Nodes []struct { + Slug string + } + } `graphql:"teams(first: 10, query: $query)"` + TeamsTotalCount struct { + TotalCount int + } `graphql:"teamsTotalCount: teams(first: 0)"` + } `graphql:"... on Organization"` + } + Collaborators struct { + Nodes []struct { + Login string + Name string + } + } `graphql:"collaborators(first: 10, query: $query)"` + CollaboratorsTotalCount struct { + TotalCount int + } `graphql:"collaboratorsTotalCount: collaborators(first: 0)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "id": githubv4.ID(prID), + "query": githubv4.String(query), + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + } + + var result responseData + err := client.Query(repo.RepoHost(), "SuggestedReviewerActors", &result, variables) + if err != nil { + return nil, 0, err + } + + // Build candidates using cascading quota logic: + // Each source has a base quota of 5, plus any unfilled quota from previous sources. + // This ensures we show up to 15 total candidates, filling gaps when earlier sources have fewer. + // Pre-seed seen with the PR author since you cannot review your own PR. + seen := make(map[string]bool) + if authorLogin := result.Node.PullRequest.Author.Login; authorLogin != "" { + seen[authorLogin] = true + } + var candidates []ReviewerCandidate + const baseQuota = 5 + + // Suggested reviewers (excluding author) + suggestionsAdded := 0 + for _, n := range result.Node.PullRequest.SuggestedActors.Nodes { + if suggestionsAdded >= baseQuota { + break + } + if n.IsAuthor { + continue + } + var candidate ReviewerCandidate + var login string + if n.Reviewer.TypeName == "User" && n.Reviewer.User.Login != "" { + login = n.Reviewer.User.Login + candidate = NewReviewerUser(login, n.Reviewer.User.Name) + } else if n.Reviewer.TypeName == "Bot" && n.Reviewer.Bot.Login != "" { + login = n.Reviewer.Bot.Login + candidate = NewReviewerBot(login) + } else { + continue + } + if !seen[login] { + seen[login] = true + candidates = append(candidates, candidate) + suggestionsAdded++ + } + } + + // Collaborators: quota = base + unfilled from suggestions + collaboratorsQuota := baseQuota + (baseQuota - suggestionsAdded) + collaboratorsAdded := 0 + for _, c := range result.Repository.Collaborators.Nodes { + if collaboratorsAdded >= collaboratorsQuota { + break + } + if c.Login == "" { + continue + } + if !seen[c.Login] { + seen[c.Login] = true + candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) + collaboratorsAdded++ + } + } + + // Teams: quota = base + unfilled from collaborators + teamsQuota := baseQuota + (collaboratorsQuota - collaboratorsAdded) + teamsAdded := 0 + ownerName := repo.RepoOwner() + for _, t := range result.Repository.Owner.Organization.Teams.Nodes { + if teamsAdded >= teamsQuota { + break + } + if t.Slug == "" { + continue + } + teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) + if !seen[teamLogin] { + seen[teamLogin] = true + candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) + teamsAdded++ + } + } + + // MoreResults uses unfiltered total counts (teams will be 0 for personal repos) + moreResults := result.Repository.CollaboratorsTotalCount.TotalCount + result.Repository.Owner.Organization.TeamsTotalCount.TotalCount + + return candidates, moreResults, nil +} + +// SuggestedReviewerActorsForRepo fetches potential reviewers for a repository. +// Unlike SuggestedReviewerActors, this doesn't require an existing PR - used for gh pr create. +// It combines results from two sources using a cascading quota system: +// - repository collaborators (base quota: 5) +// - organization teams (base quota: 5 + unfilled from collaborators) +// +// This ensures we show up to 10 total candidates, with each source filling any +// unfilled quota from the previous source. Results are deduplicated. +// Returns the candidates, a MoreResults count, and an error. +func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query string) ([]ReviewerCandidate, int, error) { + type responseData struct { + Viewer struct { + Login string + } + Repository struct { + // HACK: There's no repo-level API to check Copilot reviewer eligibility, + // so we piggyback on an open PR's suggestedReviewerActors to detect + // whether Copilot is available as a reviewer for this repository. + PullRequests struct { + Nodes []struct { + SuggestedActors struct { + Nodes []struct { + Reviewer struct { + TypeName string `graphql:"__typename"` + Bot struct { + Login string + } `graphql:"... on Bot"` + } + } + } `graphql:"suggestedReviewerActors(first: 10)"` + } + } `graphql:"pullRequests(first: 1, states: [OPEN])"` + Owner struct { + TypeName string `graphql:"__typename"` + Organization struct { + Teams struct { + Nodes []struct { + Slug string + } + } `graphql:"teams(first: 10, query: $query)"` + TeamsTotalCount struct { + TotalCount int + } `graphql:"teamsTotalCount: teams(first: 0)"` + } `graphql:"... on Organization"` + } + Collaborators struct { + Nodes []struct { + Login string + Name string + } + } `graphql:"collaborators(first: 10, query: $query)"` + CollaboratorsTotalCount struct { + TotalCount int + } `graphql:"collaboratorsTotalCount: collaborators(first: 0)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "query": githubv4.String(query), + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + } + + var result responseData + err := client.Query(repo.RepoHost(), "SuggestedReviewerActorsForRepo", &result, variables) + if err != nil { + return nil, 0, err + } + + // Build candidates using cascading quota logic. + // Pre-seed seen with the current user to exclude them from results + // since you cannot review your own PR. + seen := make(map[string]bool) + if result.Viewer.Login != "" { + seen[result.Viewer.Login] = true + } + var candidates []ReviewerCandidate + const baseQuota = 5 + + // Check for Copilot availability from open PR's suggested reviewers + for _, pr := range result.Repository.PullRequests.Nodes { + for _, actor := range pr.SuggestedActors.Nodes { + if actor.Reviewer.TypeName == "Bot" && actor.Reviewer.Bot.Login == CopilotReviewerLogin { + candidates = append(candidates, NewReviewerBot(CopilotReviewerLogin)) + seen[CopilotReviewerLogin] = true + break + } + } + } + + // Collaborators + collaboratorsAdded := 0 + for _, c := range result.Repository.Collaborators.Nodes { + if collaboratorsAdded >= baseQuota { + break + } + if c.Login == "" { + continue + } + if !seen[c.Login] { + seen[c.Login] = true + candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) + collaboratorsAdded++ + } + } + + // Teams: quota = base + unfilled from collaborators + teamsQuota := baseQuota + (baseQuota - collaboratorsAdded) + teamsAdded := 0 + ownerName := repo.RepoOwner() + for _, t := range result.Repository.Owner.Organization.Teams.Nodes { + if teamsAdded >= teamsQuota { + break + } + if t.Slug == "" { + continue + } + teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) + if !seen[teamLogin] { + seen[teamLogin] = true + candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) + teamsAdded++ + } + } + + // MoreResults uses unfiltered total counts (teams will be 0 for personal repos) + moreResults := result.Repository.CollaboratorsTotalCount.TotalCount + result.Repository.Owner.Organization.TeamsTotalCount.TotalCount + + return candidates, moreResults, nil +} + +// extractTeamSlugs extracts just the slug portion from team identifiers. +// Team identifiers can be in "org/slug" format; this returns just the slug. +func extractTeamSlugs(teams []string) []string { + slugs := make([]string, 0, len(teams)) + for _, t := range teams { + if t == "" { + continue + } + s := strings.SplitN(t, "/", 2) + slugs = append(slugs, s[len(s)-1]) + } + return slugs +} + +// toGitHubV4Strings converts a string slice to a githubv4.String slice, +// optionally appending a suffix to each element. +func toGitHubV4Strings(strs []string, suffix string) []githubv4.String { + result := make([]githubv4.String, len(strs)) + for i, s := range strs { + result[i] = githubv4.String(s + suffix) + } + return result +} diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 69dc505ca70..633b9a8c35f 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -160,18 +160,16 @@ func mockReviewerResponse(suggestions, collabs, teams, totalCollabs, totalTeams return fmt.Sprintf(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [%s]}}, + "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [%s]}}, "repository": { + "owner": {"__typename": "Organization", "teams": {"nodes": [%s]}, "teamsTotalCount": {"totalCount": %d}}, "collaborators": {"nodes": [%s]}, "collaboratorsTotalCount": {"totalCount": %d} - }, - "organization": { - "teams": {"nodes": [%s]}, - "teamsTotalCount": {"totalCount": %d} } } - }`, strings.Join(suggestionNodes, ","), strings.Join(collabNodes, ","), totalCollabs, - strings.Join(teamNodes, ","), totalTeams) + }`, strings.Join(suggestionNodes, ","), + strings.Join(teamNodes, ","), totalTeams, + strings.Join(collabNodes, ","), totalCollabs) } func TestSuggestedReviewerActors(t *testing.T) { @@ -235,18 +233,15 @@ func TestSuggestedReviewerActors(t *testing.T) { httpmock.GraphQL(`query SuggestedReviewerActors\b`), httpmock.StringResponse(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [ + "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [ {"isAuthor": true, "reviewer": {"__typename": "User", "login": "author", "name": "Author"}}, {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}}, {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s2", "name": "S2"}} ]}}, "repository": { + "owner": {"__typename": "Organization", "teams": {"nodes": [{"slug": "team1"}]}, "teamsTotalCount": {"totalCount": 3}}, "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, "collaboratorsTotalCount": {"totalCount": 5} - }, - "organization": { - "teams": {"nodes": [{"slug": "team1"}]}, - "teamsTotalCount": {"totalCount": 3} } } }`)) @@ -255,6 +250,30 @@ func TestSuggestedReviewerActors(t *testing.T) { expectedLogins: []string{"s1", "s2", "c1", "OWNER/team1"}, expectedMore: 8, }, + { + name: "author excluded from collaborators", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"author": {"login": "theauthor"}, "suggestedReviewerActors": {"nodes": [ + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} + ]}}, + "repository": { + "collaborators": {"nodes": [ + {"login": "theauthor", "name": "The Author"}, + {"login": "c1", "name": "C1"} + ]}, + "collaboratorsTotalCount": {"totalCount": 5} + } + } + }`)) + }, + expectedCount: 2, + expectedLogins: []string{"s1", "c1"}, + expectedMore: 5, + }, { name: "deduplication across sources", httpStubs: func(reg *httpmock.Registry) { @@ -263,19 +282,16 @@ func TestSuggestedReviewerActors(t *testing.T) { httpmock.GraphQL(`query SuggestedReviewerActors\b`), httpmock.StringResponse(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [ + "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [ {"isAuthor": false, "reviewer": {"__typename": "User", "login": "shareduser", "name": "Shared"}} ]}}, "repository": { + "owner": {"__typename": "Organization", "teams": {"nodes": [{"slug": "team1"}]}, "teamsTotalCount": {"totalCount": 5}}, "collaborators": {"nodes": [ {"login": "shareduser", "name": "Shared"}, {"login": "c1", "name": "C1"} ]}, "collaboratorsTotalCount": {"totalCount": 10} - }, - "organization": { - "teams": {"nodes": [{"slug": "team1"}]}, - "teamsTotalCount": {"totalCount": 5} } } }`)) @@ -291,16 +307,15 @@ func TestSuggestedReviewerActors(t *testing.T) { httpmock.GraphQL(`query SuggestedReviewerActors\b`), httpmock.StringResponse(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [ + "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [ {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} ]}}, "repository": { + "owner": {"__typename": "User"}, "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, "collaboratorsTotalCount": {"totalCount": 3} - }, - "organization": null - }, - "errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}] + } + } }`)) }, expectedCount: 2, @@ -314,17 +329,14 @@ func TestSuggestedReviewerActors(t *testing.T) { httpmock.GraphQL(`query SuggestedReviewerActors\b`), httpmock.StringResponse(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [ + "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [ {"isAuthor": false, "reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}}, {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} ]}}, "repository": { + "owner": {"__typename": "Organization", "teams": {"nodes": []}, "teamsTotalCount": {"totalCount": 0}}, "collaborators": {"nodes": []}, "collaboratorsTotalCount": {"totalCount": 5} - }, - "organization": { - "teams": {"nodes": []}, - "teamsTotalCount": {"totalCount": 0} } } }`)) @@ -362,3 +374,195 @@ func TestSuggestedReviewerActors(t *testing.T) { }) } } + +// mockReviewerResponseForRepo generates a GraphQL response for SuggestedReviewerActorsForRepo tests. +// It creates collaborators (c1, c2...) and teams (team1, team2...). +func mockReviewerResponseForRepo(collabs, teams, totalCollabs, totalTeams int) string { + return mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalTeams, false) +} + +// mockReviewerResponseForRepoWithCopilot generates a GraphQL response for SuggestedReviewerActorsForRepo tests. +// If copilotAvailable is true, includes Copilot in the first open PR's suggested reviewers. +func mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalTeams int, copilotAvailable bool) string { + var collabNodes, teamNodes []string + + for i := 1; i <= collabs; i++ { + collabNodes = append(collabNodes, + fmt.Sprintf(`{"login": "c%d", "name": "C%d"}`, i, i)) + } + for i := 1; i <= teams; i++ { + teamNodes = append(teamNodes, + fmt.Sprintf(`{"slug": "team%d"}`, i)) + } + + pullRequestsJSON := `"pullRequests": {"nodes": []}` + if copilotAvailable { + pullRequestsJSON = `"pullRequests": {"nodes": [{"suggestedReviewerActors": {"nodes": [{"reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}}]}}]}` + } + + return fmt.Sprintf(`{ + "data": { + "viewer": {"login": "testuser"}, + "repository": { + %s, + "owner": {"__typename": "Organization", "teams": {"nodes": [%s]}, "teamsTotalCount": {"totalCount": %d}}, + "collaborators": {"nodes": [%s]}, + "collaboratorsTotalCount": {"totalCount": %d} + } + } + }`, pullRequestsJSON, + strings.Join(teamNodes, ","), totalTeams, + strings.Join(collabNodes, ","), totalCollabs) +} + +func TestSuggestedReviewerActorsForRepo(t *testing.T) { + tests := []struct { + name string + httpStubs func(*httpmock.Registry) + expectedCount int + expectedLogins []string + expectedMore int + expectError bool + }{ + { + name: "both sources plentiful - 5 each from cascading quota", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(6, 6, 20, 10))) + }, + expectedCount: 10, + expectedLogins: []string{"c1", "c2", "c3", "c4", "c5", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"}, + expectedMore: 30, + }, + { + name: "few collaborators - teams fill gap", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(2, 10, 2, 15))) + }, + expectedCount: 10, + expectedLogins: []string{"c1", "c2", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8"}, + expectedMore: 17, + }, + { + name: "no collaborators - teams only", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(0, 10, 0, 20))) + }, + expectedCount: 10, + expectedLogins: []string{"OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"}, + expectedMore: 20, + }, + { + name: "personal repo - no organization teams", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(`{ + "data": { + "repository": { + "pullRequests": {"nodes": []}, + "owner": {"__typename": "User"}, + "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, + "collaboratorsTotalCount": {"totalCount": 3} + } + } + }`)) + }, + expectedCount: 1, + expectedLogins: []string{"c1"}, + expectedMore: 3, + }, + { + name: "empty repo", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(0, 0, 0, 0))) + }, + expectedCount: 0, + expectedLogins: []string{}, + expectedMore: 0, + }, + { + name: "copilot available - prepended to candidates", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepoWithCopilot(3, 2, 5, 5, true))) + }, + expectedCount: 6, + expectedLogins: []string{"copilot-pull-request-reviewer", "c1", "c2", "c3", "OWNER/team1", "OWNER/team2"}, + expectedMore: 10, + }, + { + name: "copilot not available - not included", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepoWithCopilot(3, 2, 5, 5, false))) + }, + expectedCount: 5, + expectedLogins: []string{"c1", "c2", "c3", "OWNER/team1", "OWNER/team2"}, + expectedMore: 10, + }, + { + name: "viewer excluded from collaborators", + httpStubs: func(reg *httpmock.Registry) { + // c1 matches the viewer login "testuser" won't be in this fixture, + // but we can craft a response where the viewer login matches a collaborator. + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(`{ + "data": { + "viewer": {"login": "c2"}, + "repository": { + "pullRequests": {"nodes": []}, + "owner": {"__typename": "Organization", "teams": {"nodes": []}, "teamsTotalCount": {"totalCount": 0}}, + "collaborators": {"nodes": [ + {"login": "c1", "name": "C1"}, + {"login": "c2", "name": "C2"}, + {"login": "c3", "name": "C3"} + ]}, + "collaboratorsTotalCount": {"totalCount": 3} + } + } + }`)) + }, + expectedCount: 2, + expectedLogins: []string{"c1", "c3"}, + expectedMore: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + if tt.httpStubs != nil { + tt.httpStubs(reg) + } + + client := newTestClient(reg) + repo, _ := ghrepo.FromFullName("OWNER/REPO") + + candidates, moreResults, err := SuggestedReviewerActorsForRepo(client, repo, "") + if tt.expectError { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.expectedCount, len(candidates), "candidate count mismatch") + assert.Equal(t, tt.expectedMore, moreResults, "moreResults mismatch") + + logins := make([]string, len(candidates)) + for i, c := range candidates { + logins[i] = c.Login() + } + assert.Equal(t, tt.expectedLogins, logins) + }) + } +} diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 0a0c675e8cc..da3648c31b2 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -313,7 +313,7 @@ func createRun(opts *CreateOptions) (err error) { Repo: baseRepo, State: &tb, } - err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support) + err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support, nil) if err != nil { return } diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 3949ecbb618..0dbdb3987b9 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -21,6 +21,7 @@ import ( fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -398,11 +399,38 @@ func createRun(opts *CreateOptions) error { client := ctx.Client + // Detect ActorIsAssignable feature to determine if we can use search-based + // reviewer selection (github.com) or need to use legacy ID-based selection (GHES) + issueFeatures, err := opts.Detector.IssueFeatures() + if err != nil { + return err + } + var reviewerSearchFunc func(string) prompter.MultiSelectSearchResult + if issueFeatures.ActorIsAssignable { + reviewerSearchFunc = func(query string) prompter.MultiSelectSearchResult { + candidates, moreResults, err := api.SuggestedReviewerActorsForRepo(client, ctx.PRRefs.BaseRepo(), query) + if err != nil { + return prompter.MultiSelectSearchResult{Err: err} + } + keys := make([]string, len(candidates)) + labels := make([]string, len(candidates)) + for i, c := range candidates { + keys[i] = c.Login() + labels[i] = c.DisplayName() + } + return prompter.MultiSelectSearchResult{Keys: keys, Labels: labels, MoreResults: moreResults} + } + } + state, err := NewIssueState(*ctx, *opts) if err != nil { return err } + if issueFeatures.ActorIsAssignable { + state.ActorReviewers = true + } + var openURL string if opts.WebMode { @@ -569,7 +597,7 @@ func createRun(opts *CreateOptions) error { Repo: ctx.PRRefs.BaseRepo(), State: state, } - err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, projectsV1Support) + err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, projectsV1Support, reviewerSearchFunc) if err != nil { return err } @@ -653,9 +681,12 @@ func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadata return nil, err } + copilotReplacer := shared.NewCopilotReviewerReplacer() + reviewers := copilotReplacer.ReplaceSlice(opts.Reviewers) + state := &shared.IssueMetadataState{ Type: shared.PRMetadata, - Reviewers: opts.Reviewers, + Reviewers: reviewers, Assignees: assignees, Labels: opts.Labels, ProjectTitles: opts.Projects, diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 67286902538..ed2a9cf2e63 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -434,92 +434,6 @@ func Test_createRun(t *testing.T) { }, expectedErrOut: "", }, - { - name: "dry-run-nontty-with-all-opts", - tty: false, - setup: func(opts *CreateOptions, t *testing.T) func() { - opts.TitleProvided = true - opts.BodyProvided = true - opts.Title = "TITLE" - opts.Body = "BODY" - opts.BaseBranch = "trunk" - opts.HeadBranch = "feature" - opts.Assignees = []string{"monalisa"} - opts.Labels = []string{"bug", "todo"} - opts.Projects = []string{"roadmap"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} - opts.Milestone = "big one.oh" - opts.DryRun = true - return func() {} - }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "name": "" }, - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryLabelList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryMilestoneList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "milestones": { - "nodes": [ - { "title": "GA", "id": "GAID" }, - { "title": "Big One.oh", "id": "BIGONEID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - mockRetrieveProjects(t, reg) - }, - expectedOutputs: []string{ - "Would have created a Pull Request with:", - `title: TITLE`, - `draft: false`, - `base: trunk`, - `head: feature`, - `labels: bug, todo`, - `reviewers: hubot, monalisa, /core, /robots`, - `assignees: monalisa`, - `milestones: big one.oh`, - `projects: roadmap`, - `maintainerCanModify: false`, - `body:`, - `BODY`, - ``, - }, - expectedErrOut: "", - }, { name: "dry-run-tty-with-default-base", tty: true, @@ -549,98 +463,6 @@ func Test_createRun(t *testing.T) { Dry Running pull request for feature into master in OWNER/REPO - `), - }, - { - name: "dry-run-tty-with-all-opts", - tty: true, - setup: func(opts *CreateOptions, t *testing.T) func() { - opts.TitleProvided = true - opts.BodyProvided = true - opts.Title = "TITLE" - opts.Body = "BODY" - opts.BaseBranch = "trunk" - opts.HeadBranch = "feature" - opts.Assignees = []string{"monalisa"} - opts.Labels = []string{"bug", "todo"} - opts.Projects = []string{"roadmap"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} - opts.Milestone = "big one.oh" - opts.DryRun = true - return func() {} - }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "name": "" }, - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryLabelList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryMilestoneList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "milestones": { - "nodes": [ - { "title": "GA", "id": "GAID" }, - { "title": "Big One.oh", "id": "BIGONEID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - mockRetrieveProjects(t, reg) - }, - expectedOutputs: []string{ - `Would have created a Pull Request with:`, - `Title: TITLE`, - `Draft: false`, - `Base: trunk`, - `Head: feature`, - `Labels: bug, todo`, - `Reviewers: hubot, monalisa, /core, /robots`, - `Assignees: monalisa`, - `Milestones: big one.oh`, - `Projects: roadmap`, - `MaintainerCanModify: false`, - `Body:`, - ``, - ` BODY `, - ``, - ``, - }, - expectedErrOut: heredoc.Doc(` - - Dry Running pull request for feature into trunk in OWNER/REPO - `), }, { @@ -1089,14 +911,11 @@ func Test_createRun(t *testing.T) { opts.Assignees = []string{"monalisa"} opts.Labels = []string{"bug", "todo"} opts.Projects = []string{"roadmap"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} + opts.Reviewers = []string{"hubot", "monalisa", "OWNER/core", "OWNER/robots"} opts.Milestone = "big one.oh" return func() {} }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) reg.Register( httpmock.GraphQL(`query RepositoryAssignableUsers\b`), httpmock.StringResponse(` @@ -1130,17 +949,6 @@ func Test_createRun(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) mockRetrieveProjects(t, reg) reg.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), @@ -1173,15 +981,15 @@ func Test_createRun(t *testing.T) { assert.Equal(t, "BIGONEID", inputs["milestoneId"]) })) reg.Register( - httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), httpmock.GraphQLMutation(` - { "data": { "requestReviews": { + { "data": { "requestReviewsByLogin": { "clientMutationId": "" } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) - assert.Equal(t, []interface{}{"HUBOTID", "MONAID"}, inputs["userIds"]) - assert.Equal(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + assert.Equal(t, []interface{}{"hubot", "monalisa"}, inputs["userLogins"]) + assert.Equal(t, []interface{}{"OWNER/core", "OWNER/robots"}, inputs["teamSlugs"]) assert.Equal(t, true, inputs["union"]) })) }, @@ -1682,7 +1490,7 @@ func Test_createRun(t *testing.T) { expectedOut: "https://github.com/OWNER/REPO/pull/12\n", }, { - name: "fetch org teams non-interactively if reviewer contains any team", + name: "request reviewers by login", setup: func(opts *CreateOptions, t *testing.T) func() { opts.TitleProvided = true opts.BodyProvided = true @@ -1702,42 +1510,15 @@ func Test_createRun(t *testing.T) { } } } }`, func(input map[string]interface{}) {})) 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 UserCurrent\b`), - httpmock.StringResponse(` - { "data": { "viewer": { "login": "monalisa" } } } - `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), httpmock.GraphQLMutation(` - { "data": { "requestReviews": { + { "data": { "requestReviewsByLogin": { "clientMutationId": "" } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) - assert.Equal(t, []interface{}{"HUBOTID", "MONAID"}, inputs["userIds"]) - assert.Equal(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + assert.Equal(t, []interface{}{"hubot", "monalisa"}, inputs["userLogins"]) + assert.Equal(t, []interface{}{"org/core", "org/robots"}, inputs["teamSlugs"]) assert.Equal(t, true, inputs["union"]) })) }, @@ -1745,13 +1526,13 @@ func Test_createRun(t *testing.T) { expectedErrOut: "", }, { - name: "do not fetch org teams non-interactively if reviewer does not contain any team", + name: "@copilot reviewer resolves to bot login", setup: func(opts *CreateOptions, t *testing.T) func() { opts.TitleProvided = true opts.BodyProvided = true opts.Title = "my title" opts.Body = "my body" - opts.Reviewers = []string{"hubot", "monalisa"} + opts.Reviewers = []string{"hubot", "@copilot"} opts.HeadBranch = "feature" return func() {} }, @@ -1765,49 +1546,433 @@ func Test_createRun(t *testing.T) { } } } }`, func(input map[string]interface{}) {})) 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 UserCurrent\b`), - httpmock.StringResponse(` - { "data": { "viewer": { "login": "monalisa" } } } - `)) - reg.Exclude( - t, - httpmock.GraphQL(`query OrganizationTeamList\b`), - ) - reg.Register( - httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), httpmock.GraphQLMutation(` - { "data": { "requestReviews": { + { "data": { "requestReviewsByLogin": { "clientMutationId": "" } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) - assert.Equal(t, []interface{}{"HUBOTID", "MONAID"}, inputs["userIds"]) - assert.NotEqual(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + assert.Equal(t, []interface{}{"hubot"}, inputs["userLogins"]) + assert.Equal(t, []interface{}{"copilot-pull-request-reviewer[bot]"}, inputs["botLogins"]) assert.Equal(t, true, inputs["union"]) })) }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", expectedErrOut: "", }, - { - name: "fetch org teams interactively if reviewer metadata selected", - tty: true, - setup: func(opts *CreateOptions, t *testing.T) func() { - // In order to test additional metadata, title and body cannot be provided here. - opts.HeadBranch = "feature" - return func() {} - }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + branch := "feature" + reg := &httpmock.Registry{} + reg.StubRepoInfoResponse("OWNER", "REPO", "master") + defer reg.Verify(t) + if tt.httpStubs != nil { + tt.httpStubs(reg, t) + } + + pm := &prompter.PrompterMock{} + + if tt.promptStubs != nil { + tt.promptStubs(pm) + } + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + if !tt.customBranchConfig { + cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "") + } + + if tt.cmdStubs != nil { + tt.cmdStubs(cs) + } + + opts := CreateOptions{} + opts.Prompter = pm + + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(tt.tty) + ios.SetStdinTTY(tt.tty) + ios.SetStderrTTY(tt.tty) + + browser := &browser.Stub{} + opts.IO = ios + opts.Browser = browser + opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + opts.Config = func() (gh.Config, error) { + return config.NewBlankConfig(), nil + } + opts.Remotes = func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "origin", + Resolved: "base", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + } + opts.Branch = func() (string, error) { + return branch, nil + } + opts.Finder = shared.NewMockFinder(branch, nil, nil) + opts.GitClient = &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } + cleanSetup := func() {} + if tt.setup != nil { + cleanSetup = tt.setup(&opts, t) + } + defer cleanSetup() + + // All tests in this function use github.com behavior + opts.Detector = &fd.EnabledDetectorMock{} + + if opts.HeadBranch == "" { + cs.Register(`git status --porcelain`, 0, "") + } + + err := createRun(&opts) + output := &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + BrowsedURL: browser.BrowsedURL(), + } + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + } else { + assert.NoError(t, err) + if tt.expectedOut != "" { + assert.Equal(t, tt.expectedOut, output.String()) + } + if len(tt.expectedOutputs) > 0 { + assert.Equal(t, tt.expectedOutputs, strings.Split(output.String(), "\n")) + } + assert.Equal(t, tt.expectedErrOut, output.Stderr()) + assert.Equal(t, tt.expectedBrowse, output.BrowsedURL) + } + }) + } +} + +func Test_createRun_GHES(t *testing.T) { + tests := []struct { + name string + setup func(*CreateOptions, *testing.T) func() + cmdStubs func(*run.CommandStubber) + promptStubs func(*prompter.PrompterMock) + httpStubs func(*httpmock.Registry, *testing.T) + expectedOutputs []string + expectedOut string + expectedErrOut string + tty bool + customBranchConfig bool + }{ + { + name: "dry-run-nontty-with-all-opts", + tty: false, + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "TITLE" + opts.Body = "BODY" + opts.BaseBranch = "trunk" + opts.HeadBranch = "feature" + opts.Assignees = []string{"monalisa"} + opts.Labels = []string{"bug", "todo"} + opts.Reviewers = []string{"hubot", "monalisa", "OWNER/core", "OWNER/robots"} + opts.Milestone = "big one.oh" + opts.DryRun = true + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "name": "" }, + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryLabelList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "labels": { + "nodes": [ + { "name": "TODO", "id": "TODOID" }, + { "name": "bug", "id": "BUGID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryMilestoneList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [ + { "title": "GA", "id": "GAID" }, + { "title": "Big One.oh", "id": "BIGONEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + expectedOutputs: []string{ + "Would have created a Pull Request with:", + `title: TITLE`, + `draft: false`, + `base: trunk`, + `head: feature`, + `labels: bug, todo`, + `reviewers: hubot, monalisa, OWNER/core, OWNER/robots`, + `assignees: monalisa`, + `milestones: big one.oh`, + `maintainerCanModify: false`, + `body:`, + `BODY`, + ``, + }, + expectedErrOut: "", + }, + { + name: "dry-run-tty-with-all-opts", + tty: true, + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "TITLE" + opts.Body = "BODY" + opts.BaseBranch = "trunk" + opts.HeadBranch = "feature" + opts.Assignees = []string{"monalisa"} + opts.Labels = []string{"bug", "todo"} + opts.Reviewers = []string{"hubot", "monalisa", "OWNER/core", "OWNER/robots"} + opts.Milestone = "big one.oh" + opts.DryRun = true + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "name": "" }, + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryLabelList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "labels": { + "nodes": [ + { "name": "TODO", "id": "TODOID" }, + { "name": "bug", "id": "BUGID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryMilestoneList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [ + { "title": "GA", "id": "GAID" }, + { "title": "Big One.oh", "id": "BIGONEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + expectedOutputs: []string{ + `Would have created a Pull Request with:`, + `Title: TITLE`, + `Draft: false`, + `Base: trunk`, + `Head: feature`, + `Labels: bug, todo`, + `Reviewers: hubot, monalisa, OWNER/core, OWNER/robots`, + `Assignees: monalisa`, + `Milestones: big one.oh`, + `MaintainerCanModify: false`, + `Body:`, + ``, + ` BODY `, + ``, + ``, + }, + expectedErrOut: heredoc.Doc(` + + Dry Running pull request for feature into trunk in OWNER/REPO + + `), + }, + { + name: "fetch org teams non-interactively if reviewer contains any team", + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "my title" + opts.Body = "my body" + opts.Reviewers = []string{"hubot", "monalisa", "org/core", "org/robots"} + opts.HeadBranch = "feature" + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreate\b`), + httpmock.GraphQLMutation(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12", + "id": "NEWPULLID" + } } } }`, + func(input map[string]interface{}) {})) + 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 UserCurrent\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "login": "monalisa" } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQLMutation(` + { "data": { "requestReviews": { + "clientMutationId": "" + } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) + assert.Equal(t, []interface{}{"HUBOTID", "MONAID"}, inputs["userIds"]) + assert.Equal(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + assert.Equal(t, true, inputs["union"]) + })) + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedErrOut: "", + }, + { + name: "do not fetch org teams non-interactively if reviewer does not contain any team", + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "my title" + opts.Body = "my body" + opts.Reviewers = []string{"hubot", "monalisa"} + opts.HeadBranch = "feature" + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreate\b`), + httpmock.GraphQLMutation(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12", + "id": "NEWPULLID" + } } } }`, + func(input map[string]interface{}) {})) + 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 UserCurrent\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "login": "monalisa" } } } + `)) + reg.Exclude( + t, + httpmock.GraphQL(`query OrganizationTeamList\b`), + ) + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQLMutation(` + { "data": { "requestReviews": { + "clientMutationId": "" + } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) + assert.Equal(t, []interface{}{"HUBOTID", "MONAID"}, inputs["userIds"]) + assert.NotEqual(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + assert.Equal(t, true, inputs["union"]) + })) + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedErrOut: "", + }, + { + name: "fetch org teams interactively if reviewer metadata selected", + tty: true, + setup: func(opts *CreateOptions, t *testing.T) func() { + // In order to test additional metadata, title and body cannot be provided here. + opts.HeadBranch = "feature" + return func() {} + }, cmdStubs: func(cs *run.CommandStubber) { // Stub git commits for `initDefaultTitleBody` when initializing PR state. cs.Register( @@ -1953,11 +2118,10 @@ func Test_createRun(t *testing.T) { } opts := CreateOptions{} - opts.Detector = &fd.EnabledDetectorMock{} + opts.Detector = &fd.DisabledDetectorMock{} opts.Prompter = pm ios, _, stdout, stderr := iostreams.Test() - // TODO do i need to bother with this ios.SetStdoutTTY(tt.tty) ios.SetStdinTTY(tt.tty) ios.SetStderrTTY(tt.tty) @@ -2001,23 +2165,17 @@ func Test_createRun(t *testing.T) { err := createRun(&opts) output := &test.CmdOut{ - OutBuf: stdout, - ErrBuf: stderr, - BrowsedURL: browser.BrowsedURL(), + OutBuf: stdout, + ErrBuf: stderr, } - if tt.wantErr != "" { - assert.EqualError(t, err, tt.wantErr) - } else { - assert.NoError(t, err) - if tt.expectedOut != "" { - assert.Equal(t, tt.expectedOut, output.String()) - } - if len(tt.expectedOutputs) > 0 { - assert.Equal(t, tt.expectedOutputs, strings.Split(output.String(), "\n")) - } - assert.Equal(t, tt.expectedErrOut, output.Stderr()) - assert.Equal(t, tt.expectedBrowse, output.BrowsedURL) + assert.NoError(t, err) + if tt.expectedOut != "" { + assert.Equal(t, tt.expectedOut, output.String()) + } + if len(tt.expectedOutputs) > 0 { + assert.Equal(t, tt.expectedOutputs, strings.Split(output.String(), "\n")) } + assert.Equal(t, tt.expectedErrOut, output.Stderr()) }) } } diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 31b253d9b6d..1b7c42be3b6 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -317,7 +317,7 @@ func EditFieldsSurvey(p EditPrompter, editable *Editable, editorCommand string) editable.Reviewers.Value, err = p.MultiSelectWithSearch( "Reviewers", "Search reviewers", - editable.Reviewers.Default, + editable.Reviewers.DefaultLogins, // No persistent options - teams are included in search results []string{}, editable.ReviewerSearchFunc) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 784b68cf9bc..d5c168e5f7f 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -61,11 +61,14 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par return nil } + // When ActorReviewers is true, we use login-based mutation and don't need to resolve reviewer IDs. + needReviewerIDs := len(tb.Reviewers) > 0 && !tb.ActorReviewers + // Retrieve minimal information needed to resolve metadata if this was not previously cached from additional metadata survey. if tb.MetadataResult == nil { input := api.RepoMetadataInput{ - Reviewers: len(tb.Reviewers) > 0, - TeamReviewers: len(tb.Reviewers) > 0 && slices.ContainsFunc(tb.Reviewers, func(r string) bool { + Reviewers: needReviewerIDs, + TeamReviewers: needReviewerIDs && slices.ContainsFunc(tb.Reviewers, func(r string) bool { return strings.ContainsRune(r, '/') }), Assignees: len(tb.Assignees) > 0, @@ -115,26 +118,41 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par } var userReviewers []string + var botReviewers []string var teamReviewers []string for _, r := range tb.Reviewers { if strings.ContainsRune(r, '/') { teamReviewers = append(teamReviewers, r) + } else if r == api.CopilotReviewerLogin { + botReviewers = append(botReviewers, r) } else { userReviewers = append(userReviewers, r) } } - userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) - } - params["userReviewerIds"] = userReviewerIDs + // TODO requestReviewsByLoginCleanup + // When ActorReviewers is true (github.com), pass logins directly for use with + // RequestReviewsByLogin mutation. The ID-based else branch can be removed once + // GHES supports requestReviewsByLogin. + if tb.ActorReviewers { + params["userReviewerLogins"] = userReviewers + if len(botReviewers) > 0 { + params["botReviewerLogins"] = botReviewers + } + params["teamReviewerSlugs"] = teamReviewers + } else { + userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["userReviewerIds"] = userReviewerIDs - teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) + teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["teamReviewerIds"] = teamReviewerIDs } - params["teamReviewerIds"] = teamReviewerIDs return nil } diff --git a/pkg/cmd/pr/shared/state.go b/pkg/cmd/pr/shared/state.go index b9f2c029372..0e5c31cdd0a 100644 --- a/pkg/cmd/pr/shared/state.go +++ b/pkg/cmd/pr/shared/state.go @@ -20,6 +20,7 @@ type IssueMetadataState struct { Draft bool ActorAssignees bool + ActorReviewers bool Body string Title string diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index e350671b947..cc66bbe5c4d 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -154,7 +154,7 @@ type RepoMetadataFetcher interface { RepoMetadataFetch(api.RepoMetadataInput) (*api.RepoMetadataResult, error) } -func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState, projectsV1Support gh.ProjectsV1Support) error { +func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState, projectsV1Support gh.ProjectsV1Support, reviewerSearchFunc func(string) prompter.MultiSelectSearchResult) error { isChosen := func(m string) bool { for _, c := range state.Metadata { if m == c { @@ -180,10 +180,13 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface state.Metadata = append(state.Metadata, extraFieldsOptions[i]) } - // Retrieve and process data for survey prompts based on the extra fields selected + // Retrieve and process data for survey prompts based on the extra fields selected. + // When search-based reviewer selection is available, skip the expensive assignable-users + // and teams fetch since reviewers are found dynamically via the search function. + useReviewerSearch := state.ActorReviewers && reviewerSearchFunc != nil metadataInput := api.RepoMetadataInput{ - Reviewers: isChosen("Reviewers"), - TeamReviewers: isChosen("Reviewers"), + Reviewers: isChosen("Reviewers") && !useReviewerSearch, + TeamReviewers: isChosen("Reviewers") && !useReviewerSearch, Assignees: isChosen("Assignees"), ActorAssignees: isChosen("Assignees") && state.ActorAssignees, Labels: isChosen("Labels"), @@ -197,13 +200,15 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface } var reviewers []string - for _, u := range metadataResult.AssignableUsers { - if u.Login() != metadataResult.CurrentLogin { - reviewers = append(reviewers, u.DisplayName()) + if !useReviewerSearch { + for _, u := range metadataResult.AssignableUsers { + if u.Login() != metadataResult.CurrentLogin { + reviewers = append(reviewers, u.DisplayName()) + } + } + for _, t := range metadataResult.Teams { + reviewers = append(reviewers, fmt.Sprintf("%s/%s", baseRepo.RepoOwner(), t.Slug)) } - } - for _, t := range metadataResult.Teams { - reviewers = append(reviewers, fmt.Sprintf("%s/%s", baseRepo.RepoOwner(), t.Slug)) } // Populate the list of selectable assignees and their default selections. @@ -254,7 +259,21 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface }{} if isChosen("Reviewers") { - if len(reviewers) > 0 { + if reviewerSearchFunc != nil { + selectedReviewers, err := p.MultiSelectWithSearch( + "Reviewers", + "Search reviewers", + state.Reviewers, + []string{}, + reviewerSearchFunc) + if err != nil { + return err + } + values.Reviewers = selectedReviewers + } else if len(reviewers) > 0 { + // TODO requestReviewsByLoginCleanup + // The static MultiSelect path can be removed once GHES supports + // requestReviewsByLogin and search-based selection is always used. selected, err := p.MultiSelect("Reviewers", state.Reviewers, reviewers) if err != nil { return err diff --git a/pkg/cmd/pr/shared/survey_test.go b/pkg/cmd/pr/shared/survey_test.go index 7097d0761d4..23ba96ceff7 100644 --- a/pkg/cmd/pr/shared/survey_test.go +++ b/pkg/cmd/pr/shared/survey_test.go @@ -71,7 +71,7 @@ func TestMetadataSurvey_selectAll(t *testing.T) { Assignees: []string{"hubot"}, Type: PRMetadata, } - err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported) + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -117,7 +117,7 @@ func TestMetadataSurvey_keepExisting(t *testing.T) { Assignees: []string{"hubot"}, } - err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported) + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -146,7 +146,7 @@ func TestMetadataSurveyProjectV1Deprecation(t *testing.T) { return []int{0}, nil }) - err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Supported) + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Supported, nil) require.ErrorContains(t, err, "expected test error") require.True(t, fetcher.projectsV1Requested, "expected projectsV1 to be requested") @@ -167,7 +167,7 @@ func TestMetadataSurveyProjectV1Deprecation(t *testing.T) { return []int{0}, nil }) - err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Unsupported) + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Unsupported, nil) require.ErrorContains(t, err, "expected test error") require.False(t, fetcher.projectsV1Requested, "expected projectsV1 not to be requested")