From d72018d3120dde1fb783afa3f612cd9255e1eb32 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 14 May 2025 11:04:19 -0600 Subject: [PATCH] feat(pr edit): fetch assignable actors --- api/queries_repo.go | 42 +++++++++------------- pkg/cmd/pr/edit/edit.go | 2 +- pkg/cmd/pr/edit/edit_test.go | 70 +++++++++++++++++++++++++++++++----- 3 files changed, 78 insertions(+), 36 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 732d89c8c..437261c21 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -918,39 +918,30 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput var g errgroup.Group if input.Assignees || input.Reviewers { - if input.ActorAssignees { g.Go(func() error { actors, err := RepoAssignableActors(client, repo) if err != nil { - err = fmt.Errorf("error fetching assignees: %w", err) + return fmt.Errorf("error fetching assignees: %w", err) } result.AssignableActors = actors - return err - }) - // If reviewers are also requested, we still need to fetch the assignable users - // since commands use assignable users for reviewers too, - // but Actors are not supported for requesting review (need to confirm this). - // TODO KW: find out how to do this in the above query so we don't need to - // run two potentially expensive queries. When we fetch Actors, this - // should still return Users - Users are distinguishable from other Actors - // by having a name property. Maybe we can use the Name to filter out - // non-user Actors and populate the users list for reviewers based on - // that. - // Note: this only matters for `gh pr` flows, which currently does not - // request actor assignees, so we probably won't hit this until - // `gh pr` reqeuests actor assignees. - if input.Reviewers { - g.Go(func() error { - users, err := RepoAssignableUsers(client, repo) - if err != nil { - err = fmt.Errorf("error fetching assignees: %w", err) + // Processing the bots out from the actors + // because requesting a reviewer leverages + // result.AssignableUsers. + // Note: this prevents us from needing to make another + // request to fetch assignable users when the user has + // selected to modify both reviewers and assignees. + var users []AssignableUser + for _, a := range actors { + if _, ok := a.(AssignableUser); !ok { + continue } - result.AssignableUsers = users - return err - }) - } + users = append(users, a.(AssignableUser)) + } + result.AssignableUsers = users + return nil + }) } else { // Not using Actors, fetch legacy assignable users. g.Go(func() error { @@ -962,7 +953,6 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput return err }) } - } if input.Reviewers { diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 23a75dd49..d484f74ed 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -238,7 +238,7 @@ func editRun(opts *EditOptions) error { editable.Base.Default = pr.BaseRefName editable.Reviewers.Default = pr.ReviewRequests.Logins() if issueFeatures.ActorIsAssignable { - // editable.Assignees.ActorAssignees = true + editable.Assignees.ActorAssignees = true editable.Assignees.Default = pr.AssignedActors.Logins() } else { editable.Assignees.Default = pr.Assignees.Logins() diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 64231fa70..5c2e65eba 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/cli/cli/v2/api" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" shared "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -392,6 +393,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { mockRepoMetadata(reg, false) mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestReviewersUpdate(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) @@ -448,6 +450,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { mockRepoMetadata(reg, true) mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -468,6 +471,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { mockRepoMetadata(reg, false) mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestReviewersUpdate(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) @@ -489,11 +493,50 @@ func Test_editRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { mockRepoMetadata(reg, true) mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, + { + name: "Legacy assignee user are fetched and updated on unsupported GitHub Hosts", + input: &EditOptions{ + Detector: &fd.DisabledDetectorMock{}, + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{ + URL: "https://github.com/OWNER/REPO/pull/123", + }, ghrepo.New("OWNER", "REPO")), + Interactive: false, + Editable: shared.Editable{ + Assignees: shared.EditableAssignees{ + EditableSlice: shared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Remove: []string{"octocat"}, + Edited: true, + }, + }, + }, + Fetcher: testFetcher{}, + }, + httpStubs: func(reg *httpmock.Registry) { + // Notice there is no call to mockReplaceActorsForAssignable() + // and no GraphQL call to RepositoryAssignableActors below. + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "MonaLisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + mockPullRequestUpdate(reg) + }, + stdout: "https://github.com/OWNER/REPO/pull/123\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -523,16 +566,16 @@ func Test_editRun(t *testing.T) { func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.GraphQL(`query RepositoryAssignableActors\b`), httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) + { "data": { "repository": { "suggestedActors": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, + { "login": "MonaLisa", "id": "MONAID", "__typename": "User" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) reg.Register( httpmock.GraphQL(`query RepositoryLabelList\b`), httpmock.StringResponse(` @@ -635,6 +678,15 @@ func mockPullRequestUpdate(reg *httpmock.Registry) { httpmock.StringResponse(`{}`)) } +func mockPullRequestUpdateActorAssignees(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, + func(inputs map[string]interface{}) {}), + ) +} + func mockPullRequestReviewersUpdate(reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation PullRequestUpdateRequestReviews\b`),