STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix pr edit when URL is provided

Browse source
This commit is contained in:
William Martin 2025-06-02 14:33:38 +02:00
parent b4ab34371d
commit 793a7ff459
5 changed files with 146 additions and 39 deletions
Download patch file Download diff file Expand all files Collapse all files

28
api/queries_pr.go
View file

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

26
pkg/cmd/pr/edit/edit.go
View file

@ -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 {

24
pkg/cmd/pr/edit/edit_test.go
View file

@ -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},

31
pkg/cmd/pr/shared/finder.go
View file

@ -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()
}

76
pkg/cmd/pr/shared/finder_test.go
View file

@ -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