From 32eebf704005ed0a7336f23b55456aaf6d9b82f9 Mon Sep 17 00:00:00 2001 From: Ross Golder Date: Wed, 29 Jul 2026 17:18:42 +0700 Subject: [PATCH] refactor(pulls): tighten reviewer-request flow Follow-up to 5d3db5d (#571). No user-visible CLI changes. - Replace tautological metadata unit tests with httptest-based e2e tests that exercise runRequestReview, runCancelReview, and runReviewersList against a mock Gitea API; each test captures method, path, and request body. - Add tests/integration/pulls_reviewers_test.go covering create-time --reviewer/--team-reviewer, request-review add, cancel-review by user and by team, validation error, and bad-PR error against a real Gitea instance. - Extract parseReviewRequestArgs in cmd/pulls/review_helpers.go so runRequestReview and runCancelReview share one validation/parsing path; both now route through task.ApplyReviewerChanges instead of inlining the SDK call. - Extend task.ApplyReviewerChanges to accept teamAdd/teamRm and use it from request-review, cancel-review, and the existing edit flow. - Replace task.CreatePull 8-positional-arg signature with a CreatePullOptions struct; update cmd/pulls/create.go and modules/interact/pull_create.go call sites. - Defensively filter empty CSV entries in parseReviewRequestArgs via nonEmptyValues so the validation is correct regardless of whether the underlying CsvFlag trims empty entries (kept independent of the separate CsvFlag bugfix PR). Verification: make fmt fmt-check vet lint test docs docs-check build --- cmd/pulls/cancel_review.go | 36 +--- cmd/pulls/cancel_review_test.go | 126 +++++++++---- cmd/pulls/create.go | 14 +- cmd/pulls/create_test.go | 40 +--- cmd/pulls/helpers_test.go | 89 +++++++++ cmd/pulls/request_review.go | 36 +--- cmd/pulls/request_review_test.go | 214 ++++++++++++++++------ cmd/pulls/review_helpers.go | 65 +++++++ cmd/pulls/reviewers_test.go | 87 ++++++--- modules/interact/pull_create.go | 13 +- modules/task/labels.go | 14 +- modules/task/pull_create.go | 26 ++- modules/task/pull_edit.go | 2 +- tests/integration/pulls_reviewers_test.go | 195 ++++++++++++++++++++ 14 files changed, 731 insertions(+), 226 deletions(-) create mode 100644 cmd/pulls/helpers_test.go create mode 100644 tests/integration/pulls_reviewers_test.go diff --git a/cmd/pulls/cancel_review.go b/cmd/pulls/cancel_review.go index b2c185d4..8928f05d 100644 --- a/cmd/pulls/cancel_review.go +++ b/cmd/pulls/cancel_review.go @@ -5,15 +5,13 @@ package pulls import ( stdctx "context" - "errors" "fmt" - gitea "gitea.dev/sdk" "github.com/urfave/cli/v3" "gitea.dev/tea/cmd/flags" "gitea.dev/tea/modules/context" - "gitea.dev/tea/modules/utils" + "gitea.dev/tea/modules/task" ) // CmdPullsCancelReview cancels previously requested reviews on one or more PRs @@ -32,6 +30,10 @@ At least one of --reviewer or --team-reviewer is required.`, } func runCancelReview(requestCtx stdctx.Context, cmd *cli.Command) error { + args, err := parseReviewRequestArgs(requestCtx, cmd) + if err != nil { + return err + } ctx, err := context.InitCommand(cmd) if err != nil { return err @@ -40,32 +42,14 @@ func runCancelReview(requestCtx stdctx.Context, cmd *cli.Command) error { return err } - reviewers, err := ReviewerFlag.GetValues(cmd) - if err != nil { - return err - } - teamReviewers, err := TeamReviewerFlag.GetValues(cmd) - if err != nil { - return err - } - if len(reviewers) == 0 && len(teamReviewers) == 0 { - return errors.New("at least one of --reviewer or --team-reviewer is required") - } - - indices, err := utils.ArgsToIndices(cmd.Args().Slice()) - if err != nil { - return err - } - client := ctx.Login.Client() - for _, idx := range indices { - if _, err := client.PullRequests.DeleteReviewRequests(requestCtx, ctx.Owner, ctx.Repo, idx, gitea.PullReviewRequestOptions{ - Reviewers: reviewers, - TeamReviewers: teamReviewers, - }); err != nil { + for _, idx := range args.Indices { + if err := task.ApplyReviewerChanges(requestCtx, client, ctx.Owner, ctx.Repo, idx, + nil, args.Reviewers, + nil, args.TeamReviewers); err != nil { return err } - fmt.Printf("PR#%d: canceled review requests for %v, teams %v\n", idx, reviewers, teamReviewers) + fmt.Printf("PR#%d: canceled review requests for %v, teams %v\n", idx, args.Reviewers, args.TeamReviewers) } return nil } diff --git a/cmd/pulls/cancel_review_test.go b/cmd/pulls/cancel_review_test.go index be301039..8b041b7a 100644 --- a/cmd/pulls/cancel_review_test.go +++ b/cmd/pulls/cancel_review_test.go @@ -4,40 +4,105 @@ package pulls import ( + "context" + "io" + "net/http" "testing" + gitea "gitea.dev/sdk" "github.com/stretchr/testify/assert" - "github.com/urfave/cli/v3" + "github.com/stretchr/testify/require" ) -func TestCancelReviewCommandMetadata(t *testing.T) { - cmd := &CmdPullsCancelReview +func TestRunCancelReviewUsesDelete(t *testing.T) { + var ( + gotMethod string + gotPath string + gotBody []byte + ) + repo := "owner/repo" + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/version": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + case "/api/v1/repos/" + repo + "/pulls/9/requested_reviewers": + gotMethod = r.Method + gotPath = r.URL.Path + gotBody, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusNoContent) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + }) - assert.Equal(t, "cancel-review", cmd.Name) - assert.Contains(t, cmd.Aliases, "cr") - assert.Equal(t, "Cancel requested reviews from users or teams on a pull request", cmd.Usage) - assert.Equal(t, " [...]", cmd.ArgsUsage) - assert.NotNil(t, cmd.Action) + cmd := runAction(t, CmdPullsCancelReview, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + "--reviewer", "alice", + "--team-reviewer", "devs", + "9", + }) + require.NoError(t, runCancelReview(context.Background(), cmd)) + + assert.Equal(t, http.MethodDelete, gotMethod) + assert.Equal(t, "/api/v1/repos/"+repo+"/pulls/9/requested_reviewers", gotPath) + expectJSONBody(t, gotBody, gitea.PullReviewRequestOptions{ + Reviewers: []string{"alice"}, + TeamReviewers: []string{"devs"}, + }) } -func TestCancelReviewCommandFlags(t *testing.T) { - cmd := &CmdPullsCancelReview - - expectedFlags := []string{ - "reviewer", - "team-reviewer", - } - - for _, flagName := range expectedFlags { - found := false - for _, flag := range cmd.Flags { - if flag.Names()[0] == flagName { - found = true - break - } +func TestRunCancelReviewRejectsMissingFlags(t *testing.T) { + repo := "owner/repo" + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/version" { + t.Errorf("should not hit API when --reviewer/--team-reviewer are absent: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + return } - assert.True(t, found, "Expected flag %s not found", flagName) - } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + }) + + cmd := runAction(t, CmdPullsCancelReview, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + "9", + }) + err := runCancelReview(context.Background(), cmd) + require.Error(t, err) + assert.Contains(t, err.Error(), "at least one of --reviewer or --team-reviewer is required") +} + +func TestRunCancelReviewPropagatesAPIError(t *testing.T) { + repo := "owner/repo" + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/version": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + case "/api/v1/repos/" + repo + "/pulls/9/requested_reviewers": + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"not allowed"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + }) + + cmd := runAction(t, CmdPullsCancelReview, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + "--reviewer", "alice", + "9", + }) + err := runCancelReview(context.Background(), cmd) + require.Error(t, err) + assert.Contains(t, err.Error(), "not allowed") } // TestCancelReviewReusesReviewerFlag ensures request-review and cancel-review @@ -46,13 +111,6 @@ func TestCancelReviewReusesReviewerFlag(t *testing.T) { assert.Equal(t, flagNames(CmdPullsRequestReview.Flags), flagNames(CmdPullsCancelReview.Flags), - "request-review and cancel-review should expose identical flag sets") -} - -func flagNames(flags []cli.Flag) []string { - names := make([]string, 0, len(flags)) - for _, f := range flags { - names = append(names, f.Names()[0]) - } - return names + "request-review and cancel-review should expose identical flag sets", + ) } diff --git a/cmd/pulls/create.go b/cmd/pulls/create.go index 465005a4..a062c865 100644 --- a/cmd/pulls/create.go +++ b/cmd/pulls/create.go @@ -122,11 +122,13 @@ func runPullsCreate(requestCtx stdctx.Context, cmd *cli.Command) error { return task.CreatePull( requestCtx, ctx, - ctx.String("base"), - ctx.String("head"), - allowMaintainerEdits, - opts, - reviewers, - teamReviewers, + task.CreatePullOptions{ + Base: ctx.String("base"), + Head: ctx.String("head"), + AllowMaintainerEdits: allowMaintainerEdits, + Issue: opts, + Reviewers: reviewers, + TeamReviewers: teamReviewers, + }, ) } diff --git a/cmd/pulls/create_test.go b/cmd/pulls/create_test.go index df8d2e33..a5a1c644 100644 --- a/cmd/pulls/create_test.go +++ b/cmd/pulls/create_test.go @@ -9,37 +9,13 @@ import ( "github.com/stretchr/testify/assert" ) -func TestCreateCommandMetadata(t *testing.T) { - cmd := &CmdPullsCreate - - assert.Equal(t, "create", cmd.Name) - assert.Contains(t, cmd.Aliases, "c") - assert.Equal(t, "Create a pull-request", cmd.Usage) - assert.NotNil(t, cmd.Action) -} - -func TestCreateCommandFlags(t *testing.T) { - cmd := &CmdPullsCreate - - expectedFlags := []string{ - "head", - "base", - "allow-maintainer-edits", - "agit", - "topic", - "draft", - "reviewer", - "team-reviewer", - } - - for _, flagName := range expectedFlags { - found := false - for _, flag := range cmd.Flags { - if flag.Names()[0] == flagName { - found = true - break - } - } - assert.True(t, found, "Expected flag %s not found", flagName) +// TestCreateCommandExposesReviewerFlags verifies that `pulls create` +// advertises the reviewer / team-reviewer flags introduced in this PR. +// It does not exercise the action (which is covered by integration tests). +func TestCreateCommandExposesReviewerFlags(t *testing.T) { + want := []string{"reviewer", "team-reviewer"} + have := flagNames(CmdPullsCreate.Flags) + for _, w := range want { + assert.Contains(t, have, w, "pulls create should expose --%s", w) } } diff --git a/cmd/pulls/helpers_test.go b/cmd/pulls/helpers_test.go new file mode 100644 index 00000000..940e4df9 --- /dev/null +++ b/cmd/pulls/helpers_test.go @@ -0,0 +1,89 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pulls + +import ( + "bytes" + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + + gitea "gitea.dev/sdk" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v3" + + "gitea.dev/tea/modules/config" +) + +// withMockServer spins up a minimal Gitea API stub and configures tea to +// use it as the default login. The handler is called once for the version +// probe (InitCommand → login.Client) and once per assertion the test wants +// to make on the API path under test. +func withMockServer(t *testing.T, handler http.HandlerFunc) string { + t.Helper() + + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + + configPath := filepath.Join(t.TempDir(), "config.yml") + config.SetConfigPathForTesting(configPath) + t.Cleanup(func() { + config.SetConfigPathForTesting("") + }) + config.SetConfigForTesting(config.LocalConfig{ + Logins: []config.Login{{ + Name: "test", + URL: server.URL, + Token: "token", + User: "user1", + Default: true, + }}, + }) + return server.URL +} + +// runAction constructs a cli.Command that has the action replaced with a +// no-op, then runs it through urfave/cli with the given arg list so flag +// values and positional args are populated. The returned *cli.Command is +// then passed to the real action function under test. This works around +// urfave/cli's behavior where cmd.Run calls the action, which we don't +// want — we want to inspect the parsed cmd, then call the real action +// ourselves. +// +// All cross-cutting flags (--login, --repo, --output) must be passed via +// `args` along with the command-specific ones. +func runAction(t *testing.T, src cli.Command, args []string) *cli.Command { + t.Helper() + probe := src + probe.Action = func(_ context.Context, _ *cli.Command) error { return nil } + probe.Reader = bytes.NewReader(nil) + probe.Writer = io.Discard + probe.ErrWriter = io.Discard + + fullArgs := append([]string{"test"}, args...) + require.NoError(t, probe.Run(t.Context(), fullArgs)) + return &probe +} + +// expectJSONBody asserts that the recorded request body decodes into the +// expected value. +func expectJSONBody(t *testing.T, raw []byte, want gitea.PullReviewRequestOptions) { + t.Helper() + var got gitea.PullReviewRequestOptions + require.NoError(t, json.Unmarshal(raw, &got)) + require.Equal(t, want, got) +} + +// flagNames returns the primary name of each cli.Flag in order. +func flagNames(flags []cli.Flag) []string { + names := make([]string, 0, len(flags)) + for _, f := range flags { + names = append(names, f.Names()[0]) + } + return names +} diff --git a/cmd/pulls/request_review.go b/cmd/pulls/request_review.go index a4ca542a..e9327a28 100644 --- a/cmd/pulls/request_review.go +++ b/cmd/pulls/request_review.go @@ -5,15 +5,13 @@ package pulls import ( stdctx "context" - "errors" "fmt" - gitea "gitea.dev/sdk" "github.com/urfave/cli/v3" "gitea.dev/tea/cmd/flags" "gitea.dev/tea/modules/context" - "gitea.dev/tea/modules/utils" + "gitea.dev/tea/modules/task" ) // ReviewerFlag is a CSV flag listing usernames to request review from. @@ -50,6 +48,10 @@ At least one of --reviewer or --team-reviewer is required.`, } func runRequestReview(requestCtx stdctx.Context, cmd *cli.Command) error { + args, err := parseReviewRequestArgs(requestCtx, cmd) + if err != nil { + return err + } ctx, err := context.InitCommand(cmd) if err != nil { return err @@ -58,32 +60,14 @@ func runRequestReview(requestCtx stdctx.Context, cmd *cli.Command) error { return err } - reviewers, err := ReviewerFlag.GetValues(cmd) - if err != nil { - return err - } - teamReviewers, err := TeamReviewerFlag.GetValues(cmd) - if err != nil { - return err - } - if len(reviewers) == 0 && len(teamReviewers) == 0 { - return errors.New("at least one of --reviewer or --team-reviewer is required") - } - - indices, err := utils.ArgsToIndices(cmd.Args().Slice()) - if err != nil { - return err - } - client := ctx.Login.Client() - for _, idx := range indices { - if _, err := client.PullRequests.CreateReviewRequests(requestCtx, ctx.Owner, ctx.Repo, idx, gitea.PullReviewRequestOptions{ - Reviewers: reviewers, - TeamReviewers: teamReviewers, - }); err != nil { + for _, idx := range args.Indices { + if err := task.ApplyReviewerChanges(requestCtx, client, ctx.Owner, ctx.Repo, idx, + args.Reviewers, nil, + args.TeamReviewers, nil); err != nil { return err } - fmt.Printf("PR#%d: requested reviewers %v, teams %v\n", idx, reviewers, teamReviewers) + fmt.Printf("PR#%d: requested reviewers %v, teams %v\n", idx, args.Reviewers, args.TeamReviewers) } return nil } diff --git a/cmd/pulls/request_review_test.go b/cmd/pulls/request_review_test.go index 1f025fe6..cc20a8a9 100644 --- a/cmd/pulls/request_review_test.go +++ b/cmd/pulls/request_review_test.go @@ -4,71 +4,167 @@ package pulls import ( - "errors" + "context" + "io" + "net/http" "testing" + gitea "gitea.dev/sdk" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestRequestReviewCommandMetadata(t *testing.T) { - cmd := &CmdPullsRequestReview - - assert.Equal(t, "request-review", cmd.Name) - assert.Contains(t, cmd.Aliases, "rr") - assert.Equal(t, "Request reviews from users or teams on a pull request", cmd.Usage) - assert.Equal(t, " [...]", cmd.ArgsUsage) - assert.NotNil(t, cmd.Action) -} - -func TestRequestReviewCommandFlags(t *testing.T) { - cmd := &CmdPullsRequestReview - - expectedFlags := []string{ - "reviewer", - "team-reviewer", - } - - for _, flagName := range expectedFlags { - found := false - for _, flag := range cmd.Flags { - if flag.Names()[0] == flagName { - found = true - break - } +func TestRunRequestReviewHappyPath(t *testing.T) { + var ( + gotMethod string + gotPath string + gotBody []byte + ) + repo := "owner/repo" + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/version": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + case "/api/v1/repos/" + repo + "/pulls/7/requested_reviewers": + gotMethod = r.Method + gotPath = r.URL.Path + gotBody, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusNoContent) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) } - assert.True(t, found, "Expected flag %s not found", flagName) - } + }) + + cmd := runAction(t, CmdPullsRequestReview, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + "--reviewer", "alice,bob", + "--team-reviewer", "devs", + "7", + }) + require.NoError(t, runRequestReview(context.Background(), cmd)) + + assert.Equal(t, http.MethodPost, gotMethod) + assert.Equal(t, "/api/v1/repos/"+repo+"/pulls/7/requested_reviewers", gotPath) + expectJSONBody(t, gotBody, gitea.PullReviewRequestOptions{ + Reviewers: []string{"alice", "bob"}, + TeamReviewers: []string{"devs"}, + }) } -// TestRequestReviewValidation asserts the inline guard shared between -// runRequestReview and runCancelReview: at least one of --reviewer or -// --team-reviewer must be supplied. -func TestRequestReviewValidation(t *testing.T) { - const msg = "at least one of --reviewer or --team-reviewer is required" - tests := []struct { - name string - reviewers []string - teams []string - wantErr bool - }{ - {name: "both empty", reviewers: nil, teams: nil, wantErr: true}, - {name: "empty slice reviewers", reviewers: []string{}, teams: nil, wantErr: true}, - {name: "reviewer only", reviewers: []string{"alice"}, teams: nil, wantErr: false}, - {name: "team only", reviewers: nil, teams: []string{"devs"}, wantErr: false}, - {name: "both set", reviewers: []string{"alice", "bob"}, teams: []string{"devs"}, wantErr: false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var err error - if len(tt.reviewers) == 0 && len(tt.teams) == 0 { - err = errors.New(msg) - } - if tt.wantErr { - assert.Error(t, err) - assert.Equal(t, msg, err.Error()) - } else { - assert.NoError(t, err) - } - }) - } +func TestRunRequestReviewAcceptsTeamOnly(t *testing.T) { + hits := 0 + repo := "owner/repo" + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/version": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + case "/api/v1/repos/" + repo + "/pulls/1/requested_reviewers": + hits++ + body, _ := io.ReadAll(r.Body) + expectJSONBody(t, body, gitea.PullReviewRequestOptions{ + TeamReviewers: []string{"writers"}, + }) + w.WriteHeader(http.StatusNoContent) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + }) + + cmd := runAction(t, CmdPullsRequestReview, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + "--team-reviewer", "writers", + "1", + }) + require.NoError(t, runRequestReview(context.Background(), cmd)) + assert.Equal(t, 1, hits, "expected exactly one POST to requested_reviewers") +} + +func TestRunRequestReviewRejectsMissingFlags(t *testing.T) { + repo := "owner/repo" + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/version" { + t.Errorf("should not hit API when --reviewer/--team-reviewer are absent: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + }) + + cmd := runAction(t, CmdPullsRequestReview, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + "1", + }) + err := runRequestReview(context.Background(), cmd) + require.Error(t, err) + assert.Contains(t, err.Error(), "at least one of --reviewer or --team-reviewer is required") +} + +func TestRunRequestReviewPropagatesAPIError(t *testing.T) { + repo := "owner/repo" + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/version": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + case "/api/v1/repos/" + repo + "/pulls/1/requested_reviewers": + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message":"user not found"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + }) + + cmd := runAction(t, CmdPullsRequestReview, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + "--reviewer", "ghost", + "1", + }) + err := runRequestReview(context.Background(), cmd) + require.Error(t, err) + assert.Contains(t, err.Error(), "user not found") +} + +func TestRunRequestReviewHitsAllIndices(t *testing.T) { + var hits []string + repo := "owner/repo" + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/api/v1/version": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + case r.URL.Path == "/api/v1/repos/"+repo+"/pulls/3/requested_reviewers", + r.URL.Path == "/api/v1/repos/"+repo+"/pulls/4/requested_reviewers": + hits = append(hits, r.URL.Path) + w.WriteHeader(http.StatusNoContent) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + }) + + cmd := runAction(t, CmdPullsRequestReview, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + "--reviewer", "alice", + "3", "4", + }) + require.NoError(t, runRequestReview(context.Background(), cmd)) + assert.ElementsMatch(t, []string{ + "/api/v1/repos/" + repo + "/pulls/3/requested_reviewers", + "/api/v1/repos/" + repo + "/pulls/4/requested_reviewers", + }, hits) } diff --git a/cmd/pulls/review_helpers.go b/cmd/pulls/review_helpers.go index 82db65e8..4a5075fb 100644 --- a/cmd/pulls/review_helpers.go +++ b/cmd/pulls/review_helpers.go @@ -20,6 +20,7 @@ import ( "gitea.dev/tea/modules/utils" "charm.land/huh/v2" + "github.com/urfave/cli/v3" ) // runPullReview handles the common logic for approving/rejecting pull requests @@ -126,3 +127,67 @@ func getCommentBody(ctx *context.TeaContext, extraArgs []string, promptTitle, no return body, nil } + +// ReviewRequestArgs holds the parsed inputs shared by request-review and +// cancel-review: the reviewers / team-reviewers to (un)request and the +// PR indices they apply to. +type ReviewRequestArgs struct { + Reviewers []string + TeamReviewers []string + Indices []int64 +} + +// nonEmptyValues returns v with empty / whitespace-only entries dropped. +// Used to make the reviewer-request validation robust to CSV inputs that +// contain stray commas ("alice,,bob") regardless of whether the underlying +// CsvFlag already trimmed them. Returns nil (not an empty slice) when no +// non-empty entries remain, so callers can use len(x) == 0 unambiguously. +func nonEmptyValues(v []string) []string { + var out []string + for _, s := range v { + if strings.TrimSpace(s) == "" { + continue + } + out = append(out, s) + } + return out +} + +// parseReviewRequestArgs resolves --reviewer / --team-reviewer / pull-index +// positional args from cmd, validates that at least one of the two flags +// is supplied, and returns a populated ReviewRequestArgs. Returns a wrapped +// error suitable for direct return from a CLI Action. +func parseReviewRequestArgs(requestCtx stdctx.Context, cmd *cli.Command) (*ReviewRequestArgs, error) { + ctx, err := context.InitCommand(cmd) + if err != nil { + return nil, err + } + if err := ctx.Ensure(context.CtxRequirement{RemoteRepo: true}); err != nil { + return nil, err + } + + reviewers, err := ReviewerFlag.GetValues(cmd) + if err != nil { + return nil, err + } + teamReviewers, err := TeamReviewerFlag.GetValues(cmd) + if err != nil { + return nil, err + } + reviewers = nonEmptyValues(reviewers) + teamReviewers = nonEmptyValues(teamReviewers) + if len(reviewers) == 0 && len(teamReviewers) == 0 { + return nil, errors.New("at least one of --reviewer or --team-reviewer is required") + } + + indices, err := utils.ArgsToIndices(cmd.Args().Slice()) + if err != nil { + return nil, err + } + + return &ReviewRequestArgs{ + Reviewers: reviewers, + TeamReviewers: teamReviewers, + Indices: indices, + }, nil +} diff --git a/cmd/pulls/reviewers_test.go b/cmd/pulls/reviewers_test.go index be811e9a..d7f1c2d0 100644 --- a/cmd/pulls/reviewers_test.go +++ b/cmd/pulls/reviewers_test.go @@ -4,36 +4,71 @@ package pulls import ( + "context" + "net/http" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestReviewersCommandMetadata(t *testing.T) { - cmd := &CmdPullsReviewers - - assert.Equal(t, "reviewers", cmd.Name) - assert.Contains(t, cmd.Aliases, "rs") - assert.Equal(t, - "List users that can be requested to review pull requests in this repo", - cmd.Usage) - assert.NotNil(t, cmd.Action) -} - -func TestReviewersCommandFlags(t *testing.T) { - cmd := &CmdPullsReviewers - - // The reviewers subcommand inherits flags.AllDefaultFlags, which adds - // --login, --repo, --remote and --output. Verify those are wired. - expectedFlags := []string{"login", "repo", "remote", "output"} - for _, flagName := range expectedFlags { - found := false - for _, flag := range cmd.Flags { - if flag.Names()[0] == flagName { - found = true - break - } +func TestRunReviewersListCallsGetReviewers(t *testing.T) { + repo := "owner/repo" + var ( + gotMethod string + gotPath string + ) + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/version": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + case "/api/v1/repos/" + repo + "/reviewers": + gotMethod = r.Method + gotPath = r.URL.Path + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[ + {"id":1,"login":"alice","full_name":"Alice","email":"alice@example.com"}, + {"id":2,"login":"bob","full_name":"Bob","email":"bob@example.com"} + ]`)) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) } - assert.True(t, found, "Expected default flag %s not found", flagName) - } + }) + + cmd := runAction(t, CmdPullsReviewers, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + }) + require.NoError(t, runReviewersList(context.Background(), cmd)) + + assert.Equal(t, http.MethodGet, gotMethod) + assert.Equal(t, "/api/v1/repos/"+repo+"/reviewers", gotPath) +} + +func TestRunReviewersListPropagatesAPIError(t *testing.T) { + repo := "owner/repo" + withMockServer(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/version": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"version":"1.22.0"}`)) + case "/api/v1/repos/" + repo + "/reviewers": + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"boom"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + }) + + cmd := runAction(t, CmdPullsReviewers, []string{ + "--login", "test", + "--repo", repo, + "--output", "json", + }) + err := runReviewersList(context.Background(), cmd) + require.Error(t, err) + assert.Contains(t, err.Error(), "boom") } diff --git a/modules/interact/pull_create.go b/modules/interact/pull_create.go index 7938ccb4..589a300a 100644 --- a/modules/interact/pull_create.go +++ b/modules/interact/pull_create.go @@ -137,10 +137,11 @@ func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext) (err error) return task.CreatePull( requestCtx, ctx, - base, - head, - &allowMaintainerEdits, - &opts, - nil, - nil) + task.CreatePullOptions{ + Base: base, + Head: head, + AllowMaintainerEdits: &allowMaintainerEdits, + Issue: &opts, + }, + ) } diff --git a/modules/task/labels.go b/modules/task/labels.go index 64ba2eaf..4f5b539b 100644 --- a/modules/task/labels.go +++ b/modules/task/labels.go @@ -79,18 +79,22 @@ func ApplyLabelChanges(requestCtx stdctx.Context, client *gitea.Client, owner, r } // ApplyReviewerChanges adds and removes reviewers on a pull request. -func ApplyReviewerChanges(requestCtx stdctx.Context, client *gitea.Client, owner, repo string, index int64, add, rm []string) error { - if len(rm) != 0 { +// Both add/rm and teamAdd/teamRm may be empty; only the non-empty pairs +// trigger the corresponding POST/DELETE call. +func ApplyReviewerChanges(requestCtx stdctx.Context, client *gitea.Client, owner, repo string, index int64, add, rm []string, teamAdd, teamRm []string) error { + if len(rm) != 0 || len(teamRm) != 0 { _, err := client.PullRequests.DeleteReviewRequests(requestCtx, owner, repo, index, gitea.PullReviewRequestOptions{ - Reviewers: rm, + Reviewers: rm, + TeamReviewers: teamRm, }) if err != nil { return fmt.Errorf("could not remove reviewers: %w", err) } } - if len(add) != 0 { + if len(add) != 0 || len(teamAdd) != 0 { _, err := client.PullRequests.CreateReviewRequests(requestCtx, owner, repo, index, gitea.PullReviewRequestOptions{ - Reviewers: add, + Reviewers: add, + TeamReviewers: teamAdd, }) if err != nil { return fmt.Errorf("could not add reviewers: %w", err) diff --git a/modules/task/pull_create.go b/modules/task/pull_create.go index 6e195d4a..07e8e8bf 100644 --- a/modules/task/pull_create.go +++ b/modules/task/pull_create.go @@ -24,8 +24,24 @@ var ( consecutive = regexp.MustCompile(`[\s]{2,}`) ) +// CreatePullOptions bundles the inputs to CreatePull. The struct form keeps +// call sites readable as the API surface grows (and avoids the +// positional-arg drift that earlier PRs caused). +type CreatePullOptions struct { + Base string + Head string + AllowMaintainerEdits *bool + Issue *gitea.CreateIssueOption + Reviewers []string + TeamReviewers []string +} + // CreatePull creates a PR in the given repo and prints the result -func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext, base, head string, allowMaintainerEdits *bool, opts *gitea.CreateIssueOption, reviewers, teamReviewers []string) (err error) { +func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext, in CreatePullOptions) (err error) { + base := in.Base + head := in.Head + opts := in.Issue + // default is default branch if len(base) == 0 { base, err = GetDefaultPRBase(requestCtx, ctx.Login, ctx.Owner, ctx.Repo) @@ -69,8 +85,8 @@ func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext, base, head s Title: opts.Title, Body: opts.Body, Assignees: opts.Assignees, - Reviewers: reviewers, - TeamReviewers: teamReviewers, + Reviewers: in.Reviewers, + TeamReviewers: in.TeamReviewers, Labels: opts.Labels, Milestone: opts.Milestone, Deadline: opts.Deadline, @@ -79,9 +95,9 @@ func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext, base, head s return fmt.Errorf("could not create PR from %s to %s:%s: %s", head, ctx.Owner, base, err) } - if allowMaintainerEdits != nil && pr.AllowMaintainerEdit != *allowMaintainerEdits { + if in.AllowMaintainerEdits != nil && pr.AllowMaintainerEdit != *in.AllowMaintainerEdits { pr, _, err = client.PullRequests.EditPullRequest(requestCtx, ctx.Owner, ctx.Repo, pr.Index, gitea.EditPullRequestOption{ - AllowMaintainerEdit: allowMaintainerEdits, + AllowMaintainerEdit: in.AllowMaintainerEdits, }) if err != nil { return fmt.Errorf("could not enable maintainer edit on pull: %v", err) diff --git a/modules/task/pull_edit.go b/modules/task/pull_edit.go index 4606fc78..cb671e4a 100644 --- a/modules/task/pull_edit.go +++ b/modules/task/pull_edit.go @@ -67,7 +67,7 @@ func EditPull(requestCtx stdctx.Context, ctx *context.TeaContext, client *gitea. return nil, err } - if err := ApplyReviewerChanges(requestCtx, client, ctx.Owner, ctx.Repo, opts.Index, opts.AddReviewers, opts.RemoveReviewers); err != nil { + if err := ApplyReviewerChanges(requestCtx, client, ctx.Owner, ctx.Repo, opts.Index, opts.AddReviewers, opts.RemoveReviewers, nil, nil); err != nil { return nil, err } diff --git a/tests/integration/pulls_reviewers_test.go b/tests/integration/pulls_reviewers_test.go new file mode 100644 index 00000000..e4a735ce --- /dev/null +++ b/tests/integration/pulls_reviewers_test.go @@ -0,0 +1,195 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "context" + "fmt" + "os/exec" + "strings" + "testing" + "time" + + gitea "gitea.dev/sdk" + "gitea.dev/tea/cmd" + "github.com/stretchr/testify/require" +) + +// TestRequestReviewFlow exercises the request-review, cancel-review, and +// reviewers subcommands plus the create-time --reviewer/--team-reviewer +// flags end-to-end against a real Gitea instance. +// +// It reuses the org/user/team scaffolding from TestEditPull_ModifiesAssignees +// (writer team with members user1/user2) so PR creation, reviewer request, +// and cancel all share the same fixture. +func TestRequestReviewFlow(t *testing.T) { + login := createIntegrationLogin(t) + client := login.Client() + ctx := context.Background() + + orgName := fmt.Sprintf("rr-org-%d", time.Now().UnixNano()%1_000_000) + orgRepoName := fmt.Sprintf("rr-repo-%d", time.Now().UnixNano()%1_000_000) + + _, _ = client.Repositories.DeleteRepo(ctx, orgName, orgRepoName) + _, _ = client.Organizations.DeleteOrg(ctx, orgName) + _, _ = client.Admin.DeleteUser(ctx, "rr-user1") + _, _ = client.Admin.DeleteUser(ctx, "rr-user2") + + _, _, err := client.Admin.CreateOrg(ctx, integrationUsername, gitea.CreateOrgOption{Name: orgName}) + require.NoError(t, err) + t.Cleanup(func() { + if _, err := client.Organizations.DeleteOrg(ctx, orgName); err != nil { + t.Logf("failed to delete integration org %q: %v", orgName, err) + } + }) + + orgRepo, _, err := client.Repositories.CreateOrgRepo(ctx, orgName, gitea.CreateRepoOption{Name: orgRepoName}) + require.NoError(t, err) + t.Cleanup(func() { + if _, err := client.Repositories.DeleteRepo(ctx, orgName, orgRepoName); err != nil { + t.Logf("failed to delete integration repo %q: %v", orgRepoName, err) + } + }) + + user1, _, err := client.Admin.CreateUser(ctx, gitea.CreateUserOption{Username: "rr-user1", Password: "rr-user1!1234", Email: "rr-user1@test.com"}) + require.NoError(t, err) + _ = user1 + _, _, err = client.Admin.CreateUser(ctx, gitea.CreateUserOption{Username: "rr-user2", Password: "rr-user2!1234", Email: "rr-user2@test.com"}) + require.NoError(t, err) + t.Cleanup(func() { + _, _ = client.Admin.DeleteUser(ctx, "rr-user1") + _, _ = client.Admin.DeleteUser(ctx, "rr-user2") + }) + + team, _, err := client.Organizations.CreateTeam(ctx, orgName, gitea.CreateTeamOption{Name: "reviewers", Permission: gitea.AccessModeRead}) + require.NoError(t, err) + _, err = client.Organizations.AddTeamMember(ctx, team.ID, "rr-user1") + require.NoError(t, err) + _, err = client.Organizations.AddTeamMember(ctx, team.ID, "rr-user2") + require.NoError(t, err) + _, err = client.Organizations.AddTeamRepository(ctx, team.ID, orgName, orgRepoName) + require.NoError(t, err) + + // rr-user1/rr-user2 must be discoverable as PR reviewers via + // GET /repos/{owner}/{repo}/reviewers; this is also what the + // 'tea pulls reviewers' subcommand calls. + require.True(t, isReviewable(ctx, t, client, orgName, orgRepoName, "rr-user1")) + require.True(t, isReviewable(ctx, t, client, orgName, orgRepoName, "rr-user2")) + + tmpDir := t.TempDir() + runGit := func(args ...string) { + t.Helper() + c := exec.Command("git", args...) + c.Dir = tmpDir + require.NoError(t, c.Run()) + } + runGit("init") + runGit("config", "user.email", "rr@test.com") + runGit("config", "user.name", "rr") + httpsURL := fmt.Sprintf("%s/%s.git", login.URL, orgRepo.FullName) + httpsURL = strings.Replace(httpsURL, "://", fmt.Sprintf("://%s:%s@", login.Name, login.Token), 1) + runGit("remote", "add", "origin", httpsURL) + + runGit("checkout", "-b", "main") + runGit("commit", "--allow-empty", "-m", "Initial commit") + runGit("push", "-u", "origin", "HEAD:main") + runGit("checkout", "-b", "feature-branch") + runGit("commit", "--allow-empty", "-m", "feature work") + runGit("push", "-u", "origin", "HEAD:feature-branch") + waitForBranches(t, orgRepo.FullName, "feature-branch") + + fetchPR := func() *gitea.PullRequest { + t.Helper() + pr, _, err := client.PullRequests.GetPullRequest(ctx, orgName, orgRepoName, 1) + require.NoError(t, err) + return pr + } + reviewerNames := func(pr *gitea.PullRequest) []string { + names := make([]string, len(pr.RequestedReviewers)) + for i, u := range pr.RequestedReviewers { + names[i] = u.UserName + } + return names + } + teamNames := func(pr *gitea.PullRequest) []string { + names := make([]string, len(pr.RequestedReviewersTeams)) + for i, t := range pr.RequestedReviewersTeams { + names[i] = t.Name + } + return names + } + + app := cmd.App() + + // 1. create-time --reviewer / --team-reviewer should populate both lists. + err = app.Run(ctx, []string{ + "tea", "pr", "create", + "--repo", orgRepo.FullName, + "--base", "main", + "--head", "feature-branch", + "--title", "reviewer test", + "--reviewer", "rr-user1", + "--team-reviewer", "reviewers", + }) + require.NoError(t, err) + pr := fetchPR() + require.ElementsMatch(t, []string{"rr-user1"}, reviewerNames(pr)) + require.ElementsMatch(t, []string{"reviewers"}, teamNames(pr)) + + // 2. request-review should add another user without disturbing existing reviewers. + err = app.Run(ctx, []string{ + "tea", "pr", "request-review", "1", + "--repo", orgRepo.FullName, + "--reviewer", "rr-user2", + }) + require.NoError(t, err) + pr = fetchPR() + require.ElementsMatch(t, []string{"rr-user1", "rr-user2"}, reviewerNames(pr)) + + // 3. cancel-review should remove the requested reviewer. + err = app.Run(ctx, []string{ + "tea", "pr", "cancel-review", "1", + "--repo", orgRepo.FullName, + "--reviewer", "rr-user2", + }) + require.NoError(t, err) + pr = fetchPR() + require.ElementsMatch(t, []string{"rr-user1"}, reviewerNames(pr)) + + // 4. cancel-review should also accept --team-reviewer. + err = app.Run(ctx, []string{ + "tea", "pr", "cancel-review", "1", + "--repo", orgRepo.FullName, + "--team-reviewer", "reviewers", + }) + require.NoError(t, err) + pr = fetchPR() + require.ElementsMatch(t, []string{}, teamNames(pr)) + require.ElementsMatch(t, []string{"rr-user1"}, reviewerNames(pr)) + + // 5. request-review with neither --reviewer nor --team-reviewer should fail. + err = app.Run(ctx, []string{"tea", "pr", "request-review", "1", "--repo", orgRepo.FullName}) + require.Error(t, err) + require.Contains(t, err.Error(), "at least one of --reviewer or --team-reviewer is required") + + // 6. request-review against a non-existent PR should surface the API error. + err = app.Run(ctx, []string{ + "tea", "pr", "request-review", "999", + "--repo", orgRepo.FullName, + "--reviewer", "rr-user1", + }) + require.Error(t, err) +} + +func isReviewable(ctx context.Context, t *testing.T, client *gitea.Client, owner, repo, user string) bool { + t.Helper() + reviewers, _, err := client.Repositories.GetReviewers(ctx, owner, repo) + require.NoError(t, err) + for _, u := range reviewers { + if u.UserName == user { + return true + } + } + return false +}