From 606deaf134dc7c7eabeb2914b1e3302e2968b589 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 4 Jun 2021 21:32:54 +0200 Subject: [PATCH] Allow setting empty body via editor in `issue/pr create` --- pkg/cmd/issue/create/create.go | 4 --- pkg/cmd/issue/create/create_test.go | 8 +++-- pkg/cmd/pr/create/create.go | 4 --- pkg/cmd/pr/create/create_test.go | 12 +++---- pkg/cmd/pr/shared/params.go | 9 ++--- pkg/cmd/pr/shared/params_test.go | 55 ++++++++++++++++++++++++++++- pkg/cmd/pr/shared/survey.go | 2 +- 7 files changed, 71 insertions(+), 23 deletions(-) diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index a974d8d67..4ef2ab12f 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -234,10 +234,6 @@ func createRun(opts *CreateOptions) (err error) { if err != nil { return } - - if tb.Body == "" { - tb.Body = templateContent - } } openURL, err = generatePreviewURL(apiClient, baseRepo, tb) diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index 3e69f386c..f3d70b575 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -114,6 +114,8 @@ func TestNewCmdCreate(t *testing.T) { args, err := shlex.Split(tt.cli) require.NoError(t, err) cmd.SetArgs(args) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) _, err = cmd.ExecuteC() if tt.wantsErr { assert.Error(t, err) @@ -168,7 +170,7 @@ func Test_createRun(t *testing.T) { WebMode: true, Assignees: []string{"monalisa"}, }, - wantsBrowse: "https://github.com/OWNER/REPO/issues/new?assignees=monalisa", + wantsBrowse: "https://github.com/OWNER/REPO/issues/new?assignees=monalisa&body=", wantsStderr: "Opening github.com/OWNER/REPO/issues/new in your browser.\n", }, { @@ -185,7 +187,7 @@ func Test_createRun(t *testing.T) { "viewer": { "login": "MonaLisa" } } }`)) }, - wantsBrowse: "https://github.com/OWNER/REPO/issues/new?assignees=MonaLisa", + wantsBrowse: "https://github.com/OWNER/REPO/issues/new?assignees=MonaLisa&body=", wantsStderr: "Opening github.com/OWNER/REPO/issues/new in your browser.\n", }, { @@ -214,7 +216,7 @@ func Test_createRun(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } }`)) }, - wantsBrowse: "https://github.com/OWNER/REPO/issues/new?projects=OWNER%2FREPO%2F1", + wantsBrowse: "https://github.com/OWNER/REPO/issues/new?body=&projects=OWNER%2FREPO%2F1", wantsStderr: "Opening github.com/OWNER/REPO/issues/new in your browser.\n", }, { diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 7ab899e85..b2b40c65a 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -302,10 +302,6 @@ func createRun(opts *CreateOptions) (err error) { if err != nil { return } - - if state.Body == "" { - state.Body = templateContent - } } openURL, err = generateCompareURL(*ctx, *state) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index a3d24c4d6..a19208892 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -239,7 +239,7 @@ func TestPRCreate_nontty_web(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, "", output.Stderr()) - assert.Equal(t, "https://github.com/OWNER/REPO/compare/master...feature?expand=1", output.BrowsedURL) + assert.Equal(t, "https://github.com/OWNER/REPO/compare/master...feature?body=&expand=1", output.BrowsedURL) } func TestPRCreate_recover(t *testing.T) { @@ -780,7 +780,7 @@ func TestPRCreate_web(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, "Opening github.com/OWNER/REPO/compare/master...feature in your browser.\n", output.Stderr()) - assert.Equal(t, "https://github.com/OWNER/REPO/compare/master...feature?expand=1", output.BrowsedURL) + assert.Equal(t, "https://github.com/OWNER/REPO/compare/master...feature?body=&expand=1", output.BrowsedURL) } func TestPRCreate_webLongURL(t *testing.T) { @@ -851,7 +851,7 @@ func TestPRCreate_webProject(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, "Opening github.com/OWNER/REPO/compare/master...feature in your browser.\n", output.Stderr()) - assert.Equal(t, "https://github.com/OWNER/REPO/compare/master...feature?expand=1&projects=ORG%2F1", output.BrowsedURL) + assert.Equal(t, "https://github.com/OWNER/REPO/compare/master...feature?body=&expand=1&projects=ORG%2F1", output.BrowsedURL) } func Test_determineTrackingBranch_empty(t *testing.T) { @@ -965,7 +965,7 @@ func Test_generateCompareURL(t *testing.T) { BaseBranch: "main", HeadBranchLabel: "feature", }, - want: "https://github.com/OWNER/REPO/compare/main...feature?expand=1", + want: "https://github.com/OWNER/REPO/compare/main...feature?body=&expand=1", wantErr: false, }, { @@ -978,7 +978,7 @@ func Test_generateCompareURL(t *testing.T) { state: prShared.IssueMetadataState{ Labels: []string{"one", "two three"}, }, - want: "https://github.com/OWNER/REPO/compare/a...b?expand=1&labels=one%2Ctwo+three", + want: "https://github.com/OWNER/REPO/compare/a...b?body=&expand=1&labels=one%2Ctwo+three", wantErr: false, }, { @@ -988,7 +988,7 @@ func Test_generateCompareURL(t *testing.T) { BaseBranch: "main/trunk", HeadBranchLabel: "owner:feature", }, - want: "https://github.com/OWNER/REPO/compare/main%2Ftrunk...owner%3Afeature?expand=1", + want: "https://github.com/OWNER/REPO/compare/main%2Ftrunk...owner%3Afeature?body=&expand=1", wantErr: false, }, } diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 93a5f2f58..ecd260645 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -2,13 +2,13 @@ package shared import ( "fmt" - "github.com/google/shlex" "net/url" "strings" "github.com/cli/cli/api" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/githubsearch" + "github.com/google/shlex" ) func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, baseURL string, state IssueMetadataState) (string, error) { @@ -20,9 +20,10 @@ func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, ba if state.Title != "" { q.Set("title", state.Title) } - if state.Body != "" { - q.Set("body", state.Body) - } + // We always want to set body, even if it's empty, to prevent the web interface to apply the default + // template. Since the user has the option to select a template in the terminal, assume that empty body + // here means that the user either skipped it or erased its contents. + q.Set("body", state.Body) if len(state.Assignees) > 0 { q.Set("assignees", strings.Join(state.Assignees, ",")) } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index 314218754..85dd9fa9e 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -1,7 +1,6 @@ package shared import ( - "github.com/stretchr/testify/assert" "net/http" "reflect" "testing" @@ -9,6 +8,7 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" + "github.com/stretchr/testify/assert" ) func Test_listURLWithQuery(t *testing.T) { @@ -192,3 +192,56 @@ func Test_QueryHasStateClause(t *testing.T) { assert.Equal(t, tt.hasState, gotState) } } + +func Test_WithPrAndIssueQueryParams(t *testing.T) { + type args struct { + baseURL string + state IssueMetadataState + } + tests := []struct { + name string + args args + want string + wantErr bool + }{ + { + name: "blank", + args: args{ + baseURL: "", + state: IssueMetadataState{}, + }, + want: "?body=", + }, + { + name: "no values", + args: args{ + baseURL: "http://example.com/hey", + state: IssueMetadataState{}, + }, + want: "http://example.com/hey?body=", + }, + { + name: "title and body", + args: args{ + baseURL: "http://example.com/hey", + state: IssueMetadataState{ + Title: "my title", + Body: "my bodeh", + }, + }, + want: "http://example.com/hey?body=my+bodeh&title=my+title", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := WithPrAndIssueQueryParams(nil, nil, tt.args.baseURL, tt.args.state) + if (err != nil) != tt.wantErr { + t.Errorf("WithPrAndIssueQueryParams() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("WithPrAndIssueQueryParams() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 5d8f13a37..15c930002 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -110,7 +110,7 @@ func BodySurvey(state *IssueMetadataState, templateContent, editorCommand string return err } - if state.Body != "" && preBody != state.Body { + if preBody != state.Body { state.MarkDirty() }