Skip to content

Fix pr edit when URL is provided #11057

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 2, 2025
Merged

Fix pr edit when URL is provided #11057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions api/queries_pr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
26 changes: 2 additions & 24 deletions pkg/cmd/pr/edit/edit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package edit
import (
"fmt"
"net/http"
"time"

"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/v2/api"
Expand Down Expand Up @@ -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,
}

Expand All @@ -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
Expand All @@ -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 {
Expand Down
24 changes: 18 additions & 6 deletions pkg/cmd/pr/edit/edit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{},
Expand All @@ -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},
Expand All @@ -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},
Expand Down
31 changes: 31 additions & 0 deletions pkg/cmd/pr/shared/finder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for explaining a question before raised

// 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
Expand Down Expand 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()
}

Expand Down
76 changes: 76 additions & 0 deletions pkg/cmd/pr/shared/finder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
Loading