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) }