Merge pull request #6779 from cli/fix-comments-author

Fix fetching issue/PR comments
cli · Dec 23, 2022 · 6618baa · 6618baa
2 parents 7ffa87f + 8d82a96
commit 6618baa
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 36 deletions.
diff --git a/api/export_pr.go b/api/export_pr.go
@@ -11,18 +11,6 @@ func (issue *Issue) ExportData(fields []string) map[string]interface{} {
 
 	for _, f := range fields {
 		switch f {
-		case "author":
-			author := map[string]interface{}{
-				"is_bot": issue.Author.IsBot(),
-			}
-			if issue.Author.IsBot() {
-				author["login"] = "app/" + issue.Author.Login
-			} else {
-				author["login"] = issue.Author.Login
-				author["name"] = issue.Author.Name
-				author["id"] = issue.Author.ID
-			}
-			data[f] = author
 		case "comments":
 			data[f] = issue.Comments.Nodes
 		case "assignees":
@@ -46,18 +34,6 @@ func (pr *PullRequest) ExportData(fields []string) map[string]interface{} {
 
 	for _, f := range fields {
 		switch f {
-		case "author":
-			author := map[string]interface{}{
-				"is_bot": pr.Author.IsBot(),
-			}
-			if pr.Author.IsBot() {
-				author["login"] = "app/" + pr.Author.Login
-			} else {
-				author["login"] = pr.Author.Login
-				author["name"] = pr.Author.Name
-				author["id"] = pr.Author.ID
-			}
-			data[f] = author
 		case "headRepository":
 			data[f] = pr.HeadRepository
 		case "statusCheckRollup":

diff --git a/api/queries_comments.go b/api/queries_comments.go
@@ -27,7 +27,7 @@ func (cs Comments) CurrentUserComments() []Comment {
 
 type Comment struct {
 	ID                  string         `json:"id"`
-	Author              Author         `json:"author"`
+	Author              CommentAuthor  `json:"author"`
 	AuthorAssociation   string         `json:"authorAssociation"`
 	Body                string         `json:"body"`
 	CreatedAt           time.Time      `json:"createdAt"`

diff --git a/api/queries_issue.go b/api/queries_issue.go
@@ -1,6 +1,7 @@
 package api
 
 import (
+	"encoding/json"
 	"fmt"
 	"time"
 
@@ -120,13 +121,35 @@ type Owner struct {
 }
 
 type Author struct {
-	ID    string `json:"id,omitempty"`
-	Name  string `json:"name,omitempty"`
-	Login string `json:"login"`
+	ID    string
+	Name  string
+	Login string
 }
 
-func (author *Author) IsBot() bool {
-	return author.ID == ""
+func (author Author) MarshalJSON() ([]byte, error) {
+	if author.ID == "" {
+		return json.Marshal(map[string]interface{}{
+			"is_bot": true,
+			"login":  "app/" + author.Login,
+		})
+	}
+	return json.Marshal(map[string]interface{}{
+		"is_bot": false,
+		"login":  author.Login,
+		"id":     author.ID,
+		"name":   author.Name,
+	})
+}
+
+type CommentAuthor struct {
+	Login string `json:"login"`
+	// Unfortunately, there is no easy way to add "id" and "name" fields to this struct because it's being
+	// used in both shurcool-graphql type queries and string-based queries where the response gets parsed
+	// by an ordinary JSON decoder that doesn't understand "graphql" directives via struct tags.
+	//	User  *struct {
+	//		ID   string
+	//		Name string
+	//	} `graphql:"... on User"`
 }
 
 // IssueCreate creates an issue in a GitHub repository

diff --git a/api/query_builder.go b/api/query_builder.go
@@ -24,7 +24,7 @@ var issueComments = shortenQuery(`
 	comments(first: 100) {
 		nodes {
 			id,
-			author{login},
+			author{login,...on User{id,name}},
 			authorAssociation,
 			body,
 			createdAt,
@@ -43,7 +43,7 @@ var issueComments = shortenQuery(`
 var issueCommentLast = shortenQuery(`
 	comments(last: 1) {
 		nodes {
-			author{login},
+			author{login,...on User{id,name}},
 			authorAssociation,
 			body,
 			createdAt,

diff --git a/pkg/cmd/issue/comment/comment_test.go b/pkg/cmd/issue/comment/comment_test.go
@@ -324,8 +324,8 @@ func Test_commentRun(t *testing.T) {
 				ID:  "ISSUE-ID",
 				URL: "https://github.com/OWNER/REPO/issues/123",
 				Comments: api.Comments{Nodes: []api.Comment{
-					{ID: "id1", Author: api.Author{Login: "octocat"}, URL: "https://github.com/OWNER/REPO/issues/123#issuecomment-111", ViewerDidAuthor: true},
-					{ID: "id2", Author: api.Author{Login: "monalisa"}, URL: "https://github.com/OWNER/REPO/issues/123#issuecomment-222"},
+					{ID: "id1", Author: api.CommentAuthor{Login: "octocat"}, URL: "https://github.com/OWNER/REPO/issues/123#issuecomment-111", ViewerDidAuthor: true},
+					{ID: "id2", Author: api.CommentAuthor{Login: "monalisa"}, URL: "https://github.com/OWNER/REPO/issues/123#issuecomment-222"},
 				}},
 			}, ghrepo.New("OWNER", "REPO"), nil
 		}

diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go
@@ -153,6 +153,8 @@ func findIssue(client *http.Client, baseRepoFn func() (ghrepo.Interface, error),
 	}
 
 	if fieldSet.Contains("comments") {
+		// FIXME: this re-fetches the comments connection even though the initial set of 100 were
+		// fetched in the previous request.
 		err = preloadIssueComments(client, repo, issue)
 	}
 	return issue, err

diff --git a/pkg/cmd/pr/comment/comment_test.go b/pkg/cmd/pr/comment/comment_test.go
@@ -344,8 +344,8 @@ func Test_commentRun(t *testing.T) {
 				Number: 123,
 				URL:    "https://github.com/OWNER/REPO/pull/123",
 				Comments: api.Comments{Nodes: []api.Comment{
-					{ID: "id1", Author: api.Author{Login: "octocat"}, URL: "https://github.com/OWNER/REPO/pull/123#issuecomment-111", ViewerDidAuthor: true},
-					{ID: "id2", Author: api.Author{Login: "monalisa"}, URL: "https://github.com/OWNER/REPO/pull/123#issuecomment-222"},
+					{ID: "id1", Author: api.CommentAuthor{Login: "octocat"}, URL: "https://github.com/OWNER/REPO/pull/123#issuecomment-111", ViewerDidAuthor: true},
+					{ID: "id2", Author: api.CommentAuthor{Login: "monalisa"}, URL: "https://github.com/OWNER/REPO/pull/123#issuecomment-222"},
 				}},
 			}, ghrepo.New("OWNER", "REPO"), nil
 		}