From 5fc311374aa270c713593e4f808234d4b374a338 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 4 Oct 2024 10:08:48 -0700 Subject: [PATCH 1/7] Add handling of empty titles for Issues and PRs Currently, the prompter doesn't enforce the requirement of a title for PR and Issue creation. However, that creates the UX where a user can enter a blank title, spend time filling out a detailed body, then fail to create the issue or pr when the request is sent to the api because the title is blank. This attempts to handle that before sending a request to the api, enforcing that, when prompting, the title of an issue is not blank. --- pkg/cmd/pr/shared/survey.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 5b8bde0eb..a059b288c 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -111,9 +111,16 @@ func BodySurvey(p Prompt, state *IssueMetadataState, templateContent string) err } func TitleSurvey(p Prompt, state *IssueMetadataState) error { - result, err := p.Input("Title", state.Title) - if err != nil { - return err + var err error + result := "" + for result == "" { + result, err = p.Input("Title (required)", state.Title) + if err != nil { + return err + } + if result == "" { + fmt.Println("X Title cannot be blank.") + } } if result != state.Title { From be3b6cbd8dd2fd99315404b909fd958307c42c8f Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Tue, 8 Oct 2024 16:03:14 -0700 Subject: [PATCH 2/7] Make the X in the error message red and print with io writer --- pkg/cmd/issue/create/create.go | 2 +- pkg/cmd/pr/create/create.go | 2 +- pkg/cmd/pr/shared/survey.go | 7 +++++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 82d4ab133..f51cde4c9 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -222,7 +222,7 @@ func createRun(opts *CreateOptions) (err error) { defer prShared.PreserveInput(opts.IO, &tb, &err)() if opts.Title == "" { - err = prShared.TitleSurvey(opts.Prompter, &tb) + err = prShared.TitleSurvey(opts.Prompter, opts.IO, &tb) if err != nil { return } diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 7e893395c..1242af2c5 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -379,7 +379,7 @@ func createRun(opts *CreateOptions) (err error) { } else { if !opts.TitleProvided { - err = shared.TitleSurvey(opts.Prompter, state) + err = shared.TitleSurvey(opts.Prompter, opts.IO, state) if err != nil { return } diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index a059b288c..2ee343bfb 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -110,7 +110,7 @@ func BodySurvey(p Prompt, state *IssueMetadataState, templateContent string) err return nil } -func TitleSurvey(p Prompt, state *IssueMetadataState) error { +func TitleSurvey(p Prompt, io *iostreams.IOStreams, state *IssueMetadataState) error { var err error result := "" for result == "" { @@ -119,7 +119,10 @@ func TitleSurvey(p Prompt, state *IssueMetadataState) error { return err } if result == "" { - fmt.Println("X Title cannot be blank.") + colorizeRed := io.ColorScheme().ColorFromString("red") + msg := fmt.Sprintf("%s Title cannot be blank.", colorizeRed("X")) + // For some reason, only Fprintln and Println work here. I can't use Fprintf or Printf to eliminate the line above + fmt.Fprintln(io.ErrOut, msg) } } From da945314fd3179e3eb4cfab3d1681a210969f90a Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Tue, 8 Oct 2024 16:11:05 -0700 Subject: [PATCH 3/7] Fix failing test for pr and issue create --- pkg/cmd/issue/create/create_test.go | 4 ++-- pkg/cmd/pr/create/create_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index b95b5ef66..8e49700a0 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -611,7 +611,7 @@ func TestIssueCreate_recover(t *testing.T) { pm := &prompter.PrompterMock{} pm.InputFunc = func(p, d string) (string, error) { - if p == "Title" { + if p == "Title (required)" { return d, nil } else { return "", prompter.NoSuchPromptErr(p) @@ -736,7 +736,7 @@ func TestIssueCreate_continueInBrowser(t *testing.T) { pm := &prompter.PrompterMock{} pm.InputFunc = func(p, d string) (string, error) { - if p == "Title" { + if p == "Title (required)" { return "hello", nil } else { return "", prompter.NoSuchPromptErr(p) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index cba552101..81684ff00 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1210,7 +1210,7 @@ func Test_createRun(t *testing.T) { }, promptStubs: func(pm *prompter.PrompterMock) { pm.InputFunc = func(p, d string) (string, error) { - if p == "Title" { + if p == "Title (required)" { return d, nil } else { return "", prompter.NoSuchPromptErr(p) @@ -1316,7 +1316,7 @@ func Test_createRun(t *testing.T) { } pm.InputFunc = func(p, d string) (string, error) { - if p == "Title" { + if p == "Title (required)" { return d, nil } else if p == "Body" { return d, nil From dcbd1b728cd87523768807262b44f7cc94bbb582 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Tue, 8 Oct 2024 16:32:50 -0700 Subject: [PATCH 4/7] Add test coverage for TitleSurvey change --- pkg/cmd/pr/shared/survey_test.go | 41 ++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/pkg/cmd/pr/shared/survey_test.go b/pkg/cmd/pr/shared/survey_test.go index 8df492785..0e62e0e5a 100644 --- a/pkg/cmd/pr/shared/survey_test.go +++ b/pkg/cmd/pr/shared/survey_test.go @@ -158,3 +158,44 @@ type testEditor struct { func (e testEditor) Edit(filename, text string) (string, error) { return e.edit(text) } + +func TestTitleSurvey(t *testing.T) { + tests := []struct { + name string + prompterMockInputs []string + expectedTitle string + expectStderr bool + }{ + { + name: "title provided", + prompterMockInputs: []string{"title"}, + expectedTitle: "title", + }, + { + name: "first input empty", + prompterMockInputs: []string{"", "title"}, + expectedTitle: "title", + expectStderr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + io, _, _, stderr := iostreams.Test() + pm := prompter.NewMockPrompter(t) + for _, input := range tt.prompterMockInputs { + pm.RegisterInput("Title (required)", func(string, string) (string, error) { + return input, nil + }) + } + + state := &IssueMetadataState{} + err := TitleSurvey(pm, io, state) + + assert.NoError(t, err) + assert.Equal(t, tt.expectedTitle, state.Title) + if tt.expectStderr { + assert.Equal(t, "X Title cannot be blank.\n", stderr.String()) + } + }) + } +} From 4df2e7be634cf2eb7f7d63b61351d68afc1c6c07 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Tue, 15 Oct 2024 12:16:20 -0700 Subject: [PATCH 5/7] Remove comment --- pkg/cmd/pr/shared/survey.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 2ee343bfb..3d53be4c7 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -121,7 +121,6 @@ func TitleSurvey(p Prompt, io *iostreams.IOStreams, state *IssueMetadataState) e if result == "" { colorizeRed := io.ColorScheme().ColorFromString("red") msg := fmt.Sprintf("%s Title cannot be blank.", colorizeRed("X")) - // For some reason, only Fprintln and Println work here. I can't use Fprintf or Printf to eliminate the line above fmt.Fprintln(io.ErrOut, msg) } } From 13ab02729bd1b38c12d65199bbe57a6528bd2b99 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 17 Oct 2024 14:40:17 -0700 Subject: [PATCH 6/7] Clean up Title Survey empty title message code --- pkg/cmd/pr/shared/survey.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 3d53be4c7..ce38535d9 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -119,9 +119,7 @@ func TitleSurvey(p Prompt, io *iostreams.IOStreams, state *IssueMetadataState) e return err } if result == "" { - colorizeRed := io.ColorScheme().ColorFromString("red") - msg := fmt.Sprintf("%s Title cannot be blank.", colorizeRed("X")) - fmt.Fprintln(io.ErrOut, msg) + fmt.Fprintf(io.ErrOut, "%s Title cannot be blank\n", io.ColorScheme().FailureIcon()) } } From 14acbe00a93106cccb70cfd3c5fb290618d21866 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 17 Oct 2024 14:45:35 -0700 Subject: [PATCH 7/7] Remove . from test case for TestTitleSurvey --- pkg/cmd/pr/shared/survey_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/survey_test.go b/pkg/cmd/pr/shared/survey_test.go index 0e62e0e5a..d74696460 100644 --- a/pkg/cmd/pr/shared/survey_test.go +++ b/pkg/cmd/pr/shared/survey_test.go @@ -194,7 +194,7 @@ func TestTitleSurvey(t *testing.T) { assert.NoError(t, err) assert.Equal(t, tt.expectedTitle, state.Title) if tt.expectStderr { - assert.Equal(t, "X Title cannot be blank.\n", stderr.String()) + assert.Equal(t, "X Title cannot be blank\n", stderr.String()) } }) }