From f50f615b91c965fd4f2fee29624bacaa745d510e Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Tue, 19 Mar 2024 14:29:25 -0400 Subject: [PATCH 1/4] fix: improve error handling --- .../__snapshots__/api-requests.test.ts.snap | 12 +++++----- src/utils/api-requests.test.ts | 24 ++++++++++++++++++- src/utils/api-requests.ts | 12 ++++++++-- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/utils/__snapshots__/api-requests.test.ts.snap b/src/utils/__snapshots__/api-requests.test.ts.snap index 86a0ee8e5..420db2eb8 100644 --- a/src/utils/__snapshots__/api-requests.test.ts.snap +++ b/src/utils/__snapshots__/api-requests.test.ts.snap @@ -1,34 +1,34 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`apiRequest should make a request with the correct parameters 1`] = ` +exports[`apiRequestAuth should make an authenticated request with the correct parameters 1`] = ` { "Accept": "application/json", + "Authorization": "token yourAuthToken", "Cache-Control": "no-cache", "Content-Type": "application/json", } `; -exports[`apiRequest should make a request with the correct parameters and default data 1`] = ` +exports[`apiRequestAuth should make an authenticated request with the correct parameters and default data 1`] = ` { "Accept": "application/json", + "Authorization": "token yourAuthToken", "Cache-Control": "no-cache", "Content-Type": "application/json", } `; -exports[`apiRequestAuth should make an authenticated request with the correct parameters 1`] = ` +exports[`utils/api-requests.ts should make a request with the correct parameters 1`] = ` { "Accept": "application/json", - "Authorization": "token yourAuthToken", "Cache-Control": "no-cache", "Content-Type": "application/json", } `; -exports[`apiRequestAuth should make an authenticated request with the correct parameters and default data 1`] = ` +exports[`utils/api-requests.ts should make a request with the correct parameters and default data 1`] = ` { "Accept": "application/json", - "Authorization": "token yourAuthToken", "Cache-Control": "no-cache", "Content-Type": "application/json", } diff --git a/src/utils/api-requests.test.ts b/src/utils/api-requests.test.ts index 9bb716488..41cfd3e7e 100644 --- a/src/utils/api-requests.test.ts +++ b/src/utils/api-requests.test.ts @@ -6,7 +6,18 @@ jest.mock('axios'); const url = 'https://example.com'; const method = 'get'; -describe('apiRequest', () => { +describe('utils/api-requests.ts', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it('handles null parameters', async () => { + const response = await apiRequest(null, null, null); + + expect(axios).toHaveBeenCalledTimes(0); + expect(response).toBe; + }); + it('should make a request with the correct parameters', async () => { const data = { key: 'value' }; @@ -38,6 +49,17 @@ describe('apiRequest', () => { describe('apiRequestAuth', () => { const token = 'yourAuthToken'; + afterEach(() => { + jest.resetAllMocks(); + }); + + it('handles null parameters', async () => { + const response = await apiRequestAuth(null, null, null, null); + + expect(axios).toHaveBeenCalledTimes(0); + expect(response).toBe; + }); + it('should make an authenticated request with the correct parameters', async () => { const data = { key: 'value' }; diff --git a/src/utils/api-requests.ts b/src/utils/api-requests.ts index 8938d52b7..e5f93d3e2 100644 --- a/src/utils/api-requests.ts +++ b/src/utils/api-requests.ts @@ -4,7 +4,11 @@ export function apiRequest( url: string, method: Method, data = {}, -): AxiosPromise { +): AxiosPromise | null { + if (!url && !method) { + return null; + } + axios.defaults.headers.common['Accept'] = 'application/json'; axios.defaults.headers.common['Content-Type'] = 'application/json'; axios.defaults.headers.common['Cache-Control'] = 'no-cache'; @@ -16,7 +20,11 @@ export function apiRequestAuth( method: Method, token: string, data = {}, -): AxiosPromise { +): AxiosPromise | null { + if (!url && !method && !token) { + return null; + } + axios.defaults.headers.common['Accept'] = 'application/json'; axios.defaults.headers.common['Authorization'] = `token ${token}`; axios.defaults.headers.common['Cache-Control'] = 'no-cache'; From 7399f0541e9a2723727fbd4ea294d97cf33276ab Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Tue, 19 Mar 2024 15:14:30 -0400 Subject: [PATCH 2/4] fix: improve error handling --- src/utils/helpers.test.ts | 28 ++ src/utils/helpers.ts | 11 +- src/utils/subject.test.ts | 884 ++++++++++++++++++++------------------ src/utils/subject.ts | 28 +- 4 files changed, 513 insertions(+), 438 deletions(-) diff --git a/src/utils/helpers.test.ts b/src/utils/helpers.test.ts index bea3101cb..dfe3f6e77 100644 --- a/src/utils/helpers.test.ts +++ b/src/utils/helpers.test.ts @@ -445,6 +445,34 @@ describe('utils/helpers.ts', () => { `https://github.com/manosim/notifications-test/discussions/612?${mockedNotificationReferrer}#discussioncomment-2300902`, ); }); + + it('default to base discussions url when graphql query fails', async () => { + const subject = { + title: '1.16.0', + url: null, + latest_comment_url: null, + type: 'Discussion' as SubjectType, + }; + + const requestPromise = new Promise((resolve) => + resolve(null as AxiosResponse), + ) as AxiosPromise; + + apiRequestAuthMock.mockResolvedValue(requestPromise); + + const result = await generateGitHubWebUrl( + { + ...mockedSingleNotification, + subject: subject, + }, + mockAccounts, + ); + + expect(apiRequestAuthMock).toHaveBeenCalledTimes(1); + expect(result).toBe( + `https://github.com/manosim/notifications-test/discussions?${mockedNotificationReferrer}`, + ); + }); }); it('Repository Invitation url', async () => { diff --git a/src/utils/helpers.ts b/src/utils/helpers.ts index 6da0b2525..016605e88 100644 --- a/src/utils/helpers.ts +++ b/src/utils/helpers.ts @@ -136,8 +136,9 @@ async function getDiscussionUrl( let latestCommentId: string | number; - if (comments?.length) { - latestCommentId = getLatestDiscussionComment(comments)?.databaseId; + latestCommentId = getLatestDiscussionComment(comments)?.databaseId; + + if (latestCommentId) { url += `#discussioncomment-${latestCommentId}`; } } @@ -218,9 +219,13 @@ export async function fetchDiscussion( export function getLatestDiscussionComment( comments: DiscussionCommentNode[], ): DiscussionSubcommentNode | null { + if (!comments || comments.length == 0) { + return null; + } + return comments .flatMap((comment) => comment.replies.nodes) - .concat([comments.at(-1)]) + .concat([comments[comments.length - 1]]) .reduce((a, b) => (a.createdAt > b.createdAt ? a : b)); } diff --git a/src/utils/subject.test.ts b/src/utils/subject.test.ts index b4c659afb..f63305c83 100644 --- a/src/utils/subject.test.ts +++ b/src/utils/subject.test.ts @@ -134,461 +134,503 @@ describe('utils/state.ts', () => { }); }); - describe('getGitifySubjectForDiscussion', () => { - it('answered discussion state', async () => { - const mockNotification = { - ...mockedSingleNotification, - subject: { - ...mockedSingleNotification.subject, - title: 'This is an answered discussion', - type: 'Discussion' as SubjectType, - }, - }; - - nock('https://api.github.com') - .post('/graphql') - .reply(200, { - data: { - search: { - nodes: [ - { - title: 'This is an answered discussion', - viewerSubscription: 'SUBSCRIBED', - stateReason: null, - isAnswered: true, - comments: { - nodes: [], //TODO - Update this to have real data + describe('getGitifySubjectDetails', () => { + describe('Discussions', () => { + it('answered discussion state', async () => { + const mockNotification = { + ...mockedSingleNotification, + subject: { + ...mockedSingleNotification.subject, + title: 'This is an answered discussion', + type: 'Discussion' as SubjectType, + }, + }; + + nock('https://api.github.com') + .post('/graphql') + .reply(200, { + data: { + search: { + nodes: [ + { + title: 'This is an answered discussion', + viewerSubscription: 'SUBSCRIBED', + stateReason: null, + isAnswered: true, + comments: { + nodes: [], //TODO - Update this to have real data + }, }, - }, - ], + ], + }, }, + }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('ANSWERED'); + expect(result.user).toBe(null); + }); + + it('duplicate discussion state', async () => { + const mockNotification = { + ...mockedSingleNotification, + subject: { + ...mockedSingleNotification.subject, + title: 'This is a duplicate discussion', + type: 'Discussion' as SubjectType, }, - }); - - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('ANSWERED'); - expect(result.user).toBe(null); - }); - - it('duplicate discussion state', async () => { - const mockNotification = { - ...mockedSingleNotification, - subject: { - ...mockedSingleNotification.subject, - title: 'This is a duplicate discussion', - type: 'Discussion' as SubjectType, - }, - }; - - nock('https://api.github.com') - .post('/graphql') - .reply(200, { - data: { - search: { - nodes: [ - { - title: 'This is a duplicate discussion', - viewerSubscription: 'SUBSCRIBED', - stateReason: 'DUPLICATE', - isAnswered: false, - comments: { - nodes: [], //TODO - Update this to have real data + }; + + nock('https://api.github.com') + .post('/graphql') + .reply(200, { + data: { + search: { + nodes: [ + { + title: 'This is a duplicate discussion', + viewerSubscription: 'SUBSCRIBED', + stateReason: 'DUPLICATE', + isAnswered: false, + comments: { + nodes: [], //TODO - Update this to have real data + }, }, - }, - ], + ], + }, }, + }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('DUPLICATE'); + expect(result.user).toBe(null); + }); + + it('open discussion state', async () => { + const mockNotification = { + ...mockedSingleNotification, + subject: { + ...mockedSingleNotification.subject, + title: 'This is an open discussion', + type: 'Discussion' as SubjectType, }, - }); - - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('DUPLICATE'); - expect(result.user).toBe(null); - }); - - it('open discussion state', async () => { - const mockNotification = { - ...mockedSingleNotification, - subject: { - ...mockedSingleNotification.subject, - title: 'This is an open discussion', - type: 'Discussion' as SubjectType, - }, - }; - - nock('https://api.github.com') - .post('/graphql') - .reply(200, { - data: { - search: { - nodes: [ - { - title: 'This is an open discussion', - viewerSubscription: 'SUBSCRIBED', - stateReason: null, - isAnswered: false, - comments: { - nodes: [], //TODO - Update this to have real data + }; + + nock('https://api.github.com') + .post('/graphql') + .reply(200, { + data: { + search: { + nodes: [ + { + title: 'This is an open discussion', + viewerSubscription: 'SUBSCRIBED', + stateReason: null, + isAnswered: false, + comments: { + nodes: [], //TODO - Update this to have real data + }, }, - }, - ], + ], + }, }, + }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('OPEN'); + expect(result.user).toBe(null); + }); + + it('outdated discussion state', async () => { + const mockNotification = { + ...mockedSingleNotification, + subject: { + ...mockedSingleNotification.subject, + title: 'This is an outdated discussion', + type: 'Discussion' as SubjectType, }, - }); - - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('OPEN'); - expect(result.user).toBe(null); - }); - - it('outdated discussion state', async () => { - const mockNotification = { - ...mockedSingleNotification, - subject: { - ...mockedSingleNotification.subject, - title: 'This is an outdated discussion', - type: 'Discussion' as SubjectType, - }, - }; - - nock('https://api.github.com') - .post('/graphql') - .reply(200, { - data: { - search: { - nodes: [ - { - title: 'This is an outdated discussion', - viewerSubscription: 'SUBSCRIBED', - stateReason: 'OUTDATED', - isAnswered: false, - comments: { - nodes: [], //TODO - Update this to have real data + }; + + nock('https://api.github.com') + .post('/graphql') + .reply(200, { + data: { + search: { + nodes: [ + { + title: 'This is an outdated discussion', + viewerSubscription: 'SUBSCRIBED', + stateReason: 'OUTDATED', + isAnswered: false, + comments: { + nodes: [], //TODO - Update this to have real data + }, }, - }, - ], + ], + }, }, + }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('OUTDATED'); + expect(result.user).toBe(null); + }); + + it('reopened discussion state', async () => { + const mockNotification = { + ...mockedSingleNotification, + subject: { + ...mockedSingleNotification.subject, + title: 'This is a reopened discussion', + type: 'Discussion' as SubjectType, }, - }); - - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('OUTDATED'); - expect(result.user).toBe(null); - }); - - it('reopened discussion state', async () => { - const mockNotification = { - ...mockedSingleNotification, - subject: { - ...mockedSingleNotification.subject, - title: 'This is a reopened discussion', - type: 'Discussion' as SubjectType, - }, - }; - - nock('https://api.github.com') - .post('/graphql') - .reply(200, { - data: { - search: { - nodes: [ - { - title: 'This is a reopened discussion', - viewerSubscription: 'SUBSCRIBED', - stateReason: 'REOPENED', - isAnswered: false, - comments: { - nodes: [], //TODO - Update this to have real data + }; + + nock('https://api.github.com') + .post('/graphql') + .reply(200, { + data: { + search: { + nodes: [ + { + title: 'This is a reopened discussion', + viewerSubscription: 'SUBSCRIBED', + stateReason: 'REOPENED', + isAnswered: false, + comments: { + nodes: [], //TODO - Update this to have real data + }, + }, + ], + }, + }, + }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('REOPENED'); + expect(result.user).toBe(null); + }); + + it('resolved discussion state', async () => { + const mockNotification = { + ...mockedSingleNotification, + subject: { + ...mockedSingleNotification.subject, + title: 'This is a resolved discussion', + type: 'Discussion' as SubjectType, + }, + }; + + nock('https://api.github.com') + .post('/graphql') + .reply(200, { + data: { + search: { + nodes: [ + { + title: 'This is a resolved discussion', + viewerSubscription: 'SUBSCRIBED', + stateReason: 'RESOLVED', + isAnswered: false, + comments: { + nodes: [], //TODO - Update this to have real data + }, }, - }, - ], + ], + }, }, + }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('RESOLVED'); + expect(result.user).toBe(null); + }); + + it('filtered response by subscribed', async () => { + const mockNotification = { + ...mockedSingleNotification, + subject: { + ...mockedSingleNotification.subject, + title: 'This is a discussion', + type: 'Discussion' as SubjectType, }, - }); + }; + + nock('https://api.github.com') + .post('/graphql') + .reply(200, { + data: { + search: { + nodes: [ + { + title: 'This is a discussion', + viewerSubscription: 'SUBSCRIBED', + stateReason: null, + isAnswered: false, + comments: { + nodes: [], //TODO - Update this to have real data + }, + }, + { + title: 'This is a discussion', + viewerSubscription: 'IGNORED', + stateReason: null, + isAnswered: true, + comments: { + nodes: [], //TODO - Update this to have real data + }, + }, + ], + }, + }, + }); - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); - expect(result.state).toBe('REOPENED'); - expect(result.user).toBe(null); + expect(result.state).toBe('OPEN'); + expect(result.user).toBe(null); + }); }); - it('resolved discussion state', async () => { - const mockNotification = { - ...mockedSingleNotification, - subject: { - ...mockedSingleNotification.subject, - title: 'This is a resolved discussion', - type: 'Discussion' as SubjectType, - }, - }; - - nock('https://api.github.com') - .post('/graphql') - .reply(200, { - data: { - search: { - nodes: [ - { - title: 'This is a resolved discussion', - viewerSubscription: 'SUBSCRIBED', - stateReason: 'RESOLVED', - isAnswered: false, - comments: { - nodes: [], //TODO - Update this to have real data - }, - }, - ], + describe('Issues', () => { + it('open issue state', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'open' }); + + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/comments/302888448') + .reply(200, { user: { login: 'some-user' } }); + + const result = await getGitifySubjectDetails( + mockedSingleNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('open'); + expect(result.user).toBe('some-user'); + }); + + it('closed issue state', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'closed' }); + + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/comments/302888448') + .reply(200, { user: { login: 'some-user' } }); + + const result = await getGitifySubjectDetails( + mockedSingleNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('closed'); + expect(result.user).toBe('some-user'); + }); + + it('completed issue state', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'closed', state_reason: 'completed' }); + + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/comments/302888448') + .reply(200, { user: { login: 'some-user' } }); + + const result = await getGitifySubjectDetails( + mockedSingleNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('completed'); + expect(result.user).toBe('some-user'); + }); + + it('not_planned issue state', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'open', state_reason: 'not_planned' }); + + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/comments/302888448') + .reply(200, { user: { login: 'some-user' } }); + + const result = await getGitifySubjectDetails( + mockedSingleNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('not_planned'); + expect(result.user).toBe('some-user'); + }); + + it('reopened issue state', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'open', state_reason: 'reopened' }); + + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/comments/302888448') + .reply(200, { user: { login: 'some-user' } }); + + const result = await getGitifySubjectDetails( + mockedSingleNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('reopened'); + expect(result.user).toBe('some-user'); + }); + + it('handle issues without latest_comment_url', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'open', draft: false, merged: false }); + + const result = await getGitifySubjectDetails( + { + ...mockedSingleNotification, + subject: { + ...mockedSingleNotification.subject, + latest_comment_url: null, }, }, - }); + mockAccounts.token, + ); - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('RESOLVED'); - expect(result.user).toBe(null); + expect(result.state).toBe('open'); + expect(result.user).toBeNull(); + }); }); - it('filtered response by subscribed', async () => { + describe('Pull Requests', () => { const mockNotification = { ...mockedSingleNotification, subject: { ...mockedSingleNotification.subject, - title: 'This is a discussion', - type: 'Discussion' as SubjectType, + type: 'PullRequest' as SubjectType, }, }; - nock('https://api.github.com') - .post('/graphql') - .reply(200, { - data: { - search: { - nodes: [ - { - title: 'This is a discussion', - viewerSubscription: 'SUBSCRIBED', - stateReason: null, - isAnswered: false, - comments: { - nodes: [], //TODO - Update this to have real data - }, - }, - { - title: 'This is a discussion', - viewerSubscription: 'IGNORED', - stateReason: null, - isAnswered: true, - comments: { - nodes: [], //TODO - Update this to have real data - }, - }, - ], + it('closed pull request state', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'closed', draft: false, merged: false }); + + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/comments/302888448') + .reply(200, { user: { login: 'some-user' } }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('closed'); + expect(result.user).toBe('some-user'); + }); + + it('draft pull request state', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'open', draft: true, merged: false }); + + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/comments/302888448') + .reply(200, { user: { login: 'some-user' } }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('draft'); + expect(result.user).toBe('some-user'); + }); + + it('merged pull request state', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'open', draft: false, merged: true }); + + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/comments/302888448') + .reply(200, { user: { login: 'some-user' } }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('merged'); + expect(result.user).toBe('some-user'); + }); + + it('open pull request state', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'open', draft: false, merged: false }); + + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/comments/302888448') + .reply(200, { user: { login: 'some-user' } }); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts.token, + ); + + expect(result.state).toBe('open'); + expect(result.user).toBe('some-user'); + }); + + it('handle pull request without latest_comment_url', async () => { + nock('https://api.github.com') + .get('/repos/manosim/notifications-test/issues/1') + .reply(200, { state: 'open', draft: false, merged: false }); + + const result = await getGitifySubjectDetails( + { + ...mockNotification, + subject: { + ...mockNotification.subject, + latest_comment_url: null, }, }, - }); - - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('OPEN'); - expect(result.user).toBe(null); - }); - }); - - describe('getGitifySubjectForIssue', () => { - it('open issue state', async () => { - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/1') - .reply(200, { state: 'open' }); - - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/comments/302888448') - .reply(200, { user: { login: 'some-user' } }); - - const result = await getGitifySubjectDetails( - mockedSingleNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('open'); - expect(result.user).toBe('some-user'); - }); - - it('closed issue state', async () => { - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/1') - .reply(200, { state: 'closed' }); - - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/comments/302888448') - .reply(200, { user: { login: 'some-user' } }); - - const result = await getGitifySubjectDetails( - mockedSingleNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('closed'); - expect(result.user).toBe('some-user'); - }); - - it('completed issue state', async () => { - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/1') - .reply(200, { state: 'closed', state_reason: 'completed' }); - - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/comments/302888448') - .reply(200, { user: { login: 'some-user' } }); - - const result = await getGitifySubjectDetails( - mockedSingleNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('completed'); - expect(result.user).toBe('some-user'); - }); - - it('not_planned issue state', async () => { - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/1') - .reply(200, { state: 'open', state_reason: 'not_planned' }); - - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/comments/302888448') - .reply(200, { user: { login: 'some-user' } }); + mockAccounts.token, + ); - const result = await getGitifySubjectDetails( - mockedSingleNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('not_planned'); - expect(result.user).toBe('some-user'); - }); - - it('reopened issue state', async () => { - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/1') - .reply(200, { state: 'open', state_reason: 'reopened' }); - - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/comments/302888448') - .reply(200, { user: { login: 'some-user' } }); - - const result = await getGitifySubjectDetails( - mockedSingleNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('reopened'); - expect(result.user).toBe('some-user'); - }); - }); - - describe('getGitifySubjectForPullRequest', () => { - const mockNotification = { - ...mockedSingleNotification, - subject: { - ...mockedSingleNotification.subject, - type: 'PullRequest' as SubjectType, - }, - }; - - it('closed pull request state', async () => { - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/1') - .reply(200, { state: 'closed', draft: false, merged: false }); - - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/comments/302888448') - .reply(200, { user: { login: 'some-user' } }); - - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('closed'); - expect(result.user).toBe('some-user'); - }); - - it('draft pull request state', async () => { - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/1') - .reply(200, { state: 'open', draft: true, merged: false }); - - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/comments/302888448') - .reply(200, { user: { login: 'some-user' } }); - - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('draft'); - expect(result.user).toBe('some-user'); - }); - - it('merged pull request state', async () => { - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/1') - .reply(200, { state: 'open', draft: false, merged: true }); - - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/comments/302888448') - .reply(200, { user: { login: 'some-user' } }); - - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('merged'); - expect(result.user).toBe('some-user'); - }); - - it('open pull request state', async () => { - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/1') - .reply(200, { state: 'open', draft: false, merged: false }); - - nock('https://api.github.com') - .get('/repos/manosim/notifications-test/issues/comments/302888448') - .reply(200, { user: { login: 'some-user' } }); - - const result = await getGitifySubjectDetails( - mockNotification, - mockAccounts.token, - ); - - expect(result.state).toBe('open'); - expect(result.user).toBe('some-user'); + expect(result.state).toBe('open'); + expect(result.user).toBeNull(); + }); }); }); diff --git a/src/utils/subject.ts b/src/utils/subject.ts index a1c9e7cdc..f0af419c9 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -104,16 +104,12 @@ async function getGitifySubjectForDiscussion( } } - let comments = discussion.comments.nodes; - let discussionUser = null; - - if (comments?.length) { - discussionUser = getLatestDiscussionComment(comments)?.author.login; - } + const discussionUser = getLatestDiscussionComment(discussion.comments.nodes) + ?.author.login; return { state: discussionState, - user: discussionUser, + user: discussionUser ?? null, }; } @@ -129,7 +125,7 @@ async function getGitifySubjectForIssue( return { state: issue.state_reason ?? issue.state, - user: issueCommentUser.login, + user: issueCommentUser?.login ?? null, }; } @@ -141,8 +137,6 @@ async function getGitifySubjectForPullRequest( await apiRequestAuth(notification.subject.url, 'GET', token) ).data; - const prCommentUser = await getLatestCommentUser(notification, token); - let prState: PullRequestStateType = pr.state; if (pr.merged) { prState = 'merged'; @@ -150,9 +144,11 @@ async function getGitifySubjectForPullRequest( prState = 'draft'; } + const prCommentUser = await getLatestCommentUser(notification, token); + return { state: prState, - user: prCommentUser.login, + user: prCommentUser?.login ?? null, }; } @@ -202,10 +198,14 @@ function getWorkflowRunStatus(statusDisplayName: string): CheckSuiteStatus { async function getLatestCommentUser( notification: Notification, token: string, -): Promise { +): Promise { + if (!notification.subject.latest_comment_url) { + return null; + } + const response: IssueComments = ( await apiRequestAuth(notification.subject.latest_comment_url, 'GET', token) - ).data; + )?.data; - return response.user; + return response?.user; } From 69931f1892633a48fca627f4e1b46f3d1b33a31a Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Wed, 20 Mar 2024 10:22:23 -0400 Subject: [PATCH 3/4] incorporate pr feedback --- src/utils/api-requests.test.ts | 14 -------------- src/utils/api-requests.ts | 8 -------- src/utils/subject.ts | 2 +- 3 files changed, 1 insertion(+), 23 deletions(-) diff --git a/src/utils/api-requests.test.ts b/src/utils/api-requests.test.ts index 41cfd3e7e..7d0bc6637 100644 --- a/src/utils/api-requests.test.ts +++ b/src/utils/api-requests.test.ts @@ -11,13 +11,6 @@ describe('utils/api-requests.ts', () => { jest.resetAllMocks(); }); - it('handles null parameters', async () => { - const response = await apiRequest(null, null, null); - - expect(axios).toHaveBeenCalledTimes(0); - expect(response).toBe; - }); - it('should make a request with the correct parameters', async () => { const data = { key: 'value' }; @@ -53,13 +46,6 @@ describe('apiRequestAuth', () => { jest.resetAllMocks(); }); - it('handles null parameters', async () => { - const response = await apiRequestAuth(null, null, null, null); - - expect(axios).toHaveBeenCalledTimes(0); - expect(response).toBe; - }); - it('should make an authenticated request with the correct parameters', async () => { const data = { key: 'value' }; diff --git a/src/utils/api-requests.ts b/src/utils/api-requests.ts index e5f93d3e2..47e0066e4 100644 --- a/src/utils/api-requests.ts +++ b/src/utils/api-requests.ts @@ -5,10 +5,6 @@ export function apiRequest( method: Method, data = {}, ): AxiosPromise | null { - if (!url && !method) { - return null; - } - axios.defaults.headers.common['Accept'] = 'application/json'; axios.defaults.headers.common['Content-Type'] = 'application/json'; axios.defaults.headers.common['Cache-Control'] = 'no-cache'; @@ -21,10 +17,6 @@ export function apiRequestAuth( token: string, data = {}, ): AxiosPromise | null { - if (!url && !method && !token) { - return null; - } - axios.defaults.headers.common['Accept'] = 'application/json'; axios.defaults.headers.common['Authorization'] = `token ${token}`; axios.defaults.headers.common['Cache-Control'] = 'no-cache'; diff --git a/src/utils/subject.ts b/src/utils/subject.ts index f0af419c9..cc571ee6c 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -198,7 +198,7 @@ function getWorkflowRunStatus(statusDisplayName: string): CheckSuiteStatus { async function getLatestCommentUser( notification: Notification, token: string, -): Promise { +): Promise | null { if (!notification.subject.latest_comment_url) { return null; } From c86312cbee95a554cae89223259c3b14fdb78468 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Wed, 20 Mar 2024 10:40:34 -0400 Subject: [PATCH 4/4] Merge remote-tracking branch 'origin/main' into bugfix/improve-error-handling --- .../__snapshots__/AccountNotifications.test.tsx.snap | 2 -- src/components/__snapshots__/NotificationRow.test.tsx.snap | 4 ---- src/components/__snapshots__/Repository.test.tsx.snap | 2 -- src/components/__snapshots__/Sidebar.test.tsx.snap | 2 -- src/routes/__snapshots__/LoginEnterprise.test.tsx.snap | 1 - src/routes/__snapshots__/LoginWithToken.test.tsx.snap | 1 - src/routes/__snapshots__/Settings.test.tsx.snap | 1 - 7 files changed, 13 deletions(-) diff --git a/src/components/__snapshots__/AccountNotifications.test.tsx.snap b/src/components/__snapshots__/AccountNotifications.test.tsx.snap index 1e14664e4..7bb967d17 100644 --- a/src/components/__snapshots__/AccountNotifications.test.tsx.snap +++ b/src/components/__snapshots__/AccountNotifications.test.tsx.snap @@ -12,7 +12,6 @@ exports[`components/AccountNotifications.tsx should render itself (github.com wi fill="currentColor" focusable="false" height={20} - role="img" style={ { "display": "inline-block", @@ -46,7 +45,6 @@ exports[`components/AccountNotifications.tsx should render itself (github.com wi fill="currentColor" focusable="false" height={20} - role="img" style={ { "display": "inline-block", diff --git a/src/components/__snapshots__/NotificationRow.test.tsx.snap b/src/components/__snapshots__/NotificationRow.test.tsx.snap index dfa20ba0c..b671daa41 100644 --- a/src/components/__snapshots__/NotificationRow.test.tsx.snap +++ b/src/components/__snapshots__/NotificationRow.test.tsx.snap @@ -9,7 +9,6 @@ exports[`components/NotificationRow.tsx should render itself & its children 1`] title="Open Issue" >