STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Refactor PR reviewer editing to use REST API and optimize team fetch

Browse source 
Switches pull request reviewer add/remove operations from GraphQL to the REST API, enabling separate add and remove calls for reviewers and teams. Refactors reviewer editing logic to avoid fetching organization teams unless required for interactive editing, improving performance for non-interactive flows. Updates tests and supporting code to reflect the new reviewer management and metadata fetching behavior.
This commit is contained in:
Kynan Ware 2025-09-29 13:50:11 -06:00
parent b76bc7706a
commit 87468f40db
4 changed files with 325 additions and 123 deletions
Download patch file Download diff file Expand all files Collapse all files

60
api/queries_pr.go
View file

@ -1,6 +1,8 @@
package api
import (
"bytes"
"encoding/json"
"fmt"
"net/http"
"net/url"
@ -629,17 +631,55 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter
return pr, nil
}
func UpdatePullRequestReviews(client *Client, repo ghrepo.Interface, params githubv4.RequestReviewsInput) error {
var mutation struct {
RequestReviews struct {
PullRequest struct {
ID string
}
} `graphql:"requestReviews(input: $input)"`
// AddPullRequestReviews updates the requested reviewers on a pull request using the REST API.
func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error {
if len(users) == 0 && len(teams) == 0 {
return nil
}
variables := map[string]interface{}{"input": params}
err := client.Mutate(repo.RepoHost(), "PullRequestUpdateRequestReviews", &mutation, variables)
return err
path := fmt.Sprintf("repos/%s/%s/pulls/%d/requested_reviewers", repo.RepoOwner(), repo.RepoName(), prNumber)
body := struct {
Reviewers []string `json:"reviewers,omitempty"`
TeamReviewers []string `json:"team_reviewers,omitempty"`
}{
Reviewers: users,
TeamReviewers: 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.
func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error {
if len(users) == 0 && len(teams) == 0 {
return nil
}
if users == nil {
users = []string{}
}
if teams == nil {
teams = []string{}
}
path := fmt.Sprintf("repos/%s/%s/pulls/%d/requested_reviewers", repo.RepoOwner(), repo.RepoName(), prNumber)
body := struct {
Reviewers []string `json:"reviewers"`
TeamReviewers []string `json:"team_reviewers"`
}{
Reviewers: users,
TeamReviewers: 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)
}
func UpdatePullRequestBranch(client *Client, repo ghrepo.Interface, params githubv4.UpdatePullRequestBranchInput) error {

84
pkg/cmd/pr/edit/edit.go
View file

@ -3,6 +3,8 @@ package edit
import (
"fmt"
"net/http"
"slices"
"strings"
"time"
"github.com/MakeNowJust/heredoc"
@ -13,6 +15,7 @@ import (
shared "github.com/cli/cli/v2/pkg/cmd/pr/shared"
"github.com/cli/cli/v2/pkg/cmdutil"
"github.com/cli/cli/v2/pkg/iostreams"
"github.com/cli/cli/v2/pkg/set"
"github.com/shurcooL/githubv4"
"github.com/spf13/cobra"
"golang.org/x/sync/errgroup"
@ -237,7 +240,7 @@ func editRun(opts *EditOptions) error {
findOptions := shared.FindOptions{
Selector: opts.SelectorArg,
Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"},
Fields: []string{"id", "author", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"},
Detector: opts.Detector,
}
@ -298,6 +301,18 @@ func editRun(opts *EditOptions) error {
}
if opts.Interactive {
// Remove PR author from reviewer options
// as it is not a valid option for a reviewer.
// The REST API will return an error if we
// attempt to add the PR author as a reviewer.
// However, the GraphQL API will silently ignore it.
if editable.Reviewers.Edited {
s := set.NewStringSet()
s.AddValues(editable.Reviewers.Options)
s.Remove(pr.Author.Login)
editable.Reviewers.Options = s.ToSlice()
}
editorCommand, err := opts.EditorRetriever.Retrieve()
if err != nil {
return err
@ -309,7 +324,7 @@ func editRun(opts *EditOptions) error {
}
opts.IO.StartProgressIndicator()
err = updatePullRequest(httpClient, repo, pr.ID, editable)
err = updatePullRequest(httpClient, repo, pr.ID, pr.Number, editable)
opts.IO.StopProgressIndicator()
if err != nil {
return err
@ -320,36 +335,71 @@ func editRun(opts *EditOptions) error {
return nil
}
func updatePullRequest(httpClient *http.Client, repo ghrepo.Interface, id string, editable shared.Editable) error {
func updatePullRequest(httpClient *http.Client, repo ghrepo.Interface, id string, number int, editable shared.Editable) error {
var wg errgroup.Group
wg.Go(func() error {
return shared.UpdateIssue(httpClient, repo, id, true, editable)
})
if editable.Reviewers.Edited {
wg.Go(func() error {
return updatePullRequestReviews(httpClient, repo, id, editable)
return updatePullRequestReviews(httpClient, repo, number, editable)
})
}
return wg.Wait()
}
func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, id string, editable shared.Editable) error {
userIds, teamIds, err := editable.ReviewerIds()
if err != nil {
return err
}
if userIds == nil && teamIds == nil {
func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, number int, editable shared.Editable) error {
if !editable.Reviewers.Edited {
return nil
}
union := githubv4.Boolean(false)
reviewsRequestParams := githubv4.RequestReviewsInput{
PullRequestID: id,
Union: &union,
UserIDs: ghIds(userIds),
TeamIDs: ghIds(teamIds),
// Rebuild the Value slice from non-interactive flag input.
if len(editable.Reviewers.Add) != 0 || len(editable.Reviewers.Remove) != 0 {
s := set.NewStringSet()
s.AddValues(editable.Reviewers.Add)
s.AddValues(editable.Reviewers.Default)
s.RemoveValues(editable.Reviewers.Remove)
editable.Reviewers.Value = s.ToSlice()
}
var addUsers []string
var addTeams []string
for _, r := range editable.Reviewers.Value {
if strings.ContainsRune(r, '/') {
teamSlug := strings.Split(r, "/")[1]
addTeams = append(addTeams, teamSlug)
} else {
addUsers = append(addUsers, r)
}
}
// Reviewers in Default but not in the Value have been removed interactively.
var toRemove []string
for _, r := range editable.Reviewers.Default {
if !slices.Contains(editable.Reviewers.Value, r) {
toRemove = append(toRemove, r)
}
}
var removeUsers []string
var removeTeams []string
for _, r := range toRemove {
if strings.ContainsRune(r, '/') {
teamSlug := strings.Split(r, "/")[1]
removeTeams = append(removeTeams, teamSlug)
} else {
removeUsers = append(removeUsers, r)
}
}
client := api.NewClientFromHTTP(httpClient)
return api.UpdatePullRequestReviews(client, repo, reviewsRequestParams)
wg := errgroup.Group{}
wg.Go(func() error {
return api.AddPullRequestReviews(client, repo, number, addUsers, addTeams)
})
wg.Go(func() error {
return api.RemovePullRequestReviews(client, repo, number, removeUsers, removeTeams)
})
return wg.Wait()
}
type Surveyor interface {

249
pkg/cmd/pr/edit/edit_test.go
View file

@ -354,7 +354,7 @@ func Test_editRun(t *testing.T) {
tests := []struct {
name string
input *EditOptions
httpStubs func(*httpmock.Registry)
httpStubs func(*httpmock.Registry, *testing.T)
stdout string
stderr string
}{
@ -411,11 +411,11 @@ func Test_editRun(t *testing.T) {
},
Fetcher: testFetcher{},
},
httpStubs: func(reg *httpmock.Registry) {
mockRepoMetadata(reg, false)
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true})
mockPullRequestUpdate(reg)
mockPullRequestUpdateActorAssignees(reg)
mockPullRequestReviewersUpdate(reg)
mockPullRequestAddReviewers(reg)
mockPullRequestUpdateLabels(reg)
mockProjectV2ItemUpdate(reg)
},
@ -469,8 +469,8 @@ func Test_editRun(t *testing.T) {
},
Fetcher: testFetcher{},
},
httpStubs: func(reg *httpmock.Registry) {
mockRepoMetadata(reg, true)
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: true, labels: true, projects: true, milestones: true})
mockPullRequestUpdate(reg)
mockPullRequestUpdateActorAssignees(reg)
mockPullRequestUpdateLabels(reg)
@ -483,8 +483,19 @@ func Test_editRun(t *testing.T) {
input: &EditOptions{
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{
Finder: shared.NewMockFinder("123", &api.PullRequest{ // include existing reviewers so removal logic triggers
URL: "https://github.com/OWNER/REPO/pull/123",
ReviewRequests: api.ReviewRequests{Nodes: []struct{ RequestedReviewer api.RequestedReviewer }{
{RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "core", Organization: struct {
Login string `json:"login"`
}{Login: "OWNER"}}},
{RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "external", Organization: struct {
Login string `json:"login"`
}{Login: "OWNER"}}},
{RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "monalisa"}},
{RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "hubot"}},
{RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "dependabot"}},
}},
}, ghrepo.New("OWNER", "REPO")),
Interactive: false,
Editable: shared.Editable{
@ -501,8 +512,9 @@ func Test_editRun(t *testing.T) {
Edited: true,
},
Reviewers: shared.EditableSlice{
Remove: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot", "dependabot"},
Edited: true,
Default: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot", "dependabot"},
Remove: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot", "dependabot"},
Edited: true,
},
Assignees: shared.EditableAssignees{
EditableSlice: shared.EditableSlice{
@ -530,16 +542,107 @@ func Test_editRun(t *testing.T) {
},
Fetcher: testFetcher{},
},
httpStubs: func(reg *httpmock.Registry) {
mockRepoMetadata(reg, false)
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true})
mockPullRequestUpdate(reg)
mockPullRequestReviewersUpdate(reg)
mockPullRequestRemoveReviewers(reg)
mockPullRequestUpdateLabels(reg)
mockPullRequestUpdateActorAssignees(reg)
mockProjectV2ItemUpdate(reg)
},
stdout: "https://github.com/OWNER/REPO/pull/123\n",
},
// Conditional team fetching cases
{
name: "non-interactive add only user reviewers skips team fetch",
input: &EditOptions{
Detector: &fd.EnabledDetectorMock{},
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{
Reviewers: shared.EditableSlice{Add: []string{"monalisa", "hubot"}, Edited: true},
},
Fetcher: testFetcher{},
},
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
// reviewers only (users), no team reviewers fetched
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true})
// explicitly assert that no OrganizationTeamList query occurs
reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`))
mockPullRequestUpdate(reg)
mockPullRequestAddReviewers(reg)
},
stdout: "https://github.com/OWNER/REPO/pull/123\n",
},
{
name: "non-interactive add contains team reviewers skips team fetch",
input: &EditOptions{
Detector: &fd.EnabledDetectorMock{},
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{
Reviewers: shared.EditableSlice{Add: []string{"monalisa", "OWNER/core"}, Edited: true},
},
Fetcher: testFetcher{},
},
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
// reviewer add includes team but non-interactive Add/Remove provided -> no team fetch
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true})
reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`))
mockPullRequestUpdate(reg)
mockPullRequestAddReviewers(reg)
},
stdout: "https://github.com/OWNER/REPO/pull/123\n",
},
{
name: "non-interactive reviewers remove contains team skips team fetch",
input: &EditOptions{
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123", ReviewRequests: api.ReviewRequests{Nodes: []struct{ RequestedReviewer api.RequestedReviewer }{
{RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "core", Organization: struct {
Login string `json:"login"`
}{Login: "OWNER"}}},
{RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "monalisa"}},
}}}, ghrepo.New("OWNER", "REPO")),
Interactive: false,
Editable: shared.Editable{
Reviewers: shared.EditableSlice{Remove: []string{"monalisa", "OWNER/core"}, Edited: true},
},
Fetcher: testFetcher{},
},
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true})
reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`))
mockPullRequestUpdate(reg)
mockPullRequestRemoveReviewers(reg)
},
stdout: "https://github.com/OWNER/REPO/pull/123\n",
},
{
name: "non-interactive mutate reviewers with no change to existing team reviewers skips team fetch",
input: &EditOptions{
Detector: &fd.EnabledDetectorMock{},
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{
Reviewers: shared.EditableSlice{Add: []string{"monalisa"}, Remove: []string{"hubot"}, Default: []string{"OWNER/core"}, Edited: true},
},
Fetcher: testFetcher{},
},
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
// reviewers only (users), no team reviewers fetched
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true})
// explicitly assert that no OrganizationTeamList query occurs
reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`))
mockPullRequestUpdate(reg)
mockPullRequestAddReviewers(reg)
},
stdout: "https://github.com/OWNER/REPO/pull/123\n",
},
{
name: "interactive",
input: &EditOptions{
@ -576,11 +679,11 @@ func Test_editRun(t *testing.T) {
Fetcher: testFetcher{},
EditorRetriever: testEditorRetriever{},
},
httpStubs: func(reg *httpmock.Registry) {
mockRepoMetadata(reg, false)
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: true, assignees: true, labels: true, projects: true, milestones: true})
mockPullRequestUpdate(reg)
mockPullRequestUpdateActorAssignees(reg)
mockPullRequestReviewersUpdate(reg)
mockPullRequestAddReviewers(reg)
mockPullRequestUpdateLabels(reg)
mockProjectV2ItemUpdate(reg)
},
@ -620,8 +723,9 @@ func Test_editRun(t *testing.T) {
Fetcher: testFetcher{},
EditorRetriever: testEditorRetriever{},
},
httpStubs: func(reg *httpmock.Registry) {
mockRepoMetadata(reg, true)
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
// interactive but reviewers not chosen; need everything except reviewers/teams
mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: true, labels: true, projects: true, milestones: true})
mockPullRequestUpdate(reg)
mockPullRequestUpdateActorAssignees(reg)
mockPullRequestUpdateLabels(reg)
@ -634,8 +738,19 @@ func Test_editRun(t *testing.T) {
input: &EditOptions{
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{
Finder: shared.NewMockFinder("123", &api.PullRequest{ // include existing reviewers
URL: "https://github.com/OWNER/REPO/pull/123",
ReviewRequests: api.ReviewRequests{Nodes: []struct{ RequestedReviewer api.RequestedReviewer }{
{RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "core", Organization: struct {
Login string `json:"login"`
}{Login: "OWNER"}}},
{RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "external", Organization: struct {
Login string `json:"login"`
}{Login: "OWNER"}}},
{RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "monalisa"}},
{RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "hubot"}},
{RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "dependabot"}},
}},
}, ghrepo.New("OWNER", "REPO")),
Interactive: true,
Surveyor: testSurveyor{
@ -665,10 +780,10 @@ func Test_editRun(t *testing.T) {
Fetcher: testFetcher{},
EditorRetriever: testEditorRetriever{},
},
httpStubs: func(reg *httpmock.Registry) {
mockRepoMetadata(reg, false)
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: true, assignees: true, labels: true, projects: true, milestones: true})
mockPullRequestUpdate(reg)
mockPullRequestReviewersUpdate(reg)
mockPullRequestRemoveReviewers(reg)
mockPullRequestUpdateActorAssignees(reg)
mockPullRequestUpdateLabels(reg)
mockProjectV2ItemUpdate(reg)
@ -712,7 +827,7 @@ func Test_editRun(t *testing.T) {
Fetcher: testFetcher{},
EditorRetriever: testEditorRetriever{},
},
httpStubs: func(reg *httpmock.Registry) {
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
reg.Register(
httpmock.GraphQL(`query RepositoryAssignableActors\b`),
httpmock.StringResponse(`
@ -759,7 +874,7 @@ func Test_editRun(t *testing.T) {
},
Fetcher: testFetcher{},
},
httpStubs: func(reg *httpmock.Registry) {
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
// Notice there is no call to mockReplaceActorsForAssignable()
// and no GraphQL call to RepositoryAssignableActors below.
reg.Register(
@ -787,7 +902,7 @@ func Test_editRun(t *testing.T) {
reg := &httpmock.Registry{}
defer reg.Verify(t)
tt.httpStubs(reg)
tt.httpStubs(reg, t)
httpClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }
baseRepo := func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }
@ -804,10 +919,21 @@ func Test_editRun(t *testing.T) {
}
}
func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
reg.Register(
httpmock.GraphQL(`query RepositoryAssignableActors\b`),
httpmock.StringResponse(`
type mockRepoMetadataOptions struct {
reviewers bool
teamReviewers bool // reviewers must also be true for this to have an effect.
assignees bool
labels bool
projects bool // includes both legacy (v1) and v2
milestones bool
}
func mockRepoMetadata(reg *httpmock.Registry, opt mockRepoMetadataOptions) {
// Assignable actors (users/bots) are fetched when reviewers OR assignees edited with ActorAssignees enabled.
if opt.reviewers || opt.assignees {
reg.Register(
httpmock.GraphQL(`query RepositoryAssignableActors\b`),
httpmock.StringResponse(`
{ "data": { "repository": { "suggestedActors": {
"nodes": [
{ "login": "hubot", "id": "HUBOTID", "__typename": "Bot" },
@ -816,9 +942,11 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`query RepositoryLabelList\b`),
httpmock.StringResponse(`
}
if opt.labels {
reg.Register(
httpmock.GraphQL(`query RepositoryLabelList\b`),
httpmock.StringResponse(`
{ "data": { "repository": { "labels": {
"nodes": [
{ "name": "feature", "id": "FEATUREID" },
@ -829,9 +957,11 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`query RepositoryMilestoneList\b`),
httpmock.StringResponse(`
}
if opt.milestones {
reg.Register(
httpmock.GraphQL(`query RepositoryMilestoneList\b`),
httpmock.StringResponse(`
{ "data": { "repository": { "milestones": {
"nodes": [
{ "title": "GA", "id": "GAID" },
@ -840,9 +970,11 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`query RepositoryProjectList\b`),
httpmock.StringResponse(`
}
if opt.projects {
reg.Register(
httpmock.GraphQL(`query RepositoryProjectList\b`),
httpmock.StringResponse(`
{ "data": { "repository": { "projects": {
"nodes": [
{ "name": "Cleanup", "id": "CLEANUPID" },
@ -851,9 +983,9 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`query OrganizationProjectList\b`),
httpmock.StringResponse(`
reg.Register(
httpmock.GraphQL(`query OrganizationProjectList\b`),
httpmock.StringResponse(`
{ "data": { "organization": { "projects": {
"nodes": [
{ "name": "Triage", "id": "TRIAGEID" }
@ -861,9 +993,9 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`query RepositoryProjectV2List\b`),
httpmock.StringResponse(`
reg.Register(
httpmock.GraphQL(`query RepositoryProjectV2List\b`),
httpmock.StringResponse(`
{ "data": { "repository": { "projectsV2": {
"nodes": [
{ "title": "CleanupV2", "id": "CLEANUPV2ID" },
@ -872,9 +1004,9 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`query OrganizationProjectV2List\b`),
httpmock.StringResponse(`
reg.Register(
httpmock.GraphQL(`query OrganizationProjectV2List\b`),
httpmock.StringResponse(`
{ "data": { "organization": { "projectsV2": {
"nodes": [
{ "title": "TriageV2", "id": "TRIAGEV2ID" }
@ -882,9 +1014,9 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`query UserProjectV2List\b`),
httpmock.StringResponse(`
reg.Register(
httpmock.GraphQL(`query UserProjectV2List\b`),
httpmock.StringResponse(`
{ "data": { "viewer": { "projectsV2": {
"nodes": [
{ "title": "MonalisaV2", "id": "MONALISAV2ID" }
@ -892,7 +1024,8 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
if !skipReviewers {
}
if opt.teamReviewers && opt.reviewers { // teams only relevant if reviewers edited
reg.Register(
httpmock.GraphQL(`query OrganizationTeamList\b`),
httpmock.StringResponse(`
@ -904,11 +1037,13 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
}
if opt.reviewers { // Current user fetched only when reviewers requested
reg.Register(
httpmock.GraphQL(`query UserCurrent\b`),
httpmock.StringResponse(`
{ "data": { "viewer": { "login": "monalisa" } } }
`))
{ "data": { "viewer": { "login": "monalisa" } } }
`))
}
}
@ -927,9 +1062,15 @@ func mockPullRequestUpdateActorAssignees(reg *httpmock.Registry) {
)
}
func mockPullRequestReviewersUpdate(reg *httpmock.Registry) {
func mockPullRequestAddReviewers(reg *httpmock.Registry) {
reg.Register(
httpmock.GraphQL(`mutation PullRequestUpdateRequestReviews\b`),
httpmock.REST("POST", "repos/OWNER/REPO/pulls/0/requested_reviewers"),
httpmock.StringResponse(`{}`))
}
func mockPullRequestRemoveReviewers(reg *httpmock.Registry) {
reg.Register(
httpmock.REST("DELETE", "repos/OWNER/REPO/pulls/0/requested_reviewers"),
httpmock.StringResponse(`{}`))
}

55
pkg/cmd/pr/shared/editable.go
View file

@ -2,7 +2,6 @@ package shared
import (
"fmt"
"strings"
"github.com/cli/cli/v2/api"
"github.com/cli/cli/v2/internal/ghrepo"
@ -78,37 +77,6 @@ func (e Editable) BodyValue() *string {
return &e.Body.Value
}
func (e Editable) ReviewerIds() (*[]string, *[]string, error) {
if !e.Reviewers.Edited {
return nil, nil, nil
}
if len(e.Reviewers.Add) != 0 || len(e.Reviewers.Remove) != 0 {
s := set.NewStringSet()
s.AddValues(e.Reviewers.Default)
s.AddValues(e.Reviewers.Add)
s.RemoveValues(e.Reviewers.Remove)
e.Reviewers.Value = s.ToSlice()
}
var userReviewers []string
var teamReviewers []string
for _, r := range e.Reviewers.Value {
if strings.ContainsRune(r, '/') {
teamReviewers = append(teamReviewers, r)
} else {
userReviewers = append(userReviewers, r)
}
}
userIds, err := e.Metadata.MembersToIDs(userReviewers)
if err != nil {
return nil, nil, err
}
teamIds, err := e.Metadata.TeamsToIDs(teamReviewers)
if err != nil {
return nil, nil, err
}
return &userIds, &teamIds, nil
}
func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]string, error) {
if !e.Assignees.Edited {
return nil, nil
@ -428,17 +396,20 @@ func FieldsToEditSurvey(p EditPrompter, editable *Editable) error {
}
func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) error {
// Determine whether to fetch organization teams.
// Interactive reviewer editing (Edited true, but no Add/Remove slices) still needs
// team data for selection UI. For non-interactive flows, we never need to fetch teams.
teamReviewers := false
if editable.Reviewers.Edited {
// This is likely an interactive flow since edited is set but no mutations to
// Add/Remove slices, so we need to load the teams.
if len(editable.Reviewers.Add) == 0 && len(editable.Reviewers.Remove) == 0 {
teamReviewers = true
}
}
input := api.RepoMetadataInput{
Reviewers: editable.Reviewers.Edited,
// TeamReviewers is always true if Reviewers is true because
// this is the existing `pr edit` behavior. This means
// always fetch teams.
// TODO: evaluate whether this can follow the same logic as
// `pr create` to conditionally fetch teams if a reviewer contains
// a slash.
// See https://github.com/cli/cli/blob/449920b40fc8a5015d1578ca10a301aa385a1914/pkg/cmd/pr/shared/params.go#L67-L71
// See https://github.com/cli/cli/issues/11360
TeamReviewers: editable.Reviewers.Edited,
Reviewers: editable.Reviewers.Edited,
TeamReviewers: teamReviewers,
Assignees: editable.Assignees.Edited,
ActorAssignees: editable.Assignees.ActorAssignees,
Labels: editable.Labels.Edited,