From 5a3aee056a9f0661d7c088a4a634b03e30d2d8eb Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 2 May 2025 16:43:36 +0200 Subject: [PATCH 1/2] Feature detect v1 projects on interactive pr create --- internal/prompter/test.go | 12 ++ pkg/cmd/pr/create/create.go | 7 +- pkg/cmd/pr/create/create_test.go | 210 ++++++++++++++++++++++++++++++- 3 files changed, 219 insertions(+), 10 deletions(-) diff --git a/internal/prompter/test.go b/internal/prompter/test.go index 04375ce76..dfa124fca 100644 --- a/internal/prompter/test.go +++ b/internal/prompter/test.go @@ -141,6 +141,18 @@ func IndexFor(options []string, answer string) (int, error) { return -1, NoSuchAnswerErr(answer, options) } +func IndexesFor(options []string, answers ...string) ([]int, error) { + indexes := make([]int, len(answers)) + for i, answer := range answers { + index, err := IndexFor(options, answer) + if err != nil { + return nil, err + } + indexes[i] = index + } + return indexes, nil +} + func NoSuchPromptErr(prompt string) error { return fmt.Errorf("no such prompt '%s'", prompt) } diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 6ce11a712..64469543f 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -456,7 +456,6 @@ func createRun(opts *CreateOptions) error { if err != nil { return err } - // TODO wm: revisit project support return submitPR(*opts, *ctx, *state, projectsV1Support) } @@ -553,8 +552,7 @@ func createRun(opts *CreateOptions) error { Repo: ctx.PRRefs.BaseRepo(), State: state, } - // TODO wm: revisit project support - err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, gh.ProjectsV1Supported) + err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, projectsV1Support) if err != nil { return err } @@ -583,12 +581,10 @@ func createRun(opts *CreateOptions) error { if action == shared.SubmitDraftAction { state.Draft = true - // TODO wm: revisit project support return submitPR(*opts, *ctx, *state, projectsV1Support) } if action == shared.SubmitAction { - // TODO wm: revisit project support return submitPR(*opts, *ctx, *state, projectsV1Support) } @@ -1237,7 +1233,6 @@ func generateCompareURL(ctx CreateContext, state shared.IssueMetadataState, proj ctx.PRRefs.BaseRepo(), "compare/%s...%s?expand=1", url.PathEscape(ctx.PRRefs.BaseRef()), url.PathEscape(ctx.PRRefs.QualifiedHeadRef())) - // TODO wm: revisit project support url, err := shared.WithPrAndIssueQueryParams(ctx.Client, ctx.PRRefs.BaseRepo(), u, state, projectsV1Support) if err != nil { return "", err diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 59a974df6..ac96db0d6 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1943,7 +1943,6 @@ func Test_generateCompareURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // TODO wm: projects v1 support? got, err := generateCompareURL(tt.ctx, tt.state, gh.ProjectsV1Supported) if (err != nil) != tt.wantErr { t.Errorf("generateCompareURL() error = %v, wantErr %v", err, tt.wantErr) @@ -2011,8 +2010,6 @@ func mockRetrieveProjects(_ *testing.T, reg *httpmock.Registry) { `)) } -// TODO interactive metadata tests once: 1) we have test utils for Prompter and 2) metadata questions use Prompter - // TODO projectsV1Deprecation // Remove this test. func TestProjectsV1Deprecation(t *testing.T) { @@ -2128,6 +2125,211 @@ func TestProjectsV1Deprecation(t *testing.T) { }) }) + t.Run("interactive submission", func(t *testing.T) { + t.Run("when projects v1 is supported, queries for it", func(t *testing.T) { + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "") + cs.Register("git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, "") + cs.Register(`git rev-parse --show-toplevel`, 0, "") + + // When the command is run + reg := &httpmock.Registry{} + reg.StubRepoResponse("OWNER", "REPO") + + reg.Register( + httpmock.GraphQL(`query PullRequestTemplates\b`), + httpmock.StringResponse(`{ "data": { "repository": { "pullRequestTemplates": [] } } }`), + ) + + reg.Register( + // ( is required to avoid matching projectsV2 + httpmock.GraphQL(`projects\(`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + // Register a handler to check for projects V2 just to avoid the registry panicking, even + // though we return a 500 error. This is because the project lookup is done in parallel + // so the previous error doesn't early exit. + reg.Register( + httpmock.GraphQL(`projectsV2`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + ios, _, _, _ := iostreams.Test() + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + pm := &prompter.PrompterMock{} + pm.InputFunc = func(p, _ string) (string, error) { + if p == "Title (required)" { + return "Test Title", nil + } else { + return "", prompter.NoSuchPromptErr(p) + } + } + pm.MarkdownEditorFunc = func(p, _ string, ba bool) (string, error) { + if p == "Body" { + return "Test Body", nil + } else { + return "", prompter.NoSuchPromptErr(p) + } + } + pm.SelectFunc = func(p, _ string, opts []string) (int, error) { + switch p { + case "Choose a template": + return 0, nil + case "What's next?": + return prompter.IndexFor(opts, "Add metadata") + default: + return -1, prompter.NoSuchPromptErr(p) + } + } + pm.MultiSelectFunc = func(p string, _ []string, opts []string) ([]int, error) { + return prompter.IndexesFor(opts, "Projects") + } + + opts := CreateOptions{ + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + Config: func() (gh.Config, error) { + return config.NewBlankConfig(), nil + }, + Browser: &browser.Stub{}, + IO: ios, + Prompter: pm, + GitClient: &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + }, + Finder: shared.NewMockFinder("feature", nil, nil), + Detector: &fd.EnabledDetectorMock{}, + Remotes: func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + }, + Branch: func() (string, error) { + return "feature", nil + }, + + HeadBranch: "feature", + } + + // Ignore the error because we have no way to really stub it without + // fully stubbing a GQL error structure in the request body. + _ = createRun(&opts) + + // Verify that our request contained projects + reg.Verify(t) + }) + + t.Run("when projects v1 is not supported, does not query for it", func(t *testing.T) { + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "") + cs.Register("git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, "") + cs.Register(`git rev-parse --show-toplevel`, 0, "") + + // When the command is run + reg := &httpmock.Registry{} + reg.StubRepoResponse("OWNER", "REPO") + + reg.Register( + httpmock.GraphQL(`query PullRequestTemplates\b`), + httpmock.StringResponse(`{ "data": { "repository": { "pullRequestTemplates": [] } } }`), + ) + + // ( is required to avoid matching projectsV2 + reg.Exclude(t, httpmock.GraphQL(`projects\(`)) + + ios, _, _, _ := iostreams.Test() + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + + pm := &prompter.PrompterMock{} + pm.InputFunc = func(p, _ string) (string, error) { + if p == "Title (required)" { + return "Test Title", nil + } else { + return "", prompter.NoSuchPromptErr(p) + } + } + pm.MarkdownEditorFunc = func(p, _ string, ba bool) (string, error) { + if p == "Body" { + return "Test Body", nil + } else { + return "", prompter.NoSuchPromptErr(p) + } + } + pm.SelectFunc = func(p, _ string, opts []string) (int, error) { + switch p { + case "Choose a template": + return 0, nil + case "What's next?": + return prompter.IndexFor(opts, "Add metadata") + default: + return -1, prompter.NoSuchPromptErr(p) + } + } + pm.MultiSelectFunc = func(p string, _ []string, opts []string) ([]int, error) { + return prompter.IndexesFor(opts, "Projects") + } + + opts := CreateOptions{ + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + Config: func() (gh.Config, error) { + return config.NewBlankConfig(), nil + }, + Browser: &browser.Stub{}, + IO: ios, + Prompter: pm, + GitClient: &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + }, + Finder: shared.NewMockFinder("feature", nil, nil), + Detector: &fd.DisabledDetectorMock{}, + Remotes: func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + }, + Branch: func() (string, error) { + return "feature", nil + }, + + HeadBranch: "feature", + } + + // Ignore the error because we have no way to really stub it without + // fully stubbing a GQL error structure in the request body. + _ = createRun(&opts) + + // Verify that our request did not contain projectCards + reg.Verify(t) + }) + }) + t.Run("web mode", func(t *testing.T) { t.Run("when projects v1 is supported, queries for it", func(t *testing.T) { ios, _, _, _ := iostreams.Test() @@ -2238,7 +2440,7 @@ func TestProjectsV1Deprecation(t *testing.T) { Projects: []string{"Project"}, }) - // Verify that our request contained projectCards + // Verify that our request did not contain projectCards reg.Verify(t) }) }) From 1a5b7ca60c25d0484f3d6efc419669162083da9a Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 2 May 2025 16:58:46 +0200 Subject: [PATCH 2/2] Feature detect v1 projects for preview URL As far as I can see, when there is project metadata, the preview option will never be shown in the interactive multiselect, so I don't believe this change has any functional difference. However, I did use the opportunity to drive out tests for generateCompareURL --- pkg/cmd/pr/create/create.go | 2 +- pkg/cmd/pr/create/create_test.go | 139 +++++++++++++++++++++++++++++-- 2 files changed, 134 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 64469543f..1d980b68d 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -533,7 +533,7 @@ func createRun(opts *CreateOptions) error { } } - openURL, err = generateCompareURL(*ctx, *state, gh.ProjectsV1Supported) + openURL, err = generateCompareURL(*ctx, *state, projectsV1Support) if err != nil { return err } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index ac96db0d6..bd68f19d9 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1852,11 +1852,13 @@ func mustParseQualifiedHeadRef(ref string) shared.QualifiedHeadRef { func Test_generateCompareURL(t *testing.T) { tests := []struct { - name string - ctx CreateContext - state shared.IssueMetadataState - want string - wantErr bool + name string + ctx CreateContext + state shared.IssueMetadataState + httpStubs func(*testing.T, *httpmock.Registry) + projectsV1Support gh.ProjectsV1Support + want string + wantErr bool }{ { name: "basic", @@ -1940,10 +1942,135 @@ func Test_generateCompareURL(t *testing.T) { want: "https://github.com/OWNER/REPO/compare/main...feature?body=&expand=1&template=story.md", wantErr: false, }, + // TODO projectsV1Deprecation + // Clean up these tests, but probably keep one for general project ID resolution. + { + name: "with projects, no v1 support", + ctx: CreateContext{ + PRRefs: &skipPushRefs{ + qualifiedHeadRef: shared.NewQualifiedHeadRefWithoutOwner("feature"), + baseRefs: baseRefs{ + baseRepo: api.InitRepoHostname(&api.Repository{Name: "REPO", Owner: api.RepositoryOwner{Login: "OWNER"}}, "github.com"), + baseBranchName: "main", + }, + }, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + // Ensure no v1 projects are requestd + // ( is required to avoid matching projectsV2 + reg.Exclude(t, httpmock.GraphQL(`projects\(`)) + reg.Register( + httpmock.GraphQL(`query RepositoryProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "repository": { "projectsV2": { + "nodes": [ + { "title": "ProjectTitle", "id": "PROJECTV2ID", "resourcePath": "/OWNER/REPO/projects/3" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "organization": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query UserProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + state: shared.IssueMetadataState{ + ProjectTitles: []string{"ProjectTitle"}, + }, + projectsV1Support: gh.ProjectsV1Unsupported, + want: "https://github.com/OWNER/REPO/compare/main...feature?body=&expand=1&projects=OWNER%2FREPO%2F3", + wantErr: false, + }, + { + name: "with projects, v1 support", + ctx: CreateContext{ + PRRefs: &skipPushRefs{ + qualifiedHeadRef: shared.NewQualifiedHeadRefWithoutOwner("feature"), + baseRefs: baseRefs{ + baseRepo: api.InitRepoHostname(&api.Repository{Name: "REPO", Owner: api.RepositoryOwner{Login: "OWNER"}}, "github.com"), + baseBranchName: "main", + }, + }, + }, + state: shared.IssueMetadataState{ + ProjectTitles: []string{"ProjectV1Title"}, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + // v1 project query responses + reg.Register( + httpmock.GraphQL(`query RepositoryProjectList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "projects": { + "nodes": [ + { "name": "ProjectV1Title", "id": "PROJECTV1ID", "resourcePath": "/OWNER/REPO/projects/1" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationProjectList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "projects": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + // v2 project query responses + reg.Register( + httpmock.GraphQL(`query RepositoryProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "repository": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "organization": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query UserProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + projectsV1Support: gh.ProjectsV1Supported, + want: "https://github.com/OWNER/REPO/compare/main...feature?body=&expand=1&projects=OWNER%2FREPO%2F1", + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := generateCompareURL(tt.ctx, tt.state, gh.ProjectsV1Supported) + // If http stubs are provided, register them and inject the registry into a client + // that is provided to generateCompareURL in the ctx. + if tt.httpStubs != nil { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + tt.httpStubs(t, reg) + tt.ctx.Client = api.NewClientFromHTTP(&http.Client{Transport: reg}) + } + + got, err := generateCompareURL(tt.ctx, tt.state, tt.projectsV1Support) if (err != nil) != tt.wantErr { t.Errorf("generateCompareURL() error = %v, wantErr %v", err, tt.wantErr) return