From 949df38d49ce7dad4bb99ad446ec7868e00cb578 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 24 Mar 2021 17:52:29 +0100 Subject: [PATCH] BREAKING: lookup all issue/PR labels with "AND" instead of "OR" combinator This switches to the Search API whenever labels are specified in `issue list` or `pr list`. This ensures that the results match those that would be returned in the web UI. --- api/queries_issue.go | 9 +++------ api/queries_issue_test.go | 4 ++-- pkg/cmd/issue/list/list.go | 3 +-- pkg/cmd/issue/list/list_test.go | 35 +++++++++++++++++++++++++++++++-- pkg/cmd/pr/list/http.go | 17 +++++----------- pkg/cmd/pr/list/http_test.go | 27 +++++++++++-------------- pkg/cmd/pr/list/list_test.go | 5 ++--- 7 files changed, 57 insertions(+), 43 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 679964e87..55ec25ef7 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -233,7 +233,7 @@ func IssueStatus(client *Client, repo ghrepo.Interface, currentUsername string) return &payload, nil } -func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int, authorString string, mentionString string, milestoneString string) (*IssuesAndTotalCount, error) { +func IssueList(client *Client, repo ghrepo.Interface, state string, assigneeString string, limit int, authorString string, mentionString string, milestoneString string) (*IssuesAndTotalCount, error) { var states []string switch state { case "open", "": @@ -247,10 +247,10 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str } query := fragments + ` - query IssueList($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String, $author: String, $mention: String, $milestone: String) { + query IssueList($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $assignee: String, $author: String, $mention: String, $milestone: String) { repository(owner: $owner, name: $repo) { hasIssuesEnabled - issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, labels: $labels, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mention, milestone: $milestone}) { + issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mention, milestone: $milestone}) { totalCount nodes { ...issue @@ -269,9 +269,6 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str "repo": repo.RepoName(), "states": states, } - if len(labels) > 0 { - variables["labels"] = labels - } if assigneeString != "" { variables["assignee"] = assigneeString } diff --git a/api/queries_issue_test.go b/api/queries_issue_test.go index e6aca1907..829005995 100644 --- a/api/queries_issue_test.go +++ b/api/queries_issue_test.go @@ -47,7 +47,7 @@ func TestIssueList(t *testing.T) { ) repo, _ := ghrepo.FromFullName("OWNER/REPO") - _, err := IssueList(client, repo, "open", []string{}, "", 251, "", "", "") + _, err := IssueList(client, repo, "open", "", 251, "", "", "") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -127,7 +127,7 @@ func TestIssueList_pagination(t *testing.T) { ) repo := ghrepo.New("OWNER", "REPO") - res, err := IssueList(client, repo, "", nil, "", 0, "", "", "") + res, err := IssueList(client, repo, "", "", 0, "", "", "") if err != nil { t.Fatalf("IssueList() error = %v", err) } diff --git a/pkg/cmd/issue/list/list.go b/pkg/cmd/issue/list/list.go index 88fe2fecf..fc0c3d26f 100644 --- a/pkg/cmd/issue/list/list.go +++ b/pkg/cmd/issue/list/list.go @@ -145,7 +145,7 @@ func listRun(opts *ListOptions) error { func issueList(client *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.IssuesAndTotalCount, error) { apiClient := api.NewClientFromHTTP(client) - if filters.Search != "" { + if filters.Search != "" || len(filters.Labels) > 0 { if milestoneNumber, err := strconv.ParseInt(filters.Milestone, 10, 32); err == nil { milestone, err := api.MilestoneByNumber(apiClient, repo, int32(milestoneNumber)) if err != nil { @@ -176,7 +176,6 @@ func issueList(client *http.Client, repo ghrepo.Interface, filters prShared.Filt apiClient, repo, filters.State, - filters.Labels, filterAssignee, limit, filterAuthor, diff --git a/pkg/cmd/issue/list/list_test.go b/pkg/cmd/issue/list/list_test.go index feb84b4e4..d374f34ba 100644 --- a/pkg/cmd/issue/list/list_test.go +++ b/pkg/cmd/issue/list/list_test.go @@ -123,7 +123,6 @@ func TestIssueList_tty_withFlags(t *testing.T) { assert.Equal(t, "foo", params["author"].(string)) assert.Equal(t, "me", params["mention"].(string)) assert.Equal(t, "12345", params["milestone"].(string)) - assert.Equal(t, []interface{}{"web", "bug"}, params["labels"].([]interface{})) assert.Equal(t, []interface{}{"OPEN"}, params["states"].([]interface{})) })) @@ -136,7 +135,7 @@ func TestIssueList_tty_withFlags(t *testing.T) { } } } } `)) - output, err := runCommand(http, true, "-a probablyCher -l web,bug -s open -A foo --mention me --milestone 1.x") + output, err := runCommand(http, true, "-a probablyCher -s open -A foo --mention me --milestone 1.x") if err != nil { t.Errorf("error running command `issue list`: %v", err) } @@ -476,6 +475,38 @@ func Test_issueList(t *testing.T) { })) }, }, + { + name: "with labels", + args: args{ + limit: 30, + repo: ghrepo.New("OWNER", "REPO"), + filters: prShared.FilterOptions{ + Entity: "issue", + State: "open", + Labels: []string{"hello", "one world"}, + }, + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query IssueSearch\b`), + httpmock.GraphQLQuery(` + { "data": { + "repository": { "hasIssuesEnabled": true }, + "search": { + "issueCount": 0, + "nodes": [] + } + } }`, func(_ string, params map[string]interface{}) { + assert.Equal(t, map[string]interface{}{ + "owner": "OWNER", + "repo": "REPO", + "limit": float64(30), + "query": `repo:OWNER/REPO is:issue is:open label:hello label:"one world"`, + "type": "ISSUE", + }, params) + })) + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/cmd/pr/list/http.go b/pkg/cmd/pr/list/http.go index 8936600bc..127267a86 100644 --- a/pkg/cmd/pr/list/http.go +++ b/pkg/cmd/pr/list/http.go @@ -24,7 +24,7 @@ const fragment = `fragment pr on PullRequest { }` func listPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.PullRequestAndTotalCount, error) { - if filters.Author != "" || filters.Assignee != "" || filters.Search != "" { + if filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 { return searchPullRequests(httpClient, repo, filters, limit) } @@ -48,14 +48,12 @@ func listPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters pr $limit: Int!, $endCursor: String, $baseBranch: String, - $labels: [String!], $state: [PullRequestState!] = OPEN ) { repository(owner: $owner, name: $repo) { pullRequests( states: $state, baseRefName: $baseBranch, - labels: $labels, first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC} @@ -74,9 +72,8 @@ func listPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters pr pageLimit := min(limit, 100) variables := map[string]interface{}{ - "owner": repo.RepoOwner(), - "repo": repo.RepoName(), - "labels": filters.Labels, + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), } switch filters.State { @@ -135,10 +132,6 @@ loop: } func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.PullRequestAndTotalCount, error) { - if len(filters.Labels) > 1 { - return nil, fmt.Errorf("multiple labels with --assignee are not supported") - } - type response struct { Search struct { Nodes []api.PullRequest @@ -189,8 +182,8 @@ func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters if filters.Assignee != "" { q.AssignedTo(filters.Assignee) } - if len(filters.Labels) > 0 { - q.AddLabel(filters.Labels[0]) + for _, label := range filters.Labels { + q.AddLabel(label) } if filters.BaseBranch != "" { q.SetBaseBranch(filters.BaseBranch) diff --git a/pkg/cmd/pr/list/http_test.go b/pkg/cmd/pr/list/http_test.go index 6ac4e2cfa..0a33b7a30 100644 --- a/pkg/cmd/pr/list/http_test.go +++ b/pkg/cmd/pr/list/http_test.go @@ -36,11 +36,10 @@ func Test_listPullRequests(t *testing.T) { httpmock.GraphQL(`query PullRequestList\b`), httpmock.GraphQLQuery(`{"data":{}}`, func(query string, vars map[string]interface{}) { want := map[string]interface{}{ - "owner": "OWNER", - "repo": "REPO", - "state": []interface{}{"OPEN"}, - "labels": nil, - "limit": float64(30), + "owner": "OWNER", + "repo": "REPO", + "state": []interface{}{"OPEN"}, + "limit": float64(30), } if !reflect.DeepEqual(vars, want) { t.Errorf("got GraphQL variables %#v, want %#v", vars, want) @@ -62,11 +61,10 @@ func Test_listPullRequests(t *testing.T) { httpmock.GraphQL(`query PullRequestList\b`), httpmock.GraphQLQuery(`{"data":{}}`, func(query string, vars map[string]interface{}) { want := map[string]interface{}{ - "owner": "OWNER", - "repo": "REPO", - "state": []interface{}{"CLOSED", "MERGED"}, - "labels": nil, - "limit": float64(30), + "owner": "OWNER", + "repo": "REPO", + "state": []interface{}{"CLOSED", "MERGED"}, + "limit": float64(30), } if !reflect.DeepEqual(vars, want) { t.Errorf("got GraphQL variables %#v, want %#v", vars, want) @@ -86,14 +84,11 @@ func Test_listPullRequests(t *testing.T) { }, httpStub: func(r *httpmock.Registry) { r.Register( - httpmock.GraphQL(`query PullRequestList\b`), + httpmock.GraphQL(`query PullRequestSearch\b`), httpmock.GraphQLQuery(`{"data":{}}`, func(query string, vars map[string]interface{}) { want := map[string]interface{}{ - "owner": "OWNER", - "repo": "REPO", - "state": []interface{}{"OPEN"}, - "labels": []interface{}{"hello", "one world"}, - "limit": float64(30), + "q": `repo:OWNER/REPO is:pr is:open label:hello label:"one world" sort:created-desc`, + "limit": float64(30), } if !reflect.DeepEqual(vars, want) { t.Errorf("got GraphQL variables %#v, want %#v", vars, want) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 28be3aee2..e9a6d1bb7 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -106,10 +106,9 @@ func TestPRList_filtering(t *testing.T) { httpmock.GraphQL(`query PullRequestList\b`), httpmock.GraphQLQuery(`{}`, func(_ string, params map[string]interface{}) { assert.Equal(t, []interface{}{"OPEN", "CLOSED", "MERGED"}, params["state"].([]interface{})) - assert.Equal(t, []interface{}{"one", "two", "three"}, params["labels"].([]interface{})) })) - output, err := runCommand(http, true, `-s all -l one,two -l three`) + output, err := runCommand(http, true, `-s all`) if err != nil { t.Fatal(err) } @@ -129,7 +128,7 @@ func TestPRList_filteringRemoveDuplicate(t *testing.T) { httpmock.GraphQL(`query PullRequestList\b`), httpmock.FileResponse("./fixtures/prListWithDuplicates.json")) - output, err := runCommand(http, true, "-l one,two") + output, err := runCommand(http, true, "") if err != nil { t.Fatal(err) }