From 3ce1de62a20e4c6ae4e70c51f3b1bb6508dd6c64 Mon Sep 17 00:00:00 2001 From: Francisco Miamoto Date: Thu, 30 Jul 2020 22:11:19 -0300 Subject: [PATCH 1/9] fix searching issues by the milestone name This fix involves using the REST API since the GraphQL one doesn't seem to return the necessary ID for the subsequent issues query. --- api/queries_issue.go | 13 ++++++++++++- api/queries_repo.go | 28 ++++++++++++++++++++++++++++ command/issue.go | 1 + 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 1b3bddd64..ec34db2e2 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -3,6 +3,7 @@ package api import ( "context" "fmt" + "strconv" "time" "github.com/cli/cli/internal/ghrepo" @@ -246,8 +247,18 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str if mentionString != "" { variables["mention"] = mentionString } + if milestoneString != "" { - variables["milestone"] = milestoneString + milestones, err := RepoMilestonesREST(client, repo) + if err != nil { + return nil, err + } + for i := range milestones { + if milestones[i].Title == milestoneString { + variables["milestone"] = strconv.Itoa(milestones[i].ID) + break + } + } } var response struct { diff --git a/api/queries_repo.go b/api/queries_repo.go index 06d4bfdab..1d617ae2e 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "sort" + "strconv" "strings" "time" @@ -801,3 +802,30 @@ func RepoMilestones(client *Client, repo ghrepo.Interface) ([]RepoMilestone, err return milestones, nil } + +type RepoMilestoneREST struct { + ID int + Title string +} + +func RepoMilestonesREST(client *Client, repo ghrepo.Interface) ([]RepoMilestoneREST, error) { + var allMilestones []RepoMilestoneREST + + path := fmt.Sprintf("repos/%s/%s/milestones", repo.RepoOwner(), repo.RepoName()) + + for page := 1; ; page++ { + var milestonesFromPage []RepoMilestoneREST + + err := client.REST("GET", path+"?page="+strconv.Itoa(page), nil, &milestonesFromPage) + if err != nil { + return nil, err + } + + if len(milestonesFromPage) == 0 { + break + } + allMilestones = append(allMilestones, milestonesFromPage...) + } + + return allMilestones, nil +} diff --git a/command/issue.go b/command/issue.go index 43049faa2..5d7566df6 100644 --- a/command/issue.go +++ b/command/issue.go @@ -61,6 +61,7 @@ var issueCmd = &cobra.Command{ $ gh issue list $ gh issue create --label bug $ gh issue view --web + $ gh issue list --milestone 'MVP' `), Annotations: map[string]string{ "IsCore": "true", From 75e8cb8f9c700657e147402f8d5877eb9dd29eb6 Mon Sep 17 00:00:00 2001 From: Francisco Miamoto Date: Thu, 30 Jul 2020 22:13:48 -0300 Subject: [PATCH 2/9] register mocks for the new request --- api/queries_repo_test.go | 8 ++++++++ command/issue_test.go | 5 ++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index ad0e36864..2d2cb93ad 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -54,6 +54,14 @@ func Test_RepoMetadata(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) + http.Register( + httpmock.REST("GET", "repos/OWNER/REPO/milestones"), + httpmock.StringResponse(` + [ + { "title": "GA", "id": 1 }, + { "title": "Big One.oh", "id": 2 } + ] + `)) http.Register( httpmock.GraphQL(`query RepositoryProjectList\b`), httpmock.StringResponse(` diff --git a/command/issue_test.go b/command/issue_test.go index 7e7532a1e..c88e71247 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -170,10 +170,13 @@ func TestIssueList_tty_withFlags(t *testing.T) { assert.Equal(t, "probablyCher", params["assignee"].(string)) assert.Equal(t, "foo", params["author"].(string)) assert.Equal(t, "me", params["mention"].(string)) - assert.Equal(t, "1.x", params["milestone"].(string)) assert.Equal(t, []interface{}{"web", "bug"}, params["labels"].([]interface{})) assert.Equal(t, []interface{}{"OPEN"}, params["states"].([]interface{})) })) + http.Register( + httpmock.REST("GET", "repos/OWNER/REPO/milestones"), + httpmock.StringResponse("[]"), + ) output, err := RunCommand("issue list -a probablyCher -l web,bug -s open -A foo --mention me --milestone 1.x") if err != nil { From 9a5e69a8a455753f4d37732da7913cc15dade7e4 Mon Sep 17 00:00:00 2001 From: Francisco Miamoto Date: Sat, 1 Aug 2020 23:43:36 -0300 Subject: [PATCH 3/9] use graphql for repo milestones on issue list Since we can obtain the necessary ID for the query on node ID from the GraphQL API, it makes sense to remove the REST version. --- api/queries_issue.go | 32 ++++++++++++++++++++++++++++---- api/queries_repo.go | 28 ---------------------------- api/queries_repo_test.go | 8 -------- 3 files changed, 28 insertions(+), 40 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index ec34db2e2..b027af472 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -2,8 +2,9 @@ package api import ( "context" + "encoding/base64" "fmt" - "strconv" + "strings" "time" "github.com/cli/cli/internal/ghrepo" @@ -249,13 +250,19 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str } if milestoneString != "" { - milestones, err := RepoMilestonesREST(client, repo) + milestones, err := RepoMilestones(client, repo) if err != nil { return nil, err } + for i := range milestones { - if milestones[i].Title == milestoneString { - variables["milestone"] = strconv.Itoa(milestones[i].ID) + if strings.EqualFold(milestones[i].Title, milestoneString) { + // The query for milestones requires the use of the milestone's database ID (see #1441) + id, err := milestoneNodeIdToDatabaseId(milestones[i].ID) + if err != nil { + return nil, err + } + variables["milestone"] = id break } } @@ -430,3 +437,20 @@ func IssueReopen(client *Client, repo ghrepo.Interface, issue Issue) error { return err } + +// milestoneNodeIdToDatabaseId extracts the REST Database ID from the GraphQL Node ID +func milestoneNodeIdToDatabaseId(id string) (string, error) { + // The node id is base64 obfuscated + decoded, err := base64.StdEncoding.DecodeString(id) + if err != nil { + return "", err + } + + // The decoded node ID has a pattern like '09:Milestone12345' + splitted := strings.Split(string(decoded), "Milestone") + if len(splitted) != 2 { + return "", fmt.Errorf("couldn't get database id from node id") + } + + return splitted[1], nil +} diff --git a/api/queries_repo.go b/api/queries_repo.go index 1d617ae2e..06d4bfdab 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "sort" - "strconv" "strings" "time" @@ -802,30 +801,3 @@ func RepoMilestones(client *Client, repo ghrepo.Interface) ([]RepoMilestone, err return milestones, nil } - -type RepoMilestoneREST struct { - ID int - Title string -} - -func RepoMilestonesREST(client *Client, repo ghrepo.Interface) ([]RepoMilestoneREST, error) { - var allMilestones []RepoMilestoneREST - - path := fmt.Sprintf("repos/%s/%s/milestones", repo.RepoOwner(), repo.RepoName()) - - for page := 1; ; page++ { - var milestonesFromPage []RepoMilestoneREST - - err := client.REST("GET", path+"?page="+strconv.Itoa(page), nil, &milestonesFromPage) - if err != nil { - return nil, err - } - - if len(milestonesFromPage) == 0 { - break - } - allMilestones = append(allMilestones, milestonesFromPage...) - } - - return allMilestones, nil -} diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 2d2cb93ad..ad0e36864 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -54,14 +54,6 @@ func Test_RepoMetadata(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - http.Register( - httpmock.REST("GET", "repos/OWNER/REPO/milestones"), - httpmock.StringResponse(` - [ - { "title": "GA", "id": 1 }, - { "title": "Big One.oh", "id": 2 } - ] - `)) http.Register( httpmock.GraphQL(`query RepositoryProjectList\b`), httpmock.StringResponse(` From f9b50acc42da40925596cc14b00a6c10f0d27459 Mon Sep 17 00:00:00 2001 From: Francisco Miamoto Date: Sat, 1 Aug 2020 23:45:21 -0300 Subject: [PATCH 4/9] add mock for milestones on issue list --- command/issue_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/command/issue_test.go b/command/issue_test.go index c88e71247..734703b35 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -170,13 +170,18 @@ func TestIssueList_tty_withFlags(t *testing.T) { assert.Equal(t, "probablyCher", params["assignee"].(string)) assert.Equal(t, "foo", params["author"].(string)) assert.Equal(t, "me", params["mention"].(string)) + assert.Equal(t, "12345", params["milestone"].(string)) // Database ID for the Milestone (see #1441) assert.Equal(t, []interface{}{"web", "bug"}, params["labels"].([]interface{})) assert.Equal(t, []interface{}{"OPEN"}, params["states"].([]interface{})) })) http.Register( - httpmock.REST("GET", "repos/OWNER/REPO/milestones"), - httpmock.StringResponse("[]"), - ) + httpmock.GraphQL(`query RepositoryMilestoneList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [{ "title":"1.x", "id": "MDk6TWlsZXN0b25lMTIzNDU=" }], + "pageInfo": { "hasNextPage": false } + } } } } + `)) output, err := RunCommand("issue list -a probablyCher -l web,bug -s open -A foo --mention me --milestone 1.x") if err != nil { From 3caa7cc537cc084e6d4fb44c45b6072007b3321a Mon Sep 17 00:00:00 2001 From: Francisco Miamoto Date: Sat, 1 Aug 2020 23:54:22 -0300 Subject: [PATCH 5/9] improve comments --- api/queries_issue.go | 13 ++++++------- command/issue.go | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index b027af472..91702b913 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -257,7 +257,6 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str for i := range milestones { if strings.EqualFold(milestones[i].Title, milestoneString) { - // The query for milestones requires the use of the milestone's database ID (see #1441) id, err := milestoneNodeIdToDatabaseId(milestones[i].ID) if err != nil { return nil, err @@ -439,18 +438,18 @@ func IssueReopen(client *Client, repo ghrepo.Interface, issue Issue) error { } // milestoneNodeIdToDatabaseId extracts the REST Database ID from the GraphQL Node ID -func milestoneNodeIdToDatabaseId(id string) (string, error) { - // The node id is base64 obfuscated - decoded, err := base64.StdEncoding.DecodeString(id) +// This conversion is necessary since the GraphQL API requires the use of the milestone's database ID +// for querying the related issues. +func milestoneNodeIdToDatabaseId(nodeId string) (string, error) { + // The Node ID is Base64 obfuscated, with an underlying pattern: + // "09:Milestone12345", where "12345" is the database ID + decoded, err := base64.StdEncoding.DecodeString(nodeId) if err != nil { return "", err } - - // The decoded node ID has a pattern like '09:Milestone12345' splitted := strings.Split(string(decoded), "Milestone") if len(splitted) != 2 { return "", fmt.Errorf("couldn't get database id from node id") } - return splitted[1], nil } diff --git a/command/issue.go b/command/issue.go index 5d7566df6..62ea846c2 100644 --- a/command/issue.go +++ b/command/issue.go @@ -61,7 +61,6 @@ var issueCmd = &cobra.Command{ $ gh issue list $ gh issue create --label bug $ gh issue view --web - $ gh issue list --milestone 'MVP' `), Annotations: map[string]string{ "IsCore": "true", @@ -89,6 +88,7 @@ var issueListCmd = &cobra.Command{ $ gh issue list -l "help wanted" $ gh issue list -A monalisa $ gh issue list --web + $ gh issue list --milestone 'MVP' `), Args: cmdutil.NoArgsQuoteReminder, RunE: issueList, From 0faf52d75936368ad922a3088ee58207c3352452 Mon Sep 17 00:00:00 2001 From: Francisco Miamoto Date: Sat, 1 Aug 2020 23:56:12 -0300 Subject: [PATCH 6/9] change comment to refer to pr --- command/issue_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/issue_test.go b/command/issue_test.go index 734703b35..f6d548aff 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -170,7 +170,7 @@ func TestIssueList_tty_withFlags(t *testing.T) { assert.Equal(t, "probablyCher", params["assignee"].(string)) assert.Equal(t, "foo", params["author"].(string)) assert.Equal(t, "me", params["mention"].(string)) - assert.Equal(t, "12345", params["milestone"].(string)) // Database ID for the Milestone (see #1441) + assert.Equal(t, "12345", params["milestone"].(string)) // Database ID for the Milestone (see #1462) assert.Equal(t, []interface{}{"web", "bug"}, params["labels"].([]interface{})) assert.Equal(t, []interface{}{"OPEN"}, params["states"].([]interface{})) })) From bbce1ba75571c3cc1ed4bda3745e44227f8d951e Mon Sep 17 00:00:00 2001 From: Francisco Miamoto Date: Wed, 5 Aug 2020 20:49:01 -0300 Subject: [PATCH 7/9] add check for milestone not found --- api/queries_issue.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/api/queries_issue.go b/api/queries_issue.go index 91702b913..9c441c02d 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -265,6 +265,10 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str break } } + + if variables["milestone"] == nil { + return nil, fmt.Errorf("no milestone found with title: %q", milestoneString) + } } var response struct { From 83d80e6a09fa1d51b8216a43532cfe93c9b6ff81 Mon Sep 17 00:00:00 2001 From: Francisco Miamoto Date: Wed, 5 Aug 2020 21:12:03 -0300 Subject: [PATCH 8/9] add test for milestone not found --- command/issue_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/command/issue_test.go b/command/issue_test.go index f6d548aff..735358c87 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -282,6 +282,29 @@ func TestIssueList_web(t *testing.T) { eq(t, url, expectedURL) } +func TestIssueList_milestoneNotFound(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.Register( + httpmock.GraphQL(`query IssueList\b`), + httpmock.FileResponse("../test/fixtures/issueList.json"), + ) + http.Register( + httpmock.GraphQL(`query RepositoryMilestoneList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [{ "title":"1.x", "id": "MDk6TWlsZXN0b25lMTIzNDU=" }], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + + _, err := RunCommand("issue list --milestone NotFound") + if err == nil || err.Error() != `no milestone found with title: "NotFound"` { + t.Errorf("error running command `issue list`: %v", err) + } +} + func TestIssueView_web(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() From e12c35cc174f288f253f29c268cce3b5a8dc9bf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 11 Aug 2020 19:27:26 +0200 Subject: [PATCH 9/9] Add ability to pass milestone by number --- api/queries_issue.go | 30 +++++++++++++++--------------- api/queries_repo.go | 40 ++++++++++++++++++++++++++++++++++++++++ command/issue.go | 2 +- command/issue_test.go | 35 ++++++++++++++++++++++++++++++----- 4 files changed, 86 insertions(+), 21 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 0070425b4..6ef431ed4 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "fmt" + "strconv" "strings" "time" @@ -250,25 +251,24 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str } if milestoneString != "" { - milestones, err := RepoMilestones(client, repo) - if err != nil { - return nil, err - } - - for i := range milestones { - if strings.EqualFold(milestones[i].Title, milestoneString) { - id, err := milestoneNodeIdToDatabaseId(milestones[i].ID) - if err != nil { - return nil, err - } - variables["milestone"] = id - break + var milestone *RepoMilestone + if milestoneNumber, err := strconv.Atoi(milestoneString); err == nil { + milestone, err = MilestoneByNumber(client, repo, milestoneNumber) + if err != nil { + return nil, err + } + } else { + milestone, err = MilestoneByTitle(client, repo, milestoneString) + if err != nil { + return nil, err } } - if variables["milestone"] == nil { - return nil, fmt.Errorf("no milestone found with title: %q", milestoneString) + milestoneRESTID, err := milestoneNodeIdToDatabaseId(milestone.ID) + if err != nil { + return nil, err } + variables["milestone"] = milestoneRESTID } var response struct { diff --git a/api/queries_repo.go b/api/queries_repo.go index e19bc1cf6..5a9fd587e 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -804,3 +804,43 @@ func RepoMilestones(client *Client, repo ghrepo.Interface) ([]RepoMilestone, err return milestones, nil } + +func MilestoneByTitle(client *Client, repo ghrepo.Interface, title string) (*RepoMilestone, error) { + milestones, err := RepoMilestones(client, repo) + if err != nil { + return nil, err + } + + for i := range milestones { + if strings.EqualFold(milestones[i].Title, title) { + return &milestones[i], nil + } + } + return nil, fmt.Errorf("no milestone found with title %q", title) +} + +func MilestoneByNumber(client *Client, repo ghrepo.Interface, number int) (*RepoMilestone, error) { + var query struct { + Repository struct { + Milestone *RepoMilestone `graphql:"milestone(number: $number)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + "number": githubv4.Int(number), + } + + gql := graphQLClient(client.http, repo.RepoHost()) + + err := gql.QueryNamed(context.Background(), "RepositoryMilestoneByNumber", &query, variables) + if err != nil { + return nil, err + } + if query.Repository.Milestone == nil { + return nil, fmt.Errorf("no milestone found with number '%d'", number) + } + + return query.Repository.Milestone, nil +} diff --git a/command/issue.go b/command/issue.go index ffa83f81c..d003e1df2 100644 --- a/command/issue.go +++ b/command/issue.go @@ -46,7 +46,7 @@ func init() { issueListCmd.Flags().IntP("limit", "L", 30, "Maximum number of issues to fetch") issueListCmd.Flags().StringP("author", "A", "", "Filter by author") issueListCmd.Flags().String("mention", "", "Filter by mention") - issueListCmd.Flags().StringP("milestone", "m", "", "Filter by milestone `name`") + issueListCmd.Flags().StringP("milestone", "m", "", "Filter by milestone `number` or title") issueCmd.AddCommand(issueViewCmd) issueViewCmd.Flags().BoolP("web", "w", false, "Open an issue in the browser") diff --git a/command/issue_test.go b/command/issue_test.go index f8d85f50f..27b2b64d5 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -285,11 +285,8 @@ func TestIssueList_web(t *testing.T) { func TestIssueList_milestoneNotFound(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") - http.Register( - httpmock.GraphQL(`query IssueList\b`), - httpmock.FileResponse("../test/fixtures/issueList.json"), - ) http.Register( httpmock.GraphQL(`query RepositoryMilestoneList\b`), httpmock.StringResponse(` @@ -300,11 +297,39 @@ func TestIssueList_milestoneNotFound(t *testing.T) { `)) _, err := RunCommand("issue list --milestone NotFound") - if err == nil || err.Error() != `no milestone found with title: "NotFound"` { + if err == nil || err.Error() != `no milestone found with title "NotFound"` { t.Errorf("error running command `issue list`: %v", err) } } +func TestIssueList_milestoneByNumber(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") + http.Register( + httpmock.GraphQL(`query RepositoryMilestoneByNumber\b`), + httpmock.StringResponse(` + { "data": { "repository": { "milestone": { + "id": "MDk6TWlsZXN0b25lMTIzNDU=" + } } } } + `)) + http.Register( + httpmock.GraphQL(`query IssueList\b`), + httpmock.GraphQLQuery(` + { "data": { "repository": { + "hasIssuesEnabled": true, + "issues": { "nodes": [] } + } } }`, func(_ string, params map[string]interface{}) { + assert.Equal(t, "12345", params["milestone"].(string)) // Database ID for the Milestone (see #1462) + })) + + _, err := RunCommand("issue list --milestone 13") + if err != nil { + t.Fatalf("error running issue list: %v", err) + } +} + func TestIssueView_web(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP()