Skip to content

Commit 34fc5fb

Browse files
mislavmislav
committed
Improve issue view re: overfetching, PR support
- Supports passing a PR as argument, not just issues - Makes it non-fatal when project cards were not able to load - Cleans up legacy method for fetching issues
1 parent 3cf3d6d commit 34fc5fb

File tree

11 files changed

+240
-183
lines changed

11 files changed

+240
-183
lines changed

‎api/client.go‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -146,17 +146,31 @@ type GraphQLErrorResponse struct {
146146
func (gr GraphQLErrorResponse) Error() string {
147147
errorMessages := make([]string, 0, len(gr.Errors))
148148
for _, e := range gr.Errors {
149-
errorMessages = append(errorMessages, e.Message)
149+
msg := e.Message
150+
if p := e.PathString(); p != "" {
151+
msg = fmt.Sprintf("%s (%s)", msg, p)
152+
}
153+
errorMessages = append(errorMessages, msg)
150154
}
151-
return fmt.Sprintf("GraphQL error: %s", strings.Join(errorMessages, "\n"))
155+
return fmt.Sprintf("GraphQL: %s", strings.Join(errorMessages, ", "))
152156
}
153157

154-
// Match checks if this error is only about a specific type on a specific path.
158+
// Match checks if this error is only about a specific type on a specific path. If the path argument ends
159+
// with a ".", it will match all its subpaths as well.
155160
func (gr GraphQLErrorResponse) Match(expectType, expectPath string) bool {
156-
if len(gr.Errors) != 1 {
157-
return false
161+
for _, e := range gr.Errors {
162+
if e.Type != expectType || !matchPath(e.PathString(), expectPath) {
163+
return false
164+
}
165+
}
166+
return true
167+
}
168+
169+
func matchPath(p, expect string) bool {
170+
if strings.HasSuffix(expect, ".") {
171+
return strings.HasPrefix(p, expect) || p == strings.TrimSuffix(expect, ".")
158172
}
159-
return gr.Errors[0].Type == expectType && gr.Errors[0].PathString() == expectPath
173+
return p == expect
160174
}
161175

162176
// HTTPError is an error returned by a failed API call

‎api/client_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ func TestGraphQLError(t *testing.T) {
6666
)
6767

6868
err := client.GraphQL("github.com", "", nil, &response)
69-
if err == nil || err.Error() != "GraphQL error: OH NO\nthis is fine" {
69+
if err == nil || err.Error() != "GraphQL: OH NO (repository.issue), this is fine (repository.issues.0.comments)" {
7070
t.Fatalf("got %q", err.Error())
7171
}
7272
}

‎api/queries_issue.go‎

Lines changed: 10 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -71,17 +71,19 @@ func (l Labels) Names() []string {
7171
}
7272

7373
type ProjectCards struct {
74-
Nodes []struct {
75-
Project struct {
76-
Name string `json:"name"`
77-
} `json:"project"`
78-
Column struct {
79-
Name string `json:"name"`
80-
} `json:"column"`
81-
}
74+
Nodes []*ProjectInfo
8275
TotalCount int
8376
}
8477

78+
type ProjectInfo struct {
79+
Project struct {
80+
Name string `json:"name"`
81+
} `json:"project"`
82+
Column struct {
83+
Name string `json:"name"`
84+
} `json:"column"`
85+
}
86+
8587
func (p ProjectCards) ProjectNames() []string {
8688
names := make([]string, len(p.Nodes))
8789
for i, c := range p.Nodes {
@@ -233,113 +235,6 @@ func IssueStatus(client *Client, repo ghrepo.Interface, options IssueStatusOptio
233235
return &payload, nil
234236
}
235237

236-
func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, error) {
237-
type response struct {
238-
Repository struct {
239-
Issue Issue
240-
HasIssuesEnabled bool
241-
}
242-
}
243-
244-
query := `
245-
query IssueByNumber($owner: String!, $repo: String!, $issue_number: Int!) {
246-
repository(owner: $owner, name: $repo) {
247-
hasIssuesEnabled
248-
issue(number: $issue_number) {
249-
id
250-
title
251-
state
252-
body
253-
author {
254-
login
255-
}
256-
comments(last: 1) {
257-
nodes {
258-
author {
259-
login
260-
}
261-
authorAssociation
262-
body
263-
createdAt
264-
includesCreatedEdit
265-
isMinimized
266-
minimizedReason
267-
reactionGroups {
268-
content
269-
users {
270-
totalCount
271-
}
272-
}
273-
}
274-
totalCount
275-
}
276-
number
277-
url
278-
createdAt
279-
assignees(first: 100) {
280-
nodes {
281-
id
282-
name
283-
login
284-
}
285-
totalCount
286-
}
287-
labels(first: 100) {
288-
nodes {
289-
id
290-
name
291-
description
292-
color
293-
}
294-
totalCount
295-
}
296-
projectCards(first: 100) {
297-
nodes {
298-
project {
299-
name
300-
}
301-
column {
302-
name
303-
}
304-
}
305-
totalCount
306-
}
307-
milestone {
308-
number
309-
title
310-
description
311-
dueOn
312-
}
313-
reactionGroups {
314-
content
315-
users {
316-
totalCount
317-
}
318-
}
319-
}
320-
}
321-
}`
322-
323-
variables := map[string]interface{}{
324-
"owner": repo.RepoOwner(),
325-
"repo": repo.RepoName(),
326-
"issue_number": number,
327-
}
328-
329-
var resp response
330-
err := client.GraphQL(repo.RepoHost(), query, variables, &resp)
331-
if err != nil {
332-
return nil, err
333-
}
334-
335-
if !resp.Repository.HasIssuesEnabled {
336-
337-
return nil, &IssuesDisabledError{fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo))}
338-
}
339-
340-
return &resp.Repository.Issue, nil
341-
}
342-
343238
func (i Issue) Link() string {
344239
return i.URL
345240
}

‎api/queries_repo_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ func TestGitHubRepo_notFound(t *testing.T) {
2323
if err == nil {
2424
t.Fatal("GitHubRepo did not return an error")
2525
}
26-
if wants := "GraphQL error: Could not resolve to a Repository with the name 'OWNER/REPO'."; err.Error() != wants {
26+
if wants := "GraphQL: Could not resolve to a Repository with the name 'OWNER/REPO'."; err.Error() != wants {
2727
t.Errorf("GitHubRepo error: want %q, got %q", wants, err.Error())
2828
}
2929
if repo != nil {

‎api/query_builder.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,22 @@ var issueComments = shortenQuery(`
3535
}
3636
`)
3737

38+
var issueCommentLast = shortenQuery(`
39+
comments(last: 1) {
40+
nodes {
41+
author{login},
42+
authorAssociation,
43+
body,
44+
createdAt,
45+
includesCreatedEdit,
46+
isMinimized,
47+
minimizedReason,
48+
reactionGroups{content,users{totalCount}}
49+
},
50+
totalCount
51+
}
52+
`)
53+
3854
var prReviewRequests = shortenQuery(`
3955
reviewRequests(first: 100) {
4056
nodes {
@@ -206,6 +222,8 @@ func PullRequestGraphQL(fields []string) string {
206222
q = append(q, `potentialMergeCommit{oid}`)
207223
case "comments":
208224
q = append(q, issueComments)
225+
case "lastComment": // pseudo-field
226+
q = append(q, issueCommentLast)
209227
case "reviewRequests":
210228
q = append(q, prReviewRequests)
211229
case "reviews":

‎pkg/cmd/issue/delete/delete_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ func TestIssueDelete_doesNotExist(t *testing.T) {
133133
)
134134

135135
_, err := runCommand(httpRegistry, true, "13")
136-
if err == nil || err.Error() != "GraphQL error: Could not resolve to an Issue with the number of 13." {
136+
if err == nil || err.Error() != "GraphQL: Could not resolve to an Issue with the number of 13." {
137137
t.Errorf("error running command `issue delete`: %v", err)
138138
}
139139
}

‎pkg/cmd/issue/shared/lookup.go‎

Lines changed: 21 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -13,32 +13,8 @@ import (
1313
"github.com/cli/cli/v2/internal/ghrepo"
1414
)
1515

16-
// IssueFromArg loads an issue with all its fields.
17-
// Deprecated: use IssueFromArgWithFields instead.
18-
func IssueFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), arg string) (*api.Issue, ghrepo.Interface, error) {
19-
issueNumber, baseRepo := issueMetadataFromURL(arg)
20-
21-
if issueNumber == 0 {
22-
var err error
23-
issueNumber, err = strconv.Atoi(strings.TrimPrefix(arg, "#"))
24-
if err != nil {
25-
return nil, nil, fmt.Errorf("invalid issue format: %q", arg)
26-
}
27-
}
28-
29-
if baseRepo == nil {
30-
var err error
31-
baseRepo, err = baseRepoFn()
32-
if err != nil {
33-
return nil, nil, fmt.Errorf("could not determine base repo: %w", err)
34-
}
35-
}
36-
37-
issue, err := api.IssueByNumber(apiClient, baseRepo, issueNumber)
38-
return issue, baseRepo, err
39-
}
40-
41-
// IssueFromArgWithFields loads an issue or pull request with the specified fields.
16+
// IssueFromArgWithFields loads an issue or pull request with the specified fields. If some of the fields
17+
// could not be fetched by GraphQL, this returns a non-nil issue and a *PartialLoadError.
4218
func IssueFromArgWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.Interface, error), arg string, fields []string) (*api.Issue, ghrepo.Interface, error) {
4319
issueNumber, baseRepo := issueMetadataFromURL(arg)
4420

@@ -84,6 +60,10 @@ func issueMetadataFromURL(s string) (int, ghrepo.Interface) {
8460
return issueNumber, repo
8561
}
8662

63+
type PartialLoadError struct {
64+
error
65+
}
66+
8767
func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) {
8868
type response struct {
8969
Repository struct {
@@ -114,8 +94,21 @@ func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, f
11494
client := api.NewClientFromHTTP(httpClient)
11595
if err := client.GraphQL(repo.RepoHost(), query, variables, &resp); err != nil {
11696
var gerr *api.GraphQLErrorResponse
117-
if errors.As(err, &gerr) && gerr.Match("NOT_FOUND", "repository.issue") && !resp.Repository.HasIssuesEnabled {
118-
return nil, fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo))
97+
if errors.As(err, &gerr) {
98+
if gerr.Match("NOT_FOUND", "repository.issue") && !resp.Repository.HasIssuesEnabled {
99+
return nil, fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo))
100+
} else if gerr.Match("FORBIDDEN", "repository.issue.projectCards.") {
101+
issue := resp.Repository.Issue
102+
// remove nil entries for project cards due to permission issues
103+
projects := make([]*api.ProjectInfo, 0, len(issue.ProjectCards.Nodes))
104+
for _, p := range issue.ProjectCards.Nodes {
105+
if p != nil {
106+
projects = append(projects, p)
107+
}
108+
}
109+
issue.ProjectCards.Nodes = projects
110+
return issue, &PartialLoadError{err}
111+
}
119112
}
120113
return nil, err
121114
}

0 commit comments

Comments
 (0)