From d9ca764ac29fb73fab26a2a1736c256325638ad5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 19 Dec 2019 15:13:12 +0100 Subject: [PATCH 1/5] Look up the repository only once in `issue status` GraphQL --- api/queries_issue.go | 75 +++++++++++++++++----------------- command/issue_test.go | 10 ++--- test/fixtures/issueStatus.json | 20 ++++----- 3 files changed, 50 insertions(+), 55 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index eb2b3b170..7a386d8f5 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -26,12 +26,6 @@ type IssueLabel struct { Name string } -type apiIssues struct { - Issues struct { - Nodes []Issue - } -} - const fragments = ` fragment issue on Issue { number @@ -88,36 +82,39 @@ func IssueCreate(client *Client, ghRepo Repo, params map[string]interface{}) (*I func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPayload, error) { type response struct { - Assigned apiIssues - Mentioned apiIssues - Authored apiIssues + Repository struct { + Assigned struct { + Nodes []Issue + } + Mentioned struct { + Nodes []Issue + } + Authored struct { + Nodes []Issue + } + } } query := fragments + ` - query($owner: String!, $repo: String!, $viewer: String!, $per_page: Int = 10) { - assigned: repository(owner: $owner, name: $repo) { - issues(filterBy: {assignee: $viewer, states: OPEN}, first: $per_page, orderBy: {field: CREATED_AT, direction: DESC}) { - nodes { - ...issue - } - } - } - mentioned: repository(owner: $owner, name: $repo) { - issues(filterBy: {mentioned: $viewer, states: OPEN}, first: $per_page, orderBy: {field: CREATED_AT, direction: DESC}) { - nodes { - ...issue - } - } - } - authored: repository(owner: $owner, name: $repo) { - issues(filterBy: {createdBy: $viewer, states: OPEN}, first: $per_page, orderBy: {field: CREATED_AT, direction: DESC}) { - nodes { - ...issue - } - } - } - } - ` + query($owner: String!, $repo: String!, $viewer: String!, $per_page: Int = 10) { + repository(owner: $owner, name: $repo) { + assigned: issues(filterBy: {assignee: $viewer, states: OPEN}, first: $per_page, orderBy: {field: CREATED_AT, direction: DESC}) { + nodes { + ...issue + } + } + mentioned: issues(filterBy: {mentioned: $viewer, states: OPEN}, first: $per_page, orderBy: {field: CREATED_AT, direction: DESC}) { + nodes { + ...issue + } + } + authored: issues(filterBy: {createdBy: $viewer, states: OPEN}, first: $per_page, orderBy: {field: CREATED_AT, direction: DESC}) { + nodes { + ...issue + } + } + } + }` owner := ghRepo.RepoOwner() repo := ghRepo.RepoName() @@ -134,9 +131,9 @@ func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPa } payload := IssuesPayload{ - Assigned: resp.Assigned.Issues.Nodes, - Mentioned: resp.Mentioned.Issues.Nodes, - Authored: resp.Authored.Issues.Nodes, + Assigned: resp.Repository.Assigned.Nodes, + Mentioned: resp.Repository.Mentioned.Nodes, + Authored: resp.Repository.Authored.Nodes, } return &payload, nil @@ -192,7 +189,11 @@ func IssueList(client *Client, ghRepo Repo, state string, labels []string, assig } var resp struct { - Repository apiIssues + Repository struct { + Issues struct { + Nodes []Issue + } + } } err := client.GraphQL(query, variables, &resp) diff --git a/command/issue_test.go b/command/issue_test.go index bff2b5747..5a9609579 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -45,11 +45,11 @@ func TestIssueStatus_blankSlate(t *testing.T) { http := initFakeHTTP() http.StubResponse(200, bytes.NewBufferString(` - { "data": { - "assigned": { "issues": { "nodes": [] } }, - "mentioned": { "issues": { "nodes": [] } }, - "authored": { "issues": { "nodes": [] } } - } } + { "data": { "repository": { + "assigned": { "nodes": [] }, + "mentioned": { "nodes": [] }, + "authored": { "nodes": [] } + } } } `)) output, err := RunCommand(issueStatusCmd, "issue status") diff --git a/test/fixtures/issueStatus.json b/test/fixtures/issueStatus.json index f9e831c7a..6f8c45259 100644 --- a/test/fixtures/issueStatus.json +++ b/test/fixtures/issueStatus.json @@ -1,7 +1,7 @@ { "data": { - "assigned": { - "issues": { + "repository": { + "assigned": { "nodes": [ { "number": 9, @@ -12,10 +12,8 @@ "title": "broccoli is a superfood" } ] - } - }, - "mentioned": { - "issues": { + }, + "mentioned": { "nodes": [ { "number": 8, @@ -26,14 +24,10 @@ "title": "swiss chard is neutral" } ] - } - }, - "authored": { - "issues": { + }, + "authored": { "nodes": [] } - }, - - "pageInfo": { "hasNextPage": false } + } } } From 915dd8b0ef032e466b4f0c6a5636b98d1b453d90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 19 Dec 2019 15:26:06 +0100 Subject: [PATCH 2/5] Warn about repo issues disabled on `issue status/list` --- api/queries_issue.go | 12 ++++++++++++ command/issue_test.go | 8 +++++++- test/fixtures/issueList.json | 1 + test/fixtures/issueStatus.json | 1 + 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 7a386d8f5..19c0f8b24 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -92,12 +92,14 @@ func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPa Authored struct { Nodes []Issue } + HasIssuesEnabled bool } } query := fragments + ` query($owner: String!, $repo: String!, $viewer: String!, $per_page: Int = 10) { repository(owner: $owner, name: $repo) { + hasIssuesEnabled assigned: issues(filterBy: {assignee: $viewer, states: OPEN}, first: $per_page, orderBy: {field: CREATED_AT, direction: DESC}) { nodes { ...issue @@ -130,6 +132,10 @@ func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPa return nil, err } + if !resp.Repository.HasIssuesEnabled { + return nil, fmt.Errorf("the '%s/%s' repository has disabled issues", owner, repo) + } + payload := IssuesPayload{ Assigned: resp.Repository.Assigned.Nodes, Mentioned: resp.Repository.Mentioned.Nodes, @@ -168,6 +174,7 @@ func IssueList(client *Client, ghRepo Repo, state string, labels []string, assig query := fragments + ` query($owner: String!, $repo: String!, $limit: Int, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String) { repository(owner: $owner, name: $repo) { + hasIssuesEnabled issues(first: $limit, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, labels: $labels, filterBy: {assignee: $assignee}) { nodes { ...issue @@ -193,6 +200,7 @@ func IssueList(client *Client, ghRepo Repo, state string, labels []string, assig Issues struct { Nodes []Issue } + HasIssuesEnabled bool } } @@ -201,6 +209,10 @@ func IssueList(client *Client, ghRepo Repo, state string, labels []string, assig return nil, err } + if !resp.Repository.HasIssuesEnabled { + return nil, fmt.Errorf("the '%s/%s' repository has disabled issues", owner, repo) + } + return resp.Repository.Issues.Nodes, nil } diff --git a/command/issue_test.go b/command/issue_test.go index 5a9609579..b2711a8cd 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -46,6 +46,7 @@ func TestIssueStatus_blankSlate(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { + "hasIssuesEnabled": true, "assigned": { "nodes": [] }, "mentioned": { "nodes": [] }, "authored": { "nodes": [] } @@ -102,7 +103,12 @@ func TestIssueList(t *testing.T) { func TestIssueList_withFlags(t *testing.T) { http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(`{"data": {}}`)) // Since we are testing that the flags are passed, we don't care about the response + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "hasIssuesEnabled": true, + "issues": { "nodes": [] } + } } } + `)) output, err := RunCommand(issueListCmd, "issue list -a probablyCher -l web,bug -s open") if err != nil { diff --git a/test/fixtures/issueList.json b/test/fixtures/issueList.json index b22899cee..40537f12d 100644 --- a/test/fixtures/issueList.json +++ b/test/fixtures/issueList.json @@ -1,6 +1,7 @@ { "data": { "repository": { + "hasIssuesEnabled": true, "issues": { "nodes": [ { diff --git a/test/fixtures/issueStatus.json b/test/fixtures/issueStatus.json index 6f8c45259..265986902 100644 --- a/test/fixtures/issueStatus.json +++ b/test/fixtures/issueStatus.json @@ -1,6 +1,7 @@ { "data": { "repository": { + "hasIssuesEnabled": true, "assigned": { "nodes": [ { From 66534e504b0f2cbf8639c163bbb3fa74aeb2ec02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 19 Dec 2019 15:26:26 +0100 Subject: [PATCH 3/5] Warn about repo issues disabled on `issue create` --- api/queries_issue.go | 8 ++++++-- api/queries_pr.go | 4 ++-- api/queries_repo.go | 39 +++++++++++++++++++++++++++------------ command/issue_test.go | 3 ++- 4 files changed, 37 insertions(+), 17 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 19c0f8b24..3fb20e094 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -42,11 +42,15 @@ const fragments = ` ` func IssueCreate(client *Client, ghRepo Repo, params map[string]interface{}) (*Issue, error) { - repoID, err := GitHubRepoId(client, ghRepo) + repo, err := GitHubRepo(client, ghRepo) if err != nil { return nil, err } + if !repo.HasIssuesEnabled { + return nil, fmt.Errorf("the '%s/%s' repository has disabled issues", ghRepo.RepoOwner(), ghRepo.RepoName()) + } + query := ` mutation CreateIssue($input: CreateIssueInput!) { createIssue(input: $input) { @@ -57,7 +61,7 @@ func IssueCreate(client *Client, ghRepo Repo, params map[string]interface{}) (*I }` inputParams := map[string]interface{}{ - "repositoryId": repoID, + "repositoryId": repo.ID, } for key, val := range params { inputParams[key] = val diff --git a/api/queries_pr.go b/api/queries_pr.go index 01ece0369..a3feb959a 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -376,7 +376,7 @@ func PullRequestForBranch(client *Client, ghRepo Repo, branch string) (*PullRequ } func CreatePullRequest(client *Client, ghRepo Repo, params map[string]interface{}) (*PullRequest, error) { - repoID, err := GitHubRepoId(client, ghRepo) + repo, err := GitHubRepo(client, ghRepo) if err != nil { return nil, err } @@ -391,7 +391,7 @@ func CreatePullRequest(client *Client, ghRepo Repo, params map[string]interface{ }` inputParams := map[string]interface{}{ - "repositoryId": repoID, + "repositoryId": repo.ID, } for key, val := range params { inputParams[key] = val diff --git a/api/queries_repo.go b/api/queries_repo.go index 92010b880..f974b41cb 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1,16 +1,28 @@ package api -import "fmt" +import ( + "fmt" -func GitHubRepoId(client *Client, ghRepo Repo) (string, error) { + "github.com/pkg/errors" +) + +// Repository contains information about a GitHub repo +type Repository struct { + ID string + HasIssuesEnabled bool +} + +// GitHubRepo looks up the node ID of a named repository +func GitHubRepo(client *Client, ghRepo Repo) (*Repository, error) { owner := ghRepo.RepoOwner() repo := ghRepo.RepoName() query := ` - query FindRepoID($owner:String!, $name:String!) { - repository(owner:$owner, name:$name) { - id - } + query($owner: String!, $name: String!) { + repository(owner: $owner, name: $name) { + id + hasIssuesEnabled + } }` variables := map[string]interface{}{ "owner": owner, @@ -18,14 +30,17 @@ func GitHubRepoId(client *Client, ghRepo Repo) (string, error) { } result := struct { - Repository struct { - Id string - } + Repository Repository }{} err := client.GraphQL(query, variables, &result) - if err != nil || result.Repository.Id == "" { - return "", fmt.Errorf("failed to determine GH repo ID: %s", err) + + if err != nil || result.Repository.ID == "" { + newErr := fmt.Errorf("failed to determine repository ID for '%s/%s'", owner, repo) + if err != nil { + newErr = errors.Wrap(err, newErr.Error()) + } + return nil, newErr } - return result.Repository.Id, nil + return &result.Repository, nil } diff --git a/command/issue_test.go b/command/issue_test.go index b2711a8cd..66878aa2b 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -231,7 +231,8 @@ func TestIssueCreate(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { - "id": "REPOID" + "id": "REPOID", + "hasIssuesEnabled": true } } } `)) http.StubResponse(200, bytes.NewBufferString(` From aeb7f337d2150868f6d204eb307f9ada7010c4f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 19 Dec 2019 15:32:25 +0100 Subject: [PATCH 4/5] Ensure `issue create` fails fast if issues are disabled Before, a person would be prompted for title & body before unconditionally failing due to issues being disabled. --- api/queries_issue.go | 14 +++----------- command/issue.go | 10 +++++++++- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 3fb20e094..d76e1b79e 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -41,16 +41,8 @@ const fragments = ` } ` -func IssueCreate(client *Client, ghRepo Repo, params map[string]interface{}) (*Issue, error) { - repo, err := GitHubRepo(client, ghRepo) - if err != nil { - return nil, err - } - - if !repo.HasIssuesEnabled { - return nil, fmt.Errorf("the '%s/%s' repository has disabled issues", ghRepo.RepoOwner(), ghRepo.RepoName()) - } - +// IssueCreate creates an issue in a GitHub repository +func IssueCreate(client *Client, repo *Repository, params map[string]interface{}) (*Issue, error) { query := ` mutation CreateIssue($input: CreateIssueInput!) { createIssue(input: $input) { @@ -76,7 +68,7 @@ func IssueCreate(client *Client, ghRepo Repo, params map[string]interface{}) (*I } }{} - err = client.GraphQL(query, variables, &result) + err := client.GraphQL(query, variables, &result) if err != nil { return nil, err } diff --git a/command/issue.go b/command/issue.go index df30b6e52..5383daf41 100644 --- a/command/issue.go +++ b/command/issue.go @@ -257,6 +257,14 @@ func issueCreate(cmd *cobra.Command, args []string) error { return err } + repo, err := api.GitHubRepo(apiClient, baseRepo) + if err != nil { + return err + } + if !repo.HasIssuesEnabled { + return fmt.Errorf("the '%s/%s' repository has disabled issues", baseRepo.RepoOwner(), baseRepo.RepoName()) + } + title, err := cmd.Flags().GetString("title") if err != nil { return errors.Wrap(err, "could not parse title") @@ -291,7 +299,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { "body": body, } - newIssue, err := api.IssueCreate(apiClient, baseRepo, params) + newIssue, err := api.IssueCreate(apiClient, repo, params) if err != nil { return err } From bd9b3b9bb54d1aedf5457aec31f9e42fe13c5731 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 19 Dec 2019 15:54:08 +0100 Subject: [PATCH 5/5] Add tests for `issue status/list/create` on repos with issues disabled --- command/issue_test.go | 50 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/command/issue_test.go b/command/issue_test.go index 66878aa2b..50462a982 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -73,6 +73,22 @@ Issues opened by you } } +func TestIssueStatus_disabledIssues(t *testing.T) { + initBlankContext("OWNER/REPO", "master") + http := initFakeHTTP() + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "hasIssuesEnabled": false + } } } + `)) + + _, err := RunCommand(issueStatusCmd, "issue status") + if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { + t.Errorf("error running command `issue status`: %v", err) + } +} + func TestIssueList(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() @@ -101,6 +117,7 @@ func TestIssueList(t *testing.T) { } func TestIssueList_withFlags(t *testing.T) { + initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() http.StubResponse(200, bytes.NewBufferString(` @@ -133,6 +150,22 @@ func TestIssueList_withFlags(t *testing.T) { eq(t, reqBody.Variables.States, []string{"OPEN"}) } +func TestIssueList_disabledIssues(t *testing.T) { + initBlankContext("OWNER/REPO", "master") + http := initFakeHTTP() + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "hasIssuesEnabled": false + } } } + `)) + + _, err := RunCommand(issueListCmd, "issue list") + if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { + t.Errorf("error running command `issue list`: %v", err) + } +} + func TestIssueView(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() @@ -265,6 +298,23 @@ func TestIssueCreate(t *testing.T) { eq(t, output.String(), "https://github.com/OWNER/REPO/issues/12\n") } +func TestIssueCreate_disabledIssues(t *testing.T) { + initBlankContext("OWNER/REPO", "master") + http := initFakeHTTP() + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": false + } } } + `)) + + _, err := RunCommand(issueCreateCmd, `issue create -t heres -b johnny`) + if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { + t.Errorf("error running command `issue create`: %v", err) + } +} + func TestIssueCreate_web(t *testing.T) { initBlankContext("OWNER/REPO", "master") initFakeHTTP()