From ff20a75a46922e7e6cc9dad44bcabb790529b254 Mon Sep 17 00:00:00 2001 From: nobe4 Date: Sat, 14 Sep 2024 14:39:14 +0200 Subject: [PATCH] feat(notification): add LastCommentor field (#168) --- internal/api/mock/mock.go | 6 -- internal/gh/enrichments.go | 66 +++++++++++++++++---- internal/gh/gh_test.go | 77 ++++++++++++++++++------- internal/notifications/notifications.go | 12 ++-- internal/notifications/view.go | 6 +- 5 files changed, 124 insertions(+), 43 deletions(-) diff --git a/internal/api/mock/mock.go b/internal/api/mock/mock.go index 1cdc519..d0b724d 100644 --- a/internal/api/mock/mock.go +++ b/internal/api/mock/mock.go @@ -5,8 +5,6 @@ import ( "io" "log/slog" "net/http" - - "github.com/nobe4/gh-not/internal/api" ) type Mock struct { @@ -25,10 +23,6 @@ func (e *MockError) Error() string { return fmt.Sprintf("mock error for call [%s %s]: %s", e.verb, e.endpoint, e.message) } -func New(c []Call) api.Requestor { - return &Mock{Calls: c} -} - func (m *Mock) Done() error { if m.index < len(m.Calls) { return &MockError{"", "", fmt.Sprintf("%d calls remaining", len(m.Calls)-m.index)} diff --git a/internal/gh/enrichments.go b/internal/gh/enrichments.go index 2b2a12d..a92e3b3 100644 --- a/internal/gh/enrichments.go +++ b/internal/gh/enrichments.go @@ -9,7 +9,7 @@ import ( "github.com/nobe4/gh-not/internal/notifications" ) -type Extra struct { +type ThreadExtra struct { User notifications.User `json:"user"` State string `json:"state"` HtmlUrl string `json:"html_url"` @@ -20,27 +20,73 @@ func (c *Client) Enrich(n *notifications.Notification) error { return nil } - slog.Debug("enriching", "id", n.Id, "url", n.Subject.URL) - resp, err := c.API.Request(http.MethodGet, n.Subject.URL, nil) + threadExtra, err := c.getThreadExtra(n) + if err != nil { + return err + } + n.Author = threadExtra.User + n.Subject.State = threadExtra.State + n.Subject.HtmlUrl = threadExtra.HtmlUrl + + LastCommentor, err := c.getLastCommentor(n) if err != nil { return err } + n.LatestCommentor = LastCommentor + + return nil +} + +func (c *Client) getThreadExtra(n *notifications.Notification) (ThreadExtra, error) { + if n.Subject.URL == "" { + return ThreadExtra{}, nil + } + + slog.Debug("getting the thread extra", "id", n.Id, "url", n.Subject.URL) + resp, err := c.API.Request(http.MethodGet, n.Subject.URL, nil) + if err != nil { + return ThreadExtra{}, err + } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) if err != nil { - return err + return ThreadExtra{}, err } - extra := Extra{} + extra := ThreadExtra{} err = json.Unmarshal(body, &extra) if err != nil { - return err + return ThreadExtra{}, err } - n.Author = extra.User - n.Subject.State = extra.State - n.Subject.HtmlUrl = extra.HtmlUrl + return extra, nil +} - return nil +func (c *Client) getLastCommentor(n *notifications.Notification) (notifications.User, error) { + if n.Subject.LatestCommentUrl == "" { + return notifications.User{}, nil + } + + slog.Debug("getting the last commentor", "id", n.Id, "url", n.Subject.LatestCommentUrl) + resp, err := c.API.Request(http.MethodGet, n.Subject.LatestCommentUrl, nil) + if err != nil { + return notifications.User{}, err + } + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return notifications.User{}, err + } + + comment := struct { + User notifications.User `json:"user"` + }{} + err = json.Unmarshal(body, &comment) + if err != nil { + return notifications.User{}, err + } + + return comment.User, nil } diff --git a/internal/gh/gh_test.go b/internal/gh/gh_test.go index 8faa6b4..698ef44 100644 --- a/internal/gh/gh_test.go +++ b/internal/gh/gh_test.go @@ -30,11 +30,16 @@ func mockSubjectUrl(id int) string { return "https://subject.url/" + strconv.Itoa(id) } +func mockLatestCommentUrl(id int) string { + return "https://latest.comment.url/" + strconv.Itoa(id) +} + func mockNotification(id int) *notifications.Notification { return ¬ifications.Notification{ Id: strconv.Itoa(id), Subject: notifications.Subject{ - URL: mockSubjectUrl(id), + URL: mockSubjectUrl(id), + LatestCommentUrl: mockLatestCommentUrl(id), }, } } @@ -87,13 +92,14 @@ func notificationsEqual(a, b []*notifications.Notification) bool { return true } -func mockClient(c []mock.Call) *Client { +func mockClient(c []mock.Call) (*Client, *mock.Mock) { + mock := &mock.Mock{Calls: c} return &Client{ - API: mock.New(c), + API: mock, endpoint: endpoint, maxRetry: 100, maxPage: 100, - } + }, mock } func TestIsRetryable(t *testing.T) { @@ -247,7 +253,7 @@ func TestNextPageLink(t *testing.T) { func TestRequest(t *testing.T) { t.Run("errors", func(t *testing.T) { expectedError := errors.New("error") - client := mockClient([]mock.Call{{Error: expectedError}}) + client, api := mockClient([]mock.Call{{Error: expectedError}}) _, _, err := client.request(verb, endpoint, nil) if err == nil { @@ -257,6 +263,10 @@ func TestRequest(t *testing.T) { if err != expectedError { t.Errorf("expected %#v, got %#v", expectedError, err) } + + if err := api.Done(); err != nil { + t.Fatal(err) + } }) t.Run("calls parse", func(t *testing.T) { @@ -264,7 +274,7 @@ func TestRequest(t *testing.T) { Body: io.NopCloser(strings.NewReader(`[{"id":"0"}]`)), Header: http.Header{"Link": []string{`; rel="next"`}}, } - client := mockClient([]mock.Call{{Response: response}}) + client, api := mockClient([]mock.Call{{Response: response}}) notifications, next, err := client.request(verb, endpoint, nil) if err != nil { @@ -282,6 +292,10 @@ func TestRequest(t *testing.T) { if notifications[0].Id != "0" { t.Errorf("expected notification id 0, got %s", notifications[0].Id) } + + if err := api.Done(); err != nil { + t.Fatal(err) + } }) } @@ -338,7 +352,7 @@ func TestRetry(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - client := mockClient(test.calls) + client, api := mockClient(test.calls) client.maxRetry = test.maxRetry notifications, _, err := client.retry(verb, endpoint, nil) @@ -350,6 +364,9 @@ func TestRetry(t *testing.T) { if !notificationsEqual(notifications, test.notifications) { t.Errorf("want %#v, got %#v", test.notifications, notifications) } + if err := api.Done(); err != nil { + t.Fatal(err) + } }) } } @@ -389,7 +406,6 @@ func TestPaginate(t *testing.T) { calls: []mock.Call{ {Error: retriableError}, {Error: retriableError}, - {Error: retriableError}, }, error: retryError, }, @@ -416,7 +432,6 @@ func TestPaginate(t *testing.T) { maxPage: 1, calls: []mock.Call{ {Response: mockNotificationsResponse(t, []int{0}, true)}, - {Error: sampleError}, }, notifications: mockNotifications([]int{0}), }, @@ -444,7 +459,6 @@ func TestPaginate(t *testing.T) { calls: []mock.Call{ {Response: mockNotificationsResponse(t, []int{0}, true)}, {Response: mockNotificationsResponse(t, []int{1}, true)}, - {Response: mockNotificationsResponse(t, []int{2}, true)}, }, notifications: mockNotifications([]int{0, 1}), }, @@ -452,7 +466,7 @@ func TestPaginate(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - client := mockClient(test.calls) + client, api := mockClient(test.calls) client.maxRetry = test.maxRetry client.maxPage = test.maxPage @@ -465,6 +479,9 @@ func TestPaginate(t *testing.T) { if !notificationsEqual(notifications, test.notifications) { t.Errorf("want %#v, got %#v", test.notifications, notifications) } + if err := api.Done(); err != nil { + t.Fatal(err) + } }) } } @@ -472,7 +489,7 @@ func TestPaginate(t *testing.T) { func TestEnrich(t *testing.T) { tests := []struct { name string - calls mock.Call + calls []mock.Call notification *notifications.Notification assertError func(*testing.T, error) }{ @@ -482,17 +499,25 @@ func TestEnrich(t *testing.T) { }, { name: "one notification", - calls: mock.Call{ - Endpoint: mockSubjectUrl(0), - Response: &http.Response{ - Body: io.NopCloser(strings.NewReader(`{"author":{"login":"author"},"subject":{"title":"subject"}}`)), + calls: []mock.Call{ + { + Endpoint: mockSubjectUrl(0), + Response: &http.Response{ + Body: io.NopCloser(strings.NewReader(`{"author":{"login":"author"},"subject":{"title":"subject"}}`)), + }, + }, + { + Endpoint: mockLatestCommentUrl(0), + Response: &http.Response{ + Body: io.NopCloser(strings.NewReader(`{"author":{"login":"author"}}`)), + }, }, }, notification: mockNotification(0), }, { name: "fail to enrich", - calls: mock.Call{Error: sampleError}, + calls: []mock.Call{{Error: sampleError}}, notification: mockNotification(0), assertError: func(t *testing.T, err error) { if err != sampleError { @@ -502,10 +527,12 @@ func TestEnrich(t *testing.T) { }, { name: "fail to unmarshal", - calls: mock.Call{ - Endpoint: mockSubjectUrl(0), - Response: &http.Response{ - Body: io.NopCloser(strings.NewReader(`not json`)), + calls: []mock.Call{ + { + Endpoint: mockSubjectUrl(0), + Response: &http.Response{ + Body: io.NopCloser(strings.NewReader(`not json`)), + }, }, }, notification: mockNotification(0), @@ -524,13 +551,19 @@ func TestEnrich(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - client := mockClient([]mock.Call{test.calls}) + client, api := mockClient(test.calls) err := client.Enrich(test.notification) // TODO: make this test check for the author/subject if test.assertError != nil { test.assertError(t, err) + } else if err != nil { + t.Errorf("expected no error but got %#v", err) + } + + if err := api.Done(); err != nil { + t.Fatal(err) } }) } diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index 429f3ec..78fcfc9 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -27,7 +27,8 @@ type Notification struct { Subject Subject `json:"subject"` // Enriched API fields - Author User `json:"author"` + Author User `json:"author"` + LatestCommentor User `json:"latest_commentor"` // gh-not specific fields // Those fields are not part of the GitHub API and will persist between @@ -53,9 +54,10 @@ type Meta struct { type Subject struct { // Standard API fields - Title string `json:"title"` - URL string `json:"url"` - Type string `json:"type"` + Title string `json:"title"` + URL string `json:"url"` + Type string `json:"type"` + LatestCommentUrl string `json:"latest_comment_url"` // Enriched API fields State string `json:"state"` @@ -110,6 +112,8 @@ func (n Notification) Equal(other *Notification) bool { n.Subject.HtmlUrl == other.Subject.HtmlUrl && n.Author.Login == other.Author.Login && n.Author.Type == other.Author.Type && + n.LatestCommentor.Login == other.LatestCommentor.Login && + n.LatestCommentor.Type == other.LatestCommentor.Type && n.Meta.Hidden == other.Meta.Hidden && n.Meta.Done == other.Meta.Done && n.Meta.RemoteExists == other.Meta.RemoteExists diff --git a/internal/notifications/view.go b/internal/notifications/view.go index cfc33cd..254330d 100644 --- a/internal/notifications/view.go +++ b/internal/notifications/view.go @@ -112,7 +112,11 @@ func (n Notifications) Render() error { printer.AddField(n.Repository.FullName) printer.AddField(n.Author.Login) printer.AddField(n.Subject.Title) - printer.AddField(text.RelativeTimeAgo(time.Now(), n.UpdatedAt)) + relativeTime := text.RelativeTimeAgo(time.Now(), n.UpdatedAt) + if n.LatestCommentor.Login != "" { + relativeTime += " by " + n.LatestCommentor.Login + } + printer.AddField(relativeTime) printer.EndRow() }