Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 22 additions & 19 deletions api/queries_repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -738,53 +738,56 @@ func (m *RepoMetadataResult) LabelsToIDs(names []string) ([]string, error) {
return ids, nil
}

// ProjectsToIDs returns two arrays:
// ProjectsTitlesToIDs returns two arrays:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's a lot of renaming to use the word "title" here to bring it into line with the --projects flag. Sorry if it's messy, the real changes are in create and create_test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, I saw the comment in reviewing the chain of "builds on" PRs 👍

// - the first contains IDs of projects V1
// - the second contains IDs of projects V2
// - if neither project V1 or project V2 can be found with a given name, then an error is returned
func (m *RepoMetadataResult) ProjectsToIDs(names []string) ([]string, []string, error) {
func (m *RepoMetadataResult) ProjectsTitlesToIDs(titles []string) ([]string, []string, error) {
var ids []string
var idsV2 []string
for _, projectName := range names {
id, found := m.projectNameToID(projectName)
for _, title := range titles {
id, found := m.v1ProjectNameToID(title)
if found {
ids = append(ids, id)
continue
}

idV2, found := m.projectV2TitleToID(projectName)
idV2, found := m.v2ProjectTitleToID(title)
if found {
idsV2 = append(idsV2, idV2)
continue
}

return nil, nil, fmt.Errorf("'%s' not found", projectName)
return nil, nil, fmt.Errorf("'%s' not found", title)
}
return ids, idsV2, nil
}

func (m *RepoMetadataResult) projectNameToID(projectName string) (string, bool) {
// We use the word "titles" when referring to v1 and v2 projects.
// In reality, v1 projects really have "names", so there is a bit of a
// mismatch we just need to gloss over.
func (m *RepoMetadataResult) v1ProjectNameToID(name string) (string, bool) {
for _, p := range m.Projects {
if strings.EqualFold(projectName, p.Name) {
if strings.EqualFold(name, p.Name) {
return p.ID, true
}
}

return "", false
}

func (m *RepoMetadataResult) projectV2TitleToID(projectTitle string) (string, bool) {
func (m *RepoMetadataResult) v2ProjectTitleToID(title string) (string, bool) {
for _, p := range m.ProjectsV2 {
if strings.EqualFold(projectTitle, p.Title) {
if strings.EqualFold(title, p.Title) {
return p.ID, true
}
}

return "", false
}

func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []string, projectsV1Support gh.ProjectsV1Support) ([]string, error) {
paths := make([]string, 0, len(projectNames))
func ProjectTitlesToPaths(client *Client, repo ghrepo.Interface, titles []string, projectsV1Support gh.ProjectsV1Support) ([]string, error) {
paths := make([]string, 0, len(titles))
matchedPaths := map[string]struct{}{}

// TODO: ProjectsV1Cleanup
Expand All @@ -796,9 +799,9 @@ func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []s
return nil, err
}

for _, projectName := range projectNames {
for _, title := range titles {
for _, p := range v1Projects {
if strings.EqualFold(projectName, p.Name) {
if strings.EqualFold(title, p.Name) {
pathParts := strings.Split(p.ResourcePath, "/")
var path string
if pathParts[1] == "orgs" || pathParts[1] == "users" {
Expand All @@ -807,7 +810,7 @@ func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []s
path = fmt.Sprintf("%s/%s/%s", pathParts[1], pathParts[2], pathParts[4])
}
paths = append(paths, path)
matchedPaths[projectName] = struct{}{}
matchedPaths[title] = struct{}{}
break
}
}
Expand All @@ -820,15 +823,15 @@ func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []s
return nil, err
}

for _, projectName := range projectNames {
for _, title := range titles {
// If we already found a v1 project with this name, skip it
if _, ok := matchedPaths[projectName]; ok {
if _, ok := matchedPaths[title]; ok {
continue
}

found := false
for _, p := range v2Projects {
if strings.EqualFold(projectName, p.Title) {
if strings.EqualFold(title, p.Title) {
pathParts := strings.Split(p.ResourcePath, "/")
var path string
if pathParts[1] == "orgs" || pathParts[1] == "users" {
Expand All @@ -843,7 +846,7 @@ func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []s
}

if !found {
return nil, fmt.Errorf("'%s' not found", projectName)
return nil, fmt.Errorf("'%s' not found", title)
}
}

Expand Down
8 changes: 4 additions & 4 deletions api/queries_repo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ func Test_RepoMetadata(t *testing.T) {

expectedProjectIDs := []string{"TRIAGEID", "ROADMAPID"}
expectedProjectV2IDs := []string{"TRIAGEV2ID", "ROADMAPV2ID", "MONALISAV2ID"}
projectIDs, projectV2IDs, err := result.ProjectsToIDs([]string{"triage", "roadmap", "triagev2", "roadmapv2", "monalisav2"})
projectIDs, projectV2IDs, err := result.ProjectsTitlesToIDs([]string{"triage", "roadmap", "triagev2", "roadmapv2", "monalisav2"})
if err != nil {
t.Errorf("error resolving projects: %v", err)
}
Expand Down Expand Up @@ -273,7 +273,7 @@ func Test_ProjectNamesToPaths(t *testing.T) {
} } } }
`))

projectPaths, err := ProjectNamesToPaths(client, repo, []string{"Triage", "Roadmap", "TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Supported)
projectPaths, err := ProjectTitlesToPaths(client, repo, []string{"Triage", "Roadmap", "TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Supported)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
Expand Down Expand Up @@ -331,7 +331,7 @@ func Test_ProjectNamesToPaths(t *testing.T) {
} } } }
`))

projectPaths, err := ProjectNamesToPaths(client, repo, []string{"TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Unsupported)
projectPaths, err := ProjectTitlesToPaths(client, repo, []string{"TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Unsupported)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
Expand Down Expand Up @@ -374,7 +374,7 @@ func Test_ProjectNamesToPaths(t *testing.T) {
} } } }
`))

_, err := ProjectNamesToPaths(client, repo, []string{"TriageV2"}, gh.ProjectsV1Unsupported)
_, err := ProjectTitlesToPaths(client, repo, []string{"TriageV2"}, gh.ProjectsV1Unsupported)
require.Equal(t, err, fmt.Errorf("'TriageV2' not found"))
})
}
Expand Down
30 changes: 23 additions & 7 deletions pkg/cmd/pr/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
ghContext "github.com/cli/cli/v2/context"
"github.com/cli/cli/v2/git"
"github.com/cli/cli/v2/internal/browser"
fd "github.com/cli/cli/v2/internal/featuredetection"
"github.com/cli/cli/v2/internal/gh"
"github.com/cli/cli/v2/internal/ghrepo"
"github.com/cli/cli/v2/internal/text"
Expand All @@ -31,6 +32,7 @@ import (

type CreateOptions struct {
// This struct stores user input and factory functions
Detector fd.Detector
HttpClient func() (*http.Client, error)
GitClient *git.Client
Config func() (gh.Config, error)
Expand Down Expand Up @@ -363,6 +365,20 @@ func createRun(opts *CreateOptions) error {
return err
}

httpClient, err := opts.HttpClient()
if err != nil {
return err
}

// TODO projectsV1Deprecation
// Remove this section as we should no longer need to detect
if opts.Detector == nil {
cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
opts.Detector = fd.NewDetector(cachedClient, ctx.PRRefs.BaseRepo().RepoHost())
}

projectsV1Support := opts.Detector.ProjectsV1()

client := ctx.Client

state, err := NewIssueState(*ctx, *opts)
Expand All @@ -384,7 +400,7 @@ func createRun(opts *CreateOptions) error {
if err != nil {
return err
}
openURL, err = generateCompareURL(*ctx, *state)
openURL, err = generateCompareURL(*ctx, *state, gh.ProjectsV1Supported)
if err != nil {
return err
}
Expand Down Expand Up @@ -441,7 +457,7 @@ func createRun(opts *CreateOptions) error {
return err
}
// TODO wm: revisit project support
return submitPR(*opts, *ctx, *state, gh.ProjectsV1Supported)
return submitPR(*opts, *ctx, *state, projectsV1Support)
}

if opts.RecoverFile != "" {
Expand Down Expand Up @@ -518,7 +534,7 @@ func createRun(opts *CreateOptions) error {
}
}

openURL, err = generateCompareURL(*ctx, *state)
openURL, err = generateCompareURL(*ctx, *state, gh.ProjectsV1Supported)
if err != nil {
return err
}
Expand Down Expand Up @@ -568,12 +584,12 @@ func createRun(opts *CreateOptions) error {
if action == shared.SubmitDraftAction {
state.Draft = true
// TODO wm: revisit project support
return submitPR(*opts, *ctx, *state, gh.ProjectsV1Supported)
return submitPR(*opts, *ctx, *state, projectsV1Support)
}

if action == shared.SubmitAction {
// TODO wm: revisit project support
return submitPR(*opts, *ctx, *state, gh.ProjectsV1Supported)
return submitPR(*opts, *ctx, *state, projectsV1Support)
}

err = errors.New("expected to cancel, preview, or submit")
Expand Down Expand Up @@ -1216,13 +1232,13 @@ func handlePush(opts CreateOptions, ctx CreateContext) error {
return pushBranch()
}

func generateCompareURL(ctx CreateContext, state shared.IssueMetadataState) (string, error) {
func generateCompareURL(ctx CreateContext, state shared.IssueMetadataState, projectsV1Support gh.ProjectsV1Support) (string, error) {
u := ghrepo.GenerateRepoURL(
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, gh.ProjectsV1Supported)
url, err := shared.WithPrAndIssueQueryParams(ctx.Client, ctx.PRRefs.BaseRepo(), u, state, projectsV1Support)
if err != nil {
return "", err
}
Expand Down
121 changes: 120 additions & 1 deletion pkg/cmd/pr/create/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/cli/cli/v2/git"
"github.com/cli/cli/v2/internal/browser"
"github.com/cli/cli/v2/internal/config"
fd "github.com/cli/cli/v2/internal/featuredetection"
"github.com/cli/cli/v2/internal/gh"
"github.com/cli/cli/v2/internal/ghrepo"
"github.com/cli/cli/v2/internal/prompter"
Expand Down Expand Up @@ -1618,6 +1619,7 @@ func Test_createRun(t *testing.T) {
}

opts := CreateOptions{}
opts.Detector = &fd.EnabledDetectorMock{}
opts.Prompter = pm

ios, _, stdout, stderr := iostreams.Test()
Expand Down Expand Up @@ -1941,7 +1943,8 @@ func Test_generateCompareURL(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := generateCompareURL(tt.ctx, tt.state)
// 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)
return
Expand Down Expand Up @@ -2009,3 +2012,119 @@ 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) {

t.Run("non-interactive submission", func(t *testing.T) {
t.Run("when projects v1 is supported, queries for it", func(t *testing.T) {
ios, _, _, _ := iostreams.Test()

reg := &httpmock.Registry{}
reg.StubRepoInfoResponse("OWNER", "REPO", "main")
reg.Register(
// ( is required to avoid matching projectsV2
httpmock.GraphQL(`projects\(`),
// Simulate a GraphQL error to early exit the test.
httpmock.StatusStringResponse(500, ""),
)

cs, cmdTeardown := run.Stub()
defer cmdTeardown(t)

cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "")

// Ignore the error because we have no way to really stub it without
// fully stubbing a GQL error structure in the request body.
_ = createRun(&CreateOptions{
Detector: &fd.EnabledDetectorMock{},
IO: ios,
HttpClient: func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
},
GitClient: &git.Client{
GhPath: "some/path/gh",
GitPath: "some/path/git",
},
Remotes: func() (context.Remotes, error) {
return context.Remotes{
{
Remote: &git.Remote{
Name: "upstream",
Resolved: "base",
},
Repo: ghrepo.New("OWNER", "REPO"),
},
}, nil
},
Finder: shared.NewMockFinder("feature", nil, nil),

HeadBranch: "feature",

TitleProvided: true,
BodyProvided: true,
Title: "Test Title",
Body: "Test Body",

// Required to force a lookup of projects
Projects: []string{"Project"},
})

// 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) {
ios, _, _, _ := iostreams.Test()

reg := &httpmock.Registry{}
reg.StubRepoInfoResponse("OWNER", "REPO", "main")
// ( is required to avoid matching projectsV2
reg.Exclude(t, httpmock.GraphQL(`projects\(`))

cs, cmdTeardown := run.Stub()
defer cmdTeardown(t)

cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "")

// Ignore the error because we're not really interested in it.
_ = createRun(&CreateOptions{
Detector: &fd.DisabledDetectorMock{},
IO: ios,
HttpClient: func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
},
GitClient: &git.Client{
GhPath: "some/path/gh",
GitPath: "some/path/git",
},
Remotes: func() (context.Remotes, error) {
return context.Remotes{
{
Remote: &git.Remote{
Name: "upstream",
Resolved: "base",
},
Repo: ghrepo.New("OWNER", "REPO"),
},
}, nil
},
Finder: shared.NewMockFinder("feature", nil, nil),

HeadBranch: "feature",

TitleProvided: true,
BodyProvided: true,
Title: "Test Title",
Body: "Test Body",

// Required to force a lookup of projects
Projects: []string{"Project"},
})

// Verify that our request contained projectCards
reg.Verify(t)
})
})
}
Loading
Loading