From efd3383ff959e7a40275311919c47dc343494726 Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 18 Oct 2026 20:07:47 +0200 Subject: [PATCH] fix(pr): review decision ignores comment reviews, paged review list, pending review comments hidden, team request names (follow gh) Signed-off-by: bircni --- internal/app/alias_completion_test.go | 16 ++++++ internal/app/app_test.go | 9 ++++ internal/app/checkout.go | 4 +- internal/app/checks.go | 2 +- internal/app/discussion_view.go | 5 +- internal/app/pull_clean.go | 2 +- internal/app/pull_edit.go | 4 +- internal/app/pull_mutations.go | 6 +-- internal/app/pull_review.go | 53 ++++++++++-------- internal/app/pull_review_lifecycle.go | 14 +++-- internal/app/pull_review_requests.go | 2 +- internal/app/pull_review_test.go | 18 +++++++ internal/app/pulls.go | 4 +- internal/app/status.go | 2 +- internal/services/pulls.go | 2 +- internal/services/review_requests.go | 10 +++- internal/services/review_requests_test.go | 24 +++++++++ internal/services/reviews.go | 66 ++++++++++++++++++++--- internal/services/reviews_test.go | 65 ++++++++++++++++++++++ 19 files changed, 258 insertions(+), 50 deletions(-) diff --git a/internal/app/alias_completion_test.go b/internal/app/alias_completion_test.go index c25ffe18..61eea0f5 100644 --- a/internal/app/alias_completion_test.go +++ b/internal/app/alias_completion_test.go @@ -56,6 +56,22 @@ func complete(t *testing.T, deps Dependencies, out *bytes.Buffer, args ...string return lines[:len(lines)-1], cobra.ShellCompDirective(directive) } +func TestPullCompletionStopsAfterTheLastPositionalArgument(t *testing.T) { + requests := 0 + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + _, _ = w.Write([]byte(`[{"id":9,"number":9,"title":"More"}]`)) + })) + defer server.Close() + deps, out, _ := serverDeps(t, server.URL) + for _, args := range []string{"pr view 5 ", "pr review list 5 ", "pr review view 5 9 ", "pr review submit 5 9 ", "pr review reply 5 9 "} { + items, directive := complete(t, deps, out, append(strings.Fields(args), "-R", "owner/repo", "")...) + require.Empty(t, items, args) + require.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive, args) + } + require.Zero(t, requests, "no argument is left to complete") +} + func TestEnumFlagsShareValidationAndCompletion(t *testing.T) { deps, out, _ := testDeps(t) var state string diff --git a/internal/app/app_test.go b/internal/app/app_test.go index b7a45da7..64dc01cc 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -312,6 +312,15 @@ func TestHelpConfirmAndArgumentsAnnotations(t *testing.T) { require.Contains(t, out.String(), "\nEXAMPLES\n # Drop issue 7\n $ tea issue drop 7\n") } +func TestHelpDefaultsToCurrentBranchOnlyWhereThePullRequestIsOptional(t *testing.T) { + deps, out, _ := testDeps(t) + for path, defaults := range map[string]bool{"pr review list": true, "pr view": true, "pr review view": false, "pr review dismiss": false, "pr checkout": false} { + out.Reset() + require.NoError(t, Execute(context.Background(), deps, append(strings.Fields(path), "--help"))) + require.Equal(t, defaults, strings.Contains(out.String(), "Without an argument, the pull request of the current branch"), path) + } +} + func TestHelpTopicsAndUnknownTopics(t *testing.T) { deps, out, _ := testDeps(t) for args, want := range map[string]string{ diff --git a/internal/app/checkout.go b/internal/app/checkout.go index f0ed7908..67e7634b 100644 --- a/internal/app/checkout.go +++ b/internal/app/checkout.go @@ -76,7 +76,7 @@ func (a *application) pullCheckoutCommand() *cobra.Command { var branch string var detached, update bool cmd := &cobra.Command{Use: "checkout { | | }", Aliases: []string{"co"}, Short: "Check out a pull request", Long: `Check out the head of a pull request in a local branch that tracks the head branch, so -that git push updates it and pr commands without an argument select the pull request.`, Annotations: pullArgumentsAnnotation(nil), Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { +that git push updates it and pr commands without an argument select the pull request.`, Annotations: pullArgumentRequiredAnnotation(nil), Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { if detached && (cmd.Flags().Changed("branch") || update) { return clierr.Wrap(clierr.Usage, errors.New("--detach and --branch/--update are mutually exclusive")) } @@ -149,7 +149,7 @@ that git push updates it and pr commands without an argument select the pull req cmd.Flags().BoolVar(&update, "update", false, "Fast-forward an existing branch to the PR head; reject divergence") cmd.Flags().BoolVar(&detached, "detach", false, "Check out the PR commit without creating or changing a branch") cmd.Flags().StringVarP(&branch, "branch", "b", "", "Local branch name (default: the head branch name when free, else OWNER/BRANCH or pr/NUMBER)") - cmd.ValidArgsFunction = a.completePulls("open") + cmd.ValidArgsFunction = firstArg(a.completePulls("open")) cmd.Example = `$ tea pr checkout 13 $ tea pr co feature/login $ tea pr checkout 13 --update` diff --git a/internal/app/checks.go b/internal/app/checks.go index 61168f62..8d17a485 100644 --- a/internal/app/checks.go +++ b/internal/app/checks.go @@ -86,7 +86,7 @@ tea exits with 8 while checks are pending and with 1 when a check failed, like g cmd.Flags().DurationVar(&interval, "interval", 5*time.Second, "Time between refreshes with --watch") cmd.Flags().DurationVar(&timeout, "timeout", time.Hour, "Longest time to wait with --watch") cmd.Flags().BoolVarP(&web, "web", "w", false, "Open the pull request in the browser") - cmd.ValidArgsFunction = a.completePulls("open") + cmd.ValidArgsFunction = firstArg(a.completePulls("open")) cmd.Example = `# Show the checks of the current branch's pull request $ tea pr checks diff --git a/internal/app/discussion_view.go b/internal/app/discussion_view.go index 9698d9ac..c96fe32d 100644 --- a/internal/app/discussion_view.go +++ b/internal/app/discussion_view.go @@ -238,10 +238,7 @@ func pullReviewComments(cmd *cobra.Command, t target, n int64, reviews []service return nil, err } for _, comment := range inline { - location := comment.Path - if comment.Line > 0 { - location = fmt.Sprintf("%s:%d", comment.Path, comment.Line) - } + location := comment.Location() result = append(result, discussionComment{ author: comment.Author, createdAt: comment.CreatedAt, body: comment.Body, id: comment.ID, heading: "commented on " + location, meta: [2]string{"path", location}, diff --git a/internal/app/pull_clean.go b/internal/app/pull_clean.go index 4bbff505..68450aea 100644 --- a/internal/app/pull_clean.go +++ b/internal/app/pull_clean.go @@ -120,7 +120,7 @@ func (a *application) pullCleanCommand() *cobra.Command { return a.jsonResult(cmd, result) }} cmd.Flags().BoolVar(&ignoreSHA, "ignore-sha", false, "Find the local branch by name instead of commit hash") - cmd.ValidArgsFunction = a.completePulls("closed") + cmd.ValidArgsFunction = firstArg(a.completePulls("closed")) cmd.Example = `# After #13 was merged, delete its branch locally and on Gitea $ tea pr clean 13` return cmd diff --git a/internal/app/pull_edit.go b/internal/app/pull_edit.go index 6ea89f59..d7932759 100644 --- a/internal/app/pull_edit.go +++ b/internal/app/pull_edit.go @@ -149,7 +149,7 @@ pull request, or mark it as a draft or ready for review.`, Annotations: pullArgu } _ = cmd.RegisterFlagCompletionFunc("base", a.completeBranches()) _ = cmd.RegisterFlagCompletionFunc("milestone", a.completeMilestones()) - cmd.ValidArgsFunction = a.completePulls("open") + cmd.ValidArgsFunction = firstArg(a.completePulls("open")) cmd.Example = `$ tea pr edit 13 --title "Add login support" --add-reviewer carol $ tea pr edit --base develop $ tea pr edit 13 --editor` @@ -206,7 +206,7 @@ removes or adds that marker.`, Annotations: pullArgumentsAnnotation(nil), Args: return a.jsonResult(cmd, item) }} cmd.Flags().BoolVar(&undo, "undo", false, "Turn the pull request back into a draft") - cmd.ValidArgsFunction = a.completePulls("open") + cmd.ValidArgsFunction = firstArg(a.completePulls("open")) cmd.Example = `$ tea pr ready $ tea pr ready 15 $ tea pr ready 15 --undo` diff --git a/internal/app/pull_mutations.go b/internal/app/pull_mutations.go index 83abbfeb..d4011614 100644 --- a/internal/app/pull_mutations.go +++ b/internal/app/pull_mutations.go @@ -111,7 +111,7 @@ func (a *application) pullStateCommand(closing bool) *cobra.Command { return a.jsonResult(cmd, item) }} cmd.Flags().StringVarP(&comment, "comment", "c", "", "Leave a comment before changing the state") - cmd.ValidArgsFunction = a.completePulls(map[bool]string{true: "open", false: "closed"}[closing]) + cmd.ValidArgsFunction = firstArg(a.completePulls(map[bool]string{true: "open", false: "closed"}[closing])) if closing { cmd.Flags().BoolVarP(&deleteBranch, "delete-branch", "d", false, "Delete the head branch on Gitea and, when it is checked out, locally") cmd.Example = `$ tea pr close 13 @@ -219,7 +219,7 @@ tea pr diff 13 | git apply works.`, Annotations: pullArgumentsAnnotation(textOut cmd.Flags().BoolVar(&nameOnly, "name-only", false, "List only the names of the changed files") enumFlag(cmd, &color, "color", "", "auto", "Color the diff", "auto", "always", "never") cmd.Flags().BoolVarP(&web, "web", "w", false, "Open the changed files in the browser") - cmd.ValidArgsFunction = a.completePulls("all") + cmd.ValidArgsFunction = firstArg(a.completePulls("all")) cmd.Example = `$ tea pr diff $ tea pr diff 13 --name-only $ tea pr diff 13 | git apply` @@ -384,7 +384,7 @@ the base branch.`, Annotations: pullArgumentsAnnotation(map[string]string{"tea.c } return c.Root().FlagErrorFunc()(c, err) }) - cmd.ValidArgsFunction = a.completePulls("open") + cmd.ValidArgsFunction = firstArg(a.completePulls("open")) cmd.Example = `# Merge the pull request of the current branch with the repository's default style $ tea pr merge diff --git a/internal/app/pull_review.go b/internal/app/pull_review.go index 6e8c7ea4..23341fd6 100644 --- a/internal/app/pull_review.go +++ b/internal/app/pull_review.go @@ -19,18 +19,30 @@ import ( "github.com/spf13/cobra" ) -const pullArguments = `A pull request can be supplied as argument in any of the following formats: +const pullArgumentFormats = `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" +- by the name of its head branch, such as "feature/login" or "bob:feature/login"` + +const pullArguments = pullArgumentFormats + ` Without an argument, the pull request of the current branch is selected.` +// pullArgumentsAnnotation documents a pull request argument that may be left out. func pullArgumentsAnnotation(annotations map[string]string) map[string]string { + return argumentsAnnotation(annotations, pullArguments) +} + +// pullArgumentRequiredAnnotation documents a pull request argument that has no default. +func pullArgumentRequiredAnnotation(annotations map[string]string) map[string]string { + return argumentsAnnotation(annotations, pullArgumentFormats) +} + +func argumentsAnnotation(annotations map[string]string, text string) map[string]string { if annotations == nil { annotations = map[string]string{} } - annotations["tea.arguments"] = pullArguments + annotations["tea.arguments"] = text return annotations } @@ -242,7 +254,7 @@ func (a *application) reviewCreateCommand(use, short string) *cobra.Command { cmd.Flags().BoolVarP(&useEditor, "editor", "e", false, "Write the body in your editor") cmd.Flags().StringVar(&commentsFile, "comments-file", "", "Read inline comments from a JSON `file`: an array of path, body, and new_position or old_position") cmd.Flags().StringVar(&commit, "commit", "", "Commit `sha` to review (default: the head of the pull request)") - cmd.ValidArgsFunction = a.completePulls("open") + cmd.ValidArgsFunction = firstArg(a.completePulls("open")) return cmd } @@ -442,7 +454,7 @@ func reviewPossessive(item services.Review) string { case "REQUEST_CHANGES": noun = "change request" } - return item.Author + "'s " + noun + return item.Reviewer() + "'s " + noun } // pullAndID resolves the pull request and the ID that review subcommands take as their two arguments. @@ -462,17 +474,17 @@ func (a *application) pullReviewListCommand() *cobra.Command { if err != nil { return err } - items, err := (services.Reviews{Client: t.client, Repository: t.Repo}).List(cmd.Context(), n, limit, page) + window, err := (services.Reviews{Client: t.client, Repository: t.Repo}).ListPage(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]{ + return writeTable(a, cmd, window.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}, + {Text: item.Reviewer()}, reviewStateCell(item), {Time: terminal.ParseTime(item.SubmittedAt)}, {Text: strconv.Itoa(item.CommentsCount)}, @@ -480,17 +492,17 @@ func (a *application) pullReviewListCommand() *cobra.Command { } }, Empty: "no reviews on " + ref, Context: "reviews on " + ref, - }, listPage{Number: page, More: len(items) == limit}) + }, servicePage(window, page)) }} pageFlags(cmd, &limit, &page) cmd.Example = `$ tea pr review list 13 $ tea pr review list --json id,author,state` - cmd.ValidArgsFunction = a.completePulls("all") + cmd.ValidArgsFunction = firstArg(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 { + cmd := &cobra.Command{Use: "view ", Short: "View a review", Annotations: pullArgumentRequiredAnnotation(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 @@ -501,14 +513,14 @@ func (a *application) pullReviewViewCommand() *cobra.Command { } now := a.deps.Now() return writeView(a, cmd, item, func(item services.Review) terminal.View { - meta := item.Author + meta := item.Reviewer() 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: "Reviewer", Value: item.Reviewer(), PipedOnly: true}, {Label: "State", Value: item.State, PipedOnly: true}, {Label: "Inline comments", Value: strconv.Itoa(item.CommentsCount)}, {Label: "Commit", Value: item.Commit}, @@ -541,11 +553,7 @@ func (a *application) pullReviewCommentsCommand() *cobra.Command { 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}} + return []terminal.Cell{{Text: strconv.FormatInt(item.ID, 10), Link: item.URL}, {Text: item.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}) @@ -554,7 +562,7 @@ func (a *application) pullReviewCommentsCommand() *cobra.Command { 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") + cmd.ValidArgsFunction = firstArg(a.completePulls("all")) return cmd } @@ -585,7 +593,7 @@ func (a *application) pullReviewResolveCommand(resolve bool) *cobra.Command { 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) { + cmd := &cobra.Command{Use: "reply ", Short: "Reply to an inline review comment", Annotations: pullArgumentRequiredAnnotation(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 { @@ -639,6 +647,9 @@ func (a *application) completeReviewArgs(inline bool) cobra.CompletionFunc { if len(args) == 0 { return pulls(cmd, args, toComplete) } + if len(args) > 1 { + return nil, cobra.ShellCompDirectiveNoFileComp + } n, err := number(args[0]) if err != nil { return nil, cobra.ShellCompDirectiveNoFileComp @@ -653,7 +664,7 @@ func (a *application) completeReviewArgs(inline bool) cobra.CompletionFunc { } 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 + return strconv.FormatInt(item.ID, 10), reviewStateCell(item).Text + " by " + item.Reviewer() }), err })(cmd, args, toComplete) } diff --git a/internal/app/pull_review_lifecycle.go b/internal/app/pull_review_lifecycle.go index 3ec5fa91..da7a6d1a 100644 --- a/internal/app/pull_review_lifecycle.go +++ b/internal/app/pull_review_lifecycle.go @@ -123,7 +123,7 @@ func (a *application) pendingReview(cmd *cobra.Command, service services.Reviews } 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 { + cmd := &cobra.Command{Use: "delete ", Short: "Delete a review", Annotations: pullArgumentRequiredAnnotation(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 @@ -133,7 +133,7 @@ func (a *application) pullReviewDeleteCommand() *cobra.Command { if err != nil { return err } - subject := fmt.Sprintf("review %d by %s on %s#%d", id, item.Author, t.Repo, n) + subject := fmt.Sprintf("review %d by %s on %s#%d", id, item.Reviewer(), t.Repo, n) return a.deleteConfirmed(cmd, subject, func() error { return service.Delete(cmd.Context(), n, id) }) }} cmd.Annotations["tea.confirm"] = "true" @@ -149,7 +149,7 @@ func (a *application) pullReviewDismissCommand(dismiss bool) *cobra.Command { 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) { + cmd := &cobra.Command{Use: use, Short: short, Annotations: pullArgumentRequiredAnnotation(nil), Args: argsExact(2), RunE: func(cmd *cobra.Command, args []string) (runErr error) { var draft terminal.Draft defer finishDraft(&draft, &runErr) var err error @@ -183,6 +183,14 @@ func (a *application) pullReviewDismissCommand(dismiss bool) *cobra.Command { return err } subject := fmt.Sprintf("%s on %s#%d", reviewPossessive(current), t.Repo, n) + switch { + case dismiss && current.State != "APPROVED" && current.State != "REQUEST_CHANGES": + // Gitea refuses other review types with a misleading "permission denied". + return clierr.Wrap(clierr.Usage, fmt.Errorf("only approvals and change requests can be dismissed; %s is not one", subject)) + case !dismiss && !current.Dismissed: + a.warn(cmd, "%s is not dismissed", subject) + return a.jsonResult(cmd, current) + } if dismiss { if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: "Dismiss " + subject + "?"}); err != nil { return err diff --git a/internal/app/pull_review_requests.go b/internal/app/pull_review_requests.go index 45f423b0..d53e90d2 100644 --- a/internal/app/pull_review_requests.go +++ b/internal/app/pull_review_requests.go @@ -47,7 +47,7 @@ func (a *application) pullReviewRequestCommand(add bool) *cobra.Command { 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") + cmd.ValidArgsFunction = firstArg(a.completePulls("open")) if add { cmd.Example = `$ tea pr review request 13 --reviewer carol,dave $ tea pr review request --team developers` diff --git a/internal/app/pull_review_test.go b/internal/app/pull_review_test.go index 03728135..69487d0e 100644 --- a/internal/app/pull_review_test.go +++ b/internal/app/pull_review_test.go @@ -270,6 +270,24 @@ func TestReviewCommentsFileValidatedBeforeRequests(t *testing.T) { } } +func TestReviewDismissChecksTheReviewStateFirst(t *testing.T) { + 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":"COMMENT","user":{"login":"bob"}}`)) + })) + defer server.Close() + deps, _, diagnostics := serverDeps(t, server.URL) + err := Execute(context.Background(), deps, []string{"pr", "review", "dismiss", "1", "2", "-R", "owner/repo", "--body", "Reason", "--yes"}) + require.Equal(t, clierr.Usage, clierr.ExitCode(err)) + require.ErrorContains(t, err, "only approvals and change requests can be dismissed") + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "review", "undismiss", "1", "2", "-R", "owner/repo"})) + require.Contains(t, diagnostics.String(), "is not dismissed") + require.Zero(t, mutations) +} + func TestReviewDeleteAndDismissRequireYesWithoutInput(t *testing.T) { mutations := 0 server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/app/pulls.go b/internal/app/pulls.go index e78ee2e5..570d79e9 100644 --- a/internal/app/pulls.go +++ b/internal/app/pulls.go @@ -166,7 +166,7 @@ func (a *application) pullViewCommand() *cobra.Command { if details.reviews, err = (services.Reviews{Client: t.client, Repository: t.Repo}).All(cmd.Context(), n); err != nil { return err } - item.ReviewDecision = services.ReviewDecision(services.LatestReviews(details.reviews), len(item.Reviewers)+len(item.ReviewerTeams) > 0) + item.ReviewDecision = services.ReviewDecision(details.reviews, len(item.Reviewers)+len(item.ReviewerTeams) > 0) if a.options.json != "" { return a.writeJSON(cmd, item) } @@ -179,7 +179,7 @@ func (a *application) pullViewCommand() *cobra.Command { }} cmd.Flags().BoolVarP(&web, "web", "w", false, "Open the pull request in the browser") cmd.Flags().BoolVarP(&comments, "comments", "c", false, "Show comments and reviews") - cmd.ValidArgsFunction = a.completePulls("all") + cmd.ValidArgsFunction = firstArg(a.completePulls("all")) cmd.Example = `# View the pull request of the current branch $ tea pr view diff --git a/internal/app/status.go b/internal/app/status.go index 7fac6510..381cbebd 100644 --- a/internal/app/status.go +++ b/internal/app/status.go @@ -106,7 +106,7 @@ func (a *application) currentPullStatus(cmd *cobra.Command) (*services.Pull, pul return nil, details, branch } if details.reviews, err = (services.Reviews{Client: t.client, Repository: t.Repo}).All(cmd.Context(), n); err == nil { - pull.ReviewDecision = services.ReviewDecision(services.LatestReviews(details.reviews), len(pull.Reviewers)+len(pull.ReviewerTeams) > 0) + pull.ReviewDecision = services.ReviewDecision(details.reviews, len(pull.Reviewers)+len(pull.ReviewerTeams) > 0) } if pull.HeadSHA != "" { details.checks, _ = service.CommitChecks(cmd.Context(), pull.HeadSHA) diff --git a/internal/services/pulls.go b/internal/services/pulls.go index 78c0bb11..68e61902 100644 --- a/internal/services/pulls.go +++ b/internal/services/pulls.go @@ -254,7 +254,7 @@ func (s Pulls) DecideAll(ctx context.Context, items []Pull) ([]Pull, error) { if err != nil { return err } - item.ReviewDecision = ReviewDecision(LatestReviews(all), len(item.Reviewers)+len(item.ReviewerTeams) > 0) + item.ReviewDecision = ReviewDecision(all, len(item.Reviewers)+len(item.ReviewerTeams) > 0) viewed[i] = item return nil }) diff --git a/internal/services/review_requests.go b/internal/services/review_requests.go index dd654bcb..2e4de9f7 100644 --- a/internal/services/review_requests.go +++ b/internal/services/review_requests.go @@ -97,10 +97,15 @@ func (s Reviews) changeRequests(ctx context.Context, n int64, changes ReviewRequ return nil } -// CheckRequestTeams verifies that every named team exists in the repository's organization. +// CheckRequestTeams verifies that every named team exists in the repository's organization. Gitea +// lets only organization members list teams, though outside collaborators and the pull request +// author may request reviews; when the lookup is forbidden the server checks the names, as for gh. func (s Reviews) CheckRequestTeams(ctx context.Context, names []string) error { owner, _, _ := strings.Cut(s.Repository, "/") _, isOrg, err := organization(ctx, s.Client, owner) + if IsForbidden(err) { + return nil + } if err != nil { return err } @@ -108,6 +113,9 @@ func (s Reviews) CheckRequestTeams(ctx context.Context, names []string) error { return errors.New("team reviews require an organization repository") } teams, err := (Teams{Client: s.Client}).ByName(ctx, owner) + if IsForbidden(err) { + return nil + } if err != nil { return err } diff --git a/internal/services/review_requests_test.go b/internal/services/review_requests_test.go index da7ae3e6..6a0e7217 100644 --- a/internal/services/review_requests_test.go +++ b/internal/services/review_requests_test.go @@ -54,6 +54,30 @@ func TestReviewRequestPartialFailureStopsFurtherEdits(t *testing.T) { require.Equal(t, 2, mutations) } +func TestReviewerTeamCheckSkippedWhenTeamListingIsForbidden(t *testing.T) { + var posted bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/api/v1/orgs/owner": + _, _ = w.Write([]byte(`{"id":1}`)) + case r.URL.Path == "/api/v1/orgs/owner/teams": + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"Must be an organization member"}`)) + case r.Method == http.MethodPost && r.URL.Path == "/api/v1/repos/owner/repo/pulls/1/requested_reviewers": + posted = true + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`[]`)) + default: + _, _ = w.Write([]byte(`{"number":1}`)) + } + })) + defer server.Close() + changes := ReviewRequestChanges{Add: ReviewRequestInput{Teams: []string{"devs"}}} + _, err := (Reviews{Client: transport.New(server.URL, "", false), Repository: "owner/repo"}).ChangeRequests(context.Background(), 1, changes) + require.NoError(t, err) + require.True(t, posted, "the server decides whether the team exists") +} + func TestUnknownReviewerTeamRejectsBeforeUserRemoval(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { require.Equal(t, http.MethodGet, r.Method) diff --git a/internal/services/reviews.go b/internal/services/reviews.go index 8b505de0..60bcd107 100644 --- a/internal/services/reviews.go +++ b/internal/services/reviews.go @@ -24,6 +24,7 @@ type Review struct { Body string `json:"body"` State string `json:"state"` Author string `json:"author"` + Team string `json:"team"` URL string `json:"url"` Commit string `json:"commit"` SubmittedAt string `json:"submitted_at"` @@ -31,9 +32,21 @@ type Review struct { Dismissed bool `json:"dismissed"` } +// Reviewer names who the review is by, or for a request to a team, "team NAME". +func (r Review) Reviewer() string { + if r.Author == "" && r.Team != "" { + return "team " + r.Team + } + return r.Author +} + func reviewFrom(review *gitea.PullReview) Review { + team := "" + if review.ReviewerTeam != nil { + team = review.ReviewerTeam.Name + } return Review{ - ID: review.ID, Body: review.Body, State: string(review.State), Author: login(review.Reviewer), URL: review.HTMLURL, + ID: review.ID, Body: review.Body, State: string(review.State), Author: login(review.Reviewer), Team: team, URL: review.HTMLURL, Commit: review.CommitID, SubmittedAt: timestamp(review.Submitted), CommentsCount: review.CodeCommentsCount, Dismissed: review.Dismissed, } } @@ -48,6 +61,7 @@ type ReviewComment struct { URL string `json:"url"` Path string `json:"path"` Line int64 `json:"line"` + OldLine int64 `json:"old_line"` CreatedAt string `json:"created_at"` UpdatedAt string `json:"updated_at"` } @@ -55,10 +69,22 @@ type ReviewComment struct { func reviewCommentFrom(comment *gitea.PullReviewComment) ReviewComment { return ReviewComment{ ID: comment.ID, ReviewID: comment.ReviewID, Body: comment.Body, Author: login(comment.Reviewer), Resolver: login(comment.Resolver), - URL: comment.HTMLURL, Path: comment.Path, Line: int64(comment.LineNum), CreatedAt: timestamp(comment.Created), UpdatedAt: timestamp(comment.Updated), + URL: comment.HTMLURL, Path: comment.Path, Line: int64(comment.LineNum), OldLine: int64(comment.OldLineNum), CreatedAt: timestamp(comment.Created), UpdatedAt: timestamp(comment.Updated), } } +// Location names the file and line a comment is on: "app.go:8", or "app.go:4 (old)" for a +// removed line, which has only an old position. +func (c ReviewComment) Location() string { + switch { + case c.Line > 0: + return fmt.Sprintf("%s:%d", c.Path, c.Line) + case c.OldLine > 0: + return fmt.Sprintf("%s:%d (old)", c.Path, c.OldLine) + } + return c.Path +} + // InlineComment supplies an explicit file position for review creation. type InlineComment struct { Path string `json:"path"` @@ -106,6 +132,24 @@ func (s Reviews) List(ctx context.Context, n int64, limit, page int) ([]Review, return convertAll(reviews, reviewFrom), nil } +// ListPage lists a window of reviews of pull request n. It reaches past the server's smaller +// page size, which List would silently take as the whole window. +func (s Reviews) ListPage(ctx context.Context, n int64, limit, page int) (Page[Review], error) { + window, err := listPage(limit, page, func(limit, page int) ([]*gitea.PullReview, http.Header, error) { + var headers http.Header + reviews, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) ([]*gitea.PullReview, *gitea.Response, error) { + reviews, resp, err := c.PullRequests.ListPullReviews(ctx, owner, name, n, gitea.ListPullReviewsOptions{ListOptions: listOptions(limit, page)}) + headers = headerOf(resp) + return reviews, resp, err + }) + return reviews, headers, err + }) + if err != nil { + return Page[Review]{}, err + } + return convertPage(window, 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 := s.All(ctx, n) @@ -243,12 +287,19 @@ func LatestReviews(reviews []Review) []Review { return latest } -// ReviewDecision summarizes the latest reviews like gh: changes_requested when a reviewer's latest +// ReviewDecision summarizes reviews like gh: changes_requested when a reviewer's latest decisive // review requests changes, else approved when one approves, else review_required while reviews -// are requested, else "". Dismissed reviews do not count. -func ReviewDecision(latest []Review, requested bool) string { +// are requested, else "". Comment reviews are not decisive (as in gh and Gitea) and dismissed +// reviews do not count. +func ReviewDecision(reviews []Review, requested bool) string { + decisive := make([]Review, 0, len(reviews)) + for _, review := range reviews { + if review.State != string(gitea.ReviewStateComment) { + decisive = append(decisive, review) + } + } decision := "" - for _, review := range latest { + for _, review := range LatestReviews(decisive) { switch { case review.Dismissed: case review.State == string(gitea.ReviewStateRequestChanges): @@ -268,7 +319,8 @@ func ReviewDecision(latest []Review, requested bool) string { func (s Reviews) reviewComments(ctx context.Context, n int64, reviews []Review, skipEmpty bool) ([]ReviewComment, error) { results := []ReviewComment{} for _, review := range reviews { - if skipEmpty && review.CommentsCount == 0 { + // Pending reviews are drafts that only their author and admins see; an explicit review ID still lists them. + if skipEmpty && (review.CommentsCount == 0 || review.State == string(gitea.ReviewStatePending)) { continue } comments, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) ([]*gitea.PullReviewComment, *gitea.Response, error) { diff --git a/internal/services/reviews_test.go b/internal/services/reviews_test.go index 8086467f..2905ba3e 100644 --- a/internal/services/reviews_test.go +++ b/internal/services/reviews_test.go @@ -8,6 +8,8 @@ import ( "encoding/json" "fmt" "net/http" + "strconv" + "strings" "testing" gitea "gitea.dev/sdk" @@ -53,6 +55,43 @@ func TestReviewCommentsContinueWhenServerCapsPageSize(t *testing.T) { require.Equal(t, 2, collections) } +func TestInlineCommentsSkipPendingReviews(t *testing.T) { + var fetched []string + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fetched = append(fetched, r.URL.Path) + _, _ = w.Write([]byte(`[{"id":5,"pull_request_review_id":1}]`)) + })) + reviews := []Review{{ID: 1, State: "COMMENT", CommentsCount: 1}, {ID: 2, State: "PENDING", CommentsCount: 1}} + items, err := (Reviews{Client: client, Repository: "owner/repo"}).Inline(context.Background(), 3, reviews) + require.NoError(t, err) + require.Len(t, items, 1) + require.Equal(t, []string{"/api/v1/repos/owner/repo/pulls/3/reviews/1/comments"}, fetched) +} + +func TestReviewListPageReachesPastServerPageCap(t *testing.T) { + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + size, _ := strconv.Atoi(r.URL.Query().Get("limit")) + page, _ := strconv.Atoi(r.URL.Query().Get("page")) + size = min(size, 2) // the server caps its pages at two items + var reviews []string + for id := (page-1)*size + 1; id <= min(page*size, 5); id++ { + reviews = append(reviews, fmt.Sprintf(`{"id":%d}`, id)) + } + w.Header().Set("X-Total-Count", "5") + _, _ = w.Write([]byte("[" + strings.Join(reviews, ",") + "]")) + })) + window, err := (Reviews{Client: client, Repository: "owner/repo"}).ListPage(context.Background(), 3, 4, 1) + require.NoError(t, err) + require.Len(t, window.Items, 4) + require.EqualValues(t, 4, window.Items[3].ID) + require.True(t, window.More) + window, err = (Reviews{Client: client, Repository: "owner/repo"}).ListPage(context.Background(), 3, 4, 2) + require.NoError(t, err) + require.Len(t, window.Items, 1) + require.EqualValues(t, 5, window.Items[0].ID) + require.False(t, window.More) +} + func TestReviewDismissalReturnsTheUpdatedReview(t *testing.T) { for _, dismiss := range []bool{false, true} { t.Run(fmt.Sprintf("dismiss=%t", dismiss), func(t *testing.T) { @@ -185,6 +224,17 @@ func TestReviewResolveAndReplyUseCommentEndpoints(t *testing.T) { }, paths) } +func TestReviewDecisionIgnoresCommentReviews(t *testing.T) { + reviews := []Review{ + {ID: 1, Author: "bob", State: "REQUEST_CHANGES"}, + {ID: 2, Author: "bob", State: "COMMENT"}, + {ID: 3, Author: "carol", State: "APPROVED"}, + } + assert.Equal(t, "changes_requested", ReviewDecision(reviews, false)) + assert.Equal(t, "approved", ReviewDecision([]Review{{ID: 1, Author: "carol", State: "APPROVED"}, {ID: 2, Author: "carol", State: "COMMENT"}}, false)) + assert.Equal(t, "", ReviewDecision([]Review{{ID: 1, Author: "carol", State: "COMMENT"}}, false)) +} + func TestReviewFromMapsServerFields(t *testing.T) { review := decode[gitea.PullReview](t, `{"id":2,"body":"LGTM","state":"APPROVED","user":{"login":"alice"},"html_url":"https://gitea.test/r/2", "commit_id":"abc","submitted_at":"2026-10-01T12:00:00Z","comments_count":3,"dismissed":true}`) @@ -194,6 +244,13 @@ func TestReviewFromMapsServerFields(t *testing.T) { }, reviewFrom(review)) } +func TestReviewFromNamesRequestedTeam(t *testing.T) { + review := reviewFrom(decode[gitea.PullReview](t, `{"id":251,"state":"REQUEST_REVIEW","user":null,"team":{"name":"devs"}}`)) + require.Equal(t, "devs", review.Team) + require.Equal(t, "team devs", review.Reviewer()) + require.Equal(t, "alice", Review{Author: "alice", Team: "devs"}.Reviewer()) +} + func TestReviewCommentFromMapsServerFields(t *testing.T) { comment := decode[gitea.PullReviewComment](t, `{"id":5,"pull_request_review_id":1,"body":"Nit","user":{"login":"alice"}, "resolver":{"login":"bob"},"html_url":"https://gitea.test/c/5","path":"main.go","position":12, @@ -203,3 +260,11 @@ func TestReviewCommentFromMapsServerFields(t *testing.T) { CreatedAt: "2026-10-01T12:00:00Z", UpdatedAt: "2026-10-02T12:00:00Z", }, reviewCommentFrom(comment)) } + +func TestReviewCommentLocationNamesRemovedLines(t *testing.T) { + removed := reviewCommentFrom(decode[gitea.PullReviewComment](t, `{"id":6,"path":"main.go","position":0,"original_position":4}`)) + require.EqualValues(t, 4, removed.OldLine) + require.Equal(t, "main.go:4 (old)", removed.Location()) + require.Equal(t, "main.go:12", ReviewComment{Path: "main.go", Line: 12}.Location()) + require.Equal(t, "main.go", ReviewComment{Path: "main.go"}.Location()) +}