From 7f3f05e7ea9a6edba98369e8597058cc3bde40b1 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Mon, 29 Apr 2024 06:37:37 -1000 Subject: [PATCH 1/4] refactor: simplify useNotifications --- src/__mocks__/mockedData.ts | 6 +-- src/hooks/useNotifications.test.ts | 21 +++++--- src/hooks/useNotifications.ts | 49 +++++++------------ .../__snapshots__/Notifications.test.tsx.snap | 10 ++-- src/typesGitHub.ts | 3 +- 5 files changed, 40 insertions(+), 49 deletions(-) diff --git a/src/__mocks__/mockedData.ts b/src/__mocks__/mockedData.ts index 24428a97c..27ed7d6b7 100644 --- a/src/__mocks__/mockedData.ts +++ b/src/__mocks__/mockedData.ts @@ -70,7 +70,7 @@ export const mockedCommenterUser: User = { }; export const mockedSingleNotification: Notification = { - hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + hostname: Constants.GITHUB_API_BASE_URL, id: '138661096', unread: true, reason: 'subscribed', @@ -249,7 +249,7 @@ export const mockedGitHubNotifications = [ url: 'https://api.github.com/notifications/threads/148827438', subscription_url: 'https://api.github.com/notifications/threads/148827438/subscription', - hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + hostname: Constants.GITHUB_API_BASE_URL, } as Notification, ]; @@ -359,7 +359,7 @@ export const mockedEnterpriseNotifications = [ url: 'https://github.gitify.io/api/v3/notifications/threads/3', subscription_url: 'https://github.gitify.io/api/v3/notifications/threads/3/subscription', - hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + hostname: Constants.GITHUB_API_BASE_URL, } as Notification, ]; diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts index 018ea1b8b..0ac0ae070 100644 --- a/src/hooks/useNotifications.test.ts +++ b/src/hooks/useNotifications.test.ts @@ -43,10 +43,13 @@ describe('hooks/useNotifications.ts', () => { expect(result.current.status).toBe('success'); }); - expect(result.current.notifications[0].hostname).toBe( + expect(result.current.notifications[0].hostname).toBe('api.github.com'); + expect(result.current.notifications[0].notifications.length).toBe(2); + + expect(result.current.notifications[1].hostname).toBe( 'github.gitify.io', ); - expect(result.current.notifications[1].hostname).toBe('github.com'); + expect(result.current.notifications[1].notifications.length).toBe(2); }); it('should fetch notifications with failures - github.com & enterprise', async () => { @@ -117,11 +120,12 @@ describe('hooks/useNotifications.ts', () => { }); await waitFor(() => { - expect(result.current.notifications[0].hostname).toBe( - 'github.gitify.io', - ); + expect(result.current.status).toBe('success'); }); + expect(result.current.notifications[0].hostname).toBe( + 'github.gitify.io', + ); expect(result.current.notifications[0].notifications.length).toBe(2); }); @@ -179,9 +183,10 @@ describe('hooks/useNotifications.ts', () => { }); await waitFor(() => { - expect(result.current.notifications[0].hostname).toBe('github.com'); + expect(result.current.status).toBe('success'); }); + expect(result.current.notifications[0].hostname).toBe('api.github.com'); expect(result.current.notifications[0].notifications.length).toBe(2); }); @@ -379,7 +384,7 @@ describe('hooks/useNotifications.ts', () => { expect(result.current.status).toBe('success'); }); - expect(result.current.notifications[0].hostname).toBe('github.com'); + expect(result.current.notifications[0].hostname).toBe('api.github.com'); expect(result.current.notifications[0].notifications.length).toBe(6); }); }); @@ -459,7 +464,7 @@ describe('hooks/useNotifications.ts', () => { expect(result.current.status).toBe('success'); }); - expect(result.current.notifications[0].hostname).toBe('github.com'); + expect(result.current.notifications[0].hostname).toBe('api.github.com'); expect(result.current.notifications[0].notifications.length).toBe(1); }); }); diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 6e4eab856..799c90393 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -79,15 +79,13 @@ export const useNotifications = (): NotificationsState => { const fetchNotifications = useCallback( async (accounts: AuthState, settings: SettingsState) => { function getGitHubNotifications() { - if (!isGitHubLoggedIn(accounts)) { - return; + if (isGitHubLoggedIn(accounts)) { + return listNotificationsForAuthenticatedUser( + Constants.DEFAULT_AUTH_OPTIONS.hostname, + accounts.token, + settings, + ); } - - return listNotificationsForAuthenticatedUser( - Constants.DEFAULT_AUTH_OPTIONS.hostname, - accounts.token, - settings, - ); } function getEnterpriseNotifications() { @@ -106,12 +104,16 @@ export const useNotifications = (): NotificationsState => { getGitHubNotifications(), ...getEnterpriseNotifications(), ]) - .then(([gitHubNotifications, ...entAccNotifications]) => { - const enterpriseNotifications = entAccNotifications.map( - (accountNotifications) => { + .then(([...responses]) => { + // Remove any undefined responses + const accountResponses = responses.filter( + (accountResponse) => !!accountResponse, + ); + const accountsNotifications: AccountNotifications[] = + accountResponses.map((accountNotifications) => { const { hostname } = new URL(accountNotifications.config.url); return { - hostname, + hostname: hostname, notifications: accountNotifications.data.map( (notification: Notification) => { return { @@ -121,28 +123,10 @@ export const useNotifications = (): NotificationsState => { }, ), }; - }, - ); - - const data = isGitHubLoggedIn(accounts) - ? [ - ...enterpriseNotifications, - { - hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, - notifications: gitHubNotifications.data.map( - (notification: Notification) => { - return { - ...notification, - hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, - }; - }, - ), - }, - ] - : [...enterpriseNotifications]; + }); Promise.all( - data.map(async (accountNotifications) => { + accountsNotifications.map(async (accountNotifications) => { return { hostname: accountNotifications.hostname, notifications: await Promise.all( @@ -195,6 +179,7 @@ export const useNotifications = (): NotificationsState => { }); }) .catch((err: AxiosError) => { + console.error('ADAM', err); setStatus('error'); setErrorDetails(determineFailureType(err)); }); diff --git a/src/routes/__snapshots__/Notifications.test.tsx.snap b/src/routes/__snapshots__/Notifications.test.tsx.snap index 343e755d2..1b3221cef 100644 --- a/src/routes/__snapshots__/Notifications.test.tsx.snap +++ b/src/routes/__snapshots__/Notifications.test.tsx.snap @@ -99,7 +99,7 @@ exports[`routes/Notifications.tsx should render itself & its children (show acco notifications={ [ { - "hostname": "github.com", + "hostname": "https://api.github.com", "id": "138661096", "last_read_at": "2017-05-20T17:06:51Z", "reason": "subscribed", @@ -189,7 +189,7 @@ exports[`routes/Notifications.tsx should render itself & its children (show acco "url": "https://api.github.com/notifications/threads/138661096", }, { - "hostname": "github.com", + "hostname": "https://api.github.com", "id": "148827438", "last_read_at": "2017-05-20T16:59:03Z", "reason": "author", @@ -248,7 +248,7 @@ exports[`routes/Notifications.tsx should render itself & its children (with noti notifications={ [ { - "hostname": "github.com", + "hostname": "https://api.github.com", "id": "138661096", "last_read_at": "2017-05-20T17:06:51Z", "reason": "subscribed", @@ -338,7 +338,7 @@ exports[`routes/Notifications.tsx should render itself & its children (with noti "url": "https://api.github.com/notifications/threads/138661096", }, { - "hostname": "github.com", + "hostname": "https://api.github.com", "id": "148827438", "last_read_at": "2017-05-20T16:59:03Z", "reason": "author", @@ -433,7 +433,7 @@ exports[`routes/Notifications.tsx should render itself & its children (with noti "url": "https://github.gitify.io/api/v3/notifications/threads/4", }, { - "hostname": "github.com", + "hostname": "https://api.github.com", "id": "3", "last_read_at": "2017-05-20T14:20:55Z", "reason": "subscribed", diff --git a/src/typesGitHub.ts b/src/typesGitHub.ts index 164033fec..4e5327d08 100644 --- a/src/typesGitHub.ts +++ b/src/typesGitHub.ts @@ -79,7 +79,8 @@ export interface Notification { repository: Repository; url: string; subscription_url: string; - hostname: string; // This is not in the GitHub API, but we add it to the type to make it easier to work with + // TODO - rename this to apiBaseUrl + hostname: string; // This is not in the official GitHub API. We add this to make notification interactions easier. } export type UserDetails = User & UserProfile; From 4df417a184dc61ed6e26c9380db35b4283f4547a Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Mon, 29 Apr 2024 06:43:05 -1000 Subject: [PATCH 2/4] refactor: simplify useNotifications --- src/hooks/useNotifications.ts | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 799c90393..0c49cd939 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -105,22 +105,17 @@ export const useNotifications = (): NotificationsState => { ...getEnterpriseNotifications(), ]) .then(([...responses]) => { - // Remove any undefined responses - const accountResponses = responses.filter( - (accountResponse) => !!accountResponse, - ); - const accountsNotifications: AccountNotifications[] = - accountResponses.map((accountNotifications) => { + const accountsNotifications: AccountNotifications[] = responses + .filter((response) => !!response) + .map((accountNotifications) => { const { hostname } = new URL(accountNotifications.config.url); return { - hostname: hostname, + hostname, notifications: accountNotifications.data.map( - (notification: Notification) => { - return { - ...notification, - hostname: hostname, - }; - }, + (notification: Notification) => ({ + ...notification, + hostname, + }), ), }; }); From 1ea85f22ce6734fe57597bc44fd935903865a46d Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Mon, 29 Apr 2024 07:26:48 -1000 Subject: [PATCH 3/4] refactor: simplify useNotifications --- src/hooks/useNotifications.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 0c49cd939..8d32d11ba 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -174,7 +174,6 @@ export const useNotifications = (): NotificationsState => { }); }) .catch((err: AxiosError) => { - console.error('ADAM', err); setStatus('error'); setErrorDetails(determineFailureType(err)); }); From 743bbe99926080448057c2650baa45880174c4de Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Mon, 29 Apr 2024 17:16:24 -1000 Subject: [PATCH 4/4] refactor: simplify useNotifications --- src/hooks/useNotifications.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 8d32d11ba..80e13dc6a 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -79,13 +79,15 @@ export const useNotifications = (): NotificationsState => { const fetchNotifications = useCallback( async (accounts: AuthState, settings: SettingsState) => { function getGitHubNotifications() { - if (isGitHubLoggedIn(accounts)) { - return listNotificationsForAuthenticatedUser( - Constants.DEFAULT_AUTH_OPTIONS.hostname, - accounts.token, - settings, - ); + if (!isGitHubLoggedIn(accounts)) { + return; } + + return listNotificationsForAuthenticatedUser( + Constants.DEFAULT_AUTH_OPTIONS.hostname, + accounts.token, + settings, + ); } function getEnterpriseNotifications() {