diff --git a/api/queries_pr.go b/api/queries_pr.go index 1d394e864..525418a11 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -85,25 +85,15 @@ type PullRequest struct { Assignees Assignees AssignedActors AssignedActors - // AssignedActorsUsed is a GIGANTIC hack to carry around whether we expected AssignedActors to be requested - // on this PR. This is required because the Feature Detection of support for AssignedActors occurs inside the - // PR Finder, but knowledge of support is required at the command level. However, we can't easily construct - // the feature detector at the command level because it needs knowledge of the BaseRepo, which is only available - // inside the PR Finder. This is bad and we should feel bad. - // - // The right solution is to extract argument parsing from the PR Finder into each command, so that we have access - // to the BaseRepo and can construct the feature detector there. This is what happens in the issue commands with - // `shared.ParseIssueFromArg`. - AssignedActorsUsed bool - Labels Labels - ProjectCards ProjectCards - ProjectItems ProjectItems - Milestone *Milestone - Comments Comments - ReactionGroups ReactionGroups - Reviews PullRequestReviews - LatestReviews PullRequestReviews - ReviewRequests ReviewRequests + Labels Labels + ProjectCards ProjectCards + ProjectItems ProjectItems + Milestone *Milestone + Comments Comments + ReactionGroups ReactionGroups + Reviews PullRequestReviews + LatestReviews PullRequestReviews + ReviewRequests ReviewRequests ClosingIssuesReferences ClosingIssuesReferences } diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index d14b2f462..caf99df58 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -263,12 +263,12 @@ func TestNewCmdEdit(t *testing.T) { }, { name: "argument is a URL", - input: "https://github.com/cli/cli/issues/23", + input: "https://example.com/cli/cli/issues/23", output: EditOptions{ IssueNumbers: []int{23}, Interactive: true, }, - expectedBaseRepo: ghrepo.New("cli", "cli"), + expectedBaseRepo: ghrepo.NewWithHost("cli", "cli", "example.com"), wantsErr: false, }, { diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index becbfce47..c01364d24 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -3,6 +3,7 @@ package edit import ( "fmt" "net/http" + "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" @@ -81,12 +82,28 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Finder = shared.NewFinder(f) + + // support `-R, --repo` override opts.BaseRepo = f.BaseRepo if len(args) > 0 { opts.SelectorArg = args[0] } + if opts.SelectorArg != "" { + // If a URL is provided, we need to parse it to override the + // base repository, especially the hostname part. That's because + // we need a feature detector down in this command, and that + // needs to know the API host. If the command is run outside of + // a git repo, we cannot instantiate the detector unless we have + // already parsed the URL. + if baseRepo, _, err := shared.ParseURL(opts.SelectorArg); err == nil { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } + } + flags := cmd.Flags() bodyProvided := flags.Changed("body") @@ -203,17 +220,38 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman } func editRun(opts *EditOptions) error { - findOptions := shared.FindOptions{ - Selector: opts.SelectorArg, - Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone", "assignees"}, - Detector: opts.Detector, - } - httpClient, err := opts.HttpClient() if err != nil { return err } + if opts.Detector == nil { + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost()) + } + + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, + Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"}, + Detector: opts.Detector, + } + + issueFeatures, err := opts.Detector.IssueFeatures() + if err != nil { + return err + } + + if issueFeatures.ActorIsAssignable { + findOptions.Fields = append(findOptions.Fields, "assignedActors") + } else { + findOptions.Fields = append(findOptions.Fields, "assignees") + } + pr, repo, err := opts.Finder.Find(findOptions) if err != nil { return err @@ -225,7 +263,7 @@ func editRun(opts *EditOptions) error { editable.Body.Default = pr.Body editable.Base.Default = pr.BaseRefName editable.Reviewers.Default = pr.ReviewRequests.Logins() - if pr.AssignedActorsUsed { + if issueFeatures.ActorIsAssignable { editable.Assignees.ActorAssignees = true editable.Assignees.Default = pr.AssignedActors.DisplayNames() editable.Assignees.DefaultLogins = pr.AssignedActors.Logins() diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 374625912..bb468a307 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -26,11 +26,12 @@ func TestNewCmdEdit(t *testing.T) { require.NoError(t, err) tests := []struct { - name string - input string - stdin string - output EditOptions - wantsErr bool + name string + input string + stdin string + output EditOptions + expectedBaseRepo ghrepo.Interface + wantsErr bool }{ { name: "no argument", @@ -47,6 +48,16 @@ func TestNewCmdEdit(t *testing.T) { output: EditOptions{}, wantsErr: true, }, + { + name: "URL argument", + input: "https://example.com/cli/cli/pull/23", + output: EditOptions{ + SelectorArg: "https://example.com/cli/cli/pull/23", + Interactive: true, + }, + expectedBaseRepo: ghrepo.NewWithHost("cli", "cli", "example.com"), + wantsErr: false, + }, { name: "pull request number argument", input: "23", @@ -326,6 +337,15 @@ func TestNewCmdEdit(t *testing.T) { assert.Equal(t, tt.output.SelectorArg, gotOpts.SelectorArg) assert.Equal(t, tt.output.Interactive, gotOpts.Interactive) assert.Equal(t, tt.output.Editable, gotOpts.Editable) + if tt.expectedBaseRepo != nil { + baseRepo, err := gotOpts.BaseRepo() + require.NoError(t, err) + require.True( + t, + ghrepo.IsSame(tt.expectedBaseRepo, baseRepo), + "expected base repo %+v, got %+v", tt.expectedBaseRepo, baseRepo, + ) + } }) } } @@ -344,8 +364,7 @@ func Test_editRun(t *testing.T) { Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", - AssignedActorsUsed: true, + URL: "https://github.com/OWNER/REPO/pull/123", }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -408,8 +427,7 @@ func Test_editRun(t *testing.T) { Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", - AssignedActorsUsed: true, + URL: "https://github.com/OWNER/REPO/pull/123", }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -466,8 +484,7 @@ func Test_editRun(t *testing.T) { Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", - AssignedActorsUsed: true, + URL: "https://github.com/OWNER/REPO/pull/123", }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -529,8 +546,7 @@ func Test_editRun(t *testing.T) { Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", - AssignedActorsUsed: true, + URL: "https://github.com/OWNER/REPO/pull/123", }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{ @@ -576,8 +592,7 @@ func Test_editRun(t *testing.T) { Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", - AssignedActorsUsed: true, + URL: "https://github.com/OWNER/REPO/pull/123", }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{ @@ -620,8 +635,7 @@ func Test_editRun(t *testing.T) { Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", - AssignedActorsUsed: true, + URL: "https://github.com/OWNER/REPO/pull/123", }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{ @@ -667,8 +681,7 @@ func Test_editRun(t *testing.T) { Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", - AssignedActorsUsed: true, + URL: "https://github.com/OWNER/REPO/pull/123", AssignedActors: api.AssignedActors{ Nodes: []api.Actor{ { diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index a19c69669..df8cc0fd4 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -112,7 +112,7 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err return nil, nil, errors.New("Find error: no fields specified") } - if repo, prNumber, err := f.parseURL(opts.Selector); err == nil { + if repo, prNumber, err := ParseURL(opts.Selector); err == nil { f.prNumber = prNumber f.baseRefRepo = repo } @@ -242,33 +242,6 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err } } - // Ok this is super, super horrible so bear with me. - // The `assignees` field on a Pull Request exposes users that are assigned. It is also possible for bots to be - // assigned, but they only appear under the `assignedActors` field. Ideally, the caller of `Find` would determine - // the correct field to use based on the `fd.Detector` that is passed in, but they can't construct a detector - // because the BaseRepo is only determined within this function. The more correct solution is to do what I did with - // the issue commands and decouple argument parsing from API lookup. See PR #10811 for example. - var actorAssigneesUsed bool - if fields.Contains("assignees") { - if opts.Detector == nil { - cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) - opts.Detector = fd.NewDetector(cachedClient, f.baseRefRepo.RepoHost()) - } - - issueFeatures, err := opts.Detector.IssueFeatures() - if err != nil { - return nil, nil, fmt.Errorf("error detecting issue features: %v", err) - } - - // If actors are assignable on this host then we additionally request the `assignedActors` field. - // Note that we don't remove the `assignees` field because some commands (`pr view`) do not display actor - // assignees yet, so we have to have both sets of data. - if issueFeatures.ActorIsAssignable { - fields.Add("assignedActors") - actorAssigneesUsed = true - } - } - var pr *api.PullRequest if f.prNumber > 0 { // If we have a PR number, let's look it up @@ -324,16 +297,14 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err }) } - if actorAssigneesUsed { - pr.AssignedActorsUsed = true - } - return pr, f.baseRefRepo, g.Wait() } var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) -func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) { +// ParseURL parses a pull request URL and returns the repository and pull +// request number. +func ParseURL(prURL string) (ghrepo.Interface, int, error) { if prURL == "" { return nil, 0, fmt.Errorf("invalid URL: %q", prURL) } diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 0fd96e09b..0f6da5a6e 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -9,12 +9,75 @@ import ( ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" - fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/require" ) +func TestParseURL(t *testing.T) { + tests := []struct { + name string + arg string + wantRepo ghrepo.Interface + wantNum int + wantErr string + }{ + { + name: "valid HTTPS URL", + arg: "https://example.com/owner/repo/pull/123", + wantRepo: ghrepo.NewWithHost("owner", "repo", "example.com"), + wantNum: 123, + }, + { + name: "valid HTTP URL", + arg: "http://example.com/owner/repo/pull/123", + wantRepo: ghrepo.NewWithHost("owner", "repo", "example.com"), + wantNum: 123, + }, + { + name: "empty URL", + wantErr: "invalid URL: \"\"", + }, + { + name: "invalid scheme", + arg: "ftp://github.com/owner/repo/pull/123", + wantErr: "invalid scheme: ftp", + }, + { + name: "incorrect path", + arg: "https://github.com/owner/repo/issues/123", + wantErr: "not a pull request URL: https://github.com/owner/repo/issues/123", + }, + { + name: "no PR number", + arg: "https://github.com/owner/repo/pull/", + wantErr: "not a pull request URL: https://github.com/owner/repo/pull/", + }, + { + name: "invalid PR number", + arg: "https://github.com/owner/repo/pull/foo", + wantErr: "not a pull request URL: https://github.com/owner/repo/pull/foo", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo, num, err := ParseURL(tt.arg) + + if tt.wantErr != "" { + require.Error(t, err) + require.Equal(t, tt.wantErr, err.Error()) + return + } + + require.NoError(t, err) + require.Equal(t, tt.wantNum, num) + require.NotNil(t, repo) + require.True(t, ghrepo.IsSame(tt.wantRepo, repo)) + }) + } +} + type args struct { baseRepoFn func() (ghrepo.Interface, error) branchFn func() (string, error) @@ -706,81 +769,6 @@ func TestFind(t *testing.T) { } } -func TestFindAssignableActors(t *testing.T) { - t.Run("given actors are not assignable, do nothing special", func(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - // Ensure we never request assignedActors - reg.Exclude(t, httpmock.GraphQL(`assignedActors`)) - reg.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "pullRequest":{"number":13} - }}}`)) - - f := finder{ - httpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - }, - } - - pr, _, err := f.Find(FindOptions{ - Detector: &fd.DisabledDetectorMock{}, - Fields: []string{"assignees"}, - Selector: "https://github.com/cli/cli/pull/13", - }) - require.NoError(t, err) - - require.False(t, pr.AssignedActorsUsed, "expected PR not to have assigned actors used") - }) - - t.Run("given actors are assignable, request assignedActors and indicate that on the returned PR", func(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - // Ensure that we only respond if assignedActors is requested - reg.Register( - httpmock.GraphQL(`assignedActors`), - httpmock.StringResponse(`{"data":{"repository":{ - "pullRequest":{ - "number":13, - "assignedActors": { - "nodes": [ - { - "id": "HUBOTID", - "login": "hubot", - "__typename": "Bot" - }, - { - "id": "MONAID", - "login": "MonaLisa", - "name": "Mona Display Name", - "__typename": "User" - } - ], - "totalCount": 2 - }} - }}}`)) - - f := finder{ - httpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - }, - } - - pr, _, err := f.Find(FindOptions{ - Detector: &fd.EnabledDetectorMock{}, - Fields: []string{"assignees"}, - Selector: "https://github.com/cli/cli/pull/13", - }) - require.NoError(t, err) - - require.Equal(t, []string{"hubot", "MonaLisa"}, pr.AssignedActors.Logins()) - require.True(t, pr.AssignedActorsUsed, "expected PR to have assigned actors used") - }) -} - func stubBranchConfig(branchConfig git.BranchConfig, err error) func(context.Context, string) (git.BranchConfig, error) { return func(_ context.Context, branch string) (git.BranchConfig, error) { return branchConfig, err