From 6d57f2a91e569f99f825797f628d9156a02a1fc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 11 May 2020 20:35:39 +0200 Subject: [PATCH 1/6] Faster resolve GraphQL node IDs for assignees, reviewers, and labels --- api/queries_repo.go | 100 +++++++++++++++++++++++++++++++++++++++ api/queries_repo_test.go | 95 +++++++++++++++++++++++++++++++++++++ command/pr_create.go | 19 ++++---- pkg/httpmock/stub.go | 16 +++++++ 4 files changed, 222 insertions(+), 8 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index af578413d..6eb7088fc 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -580,6 +580,106 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput return &result, err } +type RepoResolveInput struct { + Assignees []string + Reviewers []string + Labels []string + Projects []string + Milestones []string +} + +// RepoResolveMetadataIDs looks up GraphQL node IDs in bulk +func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoResolveInput) (*RepoMetadataResult, error) { + users := input.Assignees + hasUser := func(target string) bool { + for _, u := range users { + if strings.EqualFold(u, target) { + return true + } + } + return false + } + + var teams []string + for _, r := range input.Reviewers { + if i := strings.IndexRune(r, '/'); i > -1 { + teams = append(teams, r[i+1:]) + } else if !hasUser(r) { + users = append(users, r) + } + } + + // there is no way to look up projects nor milestones by name, so preload them all + mi := RepoMetadataInput{ + Projects: len(input.Projects) > 0, + Milestones: len(input.Milestones) > 0, + } + result, err := RepoMetadata(client, repo, mi) + if err != nil { + return result, err + } + if len(users) == 0 && len(teams) == 0 && len(input.Labels) == 0 { + return result, nil + } + + query := &bytes.Buffer{} + for i, u := range users { + fmt.Fprintf(query, "u%03d: user(login:%q){id,login}\n", i, u) + } + if len(input.Labels) > 0 { + fmt.Fprintf(query, "repository(owner:%q,name:%q){\n", repo.RepoOwner(), repo.RepoName()) + for i, l := range input.Labels { + fmt.Fprintf(query, "l%03d: label(name:%q){id,name}\n", i, l) + } + fmt.Fprint(query, "}\n") + } + if len(teams) > 0 { + fmt.Fprintf(query, "organization(login:%q){\n", repo.RepoOwner()) + for i, t := range teams { + fmt.Fprintf(query, "t%03d: team(slug:%q){id,slug}\n", i, t) + } + fmt.Fprint(query, "}\n") + } + + response := make(map[string]json.RawMessage) + err = client.GraphQL(query.String(), nil, &response) + if err != nil { + return result, err + } + + for key, v := range response { + switch key { + case "repository": + repoResponse := make(map[string]RepoLabel) + err := json.Unmarshal(v, &repoResponse) + if err != nil { + return result, err + } + for _, l := range repoResponse { + result.Labels = append(result.Labels, l) + } + case "organization": + orgResponse := make(map[string]OrgTeam) + err := json.Unmarshal(v, &orgResponse) + if err != nil { + return result, err + } + for _, t := range orgResponse { + result.Teams = append(result.Teams, t) + } + default: + user := RepoAssignee{} + err := json.Unmarshal(v, &user) + if err != nil { + return result, err + } + result.AssignableUsers = append(result.AssignableUsers, user) + } + } + + return result, nil +} + type RepoProject struct { ID string Name string diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index d8a75f4ff..6e36f4abc 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -6,6 +6,7 @@ import ( "io/ioutil" "testing" + "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" ) @@ -45,3 +46,97 @@ func Test_RepoCreate(t *testing.T) { t.Errorf("expected homepageUrl to be %q, got %q", "http://example.com", homepage) } } + +func Test_RepoResolveMetadataIDs(t *testing.T) { + http := &httpmock.Registry{} + client := NewClient(ReplaceTripper(http)) + + repo := ghrepo.FromFullName("OWNER/REPO") + input := RepoResolveInput{ + Assignees: []string{"monalisa", "hubot"}, + Reviewers: []string{"monalisa", "octocat", "OWNER/core", "/robots"}, + Labels: []string{"bug", "help wanted"}, + } + + expectedQuery := `u000: user(login:"monalisa"){id,login} +u001: user(login:"hubot"){id,login} +u002: user(login:"octocat"){id,login} +repository(owner:"OWNER",name:"REPO"){ +l000: label(name:"bug"){id,name} +l001: label(name:"help wanted"){id,name} +} +organization(login:"OWNER"){ +t000: team(slug:"core"){id,slug} +t001: team(slug:"robots"){id,slug} +} +` + responseJSON := ` + { "data": { + "u000": { "login": "MonaLisa", "id": "MONAID" }, + "u001": { "login": "hubot", "id": "HUBOTID" }, + "u002": { "login": "octocat", "id": "OCTOID" }, + "repository": { + "l000": { "name": "bug", "id": "BUGID" }, + "l001": { "name": "Help Wanted", "id": "HELPID" } + }, + "organization": { + "t000": { "slug": "core", "id": "COREID" }, + "t001": { "slug": "Robots", "id": "ROBOTID" } + } + } } + ` + + http.Register( + httpmock.MatchAny, + httpmock.GraphQLQuery(responseJSON, func(q string, _ map[string]interface{}) { + if q != expectedQuery { + t.Errorf("expected query %q, got %q", expectedQuery, q) + } + })) + + result, err := RepoResolveMetadataIDs(client, repo, input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expectedMemberIDs := []string{"MONAID", "HUBOTID", "OCTOID"} + memberIDs, err := result.MembersToIDs([]string{"monalisa", "hubot", "octocat"}) + if err != nil { + t.Errorf("error resolving members: %v", err) + } + if !sliceEqual(memberIDs, expectedMemberIDs) { + t.Errorf("expected members %v, got %v", expectedMemberIDs, memberIDs) + } + + expectedTeamIDs := []string{"COREID", "ROBOTID"} + teamIDs, err := result.TeamsToIDs([]string{"/core", "/robots"}) + if err != nil { + t.Errorf("error resolving teams: %v", err) + } + if !sliceEqual(teamIDs, expectedTeamIDs) { + t.Errorf("expected members %v, got %v", expectedTeamIDs, teamIDs) + } + + expectedLabelIDs := []string{"BUGID", "HELPID"} + labelIDs, err := result.LabelsToIDs([]string{"bug", "help wanted"}) + if err != nil { + t.Errorf("error resolving labels: %v", err) + } + if !sliceEqual(labelIDs, expectedLabelIDs) { + t.Errorf("expected members %v, got %v", expectedLabelIDs, labelIDs) + } +} + +func sliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + + for i := range a { + if a[i] != b[i] { + return false + } + } + + return true +} diff --git a/command/pr_create.go b/command/pr_create.go index 4ce58a681..e50855f4a 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -325,16 +325,19 @@ func prCreate(cmd *cobra.Command, _ []string) error { if tb.HasMetadata() { if tb.MetadataResult == nil { - metadataInput := api.RepoMetadataInput{ - Reviewers: len(tb.Reviewers) > 0, - Assignees: len(tb.Assignees) > 0, - Labels: len(tb.Labels) > 0, - Projects: len(tb.Projects) > 0, - Milestones: tb.Milestone != "", + var milestones []string + if tb.Milestone != "" { + milestones = append(milestones, tb.Milestone) + } + resolveInput := api.RepoResolveInput{ + Reviewers: tb.Reviewers, + Assignees: tb.Assignees, + Labels: tb.Labels, + Projects: tb.Projects, + Milestones: milestones, } - // TODO: for non-interactive mode, only translate given objects to GraphQL IDs - tb.MetadataResult, err = api.RepoMetadata(client, baseRepo, metadataInput) + tb.MetadataResult, err = api.RepoResolveMetadataIDs(client, baseRepo, resolveInput) if err != nil { return err } diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index b27ceea6c..48077ed4e 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -88,6 +88,22 @@ func GraphQLMutation(body string, cb func(map[string]interface{})) Responder { } } +func GraphQLQuery(body string, cb func(string, map[string]interface{})) Responder { + return func(req *http.Request) (*http.Response, error) { + var bodyData struct { + Query string + Variables map[string]interface{} + } + err := decodeJSONBody(req, &bodyData) + if err != nil { + return nil, err + } + cb(bodyData.Query, bodyData.Variables) + + return httpResponse(200, bytes.NewBufferString(body)), nil + } +} + func httpResponse(status int, body io.Reader) *http.Response { return &http.Response{ StatusCode: status, From 00f23b8d865062b3dd4f34067b301f52cabac7d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 12 May 2020 15:48:59 +0200 Subject: [PATCH 2/6] Store milestones as slice for consistency with other metadata This is for code simplicity. Only 1 milestone per issue or PR is allowed, like before. --- command/issue.go | 26 ++++++++++++++------------ command/pr_create.go | 24 +++++++++++------------- command/title_body_survey.go | 28 +++++++++++++++++----------- 3 files changed, 42 insertions(+), 36 deletions(-) diff --git a/command/issue.go b/command/issue.go index e5cf24026..b5e44787c 100644 --- a/command/issue.go +++ b/command/issue.go @@ -381,9 +381,11 @@ func issueCreate(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("could not parse projects: %w", err) } - milestoneTitle, err := cmd.Flags().GetString("milestone") - if err != nil { + var milestoneTitles []string + if milestoneTitle, err := cmd.Flags().GetString("milestone"); err != nil { return fmt.Errorf("could not parse milestone: %w", err) + } else if milestoneTitle != "" { + milestoneTitles = append(milestoneTitles, milestoneTitle) } if isWeb, err := cmd.Flags().GetBool("web"); err == nil && isWeb { @@ -419,10 +421,10 @@ func issueCreate(cmd *cobra.Command, args []string) error { action := SubmitAction tb := issueMetadataState{ - Assignees: assignees, - Labels: labelNames, - Projects: projectNames, - Milestone: milestoneTitle, + Assignees: assignees, + Labels: labelNames, + Projects: projectNames, + Milestones: milestoneTitles, } interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) @@ -475,7 +477,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { Assignees: len(tb.Assignees) > 0, Labels: len(tb.Labels) > 0, Projects: len(tb.Projects) > 0, - Milestones: tb.Milestone != "", + Milestones: len(tb.Milestones) > 0, } // TODO: for non-interactive mode, only translate given objects to GraphQL IDs @@ -485,7 +487,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { } } - err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestone) + err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) if err != nil { return err } @@ -504,7 +506,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { return nil } -func addMetadataToIssueParams(params map[string]interface{}, metadata *api.RepoMetadataResult, assignees, labelNames, projectNames []string, milestoneTitle string) error { +func addMetadataToIssueParams(params map[string]interface{}, metadata *api.RepoMetadataResult, assignees, labelNames, projectNames, milestoneTitles []string) error { assigneeIDs, err := metadata.MembersToIDs(assignees) if err != nil { return fmt.Errorf("could not assign user: %w", err) @@ -523,10 +525,10 @@ func addMetadataToIssueParams(params map[string]interface{}, metadata *api.RepoM } params["projectIds"] = projectIDs - if milestoneTitle != "" { - milestoneID, err := metadata.MilestoneToID(milestoneTitle) + if len(milestoneTitles) > 0 { + milestoneID, err := metadata.MilestoneToID(milestoneTitles[0]) if err != nil { - return fmt.Errorf("could not add to milestone '%s': %w", milestoneTitle, err) + return fmt.Errorf("could not add to milestone '%s': %w", milestoneTitles[0], err) } params["milestoneId"] = milestoneID } diff --git a/command/pr_create.go b/command/pr_create.go index e50855f4a..9e096c0eb 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -140,9 +140,11 @@ func prCreate(cmd *cobra.Command, _ []string) error { if err != nil { return fmt.Errorf("could not parse projects: %w", err) } - milestoneTitle, err := cmd.Flags().GetString("milestone") - if err != nil { + var milestoneTitles []string + if milestoneTitle, err := cmd.Flags().GetString("milestone"); err != nil { return fmt.Errorf("could not parse milestone: %w", err) + } else if milestoneTitle != "" { + milestoneTitles = append(milestoneTitles, milestoneTitle) } baseTrackingBranch := baseBranch @@ -201,11 +203,11 @@ func prCreate(cmd *cobra.Command, _ []string) error { } tb := issueMetadataState{ - Reviewers: reviewers, - Assignees: assignees, - Labels: labelNames, - Projects: projectNames, - Milestone: milestoneTitle, + Reviewers: reviewers, + Assignees: assignees, + Labels: labelNames, + Projects: projectNames, + Milestones: milestoneTitles, } interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) @@ -325,16 +327,12 @@ func prCreate(cmd *cobra.Command, _ []string) error { if tb.HasMetadata() { if tb.MetadataResult == nil { - var milestones []string - if tb.Milestone != "" { - milestones = append(milestones, tb.Milestone) - } resolveInput := api.RepoResolveInput{ Reviewers: tb.Reviewers, Assignees: tb.Assignees, Labels: tb.Labels, Projects: tb.Projects, - Milestones: milestones, + Milestones: tb.Milestones, } tb.MetadataResult, err = api.RepoResolveMetadataIDs(client, baseRepo, resolveInput) @@ -343,7 +341,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } } - err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestone) + err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) if err != nil { return err } diff --git a/command/title_body_survey.go b/command/title_body_survey.go index 06ab14a09..f8200c218 100644 --- a/command/title_body_survey.go +++ b/command/title_body_survey.go @@ -20,12 +20,12 @@ type issueMetadataState struct { Title string Action Action - Metadata []string - Reviewers []string - Assignees []string - Labels []string - Projects []string - Milestone string + Metadata []string + Reviewers []string + Assignees []string + Labels []string + Projects []string + Milestones []string MetadataResult *api.RepoMetadataResult } @@ -35,7 +35,7 @@ func (tb *issueMetadataState) HasMetadata() bool { len(tb.Assignees) > 0 || len(tb.Labels) > 0 || len(tb.Projects) > 0 || - tb.Milestone != "" + len(tb.Milestones) > 0 } const ( @@ -43,6 +43,8 @@ const ( PreviewAction CancelAction MetadataAction + + noMilestone = "(none)" ) var SurveyAsk = func(qs []*survey.Question, response interface{}, opts ...survey.AskOpt) error { @@ -257,7 +259,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie for _, l := range issueState.MetadataResult.Projects { projects = append(projects, l.Name) } - milestones := []string{"(none)"} + milestones := []string{noMilestone} for _, m := range issueState.MetadataResult.Milestones { milestones = append(milestones, m.Title) } @@ -321,12 +323,16 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie } if isChosen("Milestone") { if len(milestones) > 1 { + var milestoneDefault interface{} + if len(issueState.Milestones) > 0 { + milestoneDefault = issueState.Milestones[0] + } mqs = append(mqs, &survey.Question{ Name: "milestone", Prompt: &survey.Select{ Message: "Milestone", Options: milestones, - Default: issueState.Milestone, + Default: milestoneDefault, }, }) } else { @@ -339,8 +345,8 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie return fmt.Errorf("could not prompt: %w", err) } - if issueState.Milestone == "(none)" { - issueState.Milestone = "" + if len(issueState.Milestones) > 0 && issueState.Milestones[0] == noMilestone { + issueState.Milestones = issueState.Milestones[0:0] } allowPreview = !issueState.HasMetadata() From 3abc2be0f718c6ff61424c365a006361b37bc892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 12 May 2020 16:17:06 +0200 Subject: [PATCH 3/6] Switch issue create to optimized resolver and update tests --- api/queries_repo_test.go | 130 ++++++++++++++++++++++++++++++++++++++ command/issue.go | 13 ++-- command/issue_test.go | 28 +++----- command/pr_create_test.go | 43 ++++--------- 4 files changed, 156 insertions(+), 58 deletions(-) diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 6e36f4abc..94be4001c 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -46,6 +46,136 @@ func Test_RepoCreate(t *testing.T) { t.Errorf("expected homepageUrl to be %q, got %q", "http://example.com", homepage) } } +func Test_RepoMetadata(t *testing.T) { + http := &httpmock.Registry{} + client := NewClient(ReplaceTripper(http)) + + repo := ghrepo.FromFullName("OWNER/REPO") + input := RepoMetadataInput{ + Assignees: true, + Reviewers: true, + Labels: true, + Projects: true, + Milestones: true, + } + + http.Register( + httpmock.GraphQL(`\bassignableUsers\(`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "MonaLisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\blabels\(`), + httpmock.StringResponse(` + { "data": { "repository": { "labels": { + "nodes": [ + { "name": "feature", "id": "FEATUREID" }, + { "name": "TODO", "id": "TODOID" }, + { "name": "bug", "id": "BUGID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\bmilestones\(`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [ + { "title": "GA", "id": "GAID" }, + { "title": "Big One.oh", "id": "BIGONEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\brepository\(.+\bprojects\(`), + httpmock.StringResponse(` + { "data": { "repository": { "projects": { + "nodes": [ + { "name": "Cleanup", "id": "CLEANUPID" }, + { "name": "Roadmap", "id": "ROADMAPID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\borganization\(.+\bprojects\(`), + httpmock.StringResponse(` + { "data": { "organization": { "projects": { + "nodes": [ + { "name": "Triage", "id": "TRIAGEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\borganization\(.+\bteams\(`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "owners", "id": "OWNERSID" }, + { "slug": "Core", "id": "COREID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + + result, err := RepoMetadata(client, repo, input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expectedMemberIDs := []string{"MONAID", "HUBOTID"} + memberIDs, err := result.MembersToIDs([]string{"monalisa", "hubot"}) + if err != nil { + t.Errorf("error resolving members: %v", err) + } + if !sliceEqual(memberIDs, expectedMemberIDs) { + t.Errorf("expected members %v, got %v", expectedMemberIDs, memberIDs) + } + + expectedTeamIDs := []string{"COREID", "OWNERSID"} + teamIDs, err := result.TeamsToIDs([]string{"OWNER/core", "/owners"}) + if err != nil { + t.Errorf("error resolving teams: %v", err) + } + if !sliceEqual(teamIDs, expectedTeamIDs) { + t.Errorf("expected teams %v, got %v", expectedTeamIDs, teamIDs) + } + + expectedLabelIDs := []string{"BUGID", "TODOID"} + labelIDs, err := result.LabelsToIDs([]string{"bug", "todo"}) + if err != nil { + t.Errorf("error resolving labels: %v", err) + } + if !sliceEqual(labelIDs, expectedLabelIDs) { + t.Errorf("expected labels %v, got %v", expectedLabelIDs, labelIDs) + } + + expectedProjectIDs := []string{"TRIAGEID", "ROADMAPID"} + projectIDs, err := result.ProjectsToIDs([]string{"triage", "roadmap"}) + if err != nil { + t.Errorf("error resolving projects: %v", err) + } + if !sliceEqual(projectIDs, expectedProjectIDs) { + t.Errorf("expected projects %v, got %v", expectedProjectIDs, projectIDs) + } + + expectedMilestoneID := "BIGONEID" + milestoneID, err := result.MilestoneToID("big one.oh") + if err != nil { + t.Errorf("error resolving milestone: %v", err) + } + if milestoneID != expectedMilestoneID { + t.Errorf("expected milestone %v, got %v", expectedMilestoneID, milestoneID) + } +} func Test_RepoResolveMetadataIDs(t *testing.T) { http := &httpmock.Registry{} diff --git a/command/issue.go b/command/issue.go index b5e44787c..902481c7e 100644 --- a/command/issue.go +++ b/command/issue.go @@ -473,15 +473,14 @@ func issueCreate(cmd *cobra.Command, args []string) error { if tb.HasMetadata() { if tb.MetadataResult == nil { - metadataInput := api.RepoMetadataInput{ - Assignees: len(tb.Assignees) > 0, - Labels: len(tb.Labels) > 0, - Projects: len(tb.Projects) > 0, - Milestones: len(tb.Milestones) > 0, + resolveInput := api.RepoResolveInput{ + Assignees: tb.Assignees, + Labels: tb.Labels, + Projects: tb.Projects, + Milestones: tb.Milestones, } - // TODO: for non-interactive mode, only translate given objects to GraphQL IDs - tb.MetadataResult, err = api.RepoMetadata(apiClient, baseRepo, metadataInput) + tb.MetadataResult, err = api.RepoResolveMetadataIDs(apiClient, baseRepo, resolveInput) if err != nil { return err } diff --git a/command/issue_test.go b/command/issue_test.go index 5a9d4c239..c3a83b75a 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -500,27 +500,15 @@ func TestIssueCreate_metadata(t *testing.T) { } } } `)) http.Register( - httpmock.GraphQL(`\bassignableUsers\(`), + httpmock.GraphQL(`\bu000:`), httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - http.Register( - httpmock.GraphQL(`\blabels\(`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "feature", "id": "FEATUREID" }, - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } + { "data": { + "u000": { "login": "MonaLisa", "id": "MONAID" }, + "repository": { + "l000": { "name": "bug", "id": "BUGID" }, + "l001": { "name": "TODO", "id": "TODOID" } + } + } } `)) http.Register( httpmock.GraphQL(`\bmilestones\(`), diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 85966aa72..7f1b772a4 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -87,27 +87,19 @@ func TestPRCreate_metadata(t *testing.T) { ] } } } } `)) http.Register( - httpmock.GraphQL(`\bassignableUsers\(`), + httpmock.GraphQL(`\bteam\(`), httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - http.Register( - httpmock.GraphQL(`\blabels\(`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "feature", "id": "FEATUREID" }, - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } + { "data": { + "u000": { "login": "MonaLisa", "id": "MONAID" }, + "u001": { "login": "hubot", "id": "HUBOTID" }, + "repository": { + "l000": { "name": "bug", "id": "BUGID" }, + "l001": { "name": "TODO", "id": "TODOID" } + }, + "organization": { + "t000": { "slug": "core", "id": "COREID" } + } + } } `)) http.Register( httpmock.GraphQL(`\bmilestones\(`), @@ -139,17 +131,6 @@ func TestPRCreate_metadata(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - http.Register( - httpmock.GraphQL(`\borganization\(.+\bteams\(`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "owners", "id": "OWNERSID" }, - { "slug": "Core", "id": "COREID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) http.Register( httpmock.GraphQL(`\bcreatePullRequest\(`), httpmock.GraphQLMutation(` From 386a53c34aa2f49bcec23780d16ddafd3ce051c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 12 May 2020 16:48:30 +0200 Subject: [PATCH 4/6] Fix metadata resolver query --- api/queries_repo.go | 2 + api/queries_repo_test.go | 4 +- command/issue.go | 82 +++++++++++++++++++++++++++------------- command/pr_create.go | 45 ++-------------------- 4 files changed, 64 insertions(+), 69 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 6eb7088fc..c8ba31348 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -623,6 +623,7 @@ func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoRes } query := &bytes.Buffer{} + fmt.Fprint(query, "{\n") for i, u := range users { fmt.Fprintf(query, "u%03d: user(login:%q){id,login}\n", i, u) } @@ -640,6 +641,7 @@ func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoRes } fmt.Fprint(query, "}\n") } + fmt.Fprint(query, "}\n") response := make(map[string]json.RawMessage) err = client.GraphQL(query.String(), nil, &response) diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 94be4001c..a3a6876a8 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -188,7 +188,8 @@ func Test_RepoResolveMetadataIDs(t *testing.T) { Labels: []string{"bug", "help wanted"}, } - expectedQuery := `u000: user(login:"monalisa"){id,login} + expectedQuery := `{ +u000: user(login:"monalisa"){id,login} u001: user(login:"hubot"){id,login} u002: user(login:"octocat"){id,login} repository(owner:"OWNER",name:"REPO"){ @@ -199,6 +200,7 @@ organization(login:"OWNER"){ t000: team(slug:"core"){id,slug} t001: team(slug:"robots"){id,slug} } +} ` responseJSON := ` { "data": { diff --git a/command/issue.go b/command/issue.go index 902481c7e..19b4072df 100644 --- a/command/issue.go +++ b/command/issue.go @@ -471,25 +471,9 @@ func issueCreate(cmd *cobra.Command, args []string) error { "body": body, } - if tb.HasMetadata() { - if tb.MetadataResult == nil { - resolveInput := api.RepoResolveInput{ - Assignees: tb.Assignees, - Labels: tb.Labels, - Projects: tb.Projects, - Milestones: tb.Milestones, - } - - tb.MetadataResult, err = api.RepoResolveMetadataIDs(apiClient, baseRepo, resolveInput) - if err != nil { - return err - } - } - - err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) - if err != nil { - return err - } + err = addMetadataToIssueParams(apiClient, baseRepo, params, &tb) + if err != nil { + return err } newIssue, err := api.IssueCreate(apiClient, repo, params) @@ -505,33 +489,79 @@ func issueCreate(cmd *cobra.Command, args []string) error { return nil } -func addMetadataToIssueParams(params map[string]interface{}, metadata *api.RepoMetadataResult, assignees, labelNames, projectNames, milestoneTitles []string) error { - assigneeIDs, err := metadata.MembersToIDs(assignees) +func addMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, params map[string]interface{}, tb *issueMetadataState) error { + if !tb.HasMetadata() { + return nil + } + + if tb.MetadataResult == nil { + resolveInput := api.RepoResolveInput{ + Reviewers: tb.Reviewers, + Assignees: tb.Assignees, + Labels: tb.Labels, + Projects: tb.Projects, + Milestones: tb.Milestones, + } + + var err error + tb.MetadataResult, err = api.RepoResolveMetadataIDs(client, baseRepo, resolveInput) + if err != nil { + return err + } + } + + assigneeIDs, err := tb.MetadataResult.MembersToIDs(tb.Assignees) if err != nil { return fmt.Errorf("could not assign user: %w", err) } params["assigneeIds"] = assigneeIDs - labelIDs, err := metadata.LabelsToIDs(labelNames) + labelIDs, err := tb.MetadataResult.LabelsToIDs(tb.Labels) if err != nil { return fmt.Errorf("could not add label: %w", err) } params["labelIds"] = labelIDs - projectIDs, err := metadata.ProjectsToIDs(projectNames) + projectIDs, err := tb.MetadataResult.ProjectsToIDs(tb.Projects) if err != nil { return fmt.Errorf("could not add to project: %w", err) } params["projectIds"] = projectIDs - if len(milestoneTitles) > 0 { - milestoneID, err := metadata.MilestoneToID(milestoneTitles[0]) + if len(tb.Milestones) > 0 { + milestoneID, err := tb.MetadataResult.MilestoneToID(tb.Milestones[0]) if err != nil { - return fmt.Errorf("could not add to milestone '%s': %w", milestoneTitles[0], err) + return fmt.Errorf("could not add to milestone '%s': %w", tb.Milestones[0], err) } params["milestoneId"] = milestoneID } + if len(tb.Reviewers) == 0 { + return nil + } + + var userReviewers []string + var teamReviewers []string + for _, r := range tb.Reviewers { + if strings.ContainsRune(r, '/') { + teamReviewers = append(teamReviewers, r) + } else { + userReviewers = append(teamReviewers, r) + } + } + + userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["userReviewerIds"] = userReviewerIDs + + teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["teamReviewerIds"] = teamReviewerIDs + return nil } diff --git a/command/pr_create.go b/command/pr_create.go index 9e096c0eb..96dc08791 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -325,48 +325,9 @@ func prCreate(cmd *cobra.Command, _ []string) error { "headRefName": headBranchLabel, } - if tb.HasMetadata() { - if tb.MetadataResult == nil { - resolveInput := api.RepoResolveInput{ - Reviewers: tb.Reviewers, - Assignees: tb.Assignees, - Labels: tb.Labels, - Projects: tb.Projects, - Milestones: tb.Milestones, - } - - tb.MetadataResult, err = api.RepoResolveMetadataIDs(client, baseRepo, resolveInput) - if err != nil { - return err - } - } - - err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) - if err != nil { - return err - } - - var userReviewers []string - var teamReviewers []string - for _, r := range tb.Reviewers { - if strings.ContainsRune(r, '/') { - teamReviewers = append(teamReviewers, r) - } else { - userReviewers = append(teamReviewers, r) - } - } - - userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) - } - params["userReviewerIds"] = userReviewerIDs - - teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) - } - params["teamReviewerIds"] = teamReviewerIDs + err = addMetadataToIssueParams(client, baseRepo, params, &tb) + if err != nil { + return err } pr, err := api.CreatePullRequest(client, baseRepo, params) From b75c4a812d89d65312567deca2046d8b32feaba8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 12 May 2020 16:49:04 +0200 Subject: [PATCH 5/6] Guard against leaked parameters in issue/pr create tests --- command/issue_test.go | 6 ++++++ command/pr_create_test.go | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/command/issue_test.go b/command/issue_test.go index c3a83b75a..3df6910f5 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -556,6 +556,12 @@ func TestIssueCreate_metadata(t *testing.T) { eq(t, inputs["labelIds"], []interface{}{"BUGID", "TODOID"}) eq(t, inputs["projectIds"], []interface{}{"ROADMAPID"}) eq(t, inputs["milestoneId"], "BIGONEID") + if v, ok := inputs["userIds"]; ok { + t.Errorf("did not expect userIds: %v", v) + } + if v, ok := inputs["teamIds"]; ok { + t.Errorf("did not expect teamIds: %v", v) + } })) output, err := RunCommand(`issue create -t TITLE -b BODY -a monalisa -l bug -l todo -p roadmap -m 'big one.oh'`) diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 7f1b772a4..18a54e887 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -141,6 +141,12 @@ func TestPRCreate_metadata(t *testing.T) { `, func(inputs map[string]interface{}) { eq(t, inputs["title"], "TITLE") eq(t, inputs["body"], "BODY") + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } + if v, ok := inputs["userIds"]; ok { + t.Errorf("did not expect userIds: %v", v) + } })) http.Register( httpmock.GraphQL(`\bupdatePullRequest\(`), From 1ff37be361fb7253b8ce6a3f1aaf744b8c6cbfb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 13 May 2020 17:16:12 +0200 Subject: [PATCH 6/6] Fix assigning multiple user reviewers This was due to a typo. Fixes #913 --- command/issue.go | 2 +- command/pr_create_test.go | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/command/issue.go b/command/issue.go index 19b4072df..5eed13a13 100644 --- a/command/issue.go +++ b/command/issue.go @@ -546,7 +546,7 @@ func addMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par if strings.ContainsRune(r, '/') { teamReviewers = append(teamReviewers, r) } else { - userReviewers = append(teamReviewers, r) + userReviewers = append(userReviewers, r) } } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 18a54e887..f0cc34363 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -97,7 +97,8 @@ func TestPRCreate_metadata(t *testing.T) { "l001": { "name": "TODO", "id": "TODOID" } }, "organization": { - "t000": { "slug": "core", "id": "COREID" } + "t000": { "slug": "core", "id": "COREID" }, + "t001": { "slug": "robots", "id": "ROBOTID" } } } } `)) @@ -169,8 +170,8 @@ func TestPRCreate_metadata(t *testing.T) { } } } `, func(inputs map[string]interface{}) { eq(t, inputs["pullRequestId"], "NEWPULLID") - eq(t, inputs["userIds"], []interface{}{"HUBOTID"}) - eq(t, inputs["teamIds"], []interface{}{"COREID"}) + eq(t, inputs["userIds"], []interface{}{"HUBOTID", "MONAID"}) + eq(t, inputs["teamIds"], []interface{}{"COREID", "ROBOTID"}) })) cs, cmdTeardown := test.InitCmdStubber() @@ -182,7 +183,7 @@ func TestPRCreate_metadata(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push - output, err := RunCommand(`pr create -t TITLE -b BODY -a monalisa -l bug -l todo -p roadmap -m 'big one.oh' -r hubot -r /core`) + output, err := RunCommand(`pr create -t TITLE -b BODY -a monalisa -l bug -l todo -p roadmap -m 'big one.oh' -r hubot -r monalisa -r /core -r /robots`) eq(t, err, nil) eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n")