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