diff --git a/src/__mocks__/mock-state.ts b/src/__mocks__/mock-state.ts index 9624e91a1..01cb113a3 100644 --- a/src/__mocks__/mock-state.ts +++ b/src/__mocks__/mock-state.ts @@ -15,7 +15,7 @@ export const mockSettings: SettingsState = { showNotificationsCountInTray: false, openAtStartup: false, theme: Theme.SYSTEM, - colors: false, + detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, }; diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index 58e7fe31a..79b2449b8 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -73,9 +73,6 @@ export const NotificationRow: FC = ({ notification, hostname }) => { const reason = formatReason(notification.reason); const NotificationIcon = getNotificationTypeIcon(notification.subject); const iconColor = getNotificationTypeIconColor(notification.subject); - const realIconColor = settings - ? (settings.colors && iconColor) || '' - : iconColor; const updatedAt = formatDistanceToNow(parseISO(notification.updated_at), { addSuffix: true, }); @@ -91,7 +88,7 @@ export const NotificationRow: FC = ({ notification, hostname }) => { return (
diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx index cbb3e745c..92f387561 100644 --- a/src/context/App.test.tsx +++ b/src/context/App.test.tsx @@ -318,7 +318,7 @@ describe('context/App.tsx', () => { showNotificationsCountInTray: false, openAtStartup: false, theme: 'SYSTEM', - colors: null, + detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, }, @@ -362,7 +362,7 @@ describe('context/App.tsx', () => { showNotificationsCountInTray: false, openAtStartup: true, theme: 'SYSTEM', - colors: null, + detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, }, diff --git a/src/context/App.tsx b/src/context/App.tsx index 5023eb20e..925846084 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -41,7 +41,7 @@ export const defaultSettings: SettingsState = { showNotificationsCountInTray: false, openAtStartup: false, theme: Theme.SYSTEM, - colors: null, + detailedNotifications: false, markAsDoneOnOpen: false, showAccountHostname: false, }; @@ -90,7 +90,7 @@ export const AppProvider = ({ children }: { children: ReactNode }) => { unsubscribeNotification, markRepoNotifications, markRepoNotificationsDone, - } = useNotifications(settings.colors); + } = useNotifications(); useEffect(() => { restoreSettings(); @@ -102,7 +102,11 @@ export const AppProvider = ({ children }: { children: ReactNode }) => { useEffect(() => { fetchNotifications(accounts, settings); - }, [settings.participating, settings.showBots]); + }, [ + settings.participating, + settings.showBots, + settings.detailedNotifications, + ]); useEffect(() => { fetchNotifications(accounts, settings); diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts index 0df092ec6..2a41d9185 100644 --- a/src/hooks/useNotifications.test.ts +++ b/src/hooks/useNotifications.test.ts @@ -31,7 +31,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(200, notifications); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(mockAccounts, mockSettings); @@ -62,7 +62,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(mockAccounts, mockSettings); @@ -86,7 +86,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(mockAccounts, mockSettings); @@ -110,7 +110,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(mockAccounts, mockSettings); @@ -134,7 +134,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(mockAccounts, mockSettings); @@ -158,7 +158,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(mockAccounts, mockSettings); @@ -191,7 +191,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(200, notifications); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, mockSettings); @@ -216,7 +216,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(400, { message: 'Oops! Something went wrong.' }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, mockSettings); @@ -245,7 +245,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(200, notifications); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, mockSettings); @@ -271,7 +271,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, mockSettings); @@ -295,7 +295,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, mockSettings); @@ -319,7 +319,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, mockSettings); @@ -343,7 +343,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, mockSettings); @@ -367,7 +367,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(status, { message }); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, mockSettings); @@ -529,12 +529,12 @@ describe('hooks/useNotifications.ts', () => { user: mockedNotificationUser, }); - const { result } = renderHook(() => useNotifications(true)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, { ...mockSettings, - colors: true, + detailedNotifications: true, }); }); @@ -607,11 +607,12 @@ describe('hooks/useNotifications.ts', () => { }, }); - const { result } = renderHook(() => useNotifications(true)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(accounts, { ...mockSettings, + detailedNotifications: true, showBots: false, }); }); @@ -642,7 +643,7 @@ describe('hooks/useNotifications.ts', () => { .get('/notifications?participating=false') .reply(200, notifications); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.fetchNotifications(mockAccounts, mockSettings); @@ -675,7 +676,7 @@ describe('hooks/useNotifications.ts', () => { .patch(`/notifications/threads/${id}`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markNotificationRead(accounts, id, hostname); @@ -693,7 +694,7 @@ describe('hooks/useNotifications.ts', () => { .patch(`/notifications/threads/${id}`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markNotificationRead(accounts, id, hostname); @@ -716,7 +717,7 @@ describe('hooks/useNotifications.ts', () => { .patch(`/notifications/threads/${id}`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markNotificationRead(accounts, id, hostname); @@ -734,7 +735,7 @@ describe('hooks/useNotifications.ts', () => { .patch(`/notifications/threads/${id}`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markNotificationRead(accounts, id, hostname); @@ -761,7 +762,7 @@ describe('hooks/useNotifications.ts', () => { .delete(`/notifications/threads/${id}`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markNotificationDone(accounts, id, hostname); @@ -779,7 +780,7 @@ describe('hooks/useNotifications.ts', () => { .delete(`/notifications/threads/${id}`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markNotificationDone(accounts, id, hostname); @@ -802,7 +803,7 @@ describe('hooks/useNotifications.ts', () => { .delete(`/notifications/threads/${id}`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markNotificationDone(accounts, id, hostname); @@ -820,7 +821,7 @@ describe('hooks/useNotifications.ts', () => { .delete(`/notifications/threads/${id}`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markNotificationDone(accounts, id, hostname); @@ -853,7 +854,7 @@ describe('hooks/useNotifications.ts', () => { .patch(`/notifications/threads/${id}`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.unsubscribeNotification(accounts, id, hostname); @@ -877,7 +878,7 @@ describe('hooks/useNotifications.ts', () => { .patch(`/notifications/threads/${id}`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.unsubscribeNotification(accounts, id, hostname); @@ -906,7 +907,7 @@ describe('hooks/useNotifications.ts', () => { .patch(`/notifications/threads/${id}`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.unsubscribeNotification(accounts, id, hostname); @@ -930,7 +931,7 @@ describe('hooks/useNotifications.ts', () => { .patch(`/notifications/threads/${id}`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.unsubscribeNotification(accounts, id, hostname); @@ -957,7 +958,7 @@ describe('hooks/useNotifications.ts', () => { .put(`/repos/${repoSlug}/notifications`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markRepoNotifications(accounts, repoSlug, hostname); @@ -975,7 +976,7 @@ describe('hooks/useNotifications.ts', () => { .put(`/repos/${repoSlug}/notifications`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markRepoNotifications(accounts, repoSlug, hostname); @@ -998,7 +999,7 @@ describe('hooks/useNotifications.ts', () => { .put(`/repos/${repoSlug}/notifications`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markRepoNotifications(accounts, repoSlug, hostname); @@ -1016,7 +1017,7 @@ describe('hooks/useNotifications.ts', () => { .put(`/repos/${repoSlug}/notifications`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markRepoNotifications(accounts, repoSlug, hostname); @@ -1044,7 +1045,7 @@ describe('hooks/useNotifications.ts', () => { .delete(`/notifications/threads/${id}`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markRepoNotificationsDone( @@ -1066,7 +1067,7 @@ describe('hooks/useNotifications.ts', () => { .delete(`/notifications/threads/${id}`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markRepoNotificationsDone( @@ -1093,7 +1094,7 @@ describe('hooks/useNotifications.ts', () => { .delete(`/notifications/threads/${id}`) .reply(200); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markRepoNotificationsDone( @@ -1115,7 +1116,7 @@ describe('hooks/useNotifications.ts', () => { .delete(`/notifications/threads/${id}`) .reply(400); - const { result } = renderHook(() => useNotifications(false)); + const { result } = renderHook(() => useNotifications()); act(() => { result.current.markRepoNotificationsDone( diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index a248c75e3..f830be061 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -62,7 +62,7 @@ interface NotificationsState { errorDetails: GitifyError; } -export const useNotifications = (colors: boolean): NotificationsState => { +export const useNotifications = (): NotificationsState => { const [isFetching, setIsFetching] = useState(false); const [requestFailed, setRequestFailed] = useState(false); const [errorDetails, setErrorDetails] = useState(); @@ -118,6 +118,7 @@ export const useNotifications = (colors: boolean): NotificationsState => { }; }, ); + const data = isGitHubLoggedIn(accounts) ? [ ...enterpriseNotifications, @@ -135,17 +136,6 @@ export const useNotifications = (colors: boolean): NotificationsState => { ] : [...enterpriseNotifications]; - if (colors === false) { - setNotifications(data); - triggerNativeNotifications( - notifications, - data, - settings, - accounts, - ); - setIsFetching(false); - return; - } axios .all( data.map(async (accountNotifications) => { @@ -155,6 +145,10 @@ export const useNotifications = (colors: boolean): NotificationsState => { .all( accountNotifications.notifications.map( async (notification: Notification) => { + if (!settings.detailedNotifications) { + return notification; + } + const token = getTokenForHost( notification.hostname, accounts, diff --git a/src/routes/Settings.test.tsx b/src/routes/Settings.test.tsx index eefc923b3..afee0c474 100644 --- a/src/routes/Settings.test.tsx +++ b/src/routes/Settings.test.tsx @@ -337,7 +337,7 @@ describe('routes/Settings.tsx', () => { expect(ipcRenderer.send).toHaveBeenCalledWith('app-quit'); }); - it('should be able to enable colors', async () => { + it('should be able to enable detailed notifications', async () => { jest.spyOn(apiRequests, 'apiRequestAuth').mockResolvedValue({ headers: { 'x-oauth-scopes': Constants.AUTH_SCOPE.join(', '), @@ -360,15 +360,15 @@ describe('routes/Settings.tsx', () => { ); }); - await screen.findByLabelText('Use GitHub-like state colors'); + await screen.findByLabelText('Detailed notifications'); - fireEvent.click(screen.getByLabelText('Use GitHub-like state colors')); + fireEvent.click(screen.getByLabelText('Detailed notifications')); expect(updateSetting).toHaveBeenCalledTimes(1); - expect(updateSetting).toHaveBeenCalledWith('colors', true); + expect(updateSetting).toHaveBeenCalledWith('detailedNotifications', true); }); - it('should not be able to enable colors due to missing scope', async () => { + it('should not be able to enable detailed notifications due to missing scope', async () => { jest.spyOn(apiRequests, 'apiRequestAuth').mockResolvedValue({ headers: { 'x-oauth-scopes': 'read:user, notifications', @@ -393,29 +393,24 @@ describe('routes/Settings.tsx', () => { expect( screen - .getByLabelText('Use GitHub-like state colors (requires repo scope)') + .getByLabelText('Detailed notifications (requires repo scope)') .closest('input'), ).toHaveProperty('disabled', true); // click the checkbox fireEvent.click( - screen.getByLabelText( - 'Use GitHub-like state colors (requires repo scope)', - ), + screen.getByLabelText('Detailed notifications (requires repo scope)'), ); // check if the checkbox is still unchecked expect(updateSetting).not.toHaveBeenCalled(); expect( - screen.getByLabelText( - 'Use GitHub-like state colors (requires repo scope)', - ), + screen.getByLabelText('Detailed notifications (requires repo scope)'), ).not.toBe('checked'); expect( - screen.getByLabelText( - 'Use GitHub-like state colors (requires repo scope)', - ).parentNode.parentNode, + screen.getByLabelText('Detailed notifications (requires repo scope)') + .parentNode.parentNode, ).toMatchSnapshot(); }); diff --git a/src/routes/Settings.tsx b/src/routes/Settings.tsx index 5d3afdfa9..69fb2f3d1 100644 --- a/src/routes/Settings.tsx +++ b/src/routes/Settings.tsx @@ -36,7 +36,7 @@ export const SettingsRoute: FC = () => { const [isLinux, setIsLinux] = useState(false); const [appVersion, setAppVersion] = useState(null); - const [colorScope, setColorScope] = useState(false); + const [repoScope, setRepoScope] = useState(false); const openGitHubReleaseNotes = useCallback((version) => { openExternalLink( @@ -63,7 +63,7 @@ export const SettingsRoute: FC = () => { ); if (response.headers['x-oauth-scopes'].includes('repo')) - setColorScope(true); + setRepoScope(true); })(); }, [accounts.token]); @@ -132,15 +132,16 @@ export const SettingsRoute: FC = () => { }} /> - colorScope && updateSetting('colors', evt.target.checked) + repoScope && + updateSetting('detailedNotifications', evt.target.checked) } - disabled={!colorScope} + disabled={!repoScope} /> @@ -10,7 +10,7 @@ exports[`routes/Settings.tsx should not be able to enable colors due to missing
@@ -19,10 +19,10 @@ exports[`routes/Settings.tsx should not be able to enable colors due to missing >
@@ -162,7 +162,7 @@ exports[`routes/Settings.tsx should render itself & its children 1`] = ` > @@ -171,10 +171,10 @@ exports[`routes/Settings.tsx should render itself & its children 1`] = ` > diff --git a/src/types.ts b/src/types.ts index c53f9dc0e..36bb2e01e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -12,7 +12,7 @@ export type SettingsState = AppearanceSettingsState & interface AppearanceSettingsState { theme: Theme; - colors: boolean | null; + detailedNotifications: boolean; showAccountHostname: boolean; }