Skip to content

feat(api): use conditional requests and fetch all inbox notifications #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Sep 13, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/__mocks__/state-mocks.ts
Original file line number Diff line number Diff line change
@@ -85,6 +85,7 @@ const mockAppearanceSettings = {

const mockNotificationSettings = {
groupBy: GroupBy.REPOSITORY,
fetchAllNotifications: true,
participating: false,
markAsDoneOnOpen: false,
markAsDoneOnUnsubscribe: false,
26 changes: 26 additions & 0 deletions src/components/settings/NotificationSettings.test.tsx
Original file line number Diff line number Diff line change
@@ -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(
<AppContext.Provider
value={{
auth: mockAuth,
settings: mockSettings,
updateSetting,
}}
>
<MemoryRouter>
<NotificationSettings />
</MemoryRouter>
</AppContext.Provider>,
);
});

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(
20 changes: 20 additions & 0 deletions src/components/settings/NotificationSettings.tsx
Original file line number Diff line number Diff line change
@@ -25,6 +25,26 @@ export const NotificationSettings: FC = () => {
updateSetting('groupBy', evt.target.value as GroupBy);
}}
/>
<Checkbox
name="fetchAllNotifications"
label="Fetch all notifications"
checked={settings.fetchAllNotifications}
onChange={(evt) =>
updateSetting('fetchAllNotifications', evt.target.checked)
}
tooltip={
<div>
<div className="pb-3">
When <em>checked</em>, Gitify will fetch <strong>all</strong>{' '}
notifications from your inbox.
</div>
<div>
When <em>unchecked</em>, Gitify will only fetch the first page of
notifications (max 50 records per GitHub account)
</div>
</div>
}
/>
<Checkbox
name="showOnlyParticipating"
label="Show only participating"
1 change: 1 addition & 0 deletions src/context/App.tsx
Original file line number Diff line number Diff line change
@@ -66,6 +66,7 @@ const defaultAppearanceSettings = {

const defaultNotificationSettings = {
groupBy: GroupBy.REPOSITORY,
fetchAllNotifications: true,
participating: false,
markAsDoneOnOpen: false,
markAsDoneOnUnsubscribe: false,
4 changes: 2 additions & 2 deletions src/hooks/useNotifications.test.ts
Original file line number Diff line number Diff line change
@@ -298,7 +298,7 @@ describe('hooks/useNotifications.ts', () => {
});

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);
});
});

44 changes: 44 additions & 0 deletions src/routes/__snapshots__/Settings.test.tsx.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
@@ -76,6 +76,7 @@ interface AppearanceSettingsState {

interface NotificationSettingsState {
groupBy: GroupBy;
fetchAllNotifications: boolean;
participating: boolean;
markAsDoneOnOpen: boolean;
markAsDoneOnUnsubscribe: boolean;
33 changes: 21 additions & 12 deletions src/utils/api/__snapshots__/client.test.ts.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/utils/api/__snapshots__/request.test.ts.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 29 additions & 4 deletions src/utils/api/client.test.ts
Original file line number Diff line number Diff line change
@@ -83,11 +83,32 @@ describe('utils/api/client.ts', () => {
});

describe('listNotificationsForAuthenticatedUser', () => {
const mockSettings: Partial<SettingsState> = {
participating: true,
};
it('should list notifications for user - github cloud - fetchAllNotifications true', async () => {
const mockSettings: Partial<SettingsState> = {
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<SettingsState> = {
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<SettingsState> = {
participating: true,
};

await listNotificationsForAuthenticatedUser(
mockGitHubEnterpriseServerAccount,
mockSettings as SettingsState,
9 changes: 7 additions & 2 deletions src/utils/api/client.ts
Original file line number Diff line number Diff line change
@@ -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,
);
}

/**
83 changes: 79 additions & 4 deletions src/utils/api/request.ts
Original file line number Diff line number Diff line change
@@ -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;
}
}
48 changes: 47 additions & 1 deletion src/utils/api/utils.test.ts
Original file line number Diff line number Diff line change
@@ -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: '<https://api.github.com/notifications?participating=false&page=2>; rel="next", <https://api.github.com/notifications?participating=false&page=2>; 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: '<https://api.github.com/notifications?participating=false&page=2>; 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();
});
});
});
9 changes: 9 additions & 0 deletions src/utils/api/utils.ts
Original file line number Diff line number Diff line change
@@ -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;
}