From cde860be883b444e9151b3c560eea5c28420d871 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 3 Jun 2025 13:33:52 +0100 Subject: [PATCH] Iterate on `pr edit` problems with existing assignees This commit is a handful of changes around `gh pr edit --add-assignee` behavior. Most notably, fixing a bug where the assigned actors weren't being dropped. In addition to this, I was refactoring the testing setup to allow for individual test table scenarios could be contained. --- pkg/cmd/issue/edit/edit_test.go | 17 ++- pkg/cmd/pr/edit/edit.go | 1 + pkg/cmd/pr/edit/edit_test.go | 177 ++++++++++++++++++++++++-------- 3 files changed, 144 insertions(+), 51 deletions(-) diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 4840cbf7a..7d2d36662 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -631,10 +631,11 @@ func Test_editRun(t *testing.T) { }, EditFieldsSurvey: func(p prShared.EditPrompter, eo *prShared.Editable, _ string) error { // Checking that the display name is being used in the prompt. - require.Equal(t, eo.Assignees.Default, []string{"hubot", "MonaLisa (Mona Display Name)"}) + require.Equal(t, eo.Assignees.Default, []string{"hubot"}) + require.Equal(t, eo.Assignees.DefaultLogins, []string{"hubot"}) - // Mocking a selection of only MonaLisa in the prompt. - eo.Assignees.Value = []string{"MonaLisa (Mona Display Name)"} + // Adding MonaLisa as PR assignee, should preserve hubot. + eo.Assignees.Value = []string{"hubot", "MonaLisa (Mona Display Name)"} return nil }, FetchOptions: prShared.FetchOptions, @@ -662,7 +663,7 @@ func Test_editRun(t *testing.T) { // Checking that despite the display name being returned // from the EditFieldsSurvey, the ID is still // used in the mutation. - require.Contains(t, inputs["actorIds"], "MONAID") + require.Subset(t, inputs["actorIds"], []string{"MONAID", "HUBOTID"}) }), ) }, @@ -809,15 +810,9 @@ func mockIsssueNumberGetWithAssignedActors(_ *testing.T, reg *httpmock.Registry, "id": "HUBOTID", "login": "hubot", "__typename": "Bot" - }, - { - "id": "MONAID", - "login": "MonaLisa", - "name": "Mona Display Name", - "__typename": "User" } ], - "totalCount": 2 + "totalCount": 1 } } } } }`, number)), ) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 5e1f19b18..becbfce47 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -228,6 +228,7 @@ func editRun(opts *EditOptions) error { if pr.AssignedActorsUsed { editable.Assignees.ActorAssignees = true editable.Assignees.Default = pr.AssignedActors.DisplayNames() + editable.Assignees.DefaultLogins = 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 0c40e3839..0b731cd00 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -532,8 +532,31 @@ func Test_editRun(t *testing.T) { URL: "https://github.com/OWNER/REPO/pull/123", AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), - Interactive: true, - Surveyor: testSurveyor{}, + Interactive: true, + Surveyor: testSurveyor{ + fieldsToEdit: func(e *shared.Editable) error { + e.Title.Edited = true + e.Body.Edited = true + e.Reviewers.Edited = true + e.Assignees.Edited = true + e.Labels.Edited = true + e.Projects.Edited = true + e.Milestone.Edited = true + return nil + }, + editFields: func(e *shared.Editable, _ string) error { + e.Title.Value = "new title" + e.Body.Value = "new body" + e.Reviewers.Value = []string{"monalisa", "hubot", "OWNER/core", "OWNER/external"} + e.Assignees.Value = []string{"monalisa", "hubot"} + e.Labels.Value = []string{"feature", "TODO", "bug"} + e.Labels.Add = []string{"feature", "TODO", "bug"} + e.Labels.Remove = []string{"docs"} + e.Projects.Value = []string{"Cleanup", "CleanupV2"} + e.Milestone.Value = "GA" + return nil + }, + }, Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, @@ -556,8 +579,29 @@ func Test_editRun(t *testing.T) { URL: "https://github.com/OWNER/REPO/pull/123", AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), - Interactive: true, - Surveyor: testSurveyor{skipReviewers: true}, + Interactive: true, + Surveyor: testSurveyor{ + fieldsToEdit: func(e *shared.Editable) error { + e.Title.Edited = true + e.Body.Edited = true + e.Assignees.Edited = true + e.Labels.Edited = true + e.Projects.Edited = true + e.Milestone.Edited = true + return nil + }, + editFields: func(e *shared.Editable, _ string) error { + e.Title.Value = "new title" + e.Body.Value = "new body" + e.Assignees.Value = []string{"monalisa", "hubot"} + e.Labels.Value = []string{"feature", "TODO", "bug"} + e.Labels.Add = []string{"feature", "TODO", "bug"} + e.Labels.Remove = []string{"docs"} + e.Projects.Value = []string{"Cleanup", "CleanupV2"} + e.Milestone.Value = "GA" + return nil + }, + }, Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, @@ -579,8 +623,30 @@ func Test_editRun(t *testing.T) { URL: "https://github.com/OWNER/REPO/pull/123", AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), - Interactive: true, - Surveyor: testSurveyor{removeAllReviewers: true}, + Interactive: true, + Surveyor: testSurveyor{ + fieldsToEdit: func(e *shared.Editable) error { + e.Title.Edited = true + e.Body.Edited = true + e.Assignees.Edited = true + e.Labels.Edited = true + e.Projects.Edited = true + e.Milestone.Edited = true + return nil + }, + editFields: func(e *shared.Editable, _ string) error { + e.Title.Value = "new title" + e.Body.Value = "new body" + e.Reviewers.Remove = []string{"monalisa", "hubot", "OWNER/core", "OWNER/external", "dependabot"} + e.Assignees.Value = []string{"monalisa", "hubot"} + e.Labels.Value = []string{"feature", "TODO", "bug"} + e.Labels.Add = []string{"feature", "TODO", "bug"} + e.Labels.Remove = []string{"docs"} + e.Projects.Value = []string{"Cleanup", "CleanupV2"} + e.Milestone.Value = "GA" + return nil + }, + }, Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, @@ -659,6 +725,59 @@ func Test_editRun(t *testing.T) { } } +func Test_editRun_actors(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(true) + ios.SetStdinTTY(true) + ios.SetStderrTTY(true) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableActors\b`), + httpmock.StringResponse(` + { "data": { "repository": { "suggestedActors": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + mockPullRequestUpdate(reg) + mockPullRequestUpdateActorAssignees(reg) + + httpClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } + baseRepo := func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } + + 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{ + Assignees: shared.EditableAssignees{ + EditableSlice: shared.EditableSlice{ + Add: []string{"monalisa", "hubot"}, + Remove: []string{"octocat"}, + Edited: true, + }, + }, + }, + Fetcher: testFetcher{}, + } + input.IO = ios + input.HttpClient = httpClient + input.BaseRepo = baseRepo + + err := editRun(input) + assert.NoError(t, err) + assert.Equal(t, "https://github.com/OWNER/REPO/pull/123\n", stdout.String()) + assert.Equal(t, "", stderr.String()) +} + func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { reg.Register( httpmock.GraphQL(`query RepositoryAssignableActors\b`), @@ -666,7 +785,7 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { { "data": { "repository": { "suggestedActors": { "nodes": [ { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, - { "login": "MonaLisa", "id": "MONAID", "__typename": "User" } + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } ], "pageInfo": { "hasNextPage": false } } } } } @@ -813,48 +932,26 @@ func mockProjectV2ItemUpdate(reg *httpmock.Registry) { } type testFetcher struct{} -type testSurveyor struct { - skipReviewers bool - removeAllReviewers bool -} -type testEditorRetriever struct{} func (f testFetcher) EditableOptionsFetch(client *api.Client, repo ghrepo.Interface, opts *shared.Editable) error { return shared.FetchOptions(client, repo, opts) } -func (s testSurveyor) FieldsToEdit(e *shared.Editable) error { - e.Title.Edited = true - e.Body.Edited = true - if !s.skipReviewers { - e.Reviewers.Edited = true - } - e.Assignees.Edited = true - e.Labels.Edited = true - e.Projects.Edited = true - e.Milestone.Edited = true - return nil +type testSurveyor struct { + fieldsToEdit func(e *shared.Editable) error + editFields func(e *shared.Editable, editorCmd string) error } -func (s testSurveyor) EditFields(e *shared.Editable, _ string) error { - e.Title.Value = "new title" - e.Body.Value = "new body" - if !s.skipReviewers { - if s.removeAllReviewers { - e.Reviewers.Remove = []string{"monalisa", "hubot", "OWNER/core", "OWNER/external", "dependabot"} - } else { - e.Reviewers.Value = []string{"monalisa", "hubot", "OWNER/core", "OWNER/external"} - } - } - e.Assignees.Value = []string{"monalisa", "hubot"} - e.Labels.Value = []string{"feature", "TODO", "bug"} - e.Labels.Add = []string{"feature", "TODO", "bug"} - e.Labels.Remove = []string{"docs"} - e.Projects.Value = []string{"Cleanup", "CleanupV2"} - e.Milestone.Value = "GA" - return nil +func (s testSurveyor) FieldsToEdit(e *shared.Editable) error { + return s.fieldsToEdit(e) } +func (s testSurveyor) EditFields(e *shared.Editable, editorCmd string) error { + return s.editFields(e, editorCmd) +} + +type testEditorRetriever struct{} + func (t testEditorRetriever) Retrieve() (string, error) { return "vim", nil }