From 23c9ff6d89059c551c74fc7ea3ba6e1135ed47d8 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Tue, 1 Jul 2025 10:49:18 +0100 Subject: [PATCH 01/10] fix: expose `ParseURL` as a public func Signed-off-by: Babak K. Shandiz --- pkg/cmd/pr/shared/finder.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index a19c69669..51860e102 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 } @@ -333,7 +333,9 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err 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) } From 00a86a105ebac17e9dc9b0eb6f0b3547e3189145 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Tue, 1 Jul 2025 11:11:28 +0100 Subject: [PATCH 02/10] fix: remove `AssignedActorsUsed` field Signed-off-by: Babak K. Shandiz --- api/queries_pr.go | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) 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 } From 12f4794151f550be49df5bbdb5db14f1a154310a Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Tue, 1 Jul 2025 11:12:16 +0100 Subject: [PATCH 03/10] fix: remove assignee-related intervention Signed-off-by: Babak K. Shandiz --- pkg/cmd/pr/shared/finder.go | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 51860e102..df8cc0fd4 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -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,10 +297,6 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err }) } - if actorAssigneesUsed { - pr.AssignedActorsUsed = true - } - return pr, f.baseRefRepo, g.Wait() } From f65683784e0847f8403cee2eb3ea81b7bf7d9353 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Tue, 1 Jul 2025 11:12:58 +0100 Subject: [PATCH 04/10] test: remove tests verifying assigne-related behaviour Signed-off-by: Babak K. Shandiz --- pkg/cmd/pr/shared/finder_test.go | 76 -------------------------------- 1 file changed, 76 deletions(-) diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 0fd96e09b..abc754d1a 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -9,7 +9,6 @@ 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" @@ -706,81 +705,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 From b85dc94f843f81928ec9347c5417d5d5b09c7cd0 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Tue, 1 Jul 2025 11:14:50 +0100 Subject: [PATCH 05/10] refactor: select PR fields based on detected features This change is almost a revert to what had already done in #10984. Signed-off-by: Babak K. Shandiz --- pkg/cmd/pr/edit/edit.go | 47 +++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index becbfce47..e0ef29054 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,23 @@ 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, parse it to get the base repository. + 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 +215,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 +258,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() From 2c86246a1aec3e5f0366ff84130abcc876d5a7a0 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Tue, 1 Jul 2025 11:18:38 +0100 Subject: [PATCH 06/10] test: verify providing a URL arg affects the base repo Signed-off-by: Babak K. Shandiz --- pkg/cmd/pr/edit/edit_test.go | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 374625912..cf126eceb 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, + ) + } }) } } From 58ed691a6c4f43906a6d5c1dcc77dcb1822fadc0 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Tue, 1 Jul 2025 11:19:13 +0100 Subject: [PATCH 07/10] test: remove references to `AssignedActorsUsed` field Signed-off-by: Babak K. Shandiz --- pkg/cmd/pr/edit/edit_test.go | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index cf126eceb..bb468a307 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -364,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{ @@ -428,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{ @@ -486,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{ @@ -549,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{ @@ -596,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{ @@ -640,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{ @@ -687,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{ { From dc51226ce2680391c0dcee4f6b68fdd483815702 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Tue, 1 Jul 2025 11:21:24 +0100 Subject: [PATCH 08/10] test: improve test case to highlight host name override Signed-off-by: Babak K. Shandiz --- pkg/cmd/issue/edit/edit_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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, }, { From b7fa5ea7a93ee23a178f736c65b2cc0bf6eee659 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Tue, 1 Jul 2025 11:41:07 +0100 Subject: [PATCH 09/10] docs: explain PR URL parsing reason Signed-off-by: Babak K. Shandiz --- pkg/cmd/pr/edit/edit.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index e0ef29054..c01364d24 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -91,7 +91,12 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman } if opts.SelectorArg != "" { - // If a URL is provided, parse it to get the base repository. + // 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 From 8ab5e84a1202009c8c8290954fcd3b3d89eb9e0e Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Thu, 3 Jul 2025 10:21:55 +0100 Subject: [PATCH 10/10] test: add test for `ParseURL` Signed-off-by: Babak K. Shandiz --- pkg/cmd/pr/shared/finder_test.go | 64 ++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index abc754d1a..0f6da5a6e 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -14,6 +14,70 @@ import ( "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)