From 9bc2c388da010ccd279844b6af3384da27337c43 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 6 May 2025 14:34:05 -0600 Subject: [PATCH 01/20] fix(a11y prompter): input prompt default value is readable --- internal/prompter/accessible_prompter_test.go | 20 +++++++++++++++++++ internal/prompter/prompter.go | 6 +++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/internal/prompter/accessible_prompter_test.go b/internal/prompter/accessible_prompter_test.go index ed4da8de8..8a7c058c7 100644 --- a/internal/prompter/accessible_prompter_test.go +++ b/internal/prompter/accessible_prompter_test.go @@ -140,6 +140,26 @@ func TestAccessiblePrompter(t *testing.T) { assert.Equal(t, dummyDefaultValue, inputValue) }) + t.Run("Input - default value is in prompt and in readable format", func(t *testing.T) { + console := newTestVirtualTerminal(t) + p := newTestAccessiblePrompter(t, console) + dummyDefaultValue := "12345abcdefg" + + go func() { + // Wait for prompt to appear + _, err := console.ExpectString("Enter some characters (default: 12345abcdefg)") + require.NoError(t, err) + + // Enter nothing + _, err = console.SendLine("") + require.NoError(t, err) + }() + + inputValue, err := p.Input("Enter some characters", dummyDefaultValue) + require.NoError(t, err) + assert.Equal(t, dummyDefaultValue, inputValue) + }) + t.Run("Password", func(t *testing.T) { console := newTestVirtualTerminal(t) p := newTestAccessiblePrompter(t, console) diff --git a/internal/prompter/prompter.go b/internal/prompter/prompter.go index 1e4f5592a..8300059ac 100644 --- a/internal/prompter/prompter.go +++ b/internal/prompter/prompter.go @@ -131,7 +131,11 @@ func (p *accessiblePrompter) MultiSelect(prompt string, defaults []string, optio func (p *accessiblePrompter) Input(prompt, defaultValue string) (string, error) { result := defaultValue - prompt = fmt.Sprintf("%s (%s)", prompt, defaultValue) + if defaultValue != "" { + prompt = fmt.Sprintf("%s (default: %s)", prompt, defaultValue) + } else { + prompt = fmt.Sprintf("%s:", prompt) + } form := p.newForm( huh.NewGroup( huh.NewInput(). From 2ee68411a76a62ae703fb5d03fcc8e2995406204 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 6 May 2025 14:49:09 -0600 Subject: [PATCH 02/20] fix(a11y prompter): Select prompt respects defaults --- internal/prompter/accessible_prompter_test.go | 24 +++++++++++++++++++ internal/prompter/prompter.go | 7 +++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/internal/prompter/accessible_prompter_test.go b/internal/prompter/accessible_prompter_test.go index 8a7c058c7..d4dde6358 100644 --- a/internal/prompter/accessible_prompter_test.go +++ b/internal/prompter/accessible_prompter_test.go @@ -5,6 +5,7 @@ package prompter_test import ( "fmt" "io" + "slices" "strings" "testing" "time" @@ -54,6 +55,29 @@ func TestAccessiblePrompter(t *testing.T) { assert.Equal(t, 0, selectValue) }) + t.Run("Select - blank input returns default value", func(t *testing.T) { + console := newTestVirtualTerminal(t) + p := newTestAccessiblePrompter(t, console) + dummyDefaultValue := "12345abcdefg" + options := []string{"1", "2", dummyDefaultValue} + + go func() { + // Wait for prompt to appear + _, err := console.ExpectString("Input a number between 1 and 3:") + require.NoError(t, err) + + // Just press enter to accept the default + _, err = console.SendLine("") + require.NoError(t, err) + }() + + selectValue, err := p.Select("Select a number", dummyDefaultValue, options) + require.NoError(t, err) + + expectedIndex := slices.Index(options, dummyDefaultValue) + assert.Equal(t, expectedIndex, selectValue) + }) + t.Run("MultiSelect", func(t *testing.T) { console := newTestVirtualTerminal(t) p := newTestAccessiblePrompter(t, console) diff --git a/internal/prompter/prompter.go b/internal/prompter/prompter.go index 8300059ac..5c3b1238d 100644 --- a/internal/prompter/prompter.go +++ b/internal/prompter/prompter.go @@ -77,10 +77,15 @@ func (p *accessiblePrompter) newForm(groups ...*huh.Group) *huh.Form { WithOutput(p.stdout) } -func (p *accessiblePrompter) Select(prompt, _ string, options []string) (int, error) { +func (p *accessiblePrompter) Select(prompt, defaultValue string, options []string) (int, error) { var result int formOptions := []huh.Option[int]{} for i, o := range options { + // If this option is the default value, assign its index + // to the result variable. huh will treat it as a default selection. + if defaultValue == o { + result = i + } formOptions = append(formOptions, huh.NewOption(o, i)) } From ab4cfb84d220b316dff439ce513adb69429ed460 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 6 May 2025 14:56:09 -0600 Subject: [PATCH 03/20] refactor(a11yprompter): shared method for prompt defaults --- internal/prompter/accessible_prompter_test.go | 2 +- internal/prompter/prompter.go | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/internal/prompter/accessible_prompter_test.go b/internal/prompter/accessible_prompter_test.go index d4dde6358..8ae449dc9 100644 --- a/internal/prompter/accessible_prompter_test.go +++ b/internal/prompter/accessible_prompter_test.go @@ -171,7 +171,7 @@ func TestAccessiblePrompter(t *testing.T) { go func() { // Wait for prompt to appear - _, err := console.ExpectString("Enter some characters (default: 12345abcdefg)") + _, err := console.ExpectString("Enter some characters (default: 12345abcdefg):") require.NoError(t, err) // Enter nothing diff --git a/internal/prompter/prompter.go b/internal/prompter/prompter.go index 5c3b1238d..4a6a4ff6d 100644 --- a/internal/prompter/prompter.go +++ b/internal/prompter/prompter.go @@ -77,6 +77,19 @@ func (p *accessiblePrompter) newForm(groups ...*huh.Group) *huh.Form { WithOutput(p.stdout) } +// addDefaultsToPrompt adds default values to the prompt string. +func (p *accessiblePrompter) addDefaultsToPrompt(prompt string, defaultValues []string) string { + if len(defaultValues) == 1 { + prompt = fmt.Sprintf("%s (default: %s):", prompt, defaultValues[0]) + } else if len(defaultValues) > 1 { + prompt = fmt.Sprintf("%s (defaults: %s):", prompt, strings.Join(defaultValues, ", ")) + } else { + prompt = fmt.Sprintf("%s:", prompt) + } + + return prompt +} + func (p *accessiblePrompter) Select(prompt, defaultValue string, options []string) (int, error) { var result int formOptions := []huh.Option[int]{} @@ -136,11 +149,7 @@ func (p *accessiblePrompter) MultiSelect(prompt string, defaults []string, optio func (p *accessiblePrompter) Input(prompt, defaultValue string) (string, error) { result := defaultValue - if defaultValue != "" { - prompt = fmt.Sprintf("%s (default: %s)", prompt, defaultValue) - } else { - prompt = fmt.Sprintf("%s:", prompt) - } + prompt = p.addDefaultsToPrompt(prompt, []string{defaultValue}) form := p.newForm( huh.NewGroup( huh.NewInput(). From cab906151fb142d1b000ee8be1fb3101b8087661 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 6 May 2025 15:32:30 -0600 Subject: [PATCH 04/20] fix(a11y prompter): select prompt default value is readable --- internal/prompter/accessible_prompter_test.go | 25 ++++++++++++++++++- internal/prompter/prompter.go | 12 ++++++--- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/internal/prompter/accessible_prompter_test.go b/internal/prompter/accessible_prompter_test.go index 8ae449dc9..c47969c04 100644 --- a/internal/prompter/accessible_prompter_test.go +++ b/internal/prompter/accessible_prompter_test.go @@ -78,6 +78,29 @@ func TestAccessiblePrompter(t *testing.T) { assert.Equal(t, expectedIndex, selectValue) }) + t.Run("Select - default value is in prompt and in readable format", func(t *testing.T) { + console := newTestVirtualTerminal(t) + p := newTestAccessiblePrompter(t, console) + dummyDefaultValue := "12345abcdefg" + options := []string{"1", "2", dummyDefaultValue} + + go func() { + // Wait for prompt to appear + _, err := console.ExpectString("Select a number (default: 12345abcdefg)") + require.NoError(t, err) + + // Just press enter to accept the default + _, err = console.SendLine("") + require.NoError(t, err) + }() + + selectValue, err := p.Select("Select a number", dummyDefaultValue, options) + require.NoError(t, err) + + expectedIndex := slices.Index(options, dummyDefaultValue) + assert.Equal(t, expectedIndex, selectValue) + }) + t.Run("MultiSelect", func(t *testing.T) { console := newTestVirtualTerminal(t) p := newTestAccessiblePrompter(t, console) @@ -171,7 +194,7 @@ func TestAccessiblePrompter(t *testing.T) { go func() { // Wait for prompt to appear - _, err := console.ExpectString("Enter some characters (default: 12345abcdefg):") + _, err := console.ExpectString("Enter some characters (default: 12345abcdefg)") require.NoError(t, err) // Enter nothing diff --git a/internal/prompter/prompter.go b/internal/prompter/prompter.go index 4a6a4ff6d..c5bf04691 100644 --- a/internal/prompter/prompter.go +++ b/internal/prompter/prompter.go @@ -79,12 +79,15 @@ func (p *accessiblePrompter) newForm(groups ...*huh.Group) *huh.Form { // addDefaultsToPrompt adds default values to the prompt string. func (p *accessiblePrompter) addDefaultsToPrompt(prompt string, defaultValues []string) string { + // We don't show empty default values in the prompt. + defaultValues = slices.DeleteFunc(defaultValues, func(s string) bool { + return s == "" + }) + if len(defaultValues) == 1 { - prompt = fmt.Sprintf("%s (default: %s):", prompt, defaultValues[0]) + prompt = fmt.Sprintf("%s (default: %s)", prompt, defaultValues[0]) } else if len(defaultValues) > 1 { - prompt = fmt.Sprintf("%s (defaults: %s):", prompt, strings.Join(defaultValues, ", ")) - } else { - prompt = fmt.Sprintf("%s:", prompt) + prompt = fmt.Sprintf("%s (defaults: %s)", prompt, strings.Join(defaultValues, ", ")) } return prompt @@ -93,6 +96,7 @@ func (p *accessiblePrompter) addDefaultsToPrompt(prompt string, defaultValues [] func (p *accessiblePrompter) Select(prompt, defaultValue string, options []string) (int, error) { var result int formOptions := []huh.Option[int]{} + prompt = p.addDefaultsToPrompt(prompt, []string{defaultValue}) for i, o := range options { // If this option is the default value, assign its index // to the result variable. huh will treat it as a default selection. From 957667efe6b069786584aed6034e24d543b0879d Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 6 May 2025 15:33:51 -0600 Subject: [PATCH 05/20] doc(prompter): remove TODO about default value panic --- internal/prompter/prompter.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/prompter/prompter.go b/internal/prompter/prompter.go index c5bf04691..eb8e2e58f 100644 --- a/internal/prompter/prompter.go +++ b/internal/prompter/prompter.go @@ -126,7 +126,6 @@ func (p *accessiblePrompter) MultiSelect(prompt string, defaults []string, optio // If this option is in the defaults slice, // let's add its index to the result slice and huh // will treat it as a default selection. - // TODO: does an invalid default value constitute a panic? if slices.Contains(defaults, o) { result = append(result, i) } From 04aaaea142b831c76ddf4880829287ff60d0b761 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 6 May 2025 15:43:26 -0600 Subject: [PATCH 06/20] fix(a11y prompter): multi select defaults are readable --- internal/prompter/accessible_prompter_test.go | 31 +++++++++++++++++++ internal/prompter/prompter.go | 1 + 2 files changed, 32 insertions(+) diff --git a/internal/prompter/accessible_prompter_test.go b/internal/prompter/accessible_prompter_test.go index c47969c04..c112c4708 100644 --- a/internal/prompter/accessible_prompter_test.go +++ b/internal/prompter/accessible_prompter_test.go @@ -147,6 +147,37 @@ func TestAccessiblePrompter(t *testing.T) { assert.Equal(t, []int{1}, multiSelectValue) }) + t.Run("MultiSelect - default value is in prompt and in readable format", func(t *testing.T) { + console := newTestVirtualTerminal(t) + p := newTestAccessiblePrompter(t, console) + dummyDefaultValues := []string{"foo", "bar"} + options := []string{"1", "2"} + options = append(options, dummyDefaultValues...) + + go func() { + // Wait for prompt to appear + _, err := console.ExpectString("Select a number (defaults: foo, bar)") + require.NoError(t, err) + + // Don't select anything because the defaults should be selected. + + // This confirms selections + _, err = console.SendLine("0") + require.NoError(t, err) + }() + + multiSelectValues, err := p.MultiSelect("Select a number", dummyDefaultValues, options) + require.NoError(t, err) + var expectedIndices []int + + // Get the indices of the default values within the options slice + // as that's what we expect the prompter to return when no selections are made. + for _, defaultValue := range dummyDefaultValues { + expectedIndices = append(expectedIndices, slices.Index(options, defaultValue)) + } + assert.Equal(t, expectedIndices, multiSelectValues) + }) + t.Run("Input", func(t *testing.T) { console := newTestVirtualTerminal(t) p := newTestAccessiblePrompter(t, console) diff --git a/internal/prompter/prompter.go b/internal/prompter/prompter.go index eb8e2e58f..f7a98cf98 100644 --- a/internal/prompter/prompter.go +++ b/internal/prompter/prompter.go @@ -121,6 +121,7 @@ func (p *accessiblePrompter) Select(prompt, defaultValue string, options []strin func (p *accessiblePrompter) MultiSelect(prompt string, defaults []string, options []string) ([]int, error) { var result []int + prompt = p.addDefaultsToPrompt(prompt, defaults) formOptions := make([]huh.Option[int], len(options)) for i, o := range options { // If this option is in the defaults slice, From 3594f6357b8db93c8cadf9d3c1726fa98d7c8ff6 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 6 May 2025 15:50:57 -0600 Subject: [PATCH 07/20] fix(a11y prompter): confirm prompt default is readable --- internal/prompter/accessible_prompter_test.go | 20 +++++++++++++++++++ internal/prompter/prompter.go | 5 +++++ 2 files changed, 25 insertions(+) diff --git a/internal/prompter/accessible_prompter_test.go b/internal/prompter/accessible_prompter_test.go index c112c4708..32a412646 100644 --- a/internal/prompter/accessible_prompter_test.go +++ b/internal/prompter/accessible_prompter_test.go @@ -304,6 +304,26 @@ func TestAccessiblePrompter(t *testing.T) { require.Equal(t, false, confirmValue) }) + t.Run("Confirm - default value is in prompt and in readable format", func(t *testing.T) { + console := newTestVirtualTerminal(t) + p := newTestAccessiblePrompter(t, console) + defaultValue := true + + go func() { + // Wait for prompt to appear + _, err := console.ExpectString("Are you sure (default: yes)") + require.NoError(t, err) + + // Enter nothing + _, err = console.SendLine("") + require.NoError(t, err) + }() + + confirmValue, err := p.Confirm("Are you sure", defaultValue) + require.NoError(t, err) + require.Equal(t, defaultValue, confirmValue) + }) + t.Run("AuthToken", func(t *testing.T) { console := newTestVirtualTerminal(t) p := newTestAccessiblePrompter(t, console) diff --git a/internal/prompter/prompter.go b/internal/prompter/prompter.go index f7a98cf98..f8bd04c35 100644 --- a/internal/prompter/prompter.go +++ b/internal/prompter/prompter.go @@ -189,6 +189,11 @@ func (p *accessiblePrompter) Password(prompt string) (string, error) { func (p *accessiblePrompter) Confirm(prompt string, defaultValue bool) (bool, error) { result := defaultValue + if defaultValue { + prompt = p.addDefaultsToPrompt(prompt, []string{"yes"}) + } else { + prompt = p.addDefaultsToPrompt(prompt, []string{"no"}) + } form := p.newForm( huh.NewGroup( huh.NewConfirm(). From 55cd3c34cddfbd2f53c59ec347d30d8f278c3708 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 7 May 2025 15:52:25 +0200 Subject: [PATCH 08/20] Feature detect v1 projects on pr edit --- pkg/cmd/pr/edit/edit.go | 5 +++ pkg/cmd/pr/edit/edit_test.go | 71 ++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 3c8d73ad3..0428d13d6 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -6,6 +6,7 @@ import ( "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" @@ -19,6 +20,9 @@ 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 @@ -193,6 +197,7 @@ func editRun(opts *EditOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "assignees", "labels", "projectCards", "projectItems", "milestone"}, + Detector: opts.Detector, } pr, repo, err := opts.Finder.Find(findOptions) if err != nil { diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 3c4882961..b09ee104e 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" @@ -696,3 +697,73 @@ func (s testSurveyor) EditFields(e *shared.Editable, _ string) error { func (t testEditorRetriever) Retrieve() (string, error) { return "vim", nil } + +// TODO projectsV1Deprecation +// Remove this test. +func TestProjectsV1Deprecation(t *testing.T) { + t.Run("when projects v1 is supported, is included in query", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`projectCards`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + f := &cmdutil.Factory{ + IOStreams: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + } + + // Ignore the error because we have no way to really stub it without + // fully stubbing a GQL error structure in the request body. + _ = editRun(&EditOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + Detector: &fd.EnabledDetectorMock{}, + + Finder: shared.NewFinder(f), + + SelectorArg: "https://github.com/cli/cli/pull/123", + }) + + // Verify that our request contained projectCards + reg.Verify(t) + }) + + t.Run("when projects v1 is not supported, is not included in query", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Exclude(t, httpmock.GraphQL(`projectCards`)) + + f := &cmdutil.Factory{ + IOStreams: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + } + + // Ignore the error because we have no way to really stub it without + // fully stubbing a GQL error structure in the request body. + _ = editRun(&EditOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + Detector: &fd.DisabledDetectorMock{}, + + Finder: shared.NewFinder(f), + + SelectorArg: "https://github.com/cli/cli/pull/123", + }) + + // Verify that our request did not contain projectCards + reg.Verify(t) + }) +} From d78980c668dfe98bb35cb17ec2ea92d4db580a82 Mon Sep 17 00:00:00 2001 From: Art Leo Date: Thu, 8 May 2025 20:00:43 +1000 Subject: [PATCH 09/20] Fix release download test http stubbing Co-authored-by: William Martin --- pkg/cmd/release/download/download_test.go | 192 ++++++++++++++++++---- 1 file changed, 158 insertions(+), 34 deletions(-) diff --git a/pkg/cmd/release/download/download_test.go b/pkg/cmd/release/download/download_test.go index 78709dd57..dab5c0a92 100644 --- a/pkg/cmd/release/download/download_test.go +++ b/pkg/cmd/release/download/download_test.go @@ -183,6 +183,7 @@ func Test_downloadRun(t *testing.T) { name string isTTY bool opts DownloadOptions + httpStubs func(*httpmock.Registry) wantErr string wantStdout string wantStderr string @@ -196,6 +197,24 @@ func Test_downloadRun(t *testing.T) { Destination: ".", Concurrency: 2, }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", + "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" + }`) + + reg.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse(`1234`)) + reg.Register(httpmock.REST("GET", "assets/3456"), httpmock.StringResponse(`3456`)) + reg.Register(httpmock.REST("GET", "assets/5678"), httpmock.StringResponse(`5678`)) + }, wantStdout: ``, wantStderr: ``, wantFiles: []string{ @@ -213,6 +232,23 @@ func Test_downloadRun(t *testing.T) { Destination: "tmp/assets", Concurrency: 2, }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", + "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" + }`) + + reg.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse(`1234`)) + reg.Register(httpmock.REST("GET", "assets/3456"), httpmock.StringResponse(`3456`)) + }, wantStdout: ``, wantStderr: ``, wantFiles: []string{ @@ -229,6 +265,20 @@ func Test_downloadRun(t *testing.T) { Destination: ".", Concurrency: 2, }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", + "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" + }`) + }, wantStdout: ``, wantStderr: ``, wantErr: "no assets match the file pattern", @@ -242,6 +292,30 @@ func Test_downloadRun(t *testing.T) { Destination: "tmp/packages", Concurrency: 2, }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", + "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" + }`) + + reg.Register( + httpmock.REST( + "GET", + "repos/OWNER/REPO/zipball/v1.2.3", + ), + httpmock.WithHeader( + httpmock.StringResponse("somedata"), "content-disposition", "attachment; filename=zipball.zip", + ), + ) + }, wantStdout: ``, wantStderr: ``, wantFiles: []string{ @@ -257,6 +331,30 @@ func Test_downloadRun(t *testing.T) { Destination: "tmp/packages", Concurrency: 2, }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", + "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" + }`) + + reg.Register( + httpmock.REST( + "GET", + "repos/OWNER/REPO/tarball/v1.2.3", + ), + httpmock.WithHeader( + httpmock.StringResponse("somedata"), "content-disposition", "attachment; filename=tarball.tgz", + ), + ) + }, wantStdout: ``, wantStderr: ``, wantFiles: []string{ @@ -273,6 +371,30 @@ func Test_downloadRun(t *testing.T) { Concurrency: 2, ArchiveType: "tar.gz", }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", + "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" + }`) + + reg.Register( + httpmock.REST( + "GET", + "repos/OWNER/REPO/tarball/v1.2.3", + ), + httpmock.WithHeader( + httpmock.StringResponse("somedata"), "content-disposition", "attachment; filename=tarball.tgz", + ), + ) + }, wantStdout: ``, wantStderr: ``, wantFiles: []string{ @@ -289,6 +411,22 @@ func Test_downloadRun(t *testing.T) { Concurrency: 2, FilePatterns: []string{"*windows-32bit.zip"}, }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", + "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" + }`) + + reg.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse(`1234`)) + }, wantStdout: ``, wantStderr: ``, wantFiles: []string{ @@ -305,6 +443,22 @@ func Test_downloadRun(t *testing.T) { Concurrency: 2, FilePatterns: []string{"*windows-32bit.zip"}, }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", + "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" + }`) + + reg.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse(`1234`)) + }, wantStdout: `1234`, wantStderr: ``, }, @@ -324,41 +478,11 @@ func Test_downloadRun(t *testing.T) { ios.SetStderrTTY(tt.isTTY) fakeHTTP := &httpmock.Registry{} - shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", tt.opts.TagName, `{ - "assets": [ - { "name": "windows-32bit.zip", "size": 12, - "url": "https://api.github.com/assets/1234" }, - { "name": "windows-64bit.zip", "size": 34, - "url": "https://api.github.com/assets/3456" }, - { "name": "linux.tgz", "size": 56, - "url": "https://api.github.com/assets/5678" } - ], - "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", - "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" - }`) - fakeHTTP.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse(`1234`)) - fakeHTTP.Register(httpmock.REST("GET", "assets/3456"), httpmock.StringResponse(`3456`)) - fakeHTTP.Register(httpmock.REST("GET", "assets/5678"), httpmock.StringResponse(`5678`)) + defer fakeHTTP.Verify(t) - fakeHTTP.Register( - httpmock.REST( - "GET", - "repos/OWNER/REPO/tarball/v1.2.3", - ), - httpmock.WithHeader( - httpmock.StringResponse("somedata"), "content-disposition", "attachment; filename=tarball.tgz", - ), - ) - - fakeHTTP.Register( - httpmock.REST( - "GET", - "repos/OWNER/REPO/zipball/v1.2.3", - ), - httpmock.WithHeader( - httpmock.StringResponse("somedata"), "content-disposition", "attachment; filename=zipball.zip", - ), - ) + if tt.httpStubs != nil { + tt.httpStubs(fakeHTTP) + } tt.opts.IO = ios tt.opts.HttpClient = func() (*http.Client, error) { From 89bed45c55557e0952cd4aacc8c66247f84d756d Mon Sep 17 00:00:00 2001 From: Art Leo Date: Thu, 8 May 2025 20:12:26 +1000 Subject: [PATCH 10/20] Release download handles missing archive URLs Co-authored-by: William Martin --- pkg/cmd/release/download/download.go | 16 +++++- pkg/cmd/release/download/download_test.go | 60 +++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/release/download/download.go b/pkg/cmd/release/download/download.go index cdf6135b6..b90721412 100644 --- a/pkg/cmd/release/download/download.go +++ b/pkg/cmd/release/download/download.go @@ -165,10 +165,24 @@ func downloadRun(opts *DownloadOptions) error { var toDownload []shared.ReleaseAsset isArchive := false if opts.ArchiveType != "" { - var archiveURL = release.ZipballURL + var archiveURL string if opts.ArchiveType == "tar.gz" { archiveURL = release.TarballURL + } else { + archiveURL = release.ZipballURL } + + if archiveURL == "" { + errMessage := fmt.Sprintf( + "release %q with tag %q, does not have a %q archive asset.", + release.Name, release.TagName, opts.ArchiveType, + ) + if release.IsDraft { + errMessage += " Most likely, this is because it is a draft." + } + return errors.New(errMessage) + } + // create pseudo-Asset with no name and pointing to ZipBallURL or TarBallURL toDownload = append(toDownload, shared.ReleaseAsset{APIURL: archiveURL}) isArchive = true diff --git a/pkg/cmd/release/download/download_test.go b/pkg/cmd/release/download/download_test.go index dab5c0a92..9337c9b65 100644 --- a/pkg/cmd/release/download/download_test.go +++ b/pkg/cmd/release/download/download_test.go @@ -462,6 +462,66 @@ func Test_downloadRun(t *testing.T) { wantStdout: `1234`, wantStderr: ``, }, + { + name: "draft release with null tarball_url and zipball_url", + isTTY: true, + opts: DownloadOptions{ + TagName: "v1.2.3", + ArchiveType: "tar.gz", + Destination: "tmp/packages", + Concurrency: 2, + }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "tag_name": "v1.2.3", + "name": "patch-36", + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": null, + "zipball_url": null, + "draft": true + }`) + }, + wantStdout: ``, + wantStderr: ``, + wantErr: "release \"patch-36\" with tag \"v1.2.3\", does not have a \"tar.gz\" archive asset. Most likely, this is because it is a draft.", + }, + { + name: "non-draft release with null tarball_url and zipball_url", + isTTY: true, + opts: DownloadOptions{ + TagName: "v1.2.3", + ArchiveType: "tar.gz", + Destination: "tmp/packages", + Concurrency: 2, + }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "tag_name": "v1.2.3", + "name": "patch-36", + "assets": [ + { "name": "windows-32bit.zip", "size": 12, + "url": "https://api.github.com/assets/1234" }, + { "name": "windows-64bit.zip", "size": 34, + "url": "https://api.github.com/assets/3456" }, + { "name": "linux.tgz", "size": 56, + "url": "https://api.github.com/assets/5678" } + ], + "tarball_url": null, + "zipball_url": null, + "draft": false + }`) + }, + wantStdout: ``, + wantStderr: ``, + wantErr: "release \"patch-36\" with tag \"v1.2.3\", does not have a \"tar.gz\" archive asset.", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 9b89a0ac0e83d2191d54ea3810b4cc940d67732a Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 8 May 2025 07:41:36 -0600 Subject: [PATCH 11/20] fix(a11y prompter): remove invalid defaults --- internal/prompter/accessible_prompter_test.go | 46 +++++++++++++++++++ internal/prompter/prompter.go | 21 ++++++++- 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/internal/prompter/accessible_prompter_test.go b/internal/prompter/accessible_prompter_test.go index 32a412646..b2a66141a 100644 --- a/internal/prompter/accessible_prompter_test.go +++ b/internal/prompter/accessible_prompter_test.go @@ -101,6 +101,27 @@ func TestAccessiblePrompter(t *testing.T) { assert.Equal(t, expectedIndex, selectValue) }) + t.Run("Select - invalid defaults are excluded from prompt", func(t *testing.T) { + console := newTestVirtualTerminal(t) + p := newTestAccessiblePrompter(t, console) + dummyDefaultValue := "foo" + options := []string{"1", "2"} + + go func() { + // Wait for prompt to appear without the invalid default value + _, err := console.ExpectString("Select a number \r\n") + require.NoError(t, err) + + // Select option 2 + _, err = console.SendLine("2") + require.NoError(t, err) + }() + + selectValue, err := p.Select("Select a number", dummyDefaultValue, options) + require.NoError(t, err) + assert.Equal(t, 1, selectValue) + }) + t.Run("MultiSelect", func(t *testing.T) { console := newTestVirtualTerminal(t) p := newTestAccessiblePrompter(t, console) @@ -178,6 +199,31 @@ func TestAccessiblePrompter(t *testing.T) { assert.Equal(t, expectedIndices, multiSelectValues) }) + t.Run("MultiSelect - invalid defaults are excluded from prompt", func(t *testing.T) { + console := newTestVirtualTerminal(t) + p := newTestAccessiblePrompter(t, console) + dummyDefaultValues := []string{"foo", "bar"} + options := []string{"1", "2"} + + go func() { + // Wait for prompt to appear without the invalid default values + _, err := console.ExpectString("Select a number \r\n") + require.NoError(t, err) + + // Not selecting anything will fail because there are no defaults. + _, err = console.SendLine("2") + require.NoError(t, err) + + // This confirms selections + _, err = console.SendLine("0") + require.NoError(t, err) + }() + + multiSelectValues, err := p.MultiSelect("Select a number", dummyDefaultValues, options) + require.NoError(t, err) + assert.Equal(t, []int{1}, multiSelectValues) + }) + t.Run("Input", func(t *testing.T) { console := newTestVirtualTerminal(t) p := newTestAccessiblePrompter(t, console) diff --git a/internal/prompter/prompter.go b/internal/prompter/prompter.go index f8bd04c35..c2233fd92 100644 --- a/internal/prompter/prompter.go +++ b/internal/prompter/prompter.go @@ -79,24 +79,32 @@ func (p *accessiblePrompter) newForm(groups ...*huh.Group) *huh.Form { // addDefaultsToPrompt adds default values to the prompt string. func (p *accessiblePrompter) addDefaultsToPrompt(prompt string, defaultValues []string) string { - // We don't show empty default values in the prompt. + // Removing empty defaults from the slice. defaultValues = slices.DeleteFunc(defaultValues, func(s string) bool { return s == "" }) + // Pluralizing the prompt if there are multiple default values. if len(defaultValues) == 1 { prompt = fmt.Sprintf("%s (default: %s)", prompt, defaultValues[0]) } else if len(defaultValues) > 1 { prompt = fmt.Sprintf("%s (defaults: %s)", prompt, strings.Join(defaultValues, ", ")) } + // Zero-length defaultValues means return prompt unchanged. return prompt } func (p *accessiblePrompter) Select(prompt, defaultValue string, options []string) (int, error) { var result int - formOptions := []huh.Option[int]{} + + // Remove invalid default values from the defaults slice. + if !slices.Contains(options, defaultValue) { + defaultValue = "" + } + prompt = p.addDefaultsToPrompt(prompt, []string{defaultValue}) + formOptions := []huh.Option[int]{} for i, o := range options { // If this option is the default value, assign its index // to the result variable. huh will treat it as a default selection. @@ -121,6 +129,12 @@ func (p *accessiblePrompter) Select(prompt, defaultValue string, options []strin func (p *accessiblePrompter) MultiSelect(prompt string, defaults []string, options []string) ([]int, error) { var result []int + + // Remove invalid default values from the defaults slice. + defaults = slices.DeleteFunc(defaults, func(s string) bool { + return !slices.Contains(options, s) + }) + prompt = p.addDefaultsToPrompt(prompt, defaults) formOptions := make([]huh.Option[int], len(options)) for i, o := range options { @@ -189,11 +203,13 @@ func (p *accessiblePrompter) Password(prompt string) (string, error) { func (p *accessiblePrompter) Confirm(prompt string, defaultValue bool) (bool, error) { result := defaultValue + if defaultValue { prompt = p.addDefaultsToPrompt(prompt, []string{"yes"}) } else { prompt = p.addDefaultsToPrompt(prompt, []string{"no"}) } + form := p.newForm( huh.NewGroup( huh.NewConfirm(). @@ -201,6 +217,7 @@ func (p *accessiblePrompter) Confirm(prompt string, defaultValue bool) (bool, er Value(&result), ), ) + if err := form.Run(); err != nil { return false, err } From 90532e8377fca81a0a74e319b143ba37b3607ad9 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Thu, 8 May 2025 21:26:02 +0100 Subject: [PATCH 12/20] Improve assertion for disabled echo mode (#10927) * Improve assertion for disabled echo Signed-off-by: Babak K. Shandiz * Use `expect.RegExpPattern` Signed-off-by: Babak K. Shandiz --------- Signed-off-by: Babak K. Shandiz --- internal/prompter/accessible_prompter_test.go | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/internal/prompter/accessible_prompter_test.go b/internal/prompter/accessible_prompter_test.go index ed4da8de8..9ba5d59b7 100644 --- a/internal/prompter/accessible_prompter_test.go +++ b/internal/prompter/accessible_prompter_test.go @@ -164,7 +164,12 @@ func TestAccessiblePrompter(t *testing.T) { // Ensure the dummy password is not printed to the screen, // asserting that echo mode is disabled. - _, err = console.ExpectString(" \r\n\r\n") + // + // Note that since console.ExpectString returns successful if the + // expected string matches any part of the stream, we have to use an + // anchored regexp (i.e., with ^ and $) to make sure the password/token + // is not printed at all. + _, err = console.Expect(expect.RegexpPattern("^ \r\n\r\n$")) require.NoError(t, err) }) @@ -230,7 +235,12 @@ func TestAccessiblePrompter(t *testing.T) { // Ensure the dummy password is not printed to the screen, // asserting that echo mode is disabled. - _, err = console.ExpectString(" \r\n\r\n") + // + // Note that since console.ExpectString returns successful if the + // expected string matches any part of the stream, we have to use an + // anchored regexp (i.e., with ^ and $) to make sure the password/token + // is not printed at all. + _, err = console.Expect(expect.RegexpPattern("^ \r\n\r\n$")) require.NoError(t, err) }) @@ -252,6 +262,10 @@ func TestAccessiblePrompter(t *testing.T) { _, err = console.ExpectString("token is required") require.NoError(t, err) + // Wait for the retry prompt + _, err = console.ExpectString("Paste your authentication token:") + require.NoError(t, err) + // Wait to ensure huh has time to set the echo mode time.Sleep(beforePasswordSendTimeout) @@ -266,7 +280,12 @@ func TestAccessiblePrompter(t *testing.T) { // Ensure the dummy password is not printed to the screen, // asserting that echo mode is disabled. - _, err = console.ExpectString(" \r\n\r\n") + // + // Note that since console.ExpectString returns successful if the + // expected string matches any part of the stream, we have to use an + // anchored regexp (i.e., with ^ and $) to make sure the password/token + // is not printed at all. + _, err = console.Expect(expect.RegexpPattern("^ \r\n\r\n$")) require.NoError(t, err) }) From da40e087460c0d01f741fba8200b1c5d825b2bee Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 15 May 2025 08:33:30 -0600 Subject: [PATCH 13/20] doc(api): code comment typo 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..aa180a015 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -940,7 +940,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput // 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. + // `gh pr` requests actor assignees. if input.Reviewers { g.Go(func() error { users, err := RepoAssignableUsers(client, repo) From ea85b92b35dccaae5a4b359a33c74d5a83c21707 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 14/20] 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 aa180a015..978e844c4 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 bcd47f1fb178c60ad7e87f75d3cfbe00f00545d1 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 15/20] 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 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 16/20] 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 17/20] 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 18/20] 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 19/20] 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 20/20] 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