From c5cf6b9ed95e2ade207989c56dafea4851f0e212 Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 11 Oct 2026 23:17:07 +0200 Subject: [PATCH] feat(pr): review with -a/-r/-c, a review create command and the current branch's PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tea pr review takes gh's review flags: -a/--approve, -r/--request-changes and -c/--comment, with -b/--body, -F/--body-file and -e/--editor, and --pending for a review to submit later. --event is hidden but still accepted, case-insensitively, as approve, approved, request-changes, request_changes, comment or pending. tea pr review create is the same command under the name people look for. The pull request argument is optional and accepts #N, URLs and branch names; without one, the pull request of the current branch is reviewed. Without a review type, tea opens the interactive review on a terminal and otherwise fails with "choose a review type" instead of posting a comment. Review types given as a subcommand ("tea pr review approve 13") point at the flag, and approving or requesting changes on one's own pull request fails before any editor opens. The interactive review asks for the type first, hiding Approve and Request changes for the author, then for an inline body that ctrl+e opens in the configured editor, then offers to annotate the diff, and confirms with a default of yes. A declined submission keeps the body as a draft. Successful reviews report "✓ Approved pull request alice/x#13 (title)" on a terminal and print the review URL. review submit finds the caller's pending review when no ID is given, review list and comments are tables with headers, and dismiss, undismiss, delete, request and cancel-request say what they did. Every review command has examples and completes pull request and review IDs. Signed-off-by: bircni --- internal/app/app_test.go | 14 - internal/app/editor_test.go | 8 +- internal/app/pull_integration_test.go | 4 +- internal/app/pull_review.go | 795 ++++++++++++++++++-------- internal/app/pull_review_form_test.go | 45 +- internal/app/pull_review_lifecycle.go | 267 ++++++--- internal/app/pull_review_requests.go | 39 +- internal/app/pull_review_test.go | 215 ++++++- internal/app/validation_test.go | 21 +- internal/services/reviews.go | 15 + internal/terminal/review_form.go | 108 ++-- internal/terminal/review_form_test.go | 67 ++- 12 files changed, 1159 insertions(+), 439 deletions(-) diff --git a/internal/app/app_test.go b/internal/app/app_test.go index be8633d3..7c1efff1 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -416,7 +416,6 @@ func walkCommands(cmd *cobra.Command, visit func(*cobra.Command)) { // have yet to complete; see TestHelpTextsAreComplete. const ( shortsToFix = ` - pr review cancel-request ` examplesToWrite = ` issue attachment delete @@ -472,19 +471,6 @@ const ( pr list pr merge pr reopen - pr review - pr review cancel-request - pr review comments - pr review delete - pr review dismiss - pr review list - pr review reply - pr review request - pr review resolve - pr review submit - pr review undismiss - pr review unresolve - pr review view pr status pr view search issue diff --git a/internal/app/editor_test.go b/internal/app/editor_test.go index 1691488d..76f76ba6 100644 --- a/internal/app/editor_test.go +++ b/internal/app/editor_test.go @@ -132,7 +132,7 @@ func TestCreationEditorSubmitsTypedBodyAndCleansDraft(t *testing.T) { }{ {"issue", "/api/v1/repos/alice/project/issues", "", []string{"issue", "create", "--title", "Issue"}}, {"pr", "/api/v1/repos/alice/project/pulls", "", []string{"pr", "create", "--title", "PR", "--head", "feature", "--base", "main"}}, - {"review", "/api/v1/repos/alice/project/pulls/3/reviews", "COMMENT", []string{"pr", "review", "3"}}, + {"review", "/api/v1/repos/alice/project/pulls/3/reviews", "COMMENT", []string{"pr", "review", "3", "--comment"}}, } { t.Run(tc.name, func(t *testing.T) { server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -140,6 +140,10 @@ func TestCreationEditorSubmitsTypedBodyAndCleansDraft(t *testing.T) { w.WriteHeader(http.StatusNotFound) return } + if r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls/3" { + _, _ = w.Write([]byte(`{"number":3}`)) + return + } require.Equal(t, tc.path, r.URL.Path) var options struct{ Event, Body string } require.NoError(t, json.NewDecoder(r.Body).Decode(&options)) @@ -300,7 +304,7 @@ func TestReviewLifecycleEditorRejectsEmptyRequiredText(t *testing.T) { } err := Execute(context.Background(), deps, args) require.Equal(t, clierr.Usage, clierr.ExitCode(err)) - require.ErrorContains(t, err, "non-empty review text") + require.ErrorContains(t, err, "needs a") require.ErrorContains(t, err, "draft saved at "+path) if verb == "submit" { require.Equal(t, "Saved draft", editor.initial) diff --git a/internal/app/pull_integration_test.go b/internal/app/pull_integration_test.go index ff3b26ab..6c5f41c4 100644 --- a/internal/app/pull_integration_test.go +++ b/internal/app/pull_integration_test.go @@ -212,7 +212,7 @@ func TestGiteaPullEditLifecycle(t *testing.T) { commentsFile := filepath.Join(t.TempDir(), "comments.json") require.NoError(t, os.WriteFile(commentsFile, []byte(`[{"path":"change.txt","body":"Disposable inline review","new_position":1}]`), 0o600)) var review services.Review - require.NoError(t, json.Unmarshal(execute("pr", "review", n, "-R", repo.FullName, "--comments-file", commentsFile, "--json"), &review)) + require.NoError(t, json.Unmarshal(execute("pr", "review", n, "-R", repo.FullName, "--comment", "--comments-file", commentsFile, "--json"), &review)) require.NotZero(t, review.ID) require.NotEmpty(t, review.Author) require.Equal(t, "COMMENT", review.State) @@ -225,7 +225,7 @@ func TestGiteaPullEditLifecycle(t *testing.T) { require.Equal(t, "change.txt", comments[0].Path) require.EqualValues(t, 1, comments[0].Line) commentID := fmt.Sprint(comments[0].ID) - require.Contains(t, string(execute("pr", "review", "reply", n, commentID, "-R", repo.FullName, "--body", "Disposable reply")), "Disposable reply") + require.Contains(t, string(execute("pr", "review", "reply", n, commentID, "-R", repo.FullName, "--body", "Disposable reply", "--json", "body")), "Disposable reply") require.NoError(t, json.Unmarshal(execute("pr", "review", "comments", n, "-R", repo.FullName, "--json"), &comments)) require.Len(t, comments, 2) require.Equal(t, "Disposable reply", comments[1].Body) diff --git a/internal/app/pull_review.go b/internal/app/pull_review.go index 8f1fc825..379774de 100644 --- a/internal/app/pull_review.go +++ b/internal/app/pull_review.go @@ -19,280 +19,365 @@ import ( "github.com/spf13/cobra" ) +// pullArguments documents the pull request argument of pr commands in their ARGUMENTS section. +const pullArguments = `A pull request can be supplied as argument in any of the following formats: +- by number, such as "13" or "#13" +- by URL, such as "https://gitea.com/OWNER/REPO/pulls/13" +- by the name of its head branch, such as "feature/login" or "bob:feature/login" + +Without an argument, the pull request of the current branch is selected.` + +// pullArgumentsAnnotation adds the ARGUMENTS section to a pr command's help. +func pullArgumentsAnnotation(annotations map[string]string) map[string]string { + if annotations == nil { + annotations = map[string]string{} + } + annotations["tea.arguments"] = pullArguments + return annotations +} + +// reviewStates maps the review types --event accepts onto the API's review states. +var reviewStates = map[string]string{ + "approve": "APPROVED", "approved": "APPROVED", + "request-changes": "REQUEST_CHANGES", "request_changes": "REQUEST_CHANGES", + "comment": "COMMENT", "pending": "PENDING", +} + +// reviewType holds the flags that choose the type of a review. +type reviewType struct { + approve, requestChanges, comment, pending bool + // event is the hidden spelling of the type that tea 0.x and scripts use. + event string +} + +// register adds -a, -r, -c, --pending when pending is set, and the hidden --event to cmd. +func (r *reviewType) register(cmd *cobra.Command, pending bool) { + flags := cmd.Flags() + flags.BoolVarP(&r.approve, "approve", "a", false, "Approve the pull request") + flags.BoolVarP(&r.requestChanges, "request-changes", "r", false, "Request changes (requires a body)") + flags.BoolVarP(&r.comment, "comment", "c", false, "Comment without approving (requires a body or inline comments)") + values := []string{"approve", "approved", "request-changes", "request_changes", "comment"} + if pending { + flags.BoolVar(&r.pending, "pending", false, "Save the review as pending, to submit later with tea pr review submit") + values = append(values, "pending") + } + flags.StringVar(&r.event, "event", "", "Review type") + enumValues(cmd, "event", values...) + _ = flags.MarkHidden("event") +} + +// state returns the API state of the chosen review type, or "" when none was chosen. +func (r reviewType) state() (string, error) { + var states []string + for state, chosen := range map[string]bool{"APPROVED": r.approve, "REQUEST_CHANGES": r.requestChanges, "COMMENT": r.comment, "PENDING": r.pending} { + if chosen { + states = append(states, state) + } + } + if r.event != "" { + states = append(states, reviewStates[strings.ToLower(r.event)]) + } + switch len(states) { + case 0: + return "", nil + case 1: + return states[0], nil + } + return "", clierr.Wrap(clierr.Usage, errors.New("choose one review type: --approve, --request-changes or --comment")) +} + +// errReviewType asks for a review type where tea cannot prompt for one. +func errReviewType(cmd *cobra.Command) error { + return clierr.Wrap(clierr.Usage, &argumentError{problem: "choose a review type: --approve, --request-changes, or --comment", cmd: cmd}) +} + +// reviewVerbs reports a submitted review by its state: "Approved pull request alice/x#1 (Add one)". +var reviewVerbs = map[string]string{ + "APPROVED": "Approved", "REQUEST_CHANGES": "Requested changes on", "COMMENT": "Commented on", "PENDING": "Saved pending review on", +} + +// reviewNouns name a review by its state in prompts: "Submit approval for #1?". +var reviewNouns = map[string]string{ + "APPROVED": "approval", "REQUEST_CHANGES": "change request", "COMMENT": "comment", "PENDING": "pending review", +} + +// reviewWords are review types people try as a subcommand, with the flag that chooses them. +var reviewWords = map[string]string{ + "approve": "approve", "lgtm": "approve", "comment": "comment", "reject": "request-changes", "request-changes": "request-changes", +} + +// reviewArgs accepts one pull request and points review types given as a subcommand, as in +// "tea pr review approve 13", at their flags. +func reviewArgs(cmd *cobra.Command, args []string) error { + if len(args) == 0 { + return nil + } + if flag, ok := reviewWords[strings.ToLower(args[0])]; ok { + pull := "" + if len(args) > 1 { + pull = args[1] + } + return clierr.Wrap(clierr.Usage, fmt.Errorf("unknown review command %q; to %s run: tea pr review %s --%s (see tea pr review --help)", + args[0], strings.ReplaceAll(flag, "-", " "), pull, flag)) + } + if len(args) > 1 { + if _, err := parseRef(args[0], pullRef); errors.Is(err, errNotRef) { + err := fmt.Errorf("unknown command %q for %q", args[0], cmd.CommandPath()) + if suggestions := cmd.SuggestionsFor(args[0]); len(suggestions) > 0 { + err = fmt.Errorf("%w; did you mean %s?", err, strings.Join(suggestions, " or ")) + } + return clierr.Wrap(clierr.Usage, err) + } + } + return usageArgs(cobra.MaximumNArgs(1))(cmd, args) +} + +const reviewExamples = `# Approve the pull request of the current branch +$ tea pr review --approve + +# Request changes on #13 +$ tea pr review 13 -r -b "Please hash the password" + +# Comment with a body read from a file +$ tea pr review 13 -c -F notes.md + +# Review interactively: pick the type, write the body, optionally annotate the diff +$ tea pr review 13 + +# Start a pending review with inline comments, then submit it +$ tea pr review 13 --pending --comments-file inline.json +$ tea pr review submit 13 --approve + +# See who reviewed, then reply to and resolve an inline comment +$ tea pr review list 13 +$ tea pr review reply 13 36 -b "Done" +$ tea pr review resolve 36` + func (a *application) pullReviewCommand() *cobra.Command { - var event, body, file, commentsFile, commit string - var useEditor bool - review := &cobra.Command{Use: "review ", Short: "Submit or inspect pull request reviews", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) (runErr error) { - var draft terminal.Draft - defer finishDraft(&draft, &runErr) - if err := validateBodySources(cmd); err != nil { - return err - } - n, err := number(args[0]) - if err != nil { - return err - } - snapshot, err := a.deps.Settings.Load() - if err != nil { - return err - } - policy := a.policy(snapshot) - if !localFlagsChanged(cmd) && policy.CanPrompt(a.deps.Streams) && a.deps.ReviewForm != nil { - return a.createReviewInteractively(cmd, n) - } - if err := oneOf("event", event, "APPROVED", "REQUEST_CHANGES", "COMMENT", "PENDING"); err != nil { - return err - } - if useEditor { - draft, err = a.editDraft(cmd, "") - body = draft.Body - } else { - body, err = a.readBody(cmd, body, file) - } - if err != nil { - return err - } - opts := services.ReviewOptions{Event: event, Body: body, Commit: commit} - if commentsFile != "" { - if commentsFile == "-" { - return clierr.Wrap(clierr.Usage, errors.New("--comments-file requires a file; use --body-file - for stdin")) - } - file, err := os.Open(commentsFile) - if err != nil { - return err - } - defer file.Close() - decoder := json.NewDecoder(file) - decoder.DisallowUnknownFields() - if err := decoder.Decode(&opts.Comments); err != nil { - return clierr.Wrap(clierr.Usage, fmt.Errorf("invalid inline comments: %w", err)) - } - if opts.Comments == nil { - return clierr.Wrap(clierr.Usage, errors.New("inline comments must be a JSON array")) - } - if err := decoder.Decode(new(any)); err != io.EOF { - return clierr.Wrap(clierr.Usage, errors.New("inline comments file must contain exactly one JSON array")) - } - for _, comment := range opts.Comments { - if err := comment.Validate(); err != nil { - return clierr.Wrap(clierr.Usage, err) - } - } - } - prompted := false - if (event == "COMMENT" || event == "REQUEST_CHANGES" || event == "PENDING") && strings.TrimSpace(body) == "" && len(opts.Comments) == 0 { - if useEditor || cmd.Flags().Changed("body") || cmd.Flags().Changed("body-file") { - return clierr.Wrap(clierr.Usage, errors.New("review body cannot be empty without inline comments")) - } - prompted, err = a.require(cmd, &body, "body") - if err != nil { - return err - } - if strings.TrimSpace(body) == "" { - return clierr.Wrap(clierr.Usage, errors.New("review body cannot be empty")) - } - opts.Body = body - } - target, err := a.resolve(cmd.Context(), true) - if err != nil { - return err - } - if prompted { - if err := a.confirm(cmd, fmt.Sprintf("Submit %s review on %s#%d: %s", event, target.Repo, n, body)); err != nil { - return err - } - } - item, err := (services.Reviews{Client: target.client, Repository: target.Repo}).Create(cmd.Context(), n, opts) - if err != nil { - return err - } - return writeRecord(a, cmd, item, reviewRecord) - }} - review.Flags().StringVar(&event, "event", "COMMENT", "Review event: APPROVED, REQUEST_CHANGES, COMMENT, PENDING") - review.Flags().StringVar(&body, "body", "", "Review body") - review.Flags().StringVar(&file, "body-file", "", "Read body from file, or - for stdin") - review.Flags().BoolVar(&useEditor, "editor", false, "Compose the review in the configured editor") - review.Flags().StringVar(&commentsFile, "comments-file", "", "JSON array of path, body, old_position/new_position inline comments") - review.Flags().StringVar(&commit, "commit", "", "Commit to review (default: current PR head)") - for _, verb := range []string{"list", "view", "comments"} { - var limit, page int - var reviewID int64 - use := verb + " " - count := 1 - if verb == "view" { - use += " " - count = 2 - } - command := &cobra.Command{Use: use, Short: titleCase(verb) + " pull request reviews", Args: argsExact(count), RunE: func(cmd *cobra.Command, args []string) error { - n, err := number(args[0]) - if err != nil { - return err - } - if reviewID < 0 { - return clierr.Wrap(clierr.Usage, errors.New("--review must be a positive review ID")) - } - if verb == "view" { - reviewID, err = number(args[1]) - if err != nil { - return err - } - } - target, err := a.resolve(cmd.Context(), true) - if err != nil { - return err - } - service := services.Reviews{Client: target.client, Repository: target.Repo} - switch verb { - case "view": - item, err := service.View(cmd.Context(), n, reviewID) - if err != nil { - return err - } - return writeRecord(a, cmd, item, reviewRecord) - case "list": - items, err := service.List(cmd.Context(), n, limit, page) - if err != nil { - return err - } - return writeRecords(a, cmd, items, reviewRecord) - case "comments": - items, err := service.Comments(cmd.Context(), n, reviewID, limit, page) - if err != nil { - return err - } - return writeList(a, cmd, items, func(item services.ReviewComment) []terminal.Cell { - return terminal.Row(strconv.FormatInt(item.ID, 10), fmt.Sprintf("%s:%d", item.Path, item.Line), item.Author, item.Body) - }) - } - return nil - }} - if verb != "view" { - pageFlags(command, &limit, &page) - } - if verb == "comments" { - command.Flags().Int64Var(&reviewID, "review", 0, "Limit comments to one review ID") - } - review.AddCommand(command) + review := a.reviewCreateCommand("review [ | | ]", "Review a pull request or manage its reviews") + review.Long = `Submit a review on a pull request, or manage existing reviews with the commands below. + +Without an argument, the pull request of the current branch is reviewed. Choose the review +type with --approve, --request-changes or --comment. On a terminal, leaving the type out +opens an interactive review that also offers to annotate the diff in your editor.` + review.Example = reviewExamples + review.SuggestionsMinimumDistance = 2 + create := a.reviewCreateCommand("create [ | | ]", `Submit a review (same as "tea pr review")`) + create.Example = `$ tea pr review create 13 --approve +$ tea pr review create --comment -b "Looks good overall"` + review.AddCommand(create, a.pullReviewListCommand(), a.pullReviewViewCommand(), a.pullReviewCommentsCommand(), a.pullReviewReplyCommand()) + for _, resolve := range []bool{true, false} { + review.AddCommand(a.pullReviewResolveCommand(resolve)) } - for _, verb := range []string{"resolve", "unresolve"} { - review.AddCommand(&cobra.Command{Use: verb + " ", Short: titleCase(verb) + " an inline review conversation", Annotations: textOutput("conversation state changes do not return a resource"), Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { - id, err := number(args[0]) - if err != nil { - return err - } - target, err := a.resolve(cmd.Context(), true) - if err != nil { - return err - } - if err := (services.Reviews{Client: target.client, Repository: target.Repo}).Resolve(cmd.Context(), id, verb == "resolve"); err != nil { - return err - } - _, err = fmt.Fprintf(cmd.ErrOrStderr(), "Comment %d %sd\n", id, verb) - return err - }}) - } - review.AddCommand(a.pullReviewReplyCommand()) review.AddCommand(a.pullReviewRequestCommand(true), a.pullReviewRequestCommand(false)) - for _, verb := range []string{"submit", "delete", "dismiss", "undismiss"} { - review.AddCommand(a.pullReviewLifecycleCommand(verb)) - } + review.AddCommand(a.pullReviewSubmitCommand(), a.pullReviewDeleteCommand(), a.pullReviewDismissCommand(true), a.pullReviewDismissCommand(false)) return review } -func (a *application) pullReviewReplyCommand() *cobra.Command { - var body, file string +// reviewCreateCommand builds tea pr review and its explicit form tea pr review create. +func (a *application) reviewCreateCommand(use, short string) *cobra.Command { + var kind reviewType + var body, file, commentsFile, commit string var useEditor bool - cmd := &cobra.Command{Use: "reply ", Short: "Reply to an inline review comment", Args: argsExact(2), RunE: func(cmd *cobra.Command, args []string) (runErr error) { + cmd := &cobra.Command{Use: use, Short: short, Annotations: pullArgumentsAnnotation(nil), Args: reviewArgs, RunE: func(cmd *cobra.Command, args []string) (runErr error) { var draft terminal.Draft defer finishDraft(&draft, &runErr) if err := validateBodySources(cmd); err != nil { return err } - n, err := number(args[0]) + state, err := kind.state() if err != nil { return err } - id, err := number(args[1]) + comments, err := readInlineComments(commentsFile) if err != nil { return err } - if useEditor { + if state == "" { + if localFlagsChanged(cmd) || !a.canPrompt() || a.deps.ReviewForm == nil { + return errReviewType(cmd) + } + return a.reviewInteractively(cmd, args) + } + // A comment needs a body or inline comments, and a change request a body. + needsBody := state == "REQUEST_CHANGES" || (state != "APPROVED" && len(comments) == 0) + missingBody := clierr.Wrap(clierr.Usage, fmt.Errorf("a %s needs a body; pass --body, --body-file or --editor", reviewNouns[state])) + explicit := cmd.Flags().Changed("body") || cmd.Flags().Changed("body-file") + if body, err = a.readBody(cmd, body, file); err != nil { + return err + } + if needsBody && strings.TrimSpace(body) == "" && (explicit || (!useEditor && !a.canPrompt())) { + return missingBody + } + t, n, err := a.pullSelector(cmd, args) + if err != nil { + return err + } + pull, err := (services.Pulls{Client: t.client, Repository: t.Repo}).View(cmd.Context(), n) + if err != nil { + return err + } + if err := ownReview(pull, t, state); err != nil { + return err + } + switch { + case useEditor: draft, err = a.editDraft(cmd, "") - body = draft.Body - } else { - body, err = a.readBody(cmd, body, file) + case needsBody && !explicit: + draft, err = a.bodyEditor(cmd)(cmd.Context(), "") + default: + draft.Body = body } if err != nil { return err } - prompted := false - if !useEditor && !cmd.Flags().Changed("body") && !cmd.Flags().Changed("body-file") { - prompted, err = a.require(cmd, &body, "body") - if err != nil { - return err - } + if body = draft.Body; needsBody && strings.TrimSpace(body) == "" { + return missingBody } - if strings.TrimSpace(body) == "" { - return clierr.Wrap(clierr.Usage, errors.New("reply body cannot be empty")) - } - target, err := a.resolve(cmd.Context(), true) + item, err := (services.Reviews{Client: t.client, Repository: t.Repo}).Create(cmd.Context(), n, services.ReviewOptions{Event: state, Body: body, Commit: commit, Comments: comments}) if err != nil { return err } - if prompted { - if err := a.confirm(cmd, fmt.Sprintf("Reply to review comment %d on %s#%d: %s", id, target.Repo, n, body)); err != nil { - return err - } - } - item, err := (services.Reviews{Client: target.client, Repository: target.Repo}).Reply(cmd.Context(), n, id, body) - if err != nil { - return err - } - return writeRecord(a, cmd, item, func(item services.ReviewComment) terminal.Record { - return terminal.Record{Heading: terminal.Row(strconv.FormatInt(item.ID, 10)), Lines: []string{item.Body, item.URL}} - }) + return a.reportReview(cmd, t, pull, item) }} - cmd.Flags().BoolVar(&useEditor, "editor", false, "Write the reply in the configured editor") - cmd.Flags().StringVar(&body, "body", "", "Reply body") - cmd.Flags().StringVar(&file, "body-file", "", "Read body from file, or - for stdin") + kind.register(cmd, true) + cmd.Flags().StringVarP(&body, "body", "b", "", "Review body") + cmd.Flags().StringVarP(&file, "body-file", "F", "", "Read the body from `file`, or - for stdin") + cmd.Flags().BoolVarP(&useEditor, "editor", "e", false, "Write the body in your editor") + cmd.Flags().StringVar(&commentsFile, "comments-file", "", "JSON array of inline comments: path, body, and new_position or old_position") + cmd.Flags().StringVar(&commit, "commit", "", "Commit to review (default: the head of the pull request)") + cmd.ValidArgsFunction = a.completePulls("open") return cmd } -func reviewRecord(item services.Review) terminal.Record { - return terminal.Record{Heading: terminal.Row(strconv.FormatInt(item.ID, 10), item.State, item.Author), Lines: []string{item.Body, item.URL}} +// canPrompt reports whether tea may ask questions on this terminal. +func (a *application) canPrompt() bool { + s, err := a.deps.Settings.Load() + return err == nil && a.policy(s).CanPrompt(a.deps.Streams) } -func (a *application) createReviewInteractively(cmd *cobra.Command, n int64) (runErr error) { - target, err := a.resolve(cmd.Context(), true) +// ownReview refuses to approve or request changes on one's own pull request before any body is +// written, as Gitea would after. +func ownReview(pull services.Pull, t target, state string) error { + if (state == "APPROVED" || state == "REQUEST_CHANGES") && t.login() != "" && strings.EqualFold(pull.Author, t.login()) { + return clierr.Wrap(clierr.Usage, fmt.Errorf("you can't approve or request changes on your own pull request #%d; use --comment", pull.Number)) + } + return nil +} + +// reportReview reports a submitted review, then prints its URL, or the review as JSON. +func (a *application) reportReview(cmd *cobra.Command, t target, pull services.Pull, item services.Review) error { + a.done(cmd, "%s pull request %s", reviewVerbs[item.State], refLabel(t.Repo, pull.Number, pull.Title)) + switch { + case a.options.json != "": + return a.writeJSON(cmd, item) + case item.URL != "": + return a.printURL(cmd, item.URL) + } + return nil +} + +// readInlineComments reads the inline comments of --comments-file, a JSON array. +func readInlineComments(path string) ([]services.InlineComment, error) { + if path == "" { + return nil, nil + } + if path == "-" { + return nil, clierr.Wrap(clierr.Usage, errors.New("--comments-file requires a file; use --body-file - for stdin")) + } + file, err := os.Open(path) + if err != nil { + return nil, err + } + defer file.Close() + decoder := json.NewDecoder(file) + decoder.DisallowUnknownFields() + var comments []services.InlineComment + if err := decoder.Decode(&comments); err != nil { + return nil, clierr.Wrap(clierr.Usage, fmt.Errorf("invalid inline comments: %w", err)) + } + if comments == nil { + return nil, clierr.Wrap(clierr.Usage, errors.New("inline comments must be a JSON array")) + } + if err := decoder.Decode(new(any)); err != io.EOF { + return nil, clierr.Wrap(clierr.Usage, errors.New("inline comments file must contain exactly one JSON array")) + } + for _, comment := range comments { + if err := comment.Validate(); err != nil { + return nil, clierr.Wrap(clierr.Usage, err) + } + } + return comments, nil +} + +// reviewInteractively asks for the review type, body and inline comments, then submits the review. +func (a *application) reviewInteractively(cmd *cobra.Command, args []string) error { + t, n, err := a.pullSelector(cmd, args) if err != nil { return err } - values, err := a.deps.ReviewForm.CreateReview(cmd.Context(), terminal.ReviewFormChoices{ - ReviewDiff: func(context.Context) ([]terminal.ReviewCommentDraft, error) { - return a.collectDiffReviewComments(cmd, target, n) - }, - EditBody: a.bodyEditor(cmd), - }) - defer finishDraft(&values.Draft, &runErr) + pull, err := (services.Pulls{Client: t.client, Repository: t.Repo}).View(cmd.Context(), n) if err != nil { return err } - opts := services.ReviewOptions{Event: values.Event, Body: values.Body} - for _, comment := range values.Comments { - opts.Comments = append(opts.Comments, services.InlineComment(comment)) - } - description := fmt.Sprintf("Submit %s review on %s#%d", values.Event, target.Repo, n) - if values.Body != "" { - description += ": " + values.Body - } - if err := a.confirm(cmd, description); err != nil { - return err - } - item, err := (services.Reviews{Client: target.client, Repository: target.Repo}).Create(cmd.Context(), n, opts) + preferences, err := a.preferences() if err != nil { return err } - return writeRecord(a, cmd, item, reviewRecord) + choices := terminal.ReviewFormChoices{ + Header: fmt.Sprintf("Reviewing #%d %q (%s → %s) in %s", n, pull.Title, pullHead(pull, t.Repo), pull.Base, t.Repo), + Author: t.login() != "" && strings.EqualFold(pull.Author, t.login()), + Editor: preferences.Editor, + } + if preferences.Editor != "" { + choices.ReviewDiff = func(context.Context) ([]terminal.ReviewCommentDraft, error) { + return a.collectDiffReviewComments(cmd, t, n) + } + } + values, err := a.deps.ReviewForm.CreateReview(cmd.Context(), choices) + if err == nil { + err = a.confirmAction(cmd, terminal.Confirmation{Prompt: fmt.Sprintf("Submit %s for #%d?", reviewNouns[values.Event], n), DefaultYes: true}) + } + var item services.Review + if err == nil { + opts := services.ReviewOptions{Event: values.Event, Body: values.Body} + for _, comment := range values.Comments { + opts.Comments = append(opts.Comments, services.InlineComment(comment)) + } + item, err = (services.Reviews{Client: t.client, Repository: t.Repo}).Create(cmd.Context(), n, opts) + } + if err != nil { + draft := keepDraft(values.Body) + finishDraft(&draft, &err) + return err + } + return a.reportReview(cmd, t, pull, item) +} + +// keepDraft saves typed text that a canceled or failed command did not submit, so that finishDraft +// reports where it is. Empty text, or text that cannot be saved, has no draft. +func keepDraft(body string) terminal.Draft { + if strings.TrimSpace(body) == "" { + return terminal.Draft{} + } + file, err := os.CreateTemp("", "tea-draft-*.md") + if err != nil { + return terminal.Draft{} + } + _, writeErr := file.WriteString(body) + if err := errors.Join(writeErr, file.Close()); err != nil { + _ = os.Remove(file.Name()) + return terminal.Draft{} + } + return terminal.Draft{Path: file.Name(), Body: body} +} + +// pullHead names the head branch of a pull request, as OWNER:BRANCH when it is in a fork of repo. +func pullHead(item services.Pull, repo string) string { + if owner, _, ok := strings.Cut(item.HeadRepo, "/"); ok && !strings.EqualFold(item.HeadRepo, repo) { + return owner + ":" + item.Head + } + return item.Head } func (a *application) collectDiffReviewComments(cmd *cobra.Command, t target, n int64) ([]terminal.ReviewCommentDraft, error) { @@ -334,3 +419,249 @@ func (a *application) reviewDiffFile(cmd *cobra.Command, path string) ([]termina } return comments, nil } + +// reviewStateCell words a review's state for people, colored like gh: Approved, Changes requested, +// Commented, Pending or Requested, with "(dismissed)" for dismissed reviews. +func reviewStateCell(item services.Review) terminal.Cell { + words := map[string]terminal.Cell{ + "APPROVED": {Text: "Approved", Color: "green"}, "REQUEST_CHANGES": {Text: "Changes requested", Color: "red"}, + "COMMENT": {Text: "Commented"}, "PENDING": {Text: "Pending", Color: "yellow"}, "REQUEST_REVIEW": {Text: "Requested", Muted: true}, + } + cell, ok := words[item.State] + if !ok { + cell = terminal.Cell{Text: item.State} + } + if item.Dismissed { + cell = terminal.Cell{Text: cell.Text + " (dismissed)", Muted: true} + } + return cell +} + +// reviewPossessive names a review by its author and state in feedback: "bob's approval". +func reviewPossessive(item services.Review) string { + noun := "review" + switch item.State { + case "APPROVED": + noun = "approval" + case "REQUEST_CHANGES": + noun = "change request" + } + return item.Author + "'s " + noun +} + +// pullAndID resolves the pull request and the ID that review subcommands take as their two arguments. +func (a *application) pullAndID(cmd *cobra.Command, args []string) (target, int64, int64, error) { + id, err := number(args[1]) + if err != nil { + return target{}, 0, 0, err + } + t, n, err := a.pullSelector(cmd, args[:1]) + return t, n, id, err +} + +func (a *application) pullReviewListCommand() *cobra.Command { + var limit, page int + cmd := &cobra.Command{Use: "list [ | | ]", Short: "List reviews on a pull request", Annotations: pullArgumentsAnnotation(nil), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { + t, n, err := a.pullSelector(cmd, args) + if err != nil { + return err + } + items, err := (services.Reviews{Client: t.client, Repository: t.Repo}).List(cmd.Context(), n, limit, page) + if err != nil { + return err + } + ref := fmt.Sprintf("%s#%d", t.Repo, n) + return writeTable(a, cmd, items, listSpec[services.Review]{ + Columns: []terminal.Column{{Header: "ID"}, {Header: "Reviewer"}, {Header: "State"}, {Header: "Submitted"}, {Header: "Comments"}, {Header: "Body", Flex: true}}, + Row: func(item services.Review) []terminal.Cell { + return []terminal.Cell{ + {Text: strconv.FormatInt(item.ID, 10), Link: item.URL}, + {Text: item.Author}, + reviewStateCell(item), + {Time: terminal.ParseTime(item.SubmittedAt)}, + {Text: strconv.Itoa(item.CommentsCount)}, + {Text: item.Body}, + } + }, + Empty: "no reviews on " + ref, Context: "reviews on " + ref, + }, listPage{Number: page, More: len(items) == limit}) + }} + pageFlags(cmd, &limit, &page) + cmd.Example = `$ tea pr review list 13 +$ tea pr review list --json id,author,state` + cmd.ValidArgsFunction = a.completePulls("all") + return cmd +} + +func (a *application) pullReviewViewCommand() *cobra.Command { + cmd := &cobra.Command{Use: "view ", Short: "View a review", Annotations: pullArgumentsAnnotation(nil), Args: argsExact(2), RunE: func(cmd *cobra.Command, args []string) error { + t, n, id, err := a.pullAndID(cmd, args) + if err != nil { + return err + } + item, err := (services.Reviews{Client: t.client, Repository: t.Repo}).View(cmd.Context(), n, id) + if err != nil { + return err + } + now := a.deps.Now() + return writeView(a, cmd, item, func(item services.Review) terminal.View { + meta := item.Author + if submitted := terminal.ParseTime(item.SubmittedAt); !submitted.IsZero() { + meta += " • " + terminal.RelativeTime(now, submitted) + } + return terminal.View{ + Title: fmt.Sprintf("Review on %s#%d", t.Repo, n), ID: strconv.FormatInt(item.ID, 10), Badges: []terminal.Cell{reviewStateCell(item)}, Meta: meta, + Fields: []terminal.Field{ + {Label: "Reviewer", Value: item.Author, PipedOnly: true}, + {Label: "State", Value: item.State, PipedOnly: true}, + {Label: "Inline comments", Value: strconv.Itoa(item.CommentsCount)}, + {Label: "Commit", Value: item.Commit}, + }, + Body: item.Body, Kind: "review", URL: item.URL, + } + }) + }} + cmd.Example = "$ tea pr review view 13 6" + cmd.ValidArgsFunction = a.completeReviewArgs(false) + return cmd +} + +func (a *application) pullReviewCommentsCommand() *cobra.Command { + var limit, page int + var reviewID int64 + cmd := &cobra.Command{Use: "comments [ | | ]", Short: "List inline review comments", Annotations: pullArgumentsAnnotation(nil), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { + if reviewID < 0 { + return clierr.Wrap(clierr.Usage, errors.New("--review must be a positive review ID")) + } + t, n, err := a.pullSelector(cmd, args) + if err != nil { + return err + } + items, err := (services.Reviews{Client: t.client, Repository: t.Repo}).Comments(cmd.Context(), n, reviewID, limit, page) + if err != nil { + return err + } + ref := fmt.Sprintf("%s#%d", t.Repo, n) + return writeTable(a, cmd, items, listSpec[services.ReviewComment]{ + Columns: []terminal.Column{{Header: "ID"}, {Header: "File"}, {Header: "Author"}, {Header: "Resolved"}, {Header: "Body", Flex: true}}, + Row: func(item services.ReviewComment) []terminal.Cell { + location := item.Path + if item.Line > 0 { + location = fmt.Sprintf("%s:%d", item.Path, item.Line) + } + return []terminal.Cell{{Text: strconv.FormatInt(item.ID, 10), Link: item.URL}, {Text: location}, {Text: item.Author}, {Text: item.Resolver}, {Text: item.Body}} + }, + Empty: "no inline review comments on " + ref, Context: "inline review comments on " + ref, + }, listPage{Number: page, More: len(items) == limit}) + }} + pageFlags(cmd, &limit, &page) + cmd.Flags().Int64Var(&reviewID, "review", 0, "Limit comments to one review `ID`") + cmd.Example = `$ tea pr review comments 13 +$ tea pr review comments 13 --review 6` + cmd.ValidArgsFunction = a.completePulls("all") + return cmd +} + +func (a *application) pullReviewResolveCommand(resolve bool) *cobra.Command { + verb, short, done := "resolve", "Mark an inline conversation as resolved", "Resolved" + if !resolve { + verb, short, done = "unresolve", "Reopen a resolved inline conversation", "Reopened" + } + cmd := &cobra.Command{Use: verb + " ", Short: short, Annotations: textOutput("conversation state changes do not return a resource"), Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { + id, err := number(args[0]) + if err != nil { + return err + } + t, err := a.resolve(cmd.Context(), true) + if err != nil { + return err + } + if err := (services.Reviews{Client: t.client, Repository: t.Repo}).Resolve(cmd.Context(), id, resolve); err != nil { + return err + } + a.done(cmd, "%s conversation of review comment %d in %s", done, id, t.Repo) + return nil + }} + cmd.Example = fmt.Sprintf("# Find the comment IDs with: tea pr review comments 13\n$ tea pr review %s 36", verb) + return cmd +} + +func (a *application) pullReviewReplyCommand() *cobra.Command { + var body, file string + var useEditor bool + cmd := &cobra.Command{Use: "reply ", Short: "Reply to an inline review comment", Annotations: pullArgumentsAnnotation(nil), Args: argsExact(2), RunE: func(cmd *cobra.Command, args []string) (runErr error) { + var draft terminal.Draft + defer finishDraft(&draft, &runErr) + if err := validateBodySources(cmd); err != nil { + return err + } + t, n, id, err := a.pullAndID(cmd, args) + if err != nil { + return err + } + if useEditor { + draft, err = a.editDraft(cmd, "") + body = draft.Body + } else { + body, err = a.readBody(cmd, body, file) + } + if err != nil { + return err + } + if !useEditor && !cmd.Flags().Changed("body") && !cmd.Flags().Changed("body-file") { + if !a.canPrompt() { + return clierr.Wrap(clierr.Usage, errors.New("a reply needs a body; pass --body, --body-file or --editor")) + } + if draft, err = a.bodyEditor(cmd)(cmd.Context(), ""); err != nil { + return err + } + body = draft.Body + } + if strings.TrimSpace(body) == "" { + return clierr.Wrap(clierr.Usage, errors.New("reply body cannot be empty")) + } + item, err := (services.Reviews{Client: t.client, Repository: t.Repo}).Reply(cmd.Context(), n, id, body) + if err != nil { + return err + } + a.done(cmd, "Replied to review comment %d on %s#%d", id, t.Repo, n) + if a.options.json != "" { + return a.writeJSON(cmd, item) + } + return a.printURL(cmd, item.URL) + }} + cmd.Flags().StringVarP(&body, "body", "b", "", "Reply body") + cmd.Flags().StringVarP(&file, "body-file", "F", "", "Read the body from `file`, or - for stdin") + cmd.Flags().BoolVarP(&useEditor, "editor", "e", false, "Write the reply in your editor") + cmd.Example = `$ tea pr review reply 13 36 -b "Done, the password is hashed now"` + cmd.ValidArgsFunction = a.completeReviewArgs(true) + return cmd +} + +// completeReviewArgs completes an open pull request, then the IDs of its reviews, or of its inline +// review comments when inline is set. +func (a *application) completeReviewArgs(inline bool) cobra.CompletionFunc { + pulls := a.completePulls("open") + return func(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) { + if len(args) == 0 { + return pulls(cmd, args, toComplete) + } + n, err := number(args[0]) + if err != nil { + return nil, cobra.ShellCompDirectiveNoFileComp + } + return a.apiCompletion(true, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) { + service := services.Reviews{Client: t.client, Repository: t.Repo} + if inline { + items, err := service.Comments(ctx, n, 0, completionLimit, 1) + return describe(items, func(item services.ReviewComment) (string, string) { + return strconv.FormatInt(item.ID, 10), item.Author + " on " + item.Path + ": " + item.Body + }), err + } + items, err := service.List(ctx, n, completionLimit, 1) + return describe(items, func(item services.Review) (string, string) { + return strconv.FormatInt(item.ID, 10), reviewStateCell(item).Text + " by " + item.Author + }), err + })(cmd, args, toComplete) + } +} diff --git a/internal/app/pull_review_form_test.go b/internal/app/pull_review_form_test.go index 66ed8753..d1341ea9 100644 --- a/internal/app/pull_review_form_test.go +++ b/internal/app/pull_review_form_test.go @@ -9,6 +9,7 @@ import ( "errors" "net/http" "os" + "regexp" "strings" "testing" @@ -25,6 +26,10 @@ func (f reviewFormFunc) CreateReview(ctx context.Context, choices terminal.Revie func TestPullReviewImplicitInteractiveForm(t *testing.T) { var createPayload map[string]any server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls/3" { + _, _ = w.Write([]byte(`{"number":3,"title":"Add one","user":{"login":"bob"},"head":{"ref":"feat/one"},"base":{"ref":"main"}}`)) + return + } require.Equal(t, http.MethodPost, r.Method) require.Equal(t, "/api/v1/repos/alice/project/pulls/3/reviews", r.URL.Path) require.NoError(t, json.NewDecoder(r.Body).Decode(&createPayload)) @@ -32,12 +37,15 @@ func TestPullReviewImplicitInteractiveForm(t *testing.T) { })) defer server.Close() calls := 0 - prompt := &scriptedPrompter{accept: true} + prompt := &askingPrompter{scriptedPrompter: scriptedPrompter{accept: true}} deps, out, diagnostics := serverDeps(t, server.URL) + deps.Getenv = serverEnv(server.URL, map[string]string{"EDITOR": "vi"}) deps.ReviewForm = reviewFormFunc(func(_ context.Context, choices terminal.ReviewFormChoices) (terminal.ReviewFormValues, error) { calls++ + require.Equal(t, `Reviewing #3 "Add one" (feat/one → main) in alice/project`, choices.Header) + require.False(t, choices.Author) + require.Equal(t, "vi", choices.Editor) require.NotNil(t, choices.ReviewDiff) - require.NotNil(t, choices.EditBody) return terminal.ReviewFormValues{ Event: "APPROVED", Body: "Looks good", Comments: []terminal.ReviewCommentDraft{{Path: "sample.go", Body: "nice", NewLine: 2}}, @@ -47,7 +55,8 @@ func TestPullReviewImplicitInteractiveForm(t *testing.T) { deps.Streams.InTTY, deps.Streams.ErrTTY = true, true require.NoError(t, Execute(context.Background(), deps, []string{"pr", "review", "3", "-R", "alice/project", "--json=id,state", "--interactive"})) require.Equal(t, 1, calls) - require.Len(t, prompt.confirmations, 1) + require.Equal(t, []string{"Submit approval for #3?"}, prompt.confirmations) + require.Equal(t, []bool{true}, prompt.defaults) require.Equal(t, "APPROVED", createPayload["event"]) require.Equal(t, "Looks good", createPayload["body"]) comments, ok := createPayload["comments"].([]any) @@ -57,10 +66,36 @@ func TestPullReviewImplicitInteractiveForm(t *testing.T) { require.Empty(t, diagnostics.String()) } +func TestPullReviewFormKeepsTheBodyWhenSubmissionIsDeclined(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodGet, r.Method, "nothing may be submitted") + _, _ = w.Write([]byte(`{"number":3,"title":"Add one","user":{"login":"alice"},"head":{"ref":"feat/one"},"base":{"ref":"main"}}`)) + })) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + deps.ReviewForm = reviewFormFunc(func(_ context.Context, choices terminal.ReviewFormChoices) (terminal.ReviewFormValues, error) { + require.True(t, choices.Author) + require.Nil(t, choices.ReviewDiff, "annotating the diff needs an editor") + return terminal.ReviewFormValues{Event: "COMMENT", Body: "Self note"}, nil + }) + deps.Prompter = &askingPrompter{} + deps.Streams.InTTY, deps.Streams.ErrTTY = true, true + err := Execute(context.Background(), deps, []string{"pr", "review", "3", "-R", "alice/project"}) + require.ErrorIs(t, err, terminal.ErrCanceled) + path := regexp.MustCompile(`draft saved at (\S+)`).FindStringSubmatch(err.Error()) + require.Len(t, path, 2, err.Error()) + data, readErr := os.ReadFile(path[1]) + require.NoError(t, readErr) + require.Equal(t, "Self note", string(data)) + require.NoError(t, os.Remove(path[1])) +} + func TestPullReviewInteractiveDiffRoundTrip(t *testing.T) { var createPayload map[string]any server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls/4": + _, _ = w.Write([]byte(`{"number":4,"title":"Add","user":{"login":"bob"}}`)) case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls/4.diff": _, _ = w.Write([]byte(`diff --git a/sample.go b/sample.go --- a/sample.go @@ -100,6 +135,10 @@ func TestPullReviewInteractiveDiffRoundTrip(t *testing.T) { func TestPullReviewDiffFailureKeepsAnnotatedDiff(t *testing.T) { server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/repos/alice/project/pulls/4" { + _, _ = w.Write([]byte(`{"number":4,"title":"Add","user":{"login":"bob"}}`)) + return + } require.Equal(t, "/api/v1/repos/alice/project/pulls/4.diff", r.URL.Path, "no review may be submitted") _, _ = w.Write([]byte("diff --git a/sample.go b/sample.go\n")) })) diff --git a/internal/app/pull_review_lifecycle.go b/internal/app/pull_review_lifecycle.go index 531002ae..6e16b49f 100644 --- a/internal/app/pull_review_lifecycle.go +++ b/internal/app/pull_review_lifecycle.go @@ -6,6 +6,7 @@ package app import ( "errors" "fmt" + "strconv" "strings" "gitea.dev/tea/internal/clierr" @@ -14,131 +15,209 @@ import ( "github.com/spf13/cobra" ) -func (a *application) pullReviewLifecycleCommand(verb string) *cobra.Command { - var body, file, event string +func (a *application) pullReviewSubmitCommand() *cobra.Command { + var kind reviewType + var body, file string var useEditor bool - cmd := &cobra.Command{Use: verb + " ", Short: titleCase(verb) + " a pull request review", Args: argsExact(2), RunE: func(cmd *cobra.Command, args []string) (runErr error) { + cmd := &cobra.Command{Use: "submit [ | | ] []", Short: "Submit your pending review", Annotations: pullArgumentsAnnotation(nil), Args: usageArgs(cobra.MaximumNArgs(2)), RunE: func(cmd *cobra.Command, args []string) (runErr error) { var draft terminal.Draft defer finishDraft(&draft, &runErr) if err := validateBodySources(cmd); err != nil { return err } - n, err := number(args[0]) + state, err := kind.state() if err != nil { return err } - id, err := number(args[1]) + if state == "" { + return errReviewType(cmd) + } + missingBody := clierr.Wrap(clierr.Usage, errors.New("a change request needs a body; pass --body, --body-file or --editor")) + var supplied *string + if cmd.Flags().Changed("body") || cmd.Flags().Changed("body-file") { + if body, err = a.readBody(cmd, body, file); err != nil { + return err + } + if state == "REQUEST_CHANGES" && strings.TrimSpace(body) == "" { + return missingBody + } + supplied = &body + } + var id int64 + if len(args) == 2 { + if id, err = number(args[1]); err != nil { + return err + } + } + t, n, err := a.pullSelector(cmd, args[:min(len(args), 1)]) if err != nil { return err } - if verb == "submit" { - if err := oneOf("event", event, "APPROVED", "REQUEST_CHANGES", "COMMENT"); err != nil { - return err - } - } - var suppliedBody *string - if verb == "submit" || verb == "dismiss" { - body, err = a.readBody(cmd, body, file) - if err != nil { - return err - } - if cmd.Flags().Changed("body") || cmd.Flags().Changed("body-file") { - suppliedBody = &body - } - if verb == "dismiss" { - if suppliedBody == nil && !useEditor { - if _, err := a.require(cmd, &body, "body"); err != nil { - return err - } - } - if strings.TrimSpace(body) == "" && !useEditor { - return clierr.Wrap(clierr.Usage, errors.New("supply a non-empty dismissal reason with --body or --body-file")) - } - } else if suppliedBody != nil && strings.TrimSpace(body) == "" && event == "REQUEST_CHANGES" { - return clierr.Wrap(clierr.Usage, errors.New("review body cannot be empty for REQUEST_CHANGES")) - } - } - target, err := a.resolve(cmd.Context(), true) + pull, err := (services.Pulls{Client: t.client, Repository: t.Repo}).View(cmd.Context(), n) if err != nil { return err } - service := services.Reviews{Client: target.client, Repository: target.Repo} - if useEditor { - initial := "" - if verb == "submit" { - current, err := service.View(cmd.Context(), n, id) - if err != nil { - return err - } - initial = current.Body - } - draft, err = a.editDraft(cmd, initial) - if err != nil { - return err - } - body = draft.Body - suppliedBody = &body - if strings.TrimSpace(body) == "" && (verb == "dismiss" || event == "REQUEST_CHANGES") { - return clierr.Wrap(clierr.Usage, errors.New("supply non-empty review text in the editor")) - } + if err := ownReview(pull, t, state); err != nil { + return err } - if verb == "delete" || verb == "dismiss" { - message := fmt.Sprintf("%s review %d on %s#%d", verb, id, target.Repo, n) - if verb == "dismiss" { - message += ": " + body - } - if err := a.confirm(cmd, message); err != nil { + service := services.Reviews{Client: t.client, Repository: t.Repo} + if id == 0 { + if id, err = a.pendingReview(cmd, service, t, n, state); err != nil { return err } } - if verb == "submit" && event == "REQUEST_CHANGES" && suppliedBody == nil { + if useEditor || (supplied == nil && state == "REQUEST_CHANGES") { current, err := service.View(cmd.Context(), n, id) if err != nil { return err } - body = current.Body - if strings.TrimSpace(body) == "" { - body = "" - if _, err := a.require(cmd, &body, "body"); err != nil { - return err - } - if strings.TrimSpace(body) == "" { - return clierr.Wrap(clierr.Usage, errors.New("review body cannot be empty for REQUEST_CHANGES")) - } - if err := a.confirm(cmd, fmt.Sprintf("Submit REQUEST_CHANGES review on %s#%d: %s", target.Repo, n, body)); err != nil { + supplied = ¤t.Body + if useEditor { + if draft, err = a.editDraft(cmd, current.Body); err != nil { return err } + supplied = &draft.Body } - suppliedBody = &body } - if verb == "delete" { - if err := service.Delete(cmd.Context(), n, id); err != nil { - return err - } - _, err := fmt.Fprintf(cmd.ErrOrStderr(), "Deleted review %d\n", id) - return err - } - var item services.Review - if verb == "submit" { - item, err = service.Submit(cmd.Context(), n, id, services.ReviewSubmitOptions{Event: event, Body: suppliedBody}) - } else { - item, err = service.Dismiss(cmd.Context(), n, id, verb == "dismiss", body) + if state == "REQUEST_CHANGES" && strings.TrimSpace(*supplied) == "" { + return missingBody } + item, err := service.Submit(cmd.Context(), n, id, services.ReviewSubmitOptions{Event: state, Body: supplied}) if err != nil { return err } - return writeRecord(a, cmd, item, reviewRecord) + return a.reportReview(cmd, t, pull, item) }} - if verb == "submit" || verb == "dismiss" { - cmd.Flags().BoolVar(&useEditor, "editor", false, "Edit review text in the configured editor") - cmd.Flags().StringVar(&body, "body", "", "Review body or dismissal reason (submission preserves draft body when omitted)") - cmd.Flags().StringVar(&file, "body-file", "", "Read body from file, or - for stdin") - } - if verb == "submit" { - cmd.Flags().StringVar(&event, "event", "COMMENT", "Review event: APPROVED, REQUEST_CHANGES, COMMENT") - } - if verb == "delete" { - cmd.Annotations = textOutput("review deletion does not return a resource") - } + kind.register(cmd, false) + cmd.Flags().StringVarP(&body, "body", "b", "", "Review body (default: the body of the pending review)") + cmd.Flags().StringVarP(&file, "body-file", "F", "", "Read the body from `file`, or - for stdin") + cmd.Flags().BoolVarP(&useEditor, "editor", "e", false, "Edit the body of the pending review in your editor") + cmd.Example = `# Start a pending review, then submit it as an approval +$ tea pr review 13 --pending -b "First pass" --comments-file inline.json +$ tea pr review submit 13 --approve + +# Submit a pending review by its ID +$ tea pr review submit 13 21 --request-changes -b "See the inline comments"` + cmd.ValidArgsFunction = a.completeReviewArgs(false) + return cmd +} + +// pendingReview finds the pending review that the account started on pull request n. +func (a *application) pendingReview(cmd *cobra.Command, service services.Reviews, t target, n int64, state string) (int64, error) { + pending, err := service.Pending(cmd.Context(), n, t.login()) + if err != nil { + return 0, err + } + switch len(pending) { + case 0: + flag := map[string]string{"APPROVED": "--approve", "REQUEST_CHANGES": "--request-changes", "COMMENT": "--comment"}[state] + return 0, fmt.Errorf("no pending review by %s on %s#%d; to post a new review run: tea pr review %d %s", t.login(), t.Repo, n, n, flag) + case 1: + return pending[0].ID, nil + } + ids := make([]string, len(pending)) + for i, review := range pending { + ids[i] = strconv.FormatInt(review.ID, 10) + } + return 0, clierr.Wrap(clierr.Usage, fmt.Errorf("%s has several pending reviews on %s#%d (%s); pass a review ID", t.login(), t.Repo, n, strings.Join(ids, ", "))) +} + +func (a *application) pullReviewDeleteCommand() *cobra.Command { + cmd := &cobra.Command{Use: "delete ", Short: "Delete a review", Annotations: pullArgumentsAnnotation(textOutput("review deletion does not return a resource")), Args: argsExact(2), RunE: func(cmd *cobra.Command, args []string) error { + t, n, id, err := a.pullAndID(cmd, args) + if err != nil { + return err + } + service := services.Reviews{Client: t.client, Repository: t.Repo} + item, err := service.View(cmd.Context(), n, id) + if err != nil { + return err + } + subject := fmt.Sprintf("review %d by %s on %s#%d", id, item.Author, t.Repo, n) + if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: "Delete " + subject + "?", NonTTY: "delete " + subject}); err != nil { + return err + } + if err := service.Delete(cmd.Context(), n, id); err != nil { + return err + } + a.done(cmd, "Deleted %s", subject) + return nil + }} + cmd.Annotations["tea.confirm"] = "true" + cmd.Example = "$ tea pr review delete 13 21" + cmd.ValidArgsFunction = a.completeReviewArgs(false) + return cmd +} + +func (a *application) pullReviewDismissCommand(dismiss bool) *cobra.Command { + var body, file string + var useEditor bool + use, short := "undismiss ", "Restore a dismissed review" + if dismiss { + use, short = "dismiss ", "Dismiss an approval or change request" + } + cmd := &cobra.Command{Use: use, Short: short, Annotations: pullArgumentsAnnotation(nil), Args: argsExact(2), RunE: func(cmd *cobra.Command, args []string) (runErr error) { + var draft terminal.Draft + defer finishDraft(&draft, &runErr) + var err error + if dismiss { + if err := validateBodySources(cmd); err != nil { + return err + } + switch { + case useEditor: + draft, err = a.editDraft(cmd, "") + body = draft.Body + case cmd.Flags().Changed("body") || cmd.Flags().Changed("body-file"): + body, err = a.readBody(cmd, body, file) + default: + _, err = a.requireInput(cmd, &body, "body", "Reason for dismissing the review", nil) + } + if err != nil { + return err + } + if strings.TrimSpace(body) == "" { + return clierr.Wrap(clierr.Usage, errors.New("a dismissal needs a reason; pass --body, --body-file or --editor")) + } + } + t, n, id, err := a.pullAndID(cmd, args) + if err != nil { + return err + } + service := services.Reviews{Client: t.client, Repository: t.Repo} + current, err := service.View(cmd.Context(), n, id) + if err != nil { + return err + } + subject := fmt.Sprintf("%s on %s#%d", reviewPossessive(current), t.Repo, n) + if dismiss { + if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: "Dismiss " + subject + "?", NonTTY: "dismiss " + subject}); err != nil { + return err + } + } + item, err := service.Dismiss(cmd.Context(), n, id, dismiss, body) + if err != nil { + return err + } + if dismiss { + a.done(cmd, "Dismissed %s", subject) + } else { + a.done(cmd, "Restored %s", subject) + } + if a.options.json != "" { + return a.writeJSON(cmd, item) + } + return nil + }} + if dismiss { + cmd.Annotations["tea.confirm"] = "true" + cmd.Flags().StringVarP(&body, "body", "b", "", "Reason for dismissing the review (required)") + cmd.Flags().StringVarP(&file, "body-file", "F", "", "Read the reason from `file`, or - for stdin") + cmd.Flags().BoolVarP(&useEditor, "editor", "e", false, "Write the reason in your editor") + cmd.Example = `$ tea pr review dismiss 13 6 -b "Approved an outdated revision"` + } else { + cmd.Example = "$ tea pr review undismiss 13 6" + } + cmd.ValidArgsFunction = a.completeReviewArgs(false) return cmd } diff --git a/internal/app/pull_review_requests.go b/internal/app/pull_review_requests.go index 88194d1d..15a08f7b 100644 --- a/internal/app/pull_review_requests.go +++ b/internal/app/pull_review_requests.go @@ -4,22 +4,20 @@ package app import ( + "strings" + "gitea.dev/tea/internal/clierr" "gitea.dev/tea/internal/services" "github.com/spf13/cobra" ) func (a *application) pullReviewRequestCommand(add bool) *cobra.Command { - verb := "cancel-request" + use, short, verb := "cancel-request [ | | ]", "Withdraw review requests", "Withdrew review request from" if add { - verb = "request" + use, short, verb = "request [ | | ]", "Request reviews from users or teams", "Requested review from" } var input services.ReviewRequestInput - cmd := &cobra.Command{Use: verb + " ", Short: titleCase(verb) + " reviews from users or organization teams", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { - n, err := number(args[0]) - if err != nil { - return err - } + cmd := &cobra.Command{Use: use, Short: short, Annotations: pullArgumentsAnnotation(nil), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { trimNames(input.Users, input.Teams) changes := services.ReviewRequestChanges{} if add { @@ -30,17 +28,34 @@ func (a *application) pullReviewRequestCommand(add bool) *cobra.Command { if err := changes.Validate(); err != nil { return clierr.Wrap(clierr.Usage, err) } - target, err := a.resolve(cmd.Context(), true) + t, n, err := a.pullSelector(cmd, args) if err != nil { return err } - item, err := (services.Reviews{Client: target.client, Repository: target.Repo}).ChangeRequests(cmd.Context(), n, changes) + t.expandMeAll(input.Users) + item, err := (services.Reviews{Client: t.client, Repository: t.Repo}).ChangeRequests(cmd.Context(), n, changes) if err != nil { return err } - return writeRecord(a, cmd, item, pullRecord) + names := append([]string{}, input.Users...) + for _, team := range input.Teams { + names = append(names, "team "+team) + } + a.done(cmd, "%s %s on %s", verb, strings.Join(names, ", "), refLabel(t.Repo, item.Number, item.Title)) + if a.options.json != "" { + return a.writeJSON(cmd, item) + } + return nil }} - cmd.Flags().StringSliceVar(&input.Users, "reviewer", nil, "Reviewer usernames or email addresses") - cmd.Flags().StringSliceVar(&input.Teams, "team", nil, "Team names within the repository's organization") + cmd.Flags().StringSliceVarP(&input.Users, "reviewer", "r", nil, "Reviewer usernames or email addresses; @me for yourself") + cmd.Flags().StringSliceVar(&input.Teams, "team", nil, "Teams of the repository's organization") + _ = cmd.RegisterFlagCompletionFunc("reviewer", a.completeUsers()) + cmd.ValidArgsFunction = a.completePulls("open") + if add { + cmd.Example = `$ tea pr review request 13 --reviewer carol,dave +$ tea pr review request --team developers` + } else { + cmd.Example = "$ tea pr review cancel-request 13 --reviewer carol" + } return cmd } diff --git a/internal/app/pull_review_test.go b/internal/app/pull_review_test.go index 0764a9a7..8e1daca6 100644 --- a/internal/app/pull_review_test.go +++ b/internal/app/pull_review_test.go @@ -6,16 +6,23 @@ package app import ( "context" "encoding/json" + "fmt" "net/http" "os" "path/filepath" "strings" "testing" + "time" "gitea.dev/tea/internal/clierr" "github.com/stretchr/testify/require" ) +// pullJSON answers GET /pulls/N for review tests: pull request 13 "Add login" by author. +func pullJSON(author string) string { + return fmt.Sprintf(`{"number":13,"title":"Add login","user":{"login":%q},"head":{"ref":"feat/login"},"base":{"ref":"main"}}`, author) +} + func TestReviewReplyPostsBodyFromStdin(t *testing.T) { mutations := 0 server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -41,6 +48,10 @@ func TestReviewReplyPostsBodyFromStdin(t *testing.T) { func TestReviewPromptAndFlagsReachIdenticalOptions(t *testing.T) { var payloads []map[string]any server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/owner/repo/pulls/1" { + _, _ = w.Write([]byte(`{"number":1,"title":"Add login","user":{"login":"bob"}}`)) + return + } require.Equal(t, http.MethodPost, r.Method) require.Equal(t, "/api/v1/repos/owner/repo/pulls/1/reviews", r.URL.Path) var payload map[string]any @@ -50,22 +61,191 @@ func TestReviewPromptAndFlagsReachIdenticalOptions(t *testing.T) { })) defer server.Close() deps, out, diagnostics := serverDeps(t, server.URL) - prompt := &scriptedPrompter{input: "Requested changes", accept: true} + prompt := &askingPrompter{} deps.Prompter = prompt deps.Streams.InTTY, deps.Streams.ErrTTY = true, true args := []string{"pr", "review", "1", "-R", "owner/repo", "--event", "REQUEST_CHANGES"} require.NoError(t, Execute(context.Background(), deps, args)) - require.Len(t, prompt.asked, 1) - require.Len(t, prompt.confirmations, 1) + require.Equal(t, []string{"Body (markdown): "}, prompt.texts) + require.Empty(t, prompt.confirmations, "no confirmation right after typing the body") + require.Equal(t, "https://example.test/review/7\n", out.String()) out.Reset() - require.NoError(t, Execute(context.Background(), deps, append(args, "--body", "Requested changes", "--json=id,author,url"))) - require.Len(t, prompt.asked, 1) - require.Len(t, prompt.confirmations, 1) + require.NoError(t, Execute(context.Background(), deps, append(args, "--body", "typed body", "--json=id,author,url"))) + require.Len(t, prompt.texts, 1) require.Equal(t, payloads[0], payloads[1]) require.JSONEq(t, `{"id":7,"author":"alice","url":"https://example.test/review/7"}`, out.String()) require.Empty(t, diagnostics.String()) } +func TestReviewTypeFlags(t *testing.T) { + for _, tc := range []struct { + args []string + event, body string + }{ + {[]string{"--approve"}, "APPROVED", ""}, + {[]string{"-a", "-b", "LGTM"}, "APPROVED", "LGTM"}, + {[]string{"-r", "--body", "Hash it"}, "REQUEST_CHANGES", "Hash it"}, + {[]string{"-c", "-b", "Nit"}, "COMMENT", "Nit"}, + {[]string{"--pending", "-b", "Draft"}, "PENDING", "Draft"}, + {[]string{"--event", "approve"}, "APPROVED", ""}, + {[]string{"--event", "APPROVED"}, "APPROVED", ""}, + {[]string{"--event", "request_changes", "-b", "x"}, "REQUEST_CHANGES", "x"}, + {[]string{"--event", "Comment", "-b", "x"}, "COMMENT", "x"}, + } { + t.Run(strings.Join(tc.args, " "), func(t *testing.T) { + var payload map[string]any + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + require.Equal(t, "/api/v1/repos/alice/x/pulls/13", r.URL.Path) + _, _ = w.Write([]byte(pullJSON("bob"))) + return + } + require.Equal(t, "/api/v1/repos/alice/x/pulls/13/reviews", r.URL.Path) + require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) + _, _ = fmt.Fprintf(w, `{"id":7,"state":%q,"html_url":"http://gitea/alice/x/pulls/13#pullrequestreview-7"}`, payload["event"]) + })) + defer server.Close() + deps, out, diagnostics := serverDeps(t, server.URL) + deps.Getenv = serverEnv(server.URL, map[string]string{"NO_COLOR": "1"}) + deps.Streams.OutTTY = true + require.NoError(t, Execute(context.Background(), deps, append([]string{"pr", "review", "13", "-R", "alice/x"}, tc.args...))) + require.Equal(t, tc.event, payload["event"]) + require.Equal(t, tc.body, payload["body"]) + require.Equal(t, "http://gitea/alice/x/pulls/13#pullrequestreview-7\n", out.String()) + require.Equal(t, "✓ "+reviewVerbs[tc.event]+" pull request alice/x#13 (Add login)\n", diagnostics.String()) + }) + } +} + +func TestReviewCreateIsTheReviewCommand(t *testing.T) { + var event any + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(pullJSON("bob"))) + return + } + var payload map[string]any + require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) + event = payload["event"] + _, _ = w.Write([]byte(`{"id":7,"state":"APPROVED"}`)) + })) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "review", "create", "#13", "-R", "alice/x", "-a"})) + require.Equal(t, "APPROVED", event) +} + +func TestReviewUsageErrors(t *testing.T) { + for _, tc := range []struct { + args []string + want string + }{ + {[]string{"pr", "review", "13"}, "choose a review type: --approve, --request-changes, or --comment\nUsage: tea pr review [ | | ] [flags]\nRun 'tea pr review --help' for examples."}, + {[]string{"pr", "review", "create", "13"}, "Run 'tea pr review create --help' for examples."}, + {[]string{"pr", "review", "13", "--body", "x"}, "choose a review type"}, + {[]string{"pr", "review", "13", "-a", "-c"}, "choose one review type"}, + {[]string{"pr", "review", "13", "--approve", "--event", "comment"}, "choose one review type"}, + {[]string{"pr", "review", "13", "--event", "bogus"}, "--event must be"}, + {[]string{"pr", "review", "approve", "13"}, `unknown review command "approve"; to approve run: tea pr review 13 --approve (see tea pr review --help)`}, + {[]string{"pr", "review", "reject"}, `to request changes run: tea pr review --request-changes`}, + {[]string{"pr", "review", "lst", "13"}, `unknown command "lst" for "tea pr review"; did you mean list?`}, + {[]string{"pr", "review", "13", "14", "-a"}, "too many arguments"}, + {[]string{"pr", "review", "submit", "13"}, "choose a review type"}, + } { + t.Run(strings.Join(tc.args, " "), func(t *testing.T) { + requireUsageBeforeRequests(t, tc.want, tc.args...) + }) + } +} + +func TestReviewRefusesOwnApprovalBeforeTheEditor(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodGet, r.Method, "no review may be submitted") + _, _ = w.Write([]byte(pullJSON("alice"))) + })) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + editor := &failingPathEditor{} + deps.Editor = editor + err := Execute(context.Background(), deps, []string{"pr", "review", "13", "-R", "alice/x", "--approve", "--editor"}) + require.Equal(t, clierr.Usage, clierr.ExitCode(err)) + require.EqualError(t, err, "you can't approve or request changes on your own pull request #13; use --comment") +} + +func TestReviewNeedsABodyWithoutPrompts(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodGet, r.Method, "no review may be submitted") + _, _ = w.Write([]byte(pullJSON("bob"))) + })) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + err := Execute(context.Background(), deps, []string{"pr", "review", "13", "-R", "alice/x", "--request-changes"}) + require.Equal(t, clierr.Usage, clierr.ExitCode(err)) + require.EqualError(t, err, "a change request needs a body; pass --body, --body-file or --editor") +} + +func TestReviewSubmitFindsThePendingReview(t *testing.T) { + for _, tc := range []struct { + name, reviews, want string + }{ + {"one", `[{"id":5,"state":"APPROVED","user":{"login":"alice"}},{"id":6,"state":"PENDING","user":{"login":"bob"}},{"id":7,"state":"PENDING","user":{"login":"alice"}}]`, ""}, + {"none", `[{"id":6,"state":"PENDING","user":{"login":"bob"}}]`, "no pending review by alice on alice/x#13; to post a new review run: tea pr review 13 --approve"}, + {"several", `[{"id":7,"state":"PENDING","user":{"login":"alice"}},{"id":8,"state":"PENDING","user":{"login":"alice"}}]`, "alice has several pending reviews on alice/x#13 (7, 8); pass a review ID"}, + } { + t.Run(tc.name, func(t *testing.T) { + submitted := "" + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/api/v1/repos/alice/x/pulls/13": + _, _ = w.Write([]byte(pullJSON("bob"))) + case r.URL.Path == "/api/v1/repos/alice/x/pulls/13/reviews" && r.URL.Query().Get("page") == "1": + _, _ = w.Write([]byte(tc.reviews)) + case r.URL.Path == "/api/v1/repos/alice/x/pulls/13/reviews": + _, _ = w.Write([]byte(`[]`)) + case r.URL.Path == "/api/v1/repos/alice/x/pulls/13/reviews/7" && r.Method == http.MethodGet: + _, _ = w.Write([]byte(`{"id":7,"state":"PENDING","body":"First pass"}`)) + case r.URL.Path == "/api/v1/repos/alice/x/pulls/13/reviews/7": + var payload map[string]any + require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) + submitted = fmt.Sprint(payload["event"], " ", payload["body"]) + _, _ = w.Write([]byte(`{"id":7,"state":"APPROVED"}`)) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL) + } + })) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + err := Execute(context.Background(), deps, []string{"pr", "review", "submit", "13", "-R", "alice/x", "--approve"}) + if tc.want != "" { + require.EqualError(t, err, tc.want) + return + } + require.NoError(t, err) + require.Equal(t, "APPROVED First pass", submitted) + }) + } +} + +func TestReviewListTable(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/api/v1/repos/alice/x/pulls/13/reviews", r.URL.Path) + _, _ = w.Write([]byte(`[{"id":5,"state":"COMMENT","body":"Self note","user":{"login":"alice"},"submitted_at":"2026-10-11T10:00:00Z"}, + {"id":6,"state":"APPROVED","dismissed":true,"user":{"login":"bob"},"submitted_at":"2026-10-11T11:00:00Z","comments_count":2}]`)) + })) + defer server.Close() + deps, out, _ := serverDeps(t, server.URL) + deps.Getenv = serverEnv(server.URL, map[string]string{"NO_COLOR": "1"}) + deps.Streams.OutTTY = true + deps.Now = func() time.Time { return time.Date(2026, 10, 11, 12, 0, 0, 0, time.UTC) } + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "review", "list", "13", "-R", "alice/x"})) + require.Equal(t, `Showing 2 of 2 reviews on alice/x#13 + +ID REVIEWER STATE SUBMITTED COMMENTS BODY +5 alice Commented about 2 hours ago 0 Self note +6 bob Approved (dismissed) about 1 hour ago 2 +`, out.String()) +} + func TestReviewCommentsFileValidatedBeforeRequests(t *testing.T) { invalidPosition := "each inline comment requires path, body, and exactly one positive old_position or new_position" for _, tc := range []struct{ content, want string }{ @@ -78,23 +258,32 @@ func TestReviewCommentsFileValidatedBeforeRequests(t *testing.T) { t.Run(tc.content, func(t *testing.T) { file := filepath.Join(t.TempDir(), "comments.json") require.NoError(t, os.WriteFile(file, []byte(tc.content), 0o600)) - requireUsageBeforeRequests(t, tc.want, "pr", "review", "1", "--comments-file", file, "--body", "Supplied") + requireUsageBeforeRequests(t, tc.want, "pr", "review", "1", "--comments-file", file, "--comment", "--body", "Supplied") }) } } func TestReviewDeleteAndDismissRequireYesWithoutInput(t *testing.T) { - server := newAuthenticatedServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { t.Error("mutation before confirmation") })) + mutations := 0 + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + mutations++ + } + _, _ = w.Write([]byte(`{"id":2,"state":"APPROVED","user":{"login":"bob"}}`)) + })) defer server.Close() - for _, verb := range []string{"delete", "dismiss"} { + for _, tc := range []struct{ verb, want string }{ + {"delete", "confirmation required to delete review 2 by bob on owner/repo#1; rerun with --yes"}, + {"dismiss", "confirmation required to dismiss bob's approval on owner/repo#1; rerun with --yes"}, + } { deps, _, _ := serverDeps(t, server.URL) - args := []string{"pr", "review", verb, "1", "2", "-R", "owner/repo", "--no-input"} - if verb == "dismiss" { + args := []string{"pr", "review", tc.verb, "1", "2", "-R", "owner/repo", "--no-input"} + if tc.verb == "dismiss" { args = append(args, "--body", "Reason") } err := Execute(context.Background(), deps, args) - require.Error(t, err) require.Equal(t, clierr.Usage, clierr.ExitCode(err)) - require.ErrorContains(t, err, "--yes") + require.EqualError(t, err, tc.want) } + require.Zero(t, mutations) } diff --git a/internal/app/validation_test.go b/internal/app/validation_test.go index a575841b..e0858133 100644 --- a/internal/app/validation_test.go +++ b/internal/app/validation_test.go @@ -131,14 +131,15 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"pr", "list", "--milestone", "-1"}, "--milestone must be a nonnegative ID"}, {[]string{"pr", "list", "--state", "unknown"}, "--state must be open, closed, or all"}, {[]string{"pr", "list", "--limit", "0"}, `"-L, --limit" flag: must be 1..1000`}, - {[]string{"pr", "review", "1", "--no-input"}, "missing required input: supply --body"}, - {[]string{"pr", "review", "1", "--body="}, "review body cannot be empty without inline comments"}, - {[]string{"pr", "review", "1", "--event=unknown"}, "--event must be APPROVED, REQUEST_CHANGES, COMMENT, or PENDING"}, + {[]string{"pr", "review", "1", "--no-input"}, "choose a review type: --approve, --request-changes, or --comment"}, + {[]string{"pr", "review", "1", "--comment", "--body="}, "a comment needs a body; pass --body, --body-file or --editor"}, + {[]string{"pr", "review", "1", "--request-changes", "--no-input"}, "a change request needs a body"}, + {[]string{"pr", "review", "1", "--event=unknown"}, "--event must be approve, approved, request-changes, request_changes, comment, or pending"}, {[]string{"pr", "review", "request", "1", "--team", " "}, "team names cannot be empty"}, {[]string{"pr", "review", "1", "--comments-file=-"}, "--comments-file requires a file"}, - {[]string{"pr", "review", "submit", "1", "2", "--event=PENDING"}, "--event must be APPROVED, REQUEST_CHANGES, or COMMENT"}, - {[]string{"pr", "review", "submit", "1", "2", "--event=REQUEST_CHANGES", "--body="}, "review body cannot be empty for REQUEST_CHANGES"}, - {[]string{"pr", "review", "dismiss", "1", "2", "--body="}, "supply a non-empty dismissal reason with --body or --body-file"}, + {[]string{"pr", "review", "submit", "1", "2", "--event=PENDING"}, "--event must be approve, approved, request-changes, request_changes, or comment"}, + {[]string{"pr", "review", "submit", "1", "2", "--event=REQUEST_CHANGES", "--body="}, "a change request needs a body"}, + {[]string{"pr", "review", "dismiss", "1", "2", "--body="}, "a dismissal needs a reason"}, {[]string{"pr", "review", "dismiss", "1", "2", "--no-input", "--yes"}, "missing required input: supply --body"}, {[]string{"pr", "review", "delete", "1", "2", "--json"}, "review deletion does not return a resource"}, {[]string{"pr", "status", "--limit=101"}, `"--limit" flag: must be 1..100`}, @@ -345,14 +346,18 @@ func TestExplicitValuesSkipInteractiveForms(t *testing.T) { {[]string{"issue", "create", "--title", "Explicit"}, http.MethodPost, "/api/v1/repos/alice/project/issues", "number"}, {[]string{"issue", "edit", "1", "--title", "Explicit"}, http.MethodPatch, "/api/v1/repos/alice/project/issues/1", "number"}, {[]string{"pr", "create", "--title", "Explicit", "--head", "feature", "--base", "main"}, http.MethodPost, "/api/v1/repos/alice/project/pulls", "number"}, - {[]string{"pr", "review", "1", "--body", "Explicit"}, http.MethodPost, "/api/v1/repos/alice/project/pulls/1/reviews", "id"}, + {[]string{"pr", "review", "1", "--comment", "--body", "Explicit"}, http.MethodPost, "/api/v1/repos/alice/project/pulls/1/reviews", "id"}, } { t.Run(strings.Join(tc.args[:2], " "), func(t *testing.T) { mutations := 0 server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project" { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project": _, _ = w.Write([]byte(`{"full_name":"alice/project","default_branch":"main"}`)) return + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls/1": + _, _ = w.Write([]byte(`{"number":1,"user":{"login":"bob"}}`)) + return } require.Equal(t, tc.method, r.Method) require.Equal(t, tc.path, r.URL.Path) diff --git a/internal/services/reviews.go b/internal/services/reviews.go index 0596e70f..a19c1fc6 100644 --- a/internal/services/reviews.go +++ b/internal/services/reviews.go @@ -103,6 +103,21 @@ func (s Reviews) List(ctx context.Context, n int64, limit, page int) ([]Review, return convertAll(reviews, reviewFrom), nil } +// Pending returns the reviews that login started on pull request n and has not submitted yet. +func (s Reviews) Pending(ctx context.Context, n int64, login string) ([]Review, error) { + reviews, err := listAll(func(page int) ([]Review, error) { return s.List(ctx, n, 100, page) }) + if err != nil { + return nil, err + } + pending := []Review{} + for _, review := range reviews { + if review.State == string(gitea.ReviewStatePending) && strings.EqualFold(review.Author, login) { + pending = append(pending, review) + } + } + return pending, nil +} + // View retrieves a review belonging to the selected pull request. func (s Reviews) View(ctx context.Context, n, id int64) (Review, error) { return repoConvert(ctx, s.Client, s.Repository, reviewFrom, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullReview, *gitea.Response, error) { diff --git a/internal/terminal/review_form.go b/internal/terminal/review_form.go index 6c18563c..4fd73872 100644 --- a/internal/terminal/review_form.go +++ b/internal/terminal/review_form.go @@ -5,6 +5,7 @@ package terminal import ( "context" + "errors" "fmt" "strings" @@ -17,19 +18,25 @@ type ReviewCommentDraft struct { OldLine, NewLine int64 } -// ReviewFormChoices supplies callbacks for interactive pull review. +// ReviewFormChoices describes the pull request under review and how its review is collected. type ReviewFormChoices struct { - // ReviewDiff, when non-nil, runs after the user opts into annotating the diff. + // Header introduces the form, such as `Reviewing #1 "Add one.txt" (feat/one → main) in alice/x`. + Header string + // Author leaves out approving and requesting changes, which Gitea refuses on one's own pull request. + Author bool + // Body seeds the review body. + Body string + // Editor is the configured editor command that ctrl+e opens on the body; empty disables it. + Editor string + // ReviewDiff, when non-nil, offers to annotate the diff in the editor after the body. ReviewDiff func(context.Context) ([]ReviewCommentDraft, error) - EditBody BodyEditor } -// ReviewFormValues holds interactively collected review fields. +// ReviewFormValues holds an interactively collected review: its API state, body and inline comments. type ReviewFormValues struct { Event string Body string Comments []ReviewCommentDraft - Draft Draft } // ReviewForm collects interactive pull review input. @@ -37,58 +44,65 @@ type ReviewForm interface { CreateReview(context.Context, ReviewFormChoices) (ReviewFormValues, error) } -// CreateReview runs the interactive review form on stderr. +// errEmptyComment rejects a comment review with neither a body nor inline comments. +var errEmptyComment = errors.New("a comment review needs a body or inline comments") + +// CreateReview asks for the review type, then the body, then whether to annotate the diff, on stderr. func (p *FormPrompter) CreateReview(ctx context.Context, choices ReviewFormChoices) (ReviewFormValues, error) { - var comments []ReviewCommentDraft - if choices.ReviewDiff != nil { - reviewDiff := true - if err := p.runForm(ctx, huh.NewForm(huh.NewGroup( - huh.NewConfirm().Title("Review / comment the diff?").Value(&reviewDiff), - ))); err != nil { - return ReviewFormValues{}, err - } - if reviewDiff { - parsed, err := choices.ReviewDiff(ctx) - if err != nil { - return ReviewFormValues{}, fmt.Errorf("diff review failed: %w", err) - } - comments = append(comments, parsed...) - fmt.Fprintf(p.Streams.ErrOut, "Found %d code comments in your review\n", len(comments)) - } + if choices.Header != "" { + fmt.Fprintln(p.Streams.ErrOut, SafeLine(choices.Header)) } - - event := "COMMENT" + options := []huh.Option[string]{huh.NewOption("Comment", "COMMENT")} + if !choices.Author { + options = append([]huh.Option[string]{huh.NewOption("Approve", "APPROVED"), huh.NewOption("Request changes", "REQUEST_CHANGES")}, options...) + } + values := ReviewFormValues{Event: options[0].Value, Body: choices.Body} if err := p.runForm(ctx, huh.NewForm(huh.NewGroup( - huh.NewSelect[string]().Title("Your assessment:").Options( - huh.NewOption("comment", "COMMENT"), - huh.NewOption("request changes", "REQUEST_CHANGES"), - huh.NewOption("approve", "APPROVED"), - ).Value(&event), + huh.NewSelect[string]().Title("Review type:").Options(options...).Value(&values.Event), ))); err != nil { - return ReviewFormValues{}, err + return values, err } - body := "" - values := ReviewFormValues{Event: event, Comments: comments} - required := (event == "COMMENT" && len(comments) == 0) || event == "REQUEST_CHANGES" - if choices.EditBody != nil { - var err error - if body, values.Draft, err = editBody(ctx, choices.EditBody, ""); err != nil { + // A comment can consist of inline comments alone, which are collected after the body. + required := values.Event == "REQUEST_CHANGES" || (values.Event == "COMMENT" && choices.ReviewDiff == nil) + title := "Body (markdown, optional):" + if required { + title = "Body (markdown, required):" + } + args, editor := commandArgs(choices.Editor) + if editor { + title = strings.TrimSuffix(title, "):") + ", ctrl+e opens your editor):" + } + body := bodyField(title, &values.Body) + if editor { + body = body.ExternalEditor(true).Editor(args...).EditorExtension("md") + } + if required { + body = body.Validate(huh.ValidateNotEmpty()) + } + if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(body))); err != nil { + return values, err + } + values.Body = strings.TrimSpace(values.Body) + + if choices.ReviewDiff != nil { + annotate := false + if err := p.runForm(ctx, huh.NewForm(huh.NewGroup( + huh.NewConfirm().Title("Annotate the diff in your editor?").Value(&annotate), + ))); err != nil { return values, err } - } else { - field := bodyField("Concluding comment (markdown):", &body) - if required { - field = field.Title("Concluding comment (markdown, required):").Validate(huh.ValidateNotEmpty()) - } - if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(field))); err != nil { - return ReviewFormValues{}, err + if annotate { + comments, err := choices.ReviewDiff(ctx) + if err != nil { + return values, fmt.Errorf("diff review failed: %w", err) + } + values.Comments = comments + fmt.Fprintf(p.Streams.ErrOut, "Found %d inline comments in the annotated diff\n", len(comments)) } } - body = strings.TrimSpace(body) - if required && body == "" { - return values, fmt.Errorf("concluding comment cannot be empty") + if values.Event == "COMMENT" && values.Body == "" && len(values.Comments) == 0 { + return values, errEmptyComment } - values.Body = body return values, nil } diff --git a/internal/terminal/review_form_test.go b/internal/terminal/review_form_test.go index 50bc4a06..8b6185b0 100644 --- a/internal/terminal/review_form_test.go +++ b/internal/terminal/review_form_test.go @@ -6,29 +6,72 @@ package terminal import ( "context" "errors" + "strings" "testing" + "testing/iotest" "github.com/stretchr/testify/require" ) +// linePrompter is accessiblePrompter reading its input a byte at a time, so that every field of +// consecutive forms gets its own line. +func linePrompter(t *testing.T, input string) (*FormPrompter, *strings.Builder) { + t.Helper() + t.Setenv("TERM", "dumb") + p, _ := testPrompter(iotest.OneByteReader(strings.NewReader(input))) + rendered := &strings.Builder{} + p.Streams.ErrOut = rendered + return p, rendered +} + func TestCreateReviewAbortsWhenDiffReviewFails(t *testing.T) { failure := errors.New("editor failed") - p, _ := accessiblePrompter(t, "y\n1\n") + p, _ := linePrompter(t, "1\nLooks good\ny\n") _, err := p.CreateReview(context.Background(), ReviewFormChoices{ ReviewDiff: func(context.Context) ([]ReviewCommentDraft, error) { return nil, failure }, }) require.ErrorIs(t, err, failure) } -func TestReviewFormRequiresCommentOnlyWhenRequestingChanges(t *testing.T) { - p, rendered := accessiblePrompter(t, "3\n") - _, err := p.CreateReview(context.Background(), ReviewFormChoices{}) - require.NoError(t, err) - require.Contains(t, rendered.String(), "Concluding comment (markdown):") - require.NotContains(t, rendered.String(), "required") - - p, rendered = accessiblePrompter(t, "2\n") - _, err = p.CreateReview(context.Background(), ReviewFormChoices{}) - require.ErrorContains(t, err, "concluding comment cannot be empty") - require.Contains(t, rendered.String(), "Concluding comment (markdown, required):") +func TestReviewFormAsksTypeThenBody(t *testing.T) { + for _, tc := range []struct { + name, input string + choices ReviewFormChoices + want ReviewFormValues + title, err string + hidden []string + }{ + {name: "approve without body", input: "1\n\n", want: ReviewFormValues{Event: "APPROVED"}, title: "Body (markdown, optional):"}, + {name: "request changes", input: "2\nPlease add tests\n", want: ReviewFormValues{Event: "REQUEST_CHANGES", Body: "Please add tests"}, title: "Body (markdown, required):"}, + {name: "comment needs a body", input: "3\n\n", err: errEmptyComment.Error(), title: "Body (markdown, required):"}, + {name: "author can only comment", input: "1\nSelf note\n", choices: ReviewFormChoices{Author: true}, want: ReviewFormValues{Event: "COMMENT", Body: "Self note"}, hidden: []string{"Approve", "Request changes"}}, + {name: "editor shortcut", input: "1\n\n", choices: ReviewFormChoices{Editor: "vi"}, want: ReviewFormValues{Event: "APPROVED"}, title: "Body (markdown, optional, ctrl+e opens your editor):"}, + {name: "header", input: "1\n\n", choices: ReviewFormChoices{Header: "Reviewing #1 \"Add\x1b[31m one\" (feat/one → main) in alice/x"}, want: ReviewFormValues{Event: "APPROVED"}, title: "Reviewing #1 \"Add[31m one\" (feat/one → main) in alice/x"}, + } { + t.Run(tc.name, func(t *testing.T) { + p, rendered := linePrompter(t, tc.input) + values, err := p.CreateReview(context.Background(), tc.choices) + if tc.err != "" { + require.EqualError(t, err, tc.err) + } else { + require.NoError(t, err) + require.Equal(t, tc.want, values) + } + require.Contains(t, rendered.String(), tc.title) + for _, hidden := range tc.hidden { + require.NotContains(t, rendered.String(), hidden) + } + }) + } +} + +func TestReviewFormCommentMayBeInlineOnly(t *testing.T) { + p, _ := linePrompter(t, "3\n\ny\n") + values, err := p.CreateReview(context.Background(), ReviewFormChoices{ + ReviewDiff: func(context.Context) ([]ReviewCommentDraft, error) { + return []ReviewCommentDraft{{Path: "one.txt", Body: "nit", NewLine: 1}}, nil + }, + }) + require.NoError(t, err) + require.Equal(t, ReviewFormValues{Event: "COMMENT", Comments: []ReviewCommentDraft{{Path: "one.txt", Body: "nit", NewLine: 1}}}, values) }