From 793a7ff459ba7b121c4c2f21186b983b73d69fa9 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 2 Jun 2025 14:33:38 +0200 Subject: [PATCH] Fix pr edit when URL is provided --- api/queries_pr.go | 28 ++++++++---- pkg/cmd/pr/edit/edit.go | 26 +---------- pkg/cmd/pr/edit/edit_test.go | 24 +++++++--- pkg/cmd/pr/shared/finder.go | 31 +++++++++++++ pkg/cmd/pr/shared/finder_test.go | 76 ++++++++++++++++++++++++++++++++ 5 files changed, 146 insertions(+), 39 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 525418a11..1d394e864 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -85,15 +85,25 @@ type PullRequest struct { Assignees Assignees AssignedActors AssignedActors - Labels Labels - ProjectCards ProjectCards - ProjectItems ProjectItems - Milestone *Milestone - Comments Comments - ReactionGroups ReactionGroups - Reviews PullRequestReviews - LatestReviews PullRequestReviews - ReviewRequests ReviewRequests + // 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 ClosingIssuesReferences ClosingIssuesReferences } diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index c30c4f9bb..867ee9a4f 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -3,7 +3,6 @@ package edit import ( "fmt" "net/http" - "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" @@ -206,7 +205,7 @@ 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"}, + Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone", "assignees"}, Detector: opts.Detector, } @@ -215,27 +214,6 @@ func editRun(opts *EditOptions) error { 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()) - } - - 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 @@ -247,7 +225,7 @@ func editRun(opts *EditOptions) error { editable.Body.Default = pr.Body editable.Base.Default = pr.BaseRefName editable.Reviewers.Default = pr.ReviewRequests.Logins() - if issueFeatures.ActorIsAssignable { + if pr.AssignedActorsUsed { editable.Assignees.ActorAssignees = true editable.Assignees.Default = pr.AssignedActors.DisplayNames() } else { diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 3a1e8a548..0c40e3839 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -341,9 +341,11 @@ func Test_editRun(t *testing.T) { { name: "non-interactive", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -403,9 +405,11 @@ func Test_editRun(t *testing.T) { { name: "non-interactive skip reviewers", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -459,9 +463,11 @@ func Test_editRun(t *testing.T) { { name: "non-interactive remove all reviewers", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -520,9 +526,11 @@ func Test_editRun(t *testing.T) { { name: "interactive", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{}, @@ -542,9 +550,11 @@ func Test_editRun(t *testing.T) { { name: "interactive skip reviewers", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{skipReviewers: true}, @@ -563,9 +573,11 @@ func Test_editRun(t *testing.T) { { name: "interactive remove all reviewers", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{removeAllReviewers: true}, diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index b8d23c978..a19c69669 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -242,6 +242,33 @@ 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 @@ -297,6 +324,10 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err }) } + if actorAssigneesUsed { + pr.AssignedActorsUsed = true + } + return pr, f.baseRefRepo, g.Wait() } diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index abc754d1a..28c2337d3 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -9,6 +9,7 @@ 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" @@ -705,6 +706,81 @@ 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/123", + }) + 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/123", + }) + 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