From 0db7ae872f71ec7926dc96c34430a60aa75fc5bf Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 09:09:36 -0600 Subject: [PATCH 1/9] refactor(api): remove needless parenthesis Co-authored-by: Andy Feller --- api/queries_repo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index e6286bb0b..732d89c8c 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -696,7 +696,7 @@ func (m *RepoMetadataResult) MembersToIDs(names []string) ([]string, error) { for _, assigneeLogin := range names { found := false for _, u := range m.AssignableUsers { - if strings.EqualFold(assigneeLogin, (u.Login())) { + if strings.EqualFold(assigneeLogin, u.Login()) { ids = append(ids, u.ID()) found = true break From 6162a7c524d4507412e312e275ebff2d0a59da33 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 09:13:46 -0600 Subject: [PATCH 2/9] fix(api): correct var name capitalization --- pkg/cmd/pr/shared/editable_http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index cd183e565..8cd51c349 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -103,7 +103,7 @@ func replaceActorAssigneesForEditable(apiClient *api.Client, repo ghrepo.Interfa var mutation struct { ReplaceActorsForAssignable struct { - Typename string `graphql:"__typename"` + TypeName string `graphql:"__typename"` } `graphql:"replaceActorsForAssignable(input: $input)"` } From f55906810645e4a22eeb227d4b189ae9a9a0acaa Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 13 May 2025 13:34:58 -0600 Subject: [PATCH 3/9] feat(pr edit): fetch assigned actors --- api/queries_pr.go | 1 + pkg/cmd/pr/edit/edit.go | 41 ++++++++++++++++++++++++++++++++++-- pkg/cmd/pr/edit/edit_test.go | 2 ++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 5b941bb42..525418a11 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -84,6 +84,7 @@ type PullRequest struct { } Assignees Assignees + AssignedActors AssignedActors Labels Labels ProjectCards ProjectCards ProjectItems ProjectItems diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 3c8d73ad3..23a75dd49 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -3,9 +3,11 @@ package edit import ( "fmt" "net/http" + "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" shared "github.com/cli/cli/v2/pkg/cmd/pr/shared" @@ -25,6 +27,8 @@ type EditOptions struct { Fetcher EditableOptionsFetcher EditorRetriever EditorRetriever Prompter shared.EditPrompter + Detector fd.Detector + BaseRepo func() (ghrepo.Interface, error) SelectorArg string Interactive bool @@ -69,6 +73,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Finder = shared.NewFinder(f) + opts.BaseRepo = f.BaseRepo if len(args) > 0 { opts.SelectorArg = args[0] @@ -192,8 +197,35 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman func editRun(opts *EditOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, - Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "assignees", "labels", "projectCards", "projectItems", "milestone"}, + Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"}, } + + if opts.Detector == nil { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost()) + } + + issueFeatures, err := opts.Detector.IssueFeatures() + if err != nil { + return err + } + + if issueFeatures.ActorIsAssignable { + findOptions.Fields = append(findOptions.Fields, "assignedActors") + } else { + findOptions.Fields = append(findOptions.Fields, "assignees") + } + pr, repo, err := opts.Finder.Find(findOptions) if err != nil { return err @@ -205,7 +237,12 @@ func editRun(opts *EditOptions) error { editable.Body.Default = pr.Body editable.Base.Default = pr.BaseRefName editable.Reviewers.Default = pr.ReviewRequests.Logins() - editable.Assignees.Default = pr.Assignees.Logins() + if issueFeatures.ActorIsAssignable { + // editable.Assignees.ActorAssignees = true + editable.Assignees.Default = pr.AssignedActors.Logins() + } else { + editable.Assignees.Default = pr.Assignees.Logins() + } editable.Labels.Default = pr.Labels.Names() editable.Projects.Default = append(pr.ProjectCards.ProjectNames(), pr.ProjectItems.ProjectTitles()...) projectItems := map[string]string{} diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index f45984c76..64231fa70 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -507,9 +507,11 @@ func Test_editRun(t *testing.T) { tt.httpStubs(reg) httpClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } + baseRepo := func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } tt.input.IO = ios tt.input.HttpClient = httpClient + tt.input.BaseRepo = baseRepo err := editRun(tt.input) assert.NoError(t, err) 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 4/9] 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`), From 54f48cfe46e231a7f1d45bdab0706ed999690f60 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 09:47:30 -0600 Subject: [PATCH 5/9] fix(pr edit): remove merge conflict artifact, extra detector --- pkg/cmd/pr/edit/edit.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 75b8c4e09..8d2ff8ddb 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -21,9 +21,6 @@ import ( type EditOptions struct { HttpClient func() (*http.Client, error) IO *iostreams.IOStreams - // TODO projectsV1Deprecation - // Remove this detector since it is only used for test validation. - Detector fd.Detector Finder shared.PRFinder Surveyor Surveyor From f6bb1ca75653975d42a7d43c7c7ee24784a79d8a Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 13:40:23 -0600 Subject: [PATCH 6/9] refactor(pr edit): move httpclient initialization --- pkg/cmd/pr/edit/edit.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 8d2ff8ddb..ff293a511 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -201,12 +201,12 @@ func editRun(opts *EditOptions) error { Detector: opts.Detector, } - if opts.Detector == nil { - httpClient, err := opts.HttpClient() - if err != nil { - return err - } + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + if opts.Detector == nil { baseRepo, err := opts.BaseRepo() if err != nil { return err @@ -262,10 +262,6 @@ func editRun(opts *EditOptions) error { } } - httpClient, err := opts.HttpClient() - if err != nil { - return err - } apiClient := api.NewClientFromHTTP(httpClient) opts.IO.StartProgressIndicator() From ec7c8ed844ec1532b6026af5d0b57bb0d238ed58 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 13:41:38 -0600 Subject: [PATCH 7/9] test(pr edit): fix typo in test name Co-authored-by: Andy Feller --- pkg/cmd/pr/edit/edit_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 0f15d6bd1..76fb40f88 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -500,7 +500,7 @@ func Test_editRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/pull/123\n", }, { - name: "Legacy assignee user are fetched and updated on unsupported GitHub Hosts", + name: "Legacy assignee users are fetched and updated on unsupported GitHub Hosts", input: &EditOptions{ Detector: &fd.DisabledDetectorMock{}, SelectorArg: "123", From 981da867018817f7ea08dae2259e022f2f231fe0 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 13:47:51 -0600 Subject: [PATCH 8/9] doc(pr edit): condense comment for reviewer/user filtering --- api/queries_repo.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 437261c21..1bd814746 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -926,12 +926,8 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput } result.AssignableActors = actors - // 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. + // Filter actors for users to use for pull request reviewers, + // skip retrieving the same info through RepoAssignableUsers(). var users []AssignableUser for _, a := range actors { if _, ok := a.(AssignableUser); !ok { From 8bd77c0a956680b16cfc0610e61f803bbe05b7ca Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 14:01:10 -0600 Subject: [PATCH 9/9] fix(pr edit): clarify error messages for assignee actors and users --- api/queries_repo.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 1bd814746..fd5ae4679 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -922,7 +922,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput g.Go(func() error { actors, err := RepoAssignableActors(client, repo) if err != nil { - return fmt.Errorf("error fetching assignees: %w", err) + return fmt.Errorf("error fetching assignable actors: %w", err) } result.AssignableActors = actors @@ -943,7 +943,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput g.Go(func() error { users, err := RepoAssignableUsers(client, repo) if err != nil { - err = fmt.Errorf("error fetching assignees: %w", err) + err = fmt.Errorf("error fetching assignable users: %w", err) } result.AssignableUsers = users return err