diff --git a/command/pr_review.go b/command/pr_review.go index 5013d2bc2..3b01d0d5b 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -6,13 +6,16 @@ import ( "strconv" "strings" - "github.com/cli/cli/api" + "github.com/AlecAivazis/survey/v2" "github.com/spf13/cobra" + + "github.com/cli/cli/api" + "github.com/cli/cli/pkg/surveyext" + "github.com/cli/cli/utils" ) func init() { - // TODO re-register post release - // prCmd.AddCommand(prReviewCmd) + prCmd.AddCommand(prReviewCmd) prReviewCmd.Flags().BoolP("approve", "a", false, "Approve pull request") prReviewCmd.Flags().BoolP("request-changes", "r", false, "Request changes on a pull request") @@ -28,6 +31,8 @@ var prReviewCmd = &cobra.Command{ Examples: + gh pr review # add a review for the current branch's pull request + gh pr review 123 # add a review for pull request 123 gh pr review -a # mark the current branch's pull request as approved gh pr review -c -b "interesting" # comment on the current branch's pull request gh pr review 123 -r -b "needs more ascii art" # request changes on pull request 123 @@ -56,15 +61,19 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { state = api.ReviewComment } - if found != 1 { - return nil, errors.New("need exactly one of --approve, --request-changes, or --comment") - } - body, err := cmd.Flags().GetString("body") if err != nil { return nil, err } + if found == 0 && body == "" { + return nil, nil // signal interactive mode + } else if found == 0 && body != "" { + return nil, errors.New("--body unsupported without --approve, --request-changes, or --comment") + } else if found > 1 { + return nil, errors.New("need exactly one of --approve, --request-changes, or --comment") + } + if (flag == "request-changes" || flag == "comment") && body == "" { return nil, fmt.Errorf("body cannot be blank for %s review", flag) } @@ -108,7 +117,7 @@ func prReview(cmd *cobra.Command, args []string) error { } } - input, err := processReviewOpt(cmd) + reviewData, err := processReviewOpt(cmd) if err != nil { return fmt.Errorf("did not understand desired review action: %w", err) } @@ -124,12 +133,140 @@ func prReview(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("could not find pull request: %w", err) } + prNum = pr.Number } - err = api.AddReview(apiClient, pr, input) + out := colorableOut(cmd) + + if reviewData == nil { + reviewData, err = reviewSurvey(cmd) + if err != nil { + return err + } + if reviewData == nil && err == nil { + fmt.Fprint(out, "Discarding.\n") + return nil + } + } + + err = api.AddReview(apiClient, pr, reviewData) if err != nil { return fmt.Errorf("failed to create review: %w", err) } + switch reviewData.State { + case api.ReviewComment: + fmt.Fprintf(out, "%s Reviewed pull request #%d\n", utils.Gray("-"), prNum) + case api.ReviewApprove: + fmt.Fprintf(out, "%s Approved pull request #%d\n", utils.Green("✓"), prNum) + case api.ReviewRequestChanges: + fmt.Fprintf(out, "%s Requested changes to pull request #%d\n", utils.Red("+"), prNum) + } + return nil } + +func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { + editorCommand, err := determineEditor(cmd) + if err != nil { + return nil, err + } + + typeAnswers := struct { + ReviewType string + }{} + typeQs := []*survey.Question{ + { + Name: "reviewType", + Prompt: &survey.Select{ + Message: "What kind of review do you want to give?", + Options: []string{ + "Comment", + "Approve", + "Request changes", + }, + }, + }, + } + + err = SurveyAsk(typeQs, &typeAnswers) + if err != nil { + return nil, err + } + + reviewState := api.ReviewComment + + switch typeAnswers.ReviewType { + case "Approve": + reviewState = api.ReviewApprove + case "Request Changes": + reviewState = api.ReviewRequestChanges + } + + bodyAnswers := struct { + Body string + }{} + + blankAllowed := false + if reviewState == api.ReviewApprove { + blankAllowed = true + } + + bodyQs := []*survey.Question{ + &survey.Question{ + Name: "body", + Prompt: &surveyext.GhEditor{ + BlankAllowed: blankAllowed, + EditorCommand: editorCommand, + Editor: &survey.Editor{ + Message: "Review body", + FileName: "*.md", + }, + }, + }, + } + + err = SurveyAsk(bodyQs, &bodyAnswers) + if err != nil { + return nil, err + } + + if bodyAnswers.Body == "" && (reviewState == api.ReviewComment || reviewState == api.ReviewRequestChanges) { + return nil, errors.New("this type of review cannot be blank") + } + + if len(bodyAnswers.Body) > 0 { + out := colorableOut(cmd) + renderedBody, err := utils.RenderMarkdown(bodyAnswers.Body) + if err != nil { + return nil, err + } + + fmt.Fprintf(out, "Got:\n%s", renderedBody) + } + + confirm := false + confirmQs := []*survey.Question{ + { + Name: "confirm", + Prompt: &survey.Confirm{ + Message: "Submit?", + Default: true, + }, + }, + } + + err = SurveyAsk(confirmQs, &confirm) + if err != nil { + return nil, err + } + + if !confirm { + return nil, nil + } + + return &api.PullRequestReviewInput{ + Body: bodyAnswers.Body, + State: reviewState, + }, nil +} diff --git a/command/pr_review_test.go b/command/pr_review_test.go index 781b2cb06..231906ac0 100644 --- a/command/pr_review_test.go +++ b/command/pr_review_test.go @@ -4,26 +4,40 @@ import ( "bytes" "encoding/json" "io/ioutil" + "regexp" "testing" + + "github.com/cli/cli/test" ) func TestPRReview_validation(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() for _, cmd := range []string{ - `pr review 123`, `pr review --approve --comment 123`, `pr review --approve --comment -b"hey" 123`, } { http.StubRepoResponse("OWNER", "REPO") _, err := RunCommand(cmd) + if err == nil { + t.Fatal("expected error") + } eq(t, err.Error(), "did not understand desired review action: need exactly one of --approve, --request-changes, or --comment") } } +func TestPRReview_bad_body(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + _, err := RunCommand(`pr review -b "radical"`) + if err == nil { + t.Fatal("expected error") + } + eq(t, err.Error(), "did not understand desired review action: --body unsupported without --approve, --request-changes, or --comment") +} + func TestPRReview_url_arg(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -46,11 +60,13 @@ func TestPRReview_url_arg(t *testing.T) { } } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - _, err := RunCommand("pr review --approve https://github.com/OWNER/REPO/pull/123") + output, err := RunCommand("pr review --approve https://github.com/OWNER/REPO/pull/123") if err != nil { t.Fatalf("error running pr review: %s", err) } + test.ExpectLines(t, output.String(), "Approved pull request #123") + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { Variables struct { @@ -69,7 +85,6 @@ func TestPRReview_url_arg(t *testing.T) { } func TestPRReview_number_arg(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -92,11 +107,13 @@ func TestPRReview_number_arg(t *testing.T) { } } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - _, err := RunCommand("pr review --approve 123") + output, err := RunCommand("pr review --approve 123") if err != nil { t.Fatalf("error running pr review: %s", err) } + test.ExpectLines(t, output.String(), "Approved pull request #123") + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { Variables struct { @@ -115,24 +132,26 @@ func TestPRReview_number_arg(t *testing.T) { } func TestPRReview_no_arg(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, "id": "foobar123", "headRefName": "feature", "baseRefName": "master" } ] } } } }`)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - _, err := RunCommand(`pr review --comment -b "cool story"`) + output, err := RunCommand(`pr review --comment -b "cool story"`) if err != nil { t.Fatalf("error running pr review: %s", err) } + test.ExpectLines(t, output.String(), "- Reviewed pull request #123") + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { Variables struct { @@ -151,7 +170,6 @@ func TestPRReview_no_arg(t *testing.T) { } func TestPRReview_blank_comment(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -161,7 +179,6 @@ func TestPRReview_blank_comment(t *testing.T) { } func TestPRReview_blank_request_changes(t *testing.T) { - t.Skip("skipping until release is done") initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -171,7 +188,6 @@ func TestPRReview_blank_request_changes(t *testing.T) { } func TestPRReview(t *testing.T) { - t.Skip("skipping until release is done") type c struct { Cmd string ExpectedEvent string @@ -218,3 +234,170 @@ func TestPRReview(t *testing.T) { eq(t, reqBody.Variables.Input.Body, kase.ExpectedBody) } } + +func TestPRReview_interactive(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + as, teardown := initAskStubber() + defer teardown() + + as.Stub([]*QuestionStub{ + { + Name: "reviewType", + Value: "Approve", + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "body", + Value: "cool story", + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "confirm", + Value: true, + }, + }) + + output, err := RunCommand(`pr review`) + if err != nil { + t.Fatalf("got unexpected error running pr review: %s", err) + } + + test.ExpectLines(t, output.String(), + "Approved pull request #123", + "Got:", + "cool.*story") // weird because markdown rendering puts a bunch of junk between works + + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + reqBody := struct { + Variables struct { + Input struct { + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + eq(t, reqBody.Variables.Input.Event, "APPROVE") + eq(t, reqBody.Variables.Input.Body, "cool story") +} + +func TestPRReview_interactive_no_body(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + as, teardown := initAskStubber() + defer teardown() + + as.Stub([]*QuestionStub{ + { + Name: "reviewType", + Value: "Request Changes", + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "body", + Default: true, + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "confirm", + Value: true, + }, + }) + + _, err := RunCommand(`pr review`) + if err == nil { + t.Fatal("expected error") + } + eq(t, err.Error(), "this type of review cannot be blank") +} + +func TestPRReview_interactive_blank_approve(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + as, teardown := initAskStubber() + defer teardown() + + as.Stub([]*QuestionStub{ + { + Name: "reviewType", + Value: "Approve", + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "body", + Default: true, + }, + }) + as.Stub([]*QuestionStub{ + { + Name: "confirm", + Value: true, + }, + }) + + output, err := RunCommand(`pr review`) + if err != nil { + t.Fatalf("got unexpected error running pr review: %s", err) + } + + unexpect := regexp.MustCompile("Got:") + if unexpect.MatchString(output.String()) { + t.Errorf("did not expect to see body printed in %s", output.String()) + } + + test.ExpectLines(t, output.String(), "Approved pull request #123") + + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + reqBody := struct { + Variables struct { + Input struct { + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + eq(t, reqBody.Variables.Input.Event, "APPROVE") + eq(t, reqBody.Variables.Input.Body, "") + +} diff --git a/command/root.go b/command/root.go index 3ef268cb7..ef88a3184 100644 --- a/command/root.go +++ b/command/root.go @@ -338,3 +338,17 @@ func formatRemoteURL(cmd *cobra.Command, fullRepoName string) string { return fmt.Sprintf("https://%s/%s.git", defaultHostname, fullRepoName) } + +func determineEditor(cmd *cobra.Command) (string, error) { + editorCommand := os.Getenv("GH_EDITOR") + if editorCommand == "" { + ctx := contextForCommand(cmd) + cfg, err := ctx.Config() + if err != nil { + return "", fmt.Errorf("could not read config: %w", err) + } + editorCommand, _ = cfg.Get(defaultHostname, "editor") + } + + return editorCommand, nil +} diff --git a/command/title_body_survey.go b/command/title_body_survey.go index 06ab14a09..488efa34a 100644 --- a/command/title_body_survey.go +++ b/command/title_body_survey.go @@ -2,7 +2,6 @@ package command import ( "fmt" - "os" "github.com/AlecAivazis/survey/v2" "github.com/cli/cli/api" @@ -127,14 +126,9 @@ func selectTemplate(templatePaths []string) (string, error) { } func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClient *api.Client, repo ghrepo.Interface, providedTitle, providedBody string, defs defaults, templatePaths []string, allowReviewers, allowMetadata bool) error { - editorCommand := os.Getenv("GH_EDITOR") - if editorCommand == "" { - ctx := contextForCommand(cmd) - cfg, err := ctx.Config() - if err != nil { - return fmt.Errorf("could not read config: %w", err) - } - editorCommand, _ = cfg.Get(defaultHostname, "editor") + editorCommand, err := determineEditor(cmd) + if err != nil { + return err } issueState.Title = defs.Title @@ -182,7 +176,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie qs = append(qs, bodyQuestion) } - err := SurveyAsk(qs, issueState) + err = SurveyAsk(qs, issueState) if err != nil { return fmt.Errorf("could not prompt: %w", err) } diff --git a/pkg/surveyext/editor.go b/pkg/surveyext/editor.go index 3121ccf4d..9c1fca2e1 100644 --- a/pkg/surveyext/editor.go +++ b/pkg/surveyext/editor.go @@ -38,6 +38,7 @@ func init() { type GhEditor struct { *survey.Editor EditorCommand string + BlankAllowed bool } func (e *GhEditor) editorCommand() string { @@ -58,13 +59,14 @@ var EditorQuestionTemplate = ` {{- else }} {{- if and .Help (not .ShowHelp)}}{{color "cyan"}}[{{ .Config.HelpInput }} for help]{{color "reset"}} {{end}} {{- if and .Default (not .HideDefault)}}{{color "white"}}({{.Default}}) {{color "reset"}}{{end}} - {{- color "cyan"}}[(e) to launch {{ .EditorCommand }}, enter to skip] {{color "reset"}} + {{- color "cyan"}}[(e) to launch {{ .EditorCommand }}{{- if .BlankAllowed }}, enter to skip{{ end }}] {{color "reset"}} {{- end}}` // EXTENDED to pass editor name (to use in prompt) type EditorTemplateData struct { survey.Editor EditorCommand string + BlankAllowed bool Answer string ShowAnswer bool ShowHelp bool @@ -75,9 +77,10 @@ type EditorTemplateData struct { func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (interface{}, error) { err := e.Render( EditorQuestionTemplate, - // EXTENDED to support printing editor in prompt + // EXTENDED to support printing editor in prompt and BlankAllowed EditorTemplateData{ Editor: *e.Editor, + BlankAllowed: e.BlankAllowed, EditorCommand: filepath.Base(e.editorCommand()), Config: config, }, @@ -96,7 +99,7 @@ func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (int defer cursor.Show() for { - // EXTENDED to handle the e to edit / enter to skip behavior + // EXTENDED to handle the e to edit / enter to skip behavior + BlankAllowed r, _, err := rr.ReadRune() if err != nil { return "", err @@ -105,7 +108,11 @@ func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (int break } if r == '\r' || r == '\n' { - return "", nil + if e.BlankAllowed { + return "", nil + } else { + continue + } } if r == terminal.KeyInterrupt { return "", terminal.InterruptErr @@ -117,8 +124,9 @@ func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (int err = e.Render( EditorQuestionTemplate, EditorTemplateData{ - // EXTENDED to support printing editor in prompt + // EXTENDED to support printing editor in prompt, BlankAllowed Editor: *e.Editor, + BlankAllowed: e.BlankAllowed, EditorCommand: filepath.Base(e.editorCommand()), ShowHelp: true, Config: config,