Repository: gitify-app/gitify
Pull request #1073 (2024-04-30 18:12:38)

refactor: extract notification retrieval logic
`useNotifications` was becoming quite bloated.  

This PR aims to assist by moving some of the logic into `utils/notifications`, split into three main logical parts

1. fetch all notifications (raw notifications)
2. enrich notifications
3. filter notifications


Patch:
diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts
index e0c12a6c..9558e444 100644
--- a/src/hooks/useNotifications.ts
+++ b/src/hooks/useNotifications.ts
@@ -1,4 +1,3 @@
-import type { AxiosError } from 'axios';
 import { useCallback, useState } from 'react';
 
 import type {
@@ -8,24 +7,21 @@ import type {
   SettingsState,
   Status,
 } from '../types';
-import type { GitHubRESTError, Notification } from '../typesGitHub';
 import {
   ignoreNotificationThreadSubscription,
-  listNotificationsForAuthenticatedUser,
   markNotificationThreadAsDone,
   markNotificationThreadAsRead,
   markRepositoryNotificationsAsRead,
 } from '../utils/api/client';
 import { determineFailureType } from '../utils/api/errors';
-import Constants from '../utils/constants';
-import { getTokenForHost, isGitHubLoggedIn } from '../utils/helpers';
+import { getTokenForHost } from '../utils/helpers';
 import {
+  getAllNotifications,
   setTrayIconColor,
   triggerNativeNotifications,
 } from '../utils/notifications';
 import { removeNotification } from '../utils/remove-notification';
 import { removeNotifications } from '../utils/remove-notifications';
-import { getGitifySubjectDetails } from '../utils/subject';
 
 interface NotificationsState {
   notifications: AccountNotifications[];
@@ -73,107 +69,26 @@ export const useNotifications = (): NotificationsState => {
 
   const fetchNotifications = useCallback(
     async (accounts: AuthState, settings: SettingsState) => {
-      function getGitHubNotifications() {
-        if (!isGitHubLoggedIn(accounts)) {
-          return;
-        }
+      setStatus('loading');
 
-        return listNotificationsForAuthenticatedUser(
-          Constants.DEFAULT_AUTH_OPTIONS.hostname,
-          accounts.token,
+      try {
+        const fetchedNotifications = await getAllNotifications(
+          accounts,
           settings,
         );
-      }
 
-      function getEnterpriseNotifications() {
-        return accounts.enterpriseAccounts.map((account) => {
-          return listNotificationsForAuthenticatedUser(
-            account.hostname,
-            account.token,
-            settings,
-          );
-        });
+        setNotifications(fetchedNotifications);
+        triggerNativeNotifications(
+          notifications,
+          fetchedNotifications,
+          settings,
+          accounts,
+        );
+        setStatus('success');
+      } catch (err) {
+        setStatus('error');
+        setErrorDetails(determineFailureType(err));
       }
-
-      setStatus('loading');
-
-      return Promise.all([
-        getGitHubNotifications(),
-        ...getEnterpriseNotifications(),
-      ])
-        .then(([...responses]) => {
-          const accountsNotifications: AccountNotifications[] = responses
-            .filter((response) => !!response)
-            .map((accountNotifications) => {
-              const { hostname } = new URL(accountNotifications.config.url);
-              return {
-                hostname,
-                notifications: accountNotifications.data.map(
-                  (notification: Notification) => ({
-                    ...notification,
-                    hostname,
-                  }),
-                ),
-              };
-            });
-
-          Promise.all(
-            accountsNotifications.map(async (accountNotifications) => {
-              return {
-                hostname: accountNotifications.hostname,
-                notifications: await Promise.all<Notification>(
-                  accountNotifications.notifications.map(
-                    async (notification: Notification) => {
-                      if (!settings.detailedNotifications) {
-                        return notification;
-                      }
-
-                      const token = getTokenForHost(
-                        notification.hostname,
-                        accounts,
-                      );
-
-                      const additionalSubjectDetails =
-                        await getGitifySubjectDetails(notification, token);
-
-                      return {
-                        ...notification,
-                        subject: {
-                          ...notification.subject,
-                          ...additionalSubjectDetails,
-                        },
-                      };
-                    },
-                  ),
-                ).then((notifications) => {
-                  return notifications.filter((notification) => {
-                    if (
-                      !settings.showBots &&
-                      notification.subject?.user?.type === 'Bot'
-                    ) {
-                      return false;
-                    }
-
-                    return true;
-                  });
-                }),
-              };
-            }),
-          ).then((parsedNotifications) => {
-            setNotifications(parsedNotifications);
-            triggerNativeNotifications(
-              notifications,
-              parsedNotifications,
-              settings,
-              accounts,
-            );
-            setStatus('success');
-          });
-        })
-        .catch((err: AxiosError<GitHubRESTError>) => {
-          setStatus('error');
-          setErrorDetails(determineFailureType(err));
-        });
     },
     [notifications],
   );
diff --git a/src/utils/notifications.ts b/src/utils/notifications.ts
index 589f8198..142ec359 100644
--- a/src/utils/notifications.ts
+++ b/src/utils/notifications.ts
@@ -1,9 +1,16 @@
 import { ipcRenderer } from 'electron';
 import { Notification } from '../typesGitHub';
-import { openInBrowser } from '../utils/helpers';
+import {
+  getTokenForHost,
+  isGitHubLoggedIn,
+  openInBrowser,
+} from '../utils/helpers';
 import { updateTrayIcon } from './comms';
 
 import type { AccountNotifications, AuthState, SettingsState } from '../types';
+import { listNotificationsForAuthenticatedUser } from './api/client';
+import Constants from './constants';
+import { getGitifySubjectDetails } from './subject';
 
 export const setTrayIconColor = (notifications: AccountNotifications[]) => {
   const allNotificationsCount = getNotificationCount(notifications);
@@ -101,3 +108,111 @@ export const raiseSoundNotification = () => {
   audio.volume = 0.2;
   audio.play();
 };
+
+function getGitHubNotifications(accounts: AuthState, settings: SettingsState) {
+  if (!isGitHubLoggedIn(accounts)) {
+    return;
+  }
+
+  return listNotificationsForAuthenticatedUser(
+    Constants.DEFAULT_AUTH_OPTIONS.hostname,
+    accounts.token,
+    settings,
+  );
+}
+
+function getEnterpriseNotifications(
+  accounts: AuthState,
+  settings: SettingsState,
+) {
+  return accounts.enterpriseAccounts.map((account) => {
+    return listNotificationsForAuthenticatedUser(
+      account.hostname,
+      account.token,
+      settings,
+    );
+  });
+}
+
+export async function getAllNotifications(
+  accounts: AuthState,
+  settings: SettingsState,
+): Promise<AccountNotifications[]> {
+  const responses = await Promise.all([
+    getGitHubNotifications(accounts, settings),
+    ...getEnterpriseNotifications(accounts, settings),
+  ]);
+
+  const notifications: AccountNotifications[] = await Promise.all(
+    responses
+      .filter((response) => !!response)
+      .map(async (accountNotifications) => {
+        const { hostname } = new URL(accountNotifications.config.url);
+
+        let notifications = accountNotifications.data.map(
+          (notification: Notification) => ({
+            ...notification,
+            hostname,
+          }),
+        );
+
+        notifications = await enrichNotifications(
+          notifications,
+          accounts,
+          settings,
+        );
+
+        notifications = filterNotifications(notifications, settings);
+
+        return {
+          hostname,
+          notifications: notifications,
+        };
+      }),
+  );
+
+  return notifications;
+}
+
+export async function enrichNotifications(
+  notifications: Notification[],
+  accounts: AuthState,
+  settings: SettingsState,
+): Promise<Notification[]> {
+  if (!settings.detailedNotifications) {
+    return notifications;
+  }
+
+  const enrichedNotifications = await Promise.all(
+    notifications.map(async (notification: Notification) => {
+      const token = getTokenForHost(notification.hostname, accounts);
+
+      const additionalSubjectDetails = await getGitifySubjectDetails(
+        notification,
+        token,
+      );
+
+      return {
+        ...notification,
+        subject: {
+          ...notification.subject,
+          ...additionalSubjectDetails,
+        },
+      };
+    }),
+  );
+
+  return enrichedNotifications;
+}
+
+export function filterNotifications(
+  notifications: Notification[],
+  settings: SettingsState,
+): Notification[] {
+  return notifications.filter((notification) => {
+    if (!settings.showBots && notification.subject?.user?.type === 'Bot') {
+      return false;
+    }
+    return true;
+  });
+}


Test patch:
diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts
index 722217d8..146fa85d 100644
--- a/src/hooks/useNotifications.test.ts
+++ b/src/hooks/useNotifications.test.ts
@@ -398,86 +398,6 @@ describe('hooks/useNotifications.ts', () => {
         expect(result.current.notifications[0].notifications.length).toBe(6);
       });
     });
-
-    describe('showBots', () => {
-      it('should hide bot notifications when set to false', async () => {
-        const accounts: AuthState = {
-          ...mockAccounts,
-          enterpriseAccounts: [],
-          user: mockedUser,
-        };
-
-        const notifications = [
-          {
-            id: 1,
-            subject: {
-              title: 'This is an Issue.',
-              type: 'Issue',
-              url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/1',
-              latest_comment_url: null,
-            },
-            repository: {
-              full_name: 'gitify-app/notifications-test',
-            },
-          },
-          {
-            id: 2,
-            subject: {
-              title: 'This is a Pull Request.',
-              type: 'PullRequest',
-              url: 'https://api.github.com/repos/gitify-app/notifications-test/pulls/2',
-              latest_comment_url: null,
-            },
-            repository: {
-              full_name: 'gitify-app/notifications-test',
-            },
-          },
-        ];
-
-        nock('https://api.github.com')
-          .get('/notifications?participating=false')
-          .reply(200, notifications);
-        nock('https://api.github.com')
-          .get('/repos/gitify-app/notifications-test/issues/1')
-          .reply(200, {
-            state: 'closed',
-            merged: true,
-            user: {
-              login: 'some-user',
-              type: 'User',
-            },
-          });
-        nock('https://api.github.com')
-          .get('/repos/gitify-app/notifications-test/pulls/2')
-          .reply(200, {
-            state: 'closed',
-            merged: false,
-            user: {
-              login: 'some-bot',
-              type: 'Bot',
-            },
-          });
-
-        const { result } = renderHook(() => useNotifications());
-
-        act(() => {
-          result.current.fetchNotifications(accounts, {
-            ...mockSettings,
-            detailedNotifications: true,
-            showBots: false,
-          });
-        });
-
-        expect(result.current.status).toBe('loading');
-
-        await waitFor(() => {
-          expect(result.current.status).toBe('success');
-        });
-
-        expect(result.current.notifications[0].hostname).toBe('api.github.com');
-        expect(result.current.notifications[0].notifications.length).toBe(1);
-      });
-    });
   });
 
   describe('removeNotificationFromState', () => {
diff --git a/src/utils/notifications.test.ts b/src/utils/notifications.test.ts
index d6c408db..b679131b 100644
--- a/src/utils/notifications.test.ts
+++ b/src/utils/notifications.test.ts
@@ -1,15 +1,17 @@
 import { ipcRenderer } from 'electron';
 
-import { mockAccounts } from '../__mocks__/mock-state';
+import { mockAccounts, mockSettings } from '../__mocks__/mock-state';
 import {
   mockedAccountNotifications,
   mockedGitHubNotifications,
   mockedSingleAccountNotifications,
 } from '../__mocks__/mockedData';
+import { partialMockNotification } from '../__mocks__/partial-mocks';
 import { defaultSettings } from '../context/App';
 import type { SettingsState } from '../types';
 import * as helpers from './helpers';
 import * as notificationsHelpers from './notifications';
+import { filterNotifications } from './notifications';
 
 describe('utils/notifications.ts', () => {
   afterEach(() => {
@@ -137,4 +139,49 @@ describe('utils/notifications.ts', () => {
     notificationsHelpers.raiseSoundNotification();
     expect(window.Audio.prototype.play).toHaveBeenCalledTimes(1);
   });
+
+  describe('filterNotifications', () => {
+    const mockNotifications = [
+      partialMockNotification({
+        title: 'User authored notification',
+        user: {
+          login: 'user',
+          html_url: 'https://github.com/user',
+          avatar_url:
+            'https://avatars.githubusercontent.com/u/133795385?s=200&v=4',
+          type: 'User',
+        },
+      }),
+      partialMockNotification({
+        title: 'Bot authored notification',
+        user: {
+          login: 'bot',
+          html_url: 'https://github.com/bot',
+          avatar_url:
+            'https://avatars.githubusercontent.com/u/133795385?s=200&v=4',
+          type: 'Bot',
+        },
+      }),
+    ];
+
+    it('should hide bot notifications when set to false', async () => {
+      const result = filterNotifications(mockNotifications, {
+        ...mockSettings,
+        showBots: false,
+      });
+
+      expect(result.length).toBe(1);
+      expect(result).toEqual([mockNotifications[0]]);
+    });
+
+    it('should show bot notifications when set to true', async () => {
+      const result = filterNotifications(mockNotifications, {
+        ...mockSettings,
+        showBots: true,
+      });
+
+      expect(result.length).toBe(2);
+      expect(result).toEqual(mockNotifications);
+    });
+  });
 });