Merge pull request cli#2989 from cli/pr-merge-immediate-edit

mislav · web-flow · commit c148a9ba243b · 2021-02-17T19:26:17.000+01:00
pr merge: avoid prompting to enter editor after editing phase is chosen
diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go
@@ -20,6 +20,10 @@ import (
 	"github.com/spf13/cobra"
 )
 
+type editor interface {
+	Edit(string, string) (string, error)
+}
+
 type MergeOptions struct {
 	HttpClient func() (*http.Client, error)
 	Config     func() (config.Config, error)
@@ -37,6 +41,7 @@ type MergeOptions struct {
 
 	Body    string
 	BodySet bool
+	Editor  editor
 
 	IsDeleteBranchIndicated bool
 	CanDeleteLocalBranch    bool
@@ -103,6 +108,11 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm
 			opts.CanDeleteLocalBranch = !cmd.Flags().Changed("repo")
 			opts.BodySet = cmd.Flags().Changed("body")
 
+			opts.Editor = &userEditor{
+				io:     opts.IO,
+				config: opts.Config,
+			}
+
 			if runF != nil {
 				return runF(opts)
 			}
@@ -198,20 +208,14 @@ func mergeRun(opts *MergeOptions) error {
 			}
 
 			if action == shared.EditCommitMessageAction {
-				var editorCommand string
-				editorCommand, err = cmdutil.DetermineEditor(opts.Config)
-				if err != nil {
-					return err
-				}
-
 				if !payload.setCommitBody {
 					payload.commitBody, err = getMergeText(httpClient, baseRepo, pr.ID, payload.method)
 					if err != nil {
 						return err
 					}
 				}
 
-				payload.commitBody, err = commitMsgSurvey(payload.commitBody, editorCommand)
+				payload.commitBody, err = opts.Editor.Edit("*.md", payload.commitBody)
 				if err != nil {
 					return err
 				}
@@ -410,17 +414,16 @@ func confirmSurvey(allowEditMsg bool) (shared.Action, error) {
 	}
 }
 
-func commitMsgSurvey(msg string, editorCommand string) (string, error) {
-	var result string
-	q := &surveyext.GhEditor{
-		EditorCommand: editorCommand,
-		Editor: &survey.Editor{
-			Message:       "Body",
-			AppendDefault: true,
-			Default:       msg,
-			FileName:      "*.md",
-		},
+type userEditor struct {
+	io     *iostreams.IOStreams
+	config func() (config.Config, error)
+}
+
+func (e *userEditor) Edit(filename, startingText string) (string, error) {
+	editorCommand, err := cmdutil.DetermineEditor(e.config)
+	if err != nil {
+		return "", err
 	}
-	err := prompt.SurveyAskOne(q, &result)
-	return result, err
+
+	return surveyext.Edit(editorCommand, filename, startingText, e.io.In, e.io.Out, e.io.ErrOut, nil)
 }
diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go
@@ -689,61 +689,68 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) {
 }
 
 func TestPRMerge_interactiveSquashEditCommitMsg(t *testing.T) {
-	http := initFakeHTTP()
-	defer http.Verify(t)
-	http.Register(
-		httpmock.GraphQL(`query PullRequestForBranch\b`),
+	io, _, stdout, stderr := iostreams.Test()
+	io.SetStdoutTTY(true)
+	io.SetStderrTTY(true)
+
+	tr := initFakeHTTP()
+	defer tr.Verify(t)
+
+	tr.Register(
+		httpmock.GraphQL(`query PullRequestByNumber\b`),
 		httpmock.StringResponse(`
-		{ "data": { "repository": { "pullRequests": { "nodes": [{
-			"headRefName": "blueberries",
+		{ "data": { "repository": { "pullRequest": {
 			"headRepositoryOwner": {"login": "OWNER"},
 			"id": "THE-ID",
 			"number": 3,
 			"title": "title"
-		}] } } } }`))
-	http.Register(
+		} } } }`))
+	tr.Register(
 		httpmock.GraphQL(`query RepositoryInfo\b`),
 		httpmock.StringResponse(`
 		{ "data": { "repository": {
 			"mergeCommitAllowed": true,
 			"rebaseMergeAllowed": true,
 			"squashMergeAllowed": true
 		} } }`))
-	http.Register(
+	tr.Register(
 		httpmock.GraphQL(`query PullRequestMergeText\b`),
 		httpmock.StringResponse(`
 		{ "data": { "node": {
-			"viewerMergeBodyText": ""
+			"viewerMergeBodyText": "default body text"
 		} } }`))
-	http.Register(
+	tr.Register(
 		httpmock.GraphQL(`mutation PullRequestMerge\b`),
 		httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
 			assert.Equal(t, "THE-ID", input["pullRequestId"].(string))
 			assert.Equal(t, "SQUASH", input["mergeMethod"].(string))
-			assert.Equal(t, "cool story", input["commitBody"].(string))
+			assert.Equal(t, "DEFAULT BODY TEXT", input["commitBody"].(string))
 		}))
 
-	cs, cmdTeardown := run.Stub()
+	_, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
 
-	cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
-	cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
-
 	as, surveyTeardown := prompt.InitAskStubber()
 	defer surveyTeardown()
 
 	as.StubOne(2)                     // Merge method survey
 	as.StubOne(false)                 // Delete branch survey
 	as.StubOne("Edit commit message") // Confirm submit survey
-	as.StubOne("cool story")          // Edit commit message survey
 	as.StubOne("Submit")              // Confirm submit survey
 
-	output, err := runCommand(http, "blueberries", true, "")
-	if err != nil {
-		t.Fatalf("Got unexpected error running `pr merge` %s", err)
-	}
+	err := mergeRun(&MergeOptions{
+		IO:     io,
+		Editor: testEditor{},
+		HttpClient: func() (*http.Client, error) {
+			return &http.Client{Transport: tr}, nil
+		},
+		SelectorArg:     "https://github.com/OWNER/REPO/pull/123",
+		InteractiveMode: true,
+	})
+	assert.NoError(t, err)
 
-	assert.Equal(t, "✓ Squashed and merged pull request #3 (title)\n", output.Stderr())
+	assert.Equal(t, "", stdout.String())
+	assert.Equal(t, "✓ Squashed and merged pull request #3 (title)\n", stderr.String())
 }
 
 func TestPRMerge_interactiveCancelled(t *testing.T) {
@@ -885,3 +892,9 @@ func TestMergeRun_disableAutoMerge(t *testing.T) {
 	assert.Equal(t, "", stdout.String())
 	assert.Equal(t, "✓ Auto-merge disabled for pull request #123\n", stderr.String())
 }
+
+type testEditor struct{}
+
+func (e testEditor) Edit(filename, text string) (string, error) {
+	return strings.ToUpper(text), nil
+}
