diff --git a/api/queries_pr.go b/api/queries_pr.go index b3373a903..0d0fed03d 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -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 { diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index b196546cd..de65f1c72 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -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 { diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index bb468a307..803f506bc 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -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(`{}`)) } diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 9adbeb47c..7d0805a80 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -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,