From 0efdfed0680c5afc8daf32da5c44928139684ca5 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 9 May 2025 22:56:09 -0600 Subject: [PATCH] feat(issue edit): support assigning actors to issues --- pkg/cmd/issue/edit/edit_test.go | 187 +++++++++++++++++------------ pkg/cmd/pr/shared/editable_http.go | 46 +++++++ 2 files changed, 154 insertions(+), 79 deletions(-) diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 01cd74900..d27bbdee6 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -394,6 +394,7 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockRepoMetadata(t, reg) mockIssueUpdate(t, reg) + mockIssueUpdateActorAssignees(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) }, @@ -441,6 +442,8 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockIssueUpdate(t, reg) mockIssueUpdate(t, reg) + mockIssueUpdateActorAssignees(t, reg) + mockIssueUpdateActorAssignees(t, reg) mockIssueUpdateLabels(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) @@ -546,6 +549,14 @@ func Test_editRun(t *testing.T) { mockIssueNumberGet(t, reg, 123) mockIssueNumberGet(t, reg, 456) // Updating 123 should succeed. + reg.Register( + httpmock.GraphQLMutationMatcher(`mutation ReplaceActorsForAssignable\b`, func(m map[string]interface{}) bool { + return m["assignableId"] == "123" + }), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, + func(inputs map[string]interface{}) {}), + ) reg.Register( httpmock.GraphQLMutationMatcher(`mutation IssueUpdate\b`, func(m map[string]interface{}) bool { return m["id"] == "123" @@ -555,6 +566,14 @@ func Test_editRun(t *testing.T) { func(inputs map[string]interface{}) {}), ) // Updating 456 should fail. + reg.Register( + httpmock.GraphQLMutationMatcher(`mutation ReplaceActorsForAssignable\b`, func(m map[string]interface{}) bool { + return m["assignableId"] == "456" + }), + httpmock.GraphQLMutation(` + { "errors": [ { "message": "test error" } ] }`, + func(inputs map[string]interface{}) {}), + ) reg.Register( httpmock.GraphQLMutationMatcher(`mutation IssueUpdate\b`, func(m map[string]interface{}) bool { return m["id"] == "456" @@ -603,6 +622,7 @@ func Test_editRun(t *testing.T) { mockIssueProjectItemsGet(t, reg) mockRepoMetadata(t, reg) mockIssueUpdate(t, reg) + mockIssueUpdateActorAssignees(t, reg) mockIssueUpdateLabels(t, reg) mockProjectV2ItemUpdate(t, reg) }, @@ -792,6 +812,15 @@ func mockIssueUpdate(t *testing.T, reg *httpmock.Registry) { ) } +func mockIssueUpdateActorAssignees(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, + func(inputs map[string]interface{}) {}), + ) +} + func mockIssueUpdateLabels(t *testing.T, reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation LabelAdd\b`), @@ -816,6 +845,85 @@ func mockProjectV2ItemUpdate(t *testing.T, reg *httpmock.Registry) { ) } +func TestActorIsAssignable(t *testing.T) { + t.Run("when actors are assignable, query includes assignedActors", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`assignedActors`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we don't care. + _ = editRun(&EditOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Detector: &fd.EnabledDetectorMock{}, + IssueNumbers: []int{123}, + Editable: prShared.Editable{ + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "octocat"}, + Edited: true, + }, + }, + }, + }) + + reg.Verify(t) + }) + + t.Run("when actors are not assignable, query includes assignees instead", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + // This test should NOT include assignedActors in the query + reg.Exclude(t, httpmock.GraphQL(`assignedActors`)) + // It should include the regular assignees field + reg.Register( + httpmock.GraphQL(`assignees`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we're not really interested in it. + _ = editRun(&EditOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Detector: &fd.DisabledDetectorMock{}, + IssueNumbers: []int{123}, + Editable: prShared.Editable{ + Assignees: prShared.EditableAssignees{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"monalisa", "octocat"}, + Edited: true, + }, + }, + }, + }) + + reg.Verify(t) + }) +} + // TODO projectsV1Deprecation // Remove this test. func TestProjectsV1Deprecation(t *testing.T) { @@ -900,82 +1008,3 @@ func TestProjectsV1Deprecation(t *testing.T) { reg.Verify(t) }) } - -func TestActorIsAssignable(t *testing.T) { - t.Run("when actors are assignable, query includes assignedActors", func(t *testing.T) { - ios, _, _, _ := iostreams.Test() - - reg := &httpmock.Registry{} - reg.Register( - httpmock.GraphQL(`assignedActors`), - // Simulate a GraphQL error to early exit the test. - httpmock.StatusStringResponse(500, ""), - ) - - _, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - // Ignore the error because we don't care. - _ = editRun(&EditOptions{ - IO: ios, - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - }, - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil - }, - Detector: &fd.EnabledDetectorMock{}, - IssueNumbers: []int{123}, - Editable: prShared.Editable{ - Assignees: prShared.EditableAssignees{ - EditableSlice: prShared.EditableSlice{ - Add: []string{"monalisa", "octocat"}, - Edited: true, - }, - }, - }, - }) - - reg.Verify(t) - }) - - t.Run("when actors are not assignable, query includes assignees instead", func(t *testing.T) { - ios, _, _, _ := iostreams.Test() - - reg := &httpmock.Registry{} - // This test should NOT include assignedActors in the query - reg.Exclude(t, httpmock.GraphQL(`assignedActors`)) - // It should include the regular assignees field - reg.Register( - httpmock.GraphQL(`assignees`), - // Simulate a GraphQL error to early exit the test. - httpmock.StatusStringResponse(500, ""), - ) - - _, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - // Ignore the error because we're not really interested in it. - _ = editRun(&EditOptions{ - IO: ios, - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - }, - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil - }, - Detector: &fd.DisabledDetectorMock{}, - IssueNumbers: []int{123}, - Editable: prShared.Editable{ - Assignees: prShared.EditableAssignees{ - EditableSlice: prShared.EditableSlice{ - Add: []string{"monalisa", "octocat"}, - Edited: true, - }, - }, - }, - }) - - reg.Verify(t) - }) -} diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index fcc30095a..2c09f2e69 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -58,6 +58,26 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR }) } + // updateIssue mutation does not support Actors so assignment needs to + // be in a separate request when our assignees are Actors. + if options.Assignees.Edited && options.Assignees.ActorAssignees { + apiClient := api.NewClientFromHTTP(httpClient) + assigneeIds, err := options.AssigneeIds(apiClient, repo) + if err != nil { + return err + } + + // Disable the edited flag for assignees so that it doesn't + // trigger a second update in the replaceIssueFields function. + // It's important that this is done AFTER the call to + // options.AssigneeIds() above because otherwise that just exits. + options.Assignees.Edited = false + + wg.Go(func() error { + return replaceActorAssigneesForEditable(apiClient, repo, id, assigneeIds) + }) + } + if dirtyExcludingLabels(options) { wg.Go(func() error { return replaceIssueFields(httpClient, repo, id, isPR, options) @@ -67,6 +87,32 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR return wg.Wait() } +func replaceActorAssigneesForEditable(apiClient *api.Client, repo ghrepo.Interface, id string, assigneeIds *[]string) error { + type ReplaceActorsForAssignableInput struct { + AssignableID githubv4.ID `json:"assignableId"` + ActorIDs []githubv4.ID `json:"actorIds"` + } + + params := ReplaceActorsForAssignableInput{ + AssignableID: githubv4.ID(id), + ActorIDs: *ghIds(assigneeIds), + } + + var mutation struct { + ReplaceActorsForAssignable struct { + Typename string `graphql:"__typename"` + } `graphql:"replaceActorsForAssignable(input: $input)"` + } + + variables := map[string]interface{}{"input": params} + err := apiClient.Mutate(repo.RepoHost(), "ReplaceActorsForAssignable", &mutation, variables) + if err != nil { + return err + } + + return nil +} + func replaceIssueFields(httpClient *http.Client, repo ghrepo.Interface, id string, isPR bool, options Editable) error { apiClient := api.NewClientFromHTTP(httpClient) assigneeIds, err := options.AssigneeIds(apiClient, repo)