From b90814db2100e75dbb2f705e80149f616badb847 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 13 Sep 2024 08:43:19 -0400 Subject: [PATCH] feat(api): use conditional requests and fetch all inbox notifications (#1414) * feat: conditional request * feat: conditional request * feat: conditional request * feat: conditional request * rename util fn * Merge branch 'main' into feature/conditional-request Signed-off-by: Adam Setch * feat: fetch all records Signed-off-by: Adam Setch * feat: add notification setting to control fetching all Signed-off-by: Adam Setch * feat: add notification setting to control fetching all Signed-off-by: Adam Setch * Update src/components/settings/NotificationSettings.tsx Co-authored-by: Afonso Jorge Ramos * Update src/components/settings/NotificationSettings.tsx Co-authored-by: Afonso Jorge Ramos --------- Signed-off-by: Adam Setch Co-authored-by: Afonso Jorge Ramos --- src/__mocks__/state-mocks.ts | 1 + .../settings/NotificationSettings.test.tsx | 26 ++++++ .../settings/NotificationSettings.tsx | 20 +++++ src/context/App.tsx | 1 + src/hooks/useNotifications.test.ts | 4 +- .../__snapshots__/Settings.test.tsx.snap | 44 ++++++++++ src/types.ts | 1 + .../api/__snapshots__/client.test.ts.snap | 33 +++++--- .../api/__snapshots__/request.test.ts.snap | 4 +- src/utils/api/client.test.ts | 33 +++++++- src/utils/api/client.ts | 9 +- src/utils/api/request.ts | 83 ++++++++++++++++++- src/utils/api/utils.test.ts | 48 ++++++++++- src/utils/api/utils.ts | 9 ++ 14 files changed, 289 insertions(+), 27 deletions(-) diff --git a/src/__mocks__/state-mocks.ts b/src/__mocks__/state-mocks.ts index 6434f25ff..d625cf808 100644 --- a/src/__mocks__/state-mocks.ts +++ b/src/__mocks__/state-mocks.ts @@ -85,6 +85,7 @@ const mockAppearanceSettings = { const mockNotificationSettings = { groupBy: GroupBy.REPOSITORY, + fetchAllNotifications: true, participating: false, markAsDoneOnOpen: false, markAsDoneOnUnsubscribe: false, diff --git a/src/components/settings/NotificationSettings.test.tsx b/src/components/settings/NotificationSettings.test.tsx index 7b740125f..3de17e1c0 100644 --- a/src/components/settings/NotificationSettings.test.tsx +++ b/src/components/settings/NotificationSettings.test.tsx @@ -34,6 +34,32 @@ describe('routes/components/settings/NotificationSettings.tsx', () => { expect(updateSetting).toHaveBeenCalledTimes(1); expect(updateSetting).toHaveBeenCalledWith('groupBy', 'DATE'); }); + + it('should toggle the fetchAllNotifications checkbox', async () => { + await act(async () => { + render( + + + + + , + ); + }); + + fireEvent.click(screen.getByLabelText('Fetch all notifications'), { + target: { checked: true }, + }); + + expect(updateSetting).toHaveBeenCalledTimes(1); + expect(updateSetting).toHaveBeenCalledWith('fetchAllNotifications', false); + }); + it('should toggle the showOnlyParticipating checkbox', async () => { await act(async () => { render( diff --git a/src/components/settings/NotificationSettings.tsx b/src/components/settings/NotificationSettings.tsx index 329ea9994..056a85b9b 100644 --- a/src/components/settings/NotificationSettings.tsx +++ b/src/components/settings/NotificationSettings.tsx @@ -25,6 +25,26 @@ export const NotificationSettings: FC = () => { updateSetting('groupBy', evt.target.value as GroupBy); }} /> + + updateSetting('fetchAllNotifications', evt.target.checked) + } + tooltip={ +
+
+ When checked, Gitify will fetch all{' '} + notifications from your inbox. +
+
+ When unchecked, Gitify will only fetch the first page of + notifications (max 50 records per GitHub account) +
+
+ } + /> { }); expect(result.current.globalError).toBe(Errors.BAD_CREDENTIALS); - expect(logErrorSpy).toHaveBeenCalledTimes(2); + expect(logErrorSpy).toHaveBeenCalledTimes(4); }); it('should fetch notifications with different failures', async () => { @@ -341,7 +341,7 @@ describe('hooks/useNotifications.ts', () => { }); expect(result.current.globalError).toBeNull(); - expect(logErrorSpy).toHaveBeenCalledTimes(2); + expect(logErrorSpy).toHaveBeenCalledTimes(4); }); }); diff --git a/src/routes/__snapshots__/Settings.test.tsx.snap b/src/routes/__snapshots__/Settings.test.tsx.snap index 263fa1f5b..d4effd788 100644 --- a/src/routes/__snapshots__/Settings.test.tsx.snap +++ b/src/routes/__snapshots__/Settings.test.tsx.snap @@ -427,6 +427,50 @@ exports[`routes/Settings.tsx should render itself & its children 1`] = ` +
+
+ +
+ +
+
+
diff --git a/src/types.ts b/src/types.ts index 58dbe4cd9..0b96c3471 100644 --- a/src/types.ts +++ b/src/types.ts @@ -76,6 +76,7 @@ interface AppearanceSettingsState { interface NotificationSettingsState { groupBy: GroupBy; + fetchAllNotifications: boolean; participating: boolean; markAsDoneOnOpen: boolean; markAsDoneOnUnsubscribe: boolean; diff --git a/src/utils/api/__snapshots__/client.test.ts.snap b/src/utils/api/__snapshots__/client.test.ts.snap index dc98f15e5..5e586536a 100644 --- a/src/utils/api/__snapshots__/client.test.ts.snap +++ b/src/utils/api/__snapshots__/client.test.ts.snap @@ -4,7 +4,7 @@ exports[`utils/api/client.ts getAuthenticatedUser should fetch authenticated use { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -13,7 +13,7 @@ exports[`utils/api/client.ts getAuthenticatedUser should fetch authenticated use { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -22,7 +22,7 @@ exports[`utils/api/client.ts headNotifications should fetch notifications head - { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -40,12 +40,21 @@ exports[`utils/api/client.ts ignoreNotificationThreadSubscription should ignore { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; exports[`utils/api/client.ts ignoreNotificationThreadSubscription should ignore notification thread subscription - github 1`] = ` +{ + "Accept": "application/json", + "Authorization": "token token-123-456", + "Cache-Control": "", + "Content-Type": "application/json", +} +`; + +exports[`utils/api/client.ts listNotificationsForAuthenticatedUser should list notifications for user - github cloud - fetchAllNotifications false 1`] = ` { "Accept": "application/json", "Authorization": "token token-123-456", @@ -54,7 +63,7 @@ exports[`utils/api/client.ts ignoreNotificationThreadSubscription should ignore } `; -exports[`utils/api/client.ts listNotificationsForAuthenticatedUser should list notifications for user - github cloud 1`] = ` +exports[`utils/api/client.ts listNotificationsForAuthenticatedUser should list notifications for user - github cloud - fetchAllNotifications true 1`] = ` { "Accept": "application/json", "Authorization": "token token-123-456", @@ -67,7 +76,7 @@ exports[`utils/api/client.ts listNotificationsForAuthenticatedUser should list n { "Accept": "application/json", "Authorization": "token 1234568790", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -76,7 +85,7 @@ exports[`utils/api/client.ts markNotificationThreadAsDone should mark notificati { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -85,7 +94,7 @@ exports[`utils/api/client.ts markNotificationThreadAsDone should mark notificati { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -94,7 +103,7 @@ exports[`utils/api/client.ts markNotificationThreadAsRead should mark notificati { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -103,7 +112,7 @@ exports[`utils/api/client.ts markNotificationThreadAsRead should mark notificati { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -112,7 +121,7 @@ exports[`utils/api/client.ts markRepositoryNotificationsAsRead should mark repos { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -121,7 +130,7 @@ exports[`utils/api/client.ts markRepositoryNotificationsAsRead should mark repos { "Accept": "application/json", "Authorization": "token token-123-456", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; diff --git a/src/utils/api/__snapshots__/request.test.ts.snap b/src/utils/api/__snapshots__/request.test.ts.snap index bc9cdb84a..a333fa77e 100644 --- a/src/utils/api/__snapshots__/request.test.ts.snap +++ b/src/utils/api/__snapshots__/request.test.ts.snap @@ -4,7 +4,7 @@ exports[`apiRequestAuth should make an authenticated request with the correct pa { "Accept": "application/json", "Authorization": "token yourAuthToken", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; @@ -13,7 +13,7 @@ exports[`apiRequestAuth should make an authenticated request with the correct pa { "Accept": "application/json", "Authorization": "token yourAuthToken", - "Cache-Control": "no-cache", + "Cache-Control": "", "Content-Type": "application/json", } `; diff --git a/src/utils/api/client.test.ts b/src/utils/api/client.test.ts index 7f6ac0c7a..0581106cf 100644 --- a/src/utils/api/client.test.ts +++ b/src/utils/api/client.test.ts @@ -83,11 +83,32 @@ describe('utils/api/client.ts', () => { }); describe('listNotificationsForAuthenticatedUser', () => { - const mockSettings: Partial = { - participating: true, - }; + it('should list notifications for user - github cloud - fetchAllNotifications true', async () => { + const mockSettings: Partial = { + participating: true, + fetchAllNotifications: true, + }; + + await listNotificationsForAuthenticatedUser( + mockGitHubCloudAccount, + mockSettings as SettingsState, + ); + + expect(axios).toHaveBeenCalledWith({ + url: 'https://api.github.com/notifications?participating=true', + method: 'GET', + data: {}, + }); + + expect(axios.defaults.headers.common).toMatchSnapshot(); + }); + + it('should list notifications for user - github cloud - fetchAllNotifications false', async () => { + const mockSettings: Partial = { + participating: true, + fetchAllNotifications: false, + }; - it('should list notifications for user - github cloud', async () => { await listNotificationsForAuthenticatedUser( mockGitHubCloudAccount, mockSettings as SettingsState, @@ -103,6 +124,10 @@ describe('utils/api/client.ts', () => { }); it('should list notifications for user - github enterprise server', async () => { + const mockSettings: Partial = { + participating: true, + }; + await listNotificationsForAuthenticatedUser( mockGitHubEnterpriseServerAccount, mockSettings as SettingsState, diff --git a/src/utils/api/client.ts b/src/utils/api/client.ts index 595ecacfa..54115c276 100644 --- a/src/utils/api/client.ts +++ b/src/utils/api/client.ts @@ -69,8 +69,13 @@ export function listNotificationsForAuthenticatedUser( const url = getGitHubAPIBaseUrl(account.hostname); url.pathname += 'notifications'; url.searchParams.append('participating', String(settings.participating)); - - return apiRequestAuth(url.toString() as Link, 'GET', account.token); + return apiRequestAuth( + url.toString() as Link, + 'GET', + account.token, + {}, + settings.fetchAllNotifications, + ); } /** diff --git a/src/utils/api/request.ts b/src/utils/api/request.ts index 03a71b175..f469cdfcb 100644 --- a/src/utils/api/request.ts +++ b/src/utils/api/request.ts @@ -1,6 +1,20 @@ -import axios, { type AxiosPromise, type Method } from 'axios'; +import axios, { + type AxiosResponse, + type AxiosPromise, + type Method, +} from 'axios'; +import log from 'electron-log'; import type { Link, Token } from '../../types'; +import { getNextURLFromLinkHeader } from './utils'; +/** + * Perform an unauthenticated API request + * + * @param url + * @param method + * @param data + * @returns + */ export function apiRequest( url: Link, method: Method, @@ -12,15 +26,76 @@ export function apiRequest( return axios({ method, url, data }); } -export function apiRequestAuth( +/** + * Perform an authenticated API request + * + * @param url + * @param method + * @param token + * @param data + * @param fetchAllRecords whether to fetch all records or just the first page + * @returns + */ +export async function apiRequestAuth( url: Link, method: Method, token: Token, data = {}, + fetchAllRecords = false, ): AxiosPromise | null { axios.defaults.headers.common.Accept = 'application/json'; axios.defaults.headers.common.Authorization = `token ${token}`; - axios.defaults.headers.common['Cache-Control'] = 'no-cache'; axios.defaults.headers.common['Content-Type'] = 'application/json'; - return axios({ method, url, data }); + axios.defaults.headers.common['Cache-Control'] = shouldRequestWithNoCache(url) + ? 'no-cache' + : ''; + + if (!fetchAllRecords) { + return axios({ method, url, data }); + } + + let response: AxiosResponse | null = null; + let combinedData = []; + + try { + let nextUrl: string | null = url; + + while (nextUrl) { + response = await axios({ method, url: nextUrl, data }); + + // If no data is returned, break the loop + if (!response?.data) { + break; + } + + combinedData = combinedData.concat(response.data); // Accumulate data + + nextUrl = getNextURLFromLinkHeader(response); + } + } catch (error) { + log.error('API request failed:', error); + throw error; + } + + return { + ...response, + data: combinedData, + } as AxiosResponse; +} + +/** + * Return true if the request should be made with no-cache + * + * @param url + * @returns boolean + */ +function shouldRequestWithNoCache(url: string) { + const parsedUrl = new URL(url); + + switch (parsedUrl.pathname) { + case '/notifications': + return true; + default: + return false; + } } diff --git a/src/utils/api/utils.test.ts b/src/utils/api/utils.test.ts index e6d488af5..25494a18b 100644 --- a/src/utils/api/utils.test.ts +++ b/src/utils/api/utils.test.ts @@ -1,5 +1,10 @@ +import type { AxiosResponse } from 'axios'; import type { Hostname } from '../../types'; -import { getGitHubAPIBaseUrl, getGitHubGraphQLUrl } from './utils'; +import { + getGitHubAPIBaseUrl, + getGitHubGraphQLUrl, + getNextURLFromLinkHeader, +} from './utils'; describe('utils/api/utils.ts', () => { describe('getGitHubAPIBaseUrl', () => { @@ -25,4 +30,45 @@ describe('utils/api/utils.ts', () => { expect(result.toString()).toBe('https://github.gitify.io/api/graphql'); }); }); + + describe('getNextURLFromLinkHeader', () => { + it('should parse next url from link header', () => { + const mockResponse = { + headers: { + link: '; rel="next", ; rel="last"', + }, + }; + + const result = getNextURLFromLinkHeader( + mockResponse as unknown as AxiosResponse, + ); + expect(result.toString()).toBe( + 'https://api.github.com/notifications?participating=false&page=2', + ); + }); + + it('should return null if no next url in link header', () => { + const mockResponse = { + headers: { + link: '; rel="last"', + }, + }; + + const result = getNextURLFromLinkHeader( + mockResponse as unknown as AxiosResponse, + ); + expect(result).toBeNull(); + }); + + it('should return null if no link header exists', () => { + const mockResponse = { + headers: {}, + }; + + const result = getNextURLFromLinkHeader( + mockResponse as unknown as AxiosResponse, + ); + expect(result).toBeNull(); + }); + }); }); diff --git a/src/utils/api/utils.ts b/src/utils/api/utils.ts index 9ad0e966d..841ae0dc5 100644 --- a/src/utils/api/utils.ts +++ b/src/utils/api/utils.ts @@ -1,3 +1,4 @@ +import type { AxiosResponse } from 'axios'; import type { Hostname } from '../../types'; import { Constants } from '../constants'; import { isEnterpriseServerHost } from '../helpers'; @@ -22,3 +23,11 @@ export function getGitHubGraphQLUrl(hostname: Hostname): URL { return url; } + +export function getNextURLFromLinkHeader( + response: AxiosResponse, +): string | null { + const linkHeader = response.headers.link; + const matches = linkHeader?.match(/<([^>]+)>;\s*rel="next"/); + return matches ? matches[1] : null; +}