From 2362a2b95a46b1f950b3be0e5f88295ad1507204 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Thu, 23 May 2024 07:17:11 -0400 Subject: [PATCH 01/30] refactor: introduce new account types --- src/__mocks__/mock-state.ts | 17 +++ src/context/App.test.tsx | 11 +- src/context/App.tsx | 1 + src/routes/LoginWithOAuthApp.test.tsx | 2 + src/types.ts | 24 +++- src/utils/auth.test.ts | 1 + src/utils/storage.test.ts | 157 ++++++++++++++++++++++---- src/utils/storage.ts | 48 +++++++- 8 files changed, 233 insertions(+), 28 deletions(-) diff --git a/src/__mocks__/mock-state.ts b/src/__mocks__/mock-state.ts index 4a930d0e7..b078ef829 100644 --- a/src/__mocks__/mock-state.ts +++ b/src/__mocks__/mock-state.ts @@ -1,7 +1,24 @@ import { type AuthState, type SettingsState, Theme } from '../types'; +import Constants from '../utils/constants'; import { mockedEnterpriseAccounts, mockedUser } from './mockedData'; export const mockAccounts: AuthState = { + accounts: [ + { + platform: 'GitHub Cloud', + method: 'Personal Access Token', + token: 'token-123-456', + hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + user: mockedUser, + }, + { + platform: 'GitHub Enterprise Server', + method: 'OAuth App', + token: '1234568790', + hostname: 'github.gitify.io', + user: mockedUser, + }, + ], token: 'token-123-456', enterpriseAccounts: mockedEnterpriseAccounts, user: mockedUser, diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx index bf8c297f7..9dfbc3246 100644 --- a/src/context/App.test.tsx +++ b/src/context/App.test.tsx @@ -132,6 +132,7 @@ describe('context/App.tsx', () => { expect(markNotificationReadMock).toHaveBeenCalledTimes(1); expect(markNotificationReadMock).toHaveBeenCalledWith( { + accounts: [], enterpriseAccounts: [], token: null, user: null, @@ -164,7 +165,7 @@ describe('context/App.tsx', () => { expect(markNotificationDoneMock).toHaveBeenCalledTimes(1); expect(markNotificationDoneMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null, user: null }, + { accounts: [], enterpriseAccounts: [], token: null, user: null }, mockSettings, '123-456', 'github.com', @@ -193,7 +194,7 @@ describe('context/App.tsx', () => { expect(unsubscribeNotificationMock).toHaveBeenCalledTimes(1); expect(unsubscribeNotificationMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null, user: null }, + { accounts: [], enterpriseAccounts: [], token: null, user: null }, mockSettings, '123-456', 'github.com', @@ -227,7 +228,7 @@ describe('context/App.tsx', () => { expect(markRepoNotificationsMock).toHaveBeenCalledTimes(1); expect(markRepoNotificationsMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null, user: null }, + { accounts: [], enterpriseAccounts: [], token: null, user: null }, mockSettings, 'gitify-app/notifications-test', 'github.com', @@ -321,7 +322,7 @@ describe('context/App.tsx', () => { }); expect(saveStateMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null, user: null }, + { accounts: [], enterpriseAccounts: [], token: null, user: null }, { participating: true, playSound: true, @@ -366,7 +367,7 @@ describe('context/App.tsx', () => { expect(setAutoLaunchMock).toHaveBeenCalledWith(true); expect(saveStateMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null, user: null }, + { accounts: [], enterpriseAccounts: [], token: null, user: null }, { participating: false, playSound: true, diff --git a/src/context/App.tsx b/src/context/App.tsx index 77c32fe23..3fbc5fa0f 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -28,6 +28,7 @@ import { clearState, loadState, saveState } from '../utils/storage'; import { setTheme } from '../utils/theme'; const defaultAccounts: AuthState = { + accounts: [], token: null, enterpriseAccounts: [], user: null, diff --git a/src/routes/LoginWithOAuthApp.test.tsx b/src/routes/LoginWithOAuthApp.test.tsx index 63f7a256a..6bf5eddb6 100644 --- a/src/routes/LoginWithOAuthApp.test.tsx +++ b/src/routes/LoginWithOAuthApp.test.tsx @@ -18,6 +18,7 @@ describe('routes/LoginWithOAuthApp.tsx', () => { const openExternalMock = jest.spyOn(shell, 'openExternal'); const mockAccounts: AuthState = { + accounts: [], enterpriseAccounts: [], user: null, }; @@ -126,6 +127,7 @@ describe('routes/LoginWithOAuthApp.tsx', () => { { describe('addAccount', () => { const accounts: AuthState = { + accounts: [], token: null, enterpriseAccounts: [], user: null, diff --git a/src/utils/storage.test.ts b/src/utils/storage.test.ts index 34e298a24..ab6773d63 100644 --- a/src/utils/storage.test.ts +++ b/src/utils/storage.test.ts @@ -1,44 +1,159 @@ import { mockSettings } from '../__mocks__/mock-state'; +import Constants from './constants'; import { clearState, loadState, saveState } from './storage'; describe('utils/storage.ts', () => { it('should load the state from localstorage - existing', () => { jest.spyOn(localStorage.__proto__, 'getItem').mockReturnValueOnce( JSON.stringify({ - auth: { token: '123-456' }, - settings: { theme: 'DARK' }, + auth: { + accounts: [ + { + hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + platform: 'GitHub Cloud', + method: 'Personal Access Token', + token: '123-456', + user: null, + }, + ], + }, }), ); const result = loadState(); - expect(result.accounts.token).toBe('123-456'); - expect(result.settings.theme).toBe('DARK'); + + expect(result.accounts.accounts).toEqual([ + { + hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + platform: 'GitHub Cloud', + method: 'Personal Access Token', + token: '123-456', + user: null, + }, + ]); + expect(result.accounts.token).toBeUndefined(); + expect(result.accounts.enterpriseAccounts).toBeUndefined(); + expect(result.accounts.user).toBeUndefined(); }); +}); - it('should load the state from localstorage - empty', () => { - jest - .spyOn(localStorage.__proto__, 'getItem') - .mockReturnValueOnce(JSON.stringify({})); +it('should load the state from localstorage - empty', () => { + jest + .spyOn(localStorage.__proto__, 'getItem') + .mockReturnValueOnce(JSON.stringify({})); + const result = loadState(); + expect(result.accounts).toBeUndefined(); + expect(result.settings).toBeUndefined(); +}); + +it('should save the state to localstorage', () => { + jest.spyOn(localStorage.__proto__, 'setItem'); + saveState( + { + accounts: [ + { + hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + platform: 'GitHub Cloud', + method: 'Personal Access Token', + token: '123-456', + user: null, + }, + ], + token: '123-456', + enterpriseAccounts: [], + user: null, + }, + mockSettings, + ); + expect(localStorage.setItem).toHaveBeenCalledTimes(1); +}); + +it('should clear the state from localstorage', () => { + jest.spyOn(localStorage.__proto__, 'clear'); + clearState(); + expect(localStorage.clear).toHaveBeenCalledTimes(1); +}); + +describe('migrateLegacyAccounts', () => { + it('should migrate legacy accounts - personal access token only', () => { + jest.spyOn(localStorage.__proto__, 'getItem').mockReturnValueOnce( + JSON.stringify({ + auth: { token: '123-456', user: null }, + }), + ); const result = loadState(); - expect(result.accounts).toBeUndefined(); - expect(result.settings).toBeUndefined(); - }); - it('should save the state to localstorage', () => { - jest.spyOn(localStorage.__proto__, 'setItem'); - saveState( + expect(result.accounts.accounts).toEqual([ { + hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + platform: 'GitHub Cloud', + method: 'Personal Access Token', token: '123-456', - enterpriseAccounts: [], user: null, }, - mockSettings, + ]); + expect(result.accounts.token).toBeUndefined(); + expect(result.accounts.enterpriseAccounts).toBeUndefined(); + expect(result.accounts.user).toBeUndefined(); + }); + + it('should migrate legacy accounts - oauth app only', () => { + jest.spyOn(localStorage.__proto__, 'getItem').mockReturnValueOnce( + JSON.stringify({ + auth: { + enterpriseAccounts: [ + { hostname: 'github.gitify.io', token: '123-456' }, + ], + }, + }), ); - expect(localStorage.setItem).toHaveBeenCalledTimes(1); + const result = loadState(); + + expect(result.accounts.accounts).toEqual([ + { + hostname: 'github.gitify.io', + platform: 'GitHub Enterprise Server', + method: 'OAuth App', + token: '123-456', + user: null, + }, + ]); + expect(result.accounts.token).toBeUndefined(); + expect(result.accounts.enterpriseAccounts).toBeUndefined(); + expect(result.accounts.user).toBeUndefined(); }); - it('should clear the state from localstorage', () => { - jest.spyOn(localStorage.__proto__, 'clear'); - clearState(); - expect(localStorage.clear).toHaveBeenCalledTimes(1); + it('should migrate legacy accounts - combination', () => { + jest.spyOn(localStorage.__proto__, 'getItem').mockReturnValueOnce( + JSON.stringify({ + auth: { + token: '123-456', + user: null, + enterpriseAccounts: [ + { hostname: 'github.gitify.io', token: '123-456' }, + ], + }, + }), + ); + const result = loadState(); + + expect(result.accounts.accounts).toEqual([ + { + hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + platform: 'GitHub Cloud', + method: 'Personal Access Token', + token: '123-456', + user: null, + }, + { + hostname: 'github.gitify.io', + platform: 'GitHub Enterprise Server', + method: 'OAuth App', + token: '123-456', + user: null, + }, + ]); + expect(result.accounts.token).toBeUndefined(); + expect(result.accounts.enterpriseAccounts).toBeUndefined(); + expect(result.accounts.user).toBeUndefined(); }); }); diff --git a/src/utils/storage.ts b/src/utils/storage.ts index 0f3332e30..1ec72058d 100644 --- a/src/utils/storage.ts +++ b/src/utils/storage.ts @@ -7,7 +7,10 @@ export const loadState = (): { } => { const existing = localStorage.getItem(Constants.STORAGE_KEY); const { auth: accounts, settings } = (existing && JSON.parse(existing)) || {}; - return { accounts, settings }; + + const migratedAccounts = migrateLegacyAccounts(accounts); + + return { accounts: migratedAccounts, settings }; }; export const saveState = ( @@ -21,3 +24,46 @@ export const saveState = ( export const clearState = (): void => { localStorage.clear(); }; + +function migrateLegacyAccounts(accounts?: AuthState): AuthState { + const migratedAccounts = accounts?.accounts ?? []; + + if (accounts === undefined) { + return undefined; + } + + if (migratedAccounts.length > 0) { + return { + accounts: migratedAccounts, + }; + } + + if (accounts?.token) { + migratedAccounts.push({ + hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + platform: 'GitHub Cloud', + method: 'Personal Access Token', + token: accounts.token, + user: accounts.user, + }); + } + + if (accounts?.enterpriseAccounts) { + for (const legacyEnterpriseAccount of accounts.enterpriseAccounts) { + migratedAccounts.push({ + hostname: legacyEnterpriseAccount.hostname, + platform: 'GitHub Enterprise Server', + method: 'OAuth App', + token: legacyEnterpriseAccount.token, + user: null, + }); + } + } + + return { + accounts: migratedAccounts, + // token: accounts.token, + // enterpriseAccounts: accounts.enterpriseAccounts, + // user: accounts.user, + }; +} From 1c9d0b4b3a7d2e798da0a0e0e18ab5627fc48d29 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Thu, 23 May 2024 11:13:14 -0400 Subject: [PATCH 02/30] refactor: introduce new account types --- src/__mocks__/mock-state.ts | 46 ++-- src/__mocks__/mockedData.ts | 7 +- src/context/App.test.tsx | 9 +- src/context/App.tsx | 51 +++-- src/hooks/useNotifications.ts | 22 +- src/routes/LoginWithOAuthApp.tsx | 6 +- .../LoginWithPersonalAccessToken.test.tsx | 20 +- src/routes/LoginWithPersonalAccessToken.tsx | 8 +- src/routes/Settings.test.tsx | 18 +- src/types.ts | 34 +-- .../{auth.test.ts => auth/index.test.ts} | 113 +++++++--- src/utils/{auth.ts => auth/index.ts} | 56 ++--- src/utils/auth/types.ts | 32 +++ src/utils/helpers.test.ts | 50 ++++- src/utils/helpers.ts | 46 ++-- src/utils/notifications.ts | 6 +- src/utils/storage.test.ts | 208 +++++++++--------- src/utils/storage.ts | 3 - 18 files changed, 435 insertions(+), 300 deletions(-) rename src/utils/{auth.test.ts => auth/index.test.ts} (70%) rename src/utils/{auth.ts => auth/index.ts} (85%) create mode 100644 src/utils/auth/types.ts diff --git a/src/__mocks__/mock-state.ts b/src/__mocks__/mock-state.ts index b078ef829..c740b6a01 100644 --- a/src/__mocks__/mock-state.ts +++ b/src/__mocks__/mock-state.ts @@ -1,29 +1,35 @@ -import { type AuthState, type SettingsState, Theme } from '../types'; +import { + type AuthAccount, + type AuthState, + type SettingsState, + Theme, +} from '../types'; import Constants from '../utils/constants'; -import { mockedEnterpriseAccounts, mockedUser } from './mockedData'; +import { mockedUser } from './mockedData'; -export const mockAccounts: AuthState = { - accounts: [ - { - platform: 'GitHub Cloud', - method: 'Personal Access Token', - token: 'token-123-456', - hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, - user: mockedUser, - }, - { - platform: 'GitHub Enterprise Server', - method: 'OAuth App', - token: '1234568790', - hostname: 'github.gitify.io', - user: mockedUser, - }, - ], +export const mockPersonalAccessTokenAccount: AuthAccount = { + platform: 'GitHub Cloud', + method: 'Personal Access Token', token: 'token-123-456', - enterpriseAccounts: mockedEnterpriseAccounts, + hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, + user: mockedUser, +}; + +export const mockOAuthAccount: AuthAccount = { + platform: 'GitHub Enterprise Server', + method: 'OAuth App', + token: '1234568790', + hostname: 'github.gitify.io', user: mockedUser, }; +export const mockAccounts: AuthState = { + accounts: [mockPersonalAccessTokenAccount, mockOAuthAccount], + // token: 'token-123-456', + // enterpriseAccounts: mockedEnterpriseAccounts, + // user: mockedUser, +}; + export const mockSettings: SettingsState = { participating: false, playSound: true, diff --git a/src/__mocks__/mockedData.ts b/src/__mocks__/mockedData.ts index 085fa8e1a..6ffd63dc7 100644 --- a/src/__mocks__/mockedData.ts +++ b/src/__mocks__/mockedData.ts @@ -1,8 +1,4 @@ -import type { - AccountNotifications, - EnterpriseAccount, - GitifyUser, -} from '../types'; +import type { AccountNotifications, GitifyUser } from '../types'; import type { Discussion, DiscussionAuthor, @@ -12,6 +8,7 @@ import type { Repository, User, } from '../typesGitHub'; +import type { EnterpriseAccount } from '../utils/auth/types'; import Constants from '../utils/constants'; export const mockedEnterpriseAccounts: EnterpriseAccount[] = [ diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx index 9dfbc3246..eaf9ea4a4 100644 --- a/src/context/App.test.tsx +++ b/src/context/App.test.tsx @@ -235,17 +235,20 @@ describe('context/App.tsx', () => { ); }); - it('should call validateToken', async () => { + it('should call loginWithPersonalAccessToken', async () => { apiRequestAuthMock.mockResolvedValueOnce(null); const TestComponent = () => { - const { validateToken } = useContext(AppContext); + const { loginWithPersonalAccessToken } = useContext(AppContext); return ( - ); - }; - - const { getByText } = customRender(); - - markRepoNotificationsDoneMock.mockReset(); - - fireEvent.click(getByText('Test Case')); - - expect(markRepoNotificationsDoneMock).toHaveBeenCalledTimes(1); - expect(markRepoNotificationsDoneMock).toHaveBeenCalledWith( - { accounts: [], enterpriseAccounts: [], token: null, user: null }, - mockSettings, - 'gitify-app/notifications-test', - 'github.com', - ); - }); - it('should call loginWithPersonalAccessToken', async () => { apiRequestAuthMock.mockResolvedValueOnce(null); From c16edd23c12b75161e17fd363da7f15c6819eec1 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 31 May 2024 07:39:22 -0400 Subject: [PATCH 22/30] Merge branch 'refactor/new-account-type' of https://github.com/gitify-app/gitify into refactor/new-account-type --- src/context/App.test.tsx | 160 +++++++++++++++++++++------------------ 1 file changed, 85 insertions(+), 75 deletions(-) diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx index 3e2db6232..2e9ef6b9d 100644 --- a/src/context/App.test.tsx +++ b/src/context/App.test.tsx @@ -268,9 +268,19 @@ describe('context/App.tsx', () => { 'github.com', ); }); + }); + + describe('authentication methods', () => { + const apiRequestAuthMock = jest.spyOn(apiRequests, 'apiRequestAuth'); + const fetchNotificationsMock = jest.fn(); + + beforeEach(() => { + (useNotifications as jest.Mock).mockReturnValue({ + fetchNotifications: fetchNotificationsMock, + }); + }); it('should call loginWithPersonalAccessToken', async () => { - const apiRequestAuthMock = jest.spyOn(apiRequests, 'apiRequestAuth'); apiRequestAuthMock.mockResolvedValueOnce(null); const TestComponent = () => { @@ -334,93 +344,93 @@ describe('context/App.tsx', () => { expect(clearStateMock).toHaveBeenCalledTimes(1); }); +}); - describe('settings methods', () => { - it('should call updateSetting', async () => { - const saveStateMock = jest - .spyOn(storage, 'saveState') - .mockImplementation(jest.fn()); +describe('settings methods', () => { + it('should call updateSetting', async () => { + const saveStateMock = jest + .spyOn(storage, 'saveState') + .mockImplementation(jest.fn()); - const TestComponent = () => { - const { updateSetting } = useContext(AppContext); + const TestComponent = () => { + const { updateSetting } = useContext(AppContext); - return ( - - ); - }; + return ( + + ); + }; - const { getByText } = customRender(); + const { getByText } = customRender(); - act(() => { - fireEvent.click(getByText('Test Case')); - }); + act(() => { + fireEvent.click(getByText('Test Case')); + }); - expect(saveStateMock).toHaveBeenCalledWith({ - auth: { accounts: [], enterpriseAccounts: [], token: null, user: null }, - settings: { - participating: true, - playSound: true, - showNotifications: true, - showBots: true, - showNotificationsCountInTray: false, - openAtStartup: false, - theme: 'SYSTEM', - detailedNotifications: true, - markAsDoneOnOpen: false, - showAccountHostname: false, - delayNotificationState: false, - }, - }); + expect(saveStateMock).toHaveBeenCalledWith({ + auth: { accounts: [], enterpriseAccounts: [], token: null, user: null }, + settings: { + participating: true, + playSound: true, + showNotifications: true, + showBots: true, + showNotificationsCountInTray: false, + openAtStartup: false, + theme: 'SYSTEM', + detailedNotifications: true, + markAsDoneOnOpen: false, + showAccountHostname: false, + delayNotificationState: false, + }, }); + }); - it('should call updateSetting and set auto launch(openAtStartup)', async () => { - const setAutoLaunchMock = jest.spyOn(comms, 'setAutoLaunch'); - const saveStateMock = jest - .spyOn(storage, 'saveState') - .mockImplementation(jest.fn()); + it('should call updateSetting and set auto launch(openAtStartup)', async () => { + const setAutoLaunchMock = jest.spyOn(comms, 'setAutoLaunch'); + const saveStateMock = jest + .spyOn(storage, 'saveState') + .mockImplementation(jest.fn()); - const TestComponent = () => { - const { updateSetting } = useContext(AppContext); + const TestComponent = () => { + const { updateSetting } = useContext(AppContext); - return ( - - ); - }; + return ( + + ); + }; - const { getByText } = customRender(); + const { getByText } = customRender(); - act(() => { - fireEvent.click(getByText('Test Case')); - }); + act(() => { + fireEvent.click(getByText('Test Case')); + }); - expect(setAutoLaunchMock).toHaveBeenCalledWith(true); - - expect(saveStateMock).toHaveBeenCalledWith({ - auth: { accounts: [], enterpriseAccounts: [], token: null, user: null }, - settings: { - participating: false, - playSound: true, - showNotifications: true, - showBots: true, - showNotificationsCountInTray: false, - openAtStartup: true, - theme: 'SYSTEM', - detailedNotifications: true, - markAsDoneOnOpen: false, - showAccountHostname: false, - delayNotificationState: false, - }, - }); + expect(setAutoLaunchMock).toHaveBeenCalledWith(true); + + expect(saveStateMock).toHaveBeenCalledWith({ + auth: { accounts: [], enterpriseAccounts: [], token: null, user: null }, + settings: { + participating: false, + playSound: true, + showNotifications: true, + showBots: true, + showNotificationsCountInTray: false, + openAtStartup: true, + theme: 'SYSTEM', + detailedNotifications: true, + markAsDoneOnOpen: false, + showAccountHostname: false, + delayNotificationState: false, + }, }); }); }); From 5d97d1c3b03d84a2d0cdf2e0fcc87d6b567ed013 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 31 May 2024 07:41:31 -0400 Subject: [PATCH 23/30] Merge branch 'refactor/new-account-type' of https://github.com/gitify-app/gitify into refactor/new-account-type --- src/context/App.test.tsx | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx index 2e9ef6b9d..14f596fd9 100644 --- a/src/context/App.test.tsx +++ b/src/context/App.test.tsx @@ -322,28 +322,28 @@ describe('context/App.tsx', () => { ); }); }); +}); - it('should call logout', async () => { - const clearStateMock = jest.spyOn(storage, 'clearState'); - - const TestComponent = () => { - const { logout } = useContext(AppContext); +it('should call logout', async () => { + const clearStateMock = jest.spyOn(storage, 'clearState'); - return ( - - ); - }; + const TestComponent = () => { + const { logout } = useContext(AppContext); - const { getByText } = customRender(); + return ( + + ); + }; - act(() => { - fireEvent.click(getByText('Test Case')); - }); + const { getByText } = customRender(); - expect(clearStateMock).toHaveBeenCalledTimes(1); + act(() => { + fireEvent.click(getByText('Test Case')); }); + + expect(clearStateMock).toHaveBeenCalledTimes(1); }); describe('settings methods', () => { From a03544e14582e265db3e459800d6502564d34c89 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 31 May 2024 07:43:54 -0400 Subject: [PATCH 24/30] Merge branch 'refactor/new-account-type' of https://github.com/gitify-app/gitify into refactor/new-account-type --- src/context/App.test.tsx | 170 +++++++++++++++++++-------------------- 1 file changed, 85 insertions(+), 85 deletions(-) diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx index 14f596fd9..71dc830d9 100644 --- a/src/context/App.test.tsx +++ b/src/context/App.test.tsx @@ -322,44 +322,15 @@ describe('context/App.tsx', () => { ); }); }); -}); - -it('should call logout', async () => { - const clearStateMock = jest.spyOn(storage, 'clearState'); - - const TestComponent = () => { - const { logout } = useContext(AppContext); - - return ( - - ); - }; - - const { getByText } = customRender(); - - act(() => { - fireEvent.click(getByText('Test Case')); - }); - - expect(clearStateMock).toHaveBeenCalledTimes(1); -}); -describe('settings methods', () => { - it('should call updateSetting', async () => { - const saveStateMock = jest - .spyOn(storage, 'saveState') - .mockImplementation(jest.fn()); + it('should call logout', async () => { + const clearStateMock = jest.spyOn(storage, 'clearState'); const TestComponent = () => { - const { updateSetting } = useContext(AppContext); + const { logout } = useContext(AppContext); return ( - ); @@ -371,66 +342,95 @@ describe('settings methods', () => { fireEvent.click(getByText('Test Case')); }); - expect(saveStateMock).toHaveBeenCalledWith({ - auth: { accounts: [], enterpriseAccounts: [], token: null, user: null }, - settings: { - participating: true, - playSound: true, - showNotifications: true, - showBots: true, - showNotificationsCountInTray: false, - openAtStartup: false, - theme: 'SYSTEM', - detailedNotifications: true, - markAsDoneOnOpen: false, - showAccountHostname: false, - delayNotificationState: false, - }, - }); + expect(clearStateMock).toHaveBeenCalledTimes(1); }); - it('should call updateSetting and set auto launch(openAtStartup)', async () => { - const setAutoLaunchMock = jest.spyOn(comms, 'setAutoLaunch'); - const saveStateMock = jest - .spyOn(storage, 'saveState') - .mockImplementation(jest.fn()); + describe('settings methods', () => { + it('should call updateSetting', async () => { + const saveStateMock = jest + .spyOn(storage, 'saveState') + .mockImplementation(jest.fn()); - const TestComponent = () => { - const { updateSetting } = useContext(AppContext); + const TestComponent = () => { + const { updateSetting } = useContext(AppContext); - return ( - - ); - }; + return ( + + ); + }; - const { getByText } = customRender(); + const { getByText } = customRender(); - act(() => { - fireEvent.click(getByText('Test Case')); + act(() => { + fireEvent.click(getByText('Test Case')); + }); + + expect(saveStateMock).toHaveBeenCalledWith({ + auth: { accounts: [], enterpriseAccounts: [], token: null, user: null }, + settings: { + participating: true, + playSound: true, + showNotifications: true, + showBots: true, + showNotificationsCountInTray: false, + openAtStartup: false, + theme: 'SYSTEM', + detailedNotifications: true, + markAsDoneOnOpen: false, + showAccountHostname: false, + delayNotificationState: false, + }, + }); }); - expect(setAutoLaunchMock).toHaveBeenCalledWith(true); - - expect(saveStateMock).toHaveBeenCalledWith({ - auth: { accounts: [], enterpriseAccounts: [], token: null, user: null }, - settings: { - participating: false, - playSound: true, - showNotifications: true, - showBots: true, - showNotificationsCountInTray: false, - openAtStartup: true, - theme: 'SYSTEM', - detailedNotifications: true, - markAsDoneOnOpen: false, - showAccountHostname: false, - delayNotificationState: false, - }, + it('should call updateSetting and set auto launch(openAtStartup)', async () => { + const setAutoLaunchMock = jest.spyOn(comms, 'setAutoLaunch'); + const saveStateMock = jest + .spyOn(storage, 'saveState') + .mockImplementation(jest.fn()); + + const TestComponent = () => { + const { updateSetting } = useContext(AppContext); + + return ( + + ); + }; + + const { getByText } = customRender(); + + act(() => { + fireEvent.click(getByText('Test Case')); + }); + + expect(setAutoLaunchMock).toHaveBeenCalledWith(true); + + expect(saveStateMock).toHaveBeenCalledWith({ + auth: { accounts: [], enterpriseAccounts: [], token: null, user: null }, + settings: { + participating: false, + playSound: true, + showNotifications: true, + showBots: true, + showNotificationsCountInTray: false, + openAtStartup: true, + theme: 'SYSTEM', + detailedNotifications: true, + markAsDoneOnOpen: false, + showAccountHostname: false, + delayNotificationState: false, + }, + }); }); }); }); From 4083ad57016e33a1fde71771d3267fa94f9a32b8 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 31 May 2024 07:49:15 -0400 Subject: [PATCH 25/30] Merge branch 'refactor/new-account-type' of https://github.com/gitify-app/gitify into refactor/new-account-type --- src/routes/LoginWithOAuthApp.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/routes/LoginWithOAuthApp.test.tsx b/src/routes/LoginWithOAuthApp.test.tsx index 5eb76626d..cd062d6a4 100644 --- a/src/routes/LoginWithOAuthApp.test.tsx +++ b/src/routes/LoginWithOAuthApp.test.tsx @@ -2,7 +2,7 @@ import { fireEvent, render, screen } from '@testing-library/react'; import { shell } from 'electron'; import { MemoryRouter } from 'react-router-dom'; import { AppContext } from '../context/App'; -import type { AuthState as mockAuthState } from '../types'; +import type { AuthState } from '../types'; import { LoginWithOAuthApp, validate } from './LoginWithOAuthApp'; const mockNavigate = jest.fn(); @@ -14,7 +14,7 @@ jest.mock('react-router-dom', () => ({ describe('routes/LoginWithOAuthApp.tsx', () => { const openExternalMock = jest.spyOn(shell, 'openExternal'); - const mockAuth: mockAuthState = { + const mockAuth: AuthState = { accounts: [], }; From 0855c53cec218bd13c943e4369a00e2b80ee88f1 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 31 May 2024 08:00:15 -0400 Subject: [PATCH 26/30] Merge branch 'refactor/new-account-type' of https://github.com/gitify-app/gitify into refactor/new-account-type --- src/utils/auth/auth.test.ts | 278 ------------------------------------ 1 file changed, 278 deletions(-) delete mode 100644 src/utils/auth/auth.test.ts diff --git a/src/utils/auth/auth.test.ts b/src/utils/auth/auth.test.ts deleted file mode 100644 index 6afe6d7e7..000000000 --- a/src/utils/auth/auth.test.ts +++ /dev/null @@ -1,278 +0,0 @@ -import remote from '@electron/remote'; -import type { AxiosPromise, AxiosResponse } from 'axios'; -import type { AuthState } from '../../types'; -import * as apiRequests from '../api/request'; -import * as auth from './utils'; -import { getNewOAuthAppURL, getNewTokenURL } from './utils'; - -const browserWindow = new remote.BrowserWindow(); - -describe('utils/auth/utils.ts', () => { - describe('authGitHub', () => { - const loadURLMock = jest.spyOn(browserWindow, 'loadURL'); - - beforeEach(() => { - loadURLMock.mockReset(); - }); - - it('should call authGitHub - success', async () => { - // Casting to jest.Mock avoids Typescript errors, where the spy is expected to match all the original - // function's typing. I might fix all that if the return type of this was actually used, or if I was - // writing this test for a new feature. Since I'm just upgrading Jest, jest.Mock is a nice escape hatch - ( - jest.spyOn(browserWindow.webContents, 'on') as jest.Mock - ).mockImplementation((event, callback): void => { - if (event === 'will-redirect') { - const event = new Event('will-redirect'); - callback(event, 'http://github.com/?code=123-456'); - } - }); - - const res = await auth.authGitHub(); - - expect(res.authCode).toBe('123-456'); - - expect( - browserWindow.webContents.session.clearStorageData, - ).toHaveBeenCalledTimes(1); - - expect(loadURLMock).toHaveBeenCalledTimes(1); - expect(loadURLMock).toHaveBeenCalledWith( - 'https://github.com/login/oauth/authorize?client_id=FAKE_CLIENT_ID_123&scope=read:user,notifications,repo', - ); - - expect(browserWindow.destroy).toHaveBeenCalledTimes(1); - }); - - it('should call authGitHub - failure', async () => { - ( - jest.spyOn(browserWindow.webContents, 'on') as jest.Mock - ).mockImplementation((event, callback): void => { - if (event === 'will-redirect') { - const event = new Event('will-redirect'); - callback(event, 'http://www.github.com/?error=Oops'); - } - }); - - await expect(async () => await auth.authGitHub()).rejects.toEqual( - "Oops! Something went wrong and we couldn't log you in using GitHub. Please try again.", - ); - expect(loadURLMock).toHaveBeenCalledTimes(1); - }); - }); - - describe('getToken', () => { - const authCode = '123-456'; - const apiRequestMock = jest.spyOn(apiRequests, 'apiRequest'); - - it('should get a token - success', async () => { - const requestPromise = new Promise((resolve) => - resolve({ data: { access_token: 'this-is-a-token' } } as AxiosResponse), - ) as AxiosPromise; - - apiRequestMock.mockResolvedValueOnce(requestPromise); - - const res = await auth.getToken(authCode); - - expect(apiRequests.apiRequest).toHaveBeenCalledWith( - 'https://github.com/login/oauth/access_token', - 'POST', - { - client_id: 'FAKE_CLIENT_ID_123', - client_secret: 'FAKE_CLIENT_SECRET_123', - code: '123-456', - }, - ); - expect(res.token).toBe('this-is-a-token'); - expect(res.hostname).toBe('github.com'); - }); - - it('should get a token - failure', async () => { - const message = 'Something went wrong.'; - - const requestPromise = new Promise((_, reject) => - reject({ data: { message } } as AxiosResponse), - ) as AxiosPromise; - - apiRequestMock.mockResolvedValueOnce(requestPromise); - - const call = async () => await auth.getToken(authCode); - - await expect(call).rejects.toEqual({ data: { message } }); - }); - }); - - describe('addAccount', () => { - let mockAuthState: AuthState; - - beforeEach(() => { - mockAuthState = { - accounts: [], - }; - }); - - describe('should add GitHub Cloud account', () => { - it('should add personal access token account', async () => { - const result = await auth.addAccount( - mockAuthState, - 'Personal Access Token', - '123-456', - 'github.com', - ); - - expect(result.accounts).toEqual([ - { - hostname: 'github.com', - method: 'Personal Access Token', - platform: 'GitHub Cloud', - token: '123-456', - user: undefined, - }, - ]); - }); - - it('should add oauth app account', async () => { - const result = await auth.addAccount( - mockAuthState, - 'OAuth App', - '123-456', - 'github.com', - ); - - expect(result.accounts).toEqual([ - { - hostname: 'github.com', - method: 'OAuth App', - platform: 'GitHub Cloud', - token: '123-456', - user: undefined, - }, - ]); - }); - }); - - describe('should add GitHub Enterprise Server account', () => { - it('should add personal access token account', async () => { - const result = await auth.addAccount( - mockAuthState, - 'Personal Access Token', - '123-456', - 'github.gitify.io', - ); - - expect(result.accounts).toEqual([ - { - hostname: 'github.gitify.io', - method: 'Personal Access Token', - platform: 'GitHub Enterprise Server', - token: '123-456', - user: undefined, - }, - ]); - }); - - it('should add oauth app account', async () => { - const result = await auth.addAccount( - mockAuthState, - 'OAuth App', - '123-456', - 'github.gitify.io', - ); - - expect(result.accounts).toEqual([ - { - hostname: 'github.gitify.io', - method: 'OAuth App', - platform: 'GitHub Enterprise Server', - token: '123-456', - user: undefined, - }, - ]); - }); - }); - }); - - describe('getNewTokenURL', () => { - it('should generate new PAT url - github cloud', () => { - expect( - getNewTokenURL('github.com').startsWith( - 'https://github.com/settings/tokens/new', - ), - ).toBeTruthy(); - }); - - it('should generate new PAT url - github server', () => { - expect( - getNewTokenURL('github.gitify.io').startsWith( - 'https://github.gitify.io/settings/tokens/new', - ), - ).toBeTruthy(); - }); - }); - - describe('getNewOAuthAppURL', () => { - it('should generate new oauth app url - github cloud', () => { - expect( - getNewOAuthAppURL('github.com').startsWith( - 'https://github.com/settings/applications/new', - ), - ).toBeTruthy(); - }); - - it('should generate new oauth app url - github server', () => { - expect( - getNewOAuthAppURL('github.gitify.io').startsWith( - 'https://github.gitify.io/settings/applications/new', - ), - ).toBeTruthy(); - }); - }); - - describe('isValidHostname', () => { - it('should validate hostname - github cloud', () => { - expect(auth.isValidHostname('github.com')).toBeTruthy(); - }); - - it('should validate hostname - github enterprise server', () => { - expect(auth.isValidHostname('github.gitify.io')).toBeTruthy(); - }); - - it('should invalidate hostname - empty', () => { - expect(auth.isValidHostname('')).toBeFalsy(); - }); - - it('should invalidate hostname - invalid', () => { - expect(auth.isValidHostname('github')).toBeFalsy(); - }); - }); - - describe('isValidClientId', () => { - it('should validate client id - valid', () => { - expect(auth.isValidClientId('1234567890_ASDFGHJKL')).toBeTruthy(); - }); - - it('should validate client id - empty', () => { - expect(auth.isValidClientId('')).toBeFalsy(); - }); - - it('should validate client id - invalid', () => { - expect(auth.isValidClientId('1234567890asdfg')).toBeFalsy(); - }); - }); - - describe('isValidToken', () => { - it('should validate token - valid', () => { - expect( - auth.isValidToken('1234567890_asdfghjklPOIUYTREWQ0987654321'), - ).toBeTruthy(); - }); - - it('should validate token - empty', () => { - expect(auth.isValidToken('')).toBeFalsy(); - }); - - it('should validate token - invalid', () => { - expect(auth.isValidToken('1234567890asdfg')).toBeFalsy(); - }); - }); -}); From e4b15dc9eed472cbd4a0e70a1c41f607fdd633c6 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 31 May 2024 08:50:24 -0400 Subject: [PATCH 27/30] Merge branch 'refactor/new-account-type' of https://github.com/gitify-app/gitify into refactor/new-account-type --- src/utils/storage.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/utils/storage.test.ts b/src/utils/storage.test.ts index 4f5673e7d..42ca9a879 100644 --- a/src/utils/storage.test.ts +++ b/src/utils/storage.test.ts @@ -17,6 +17,7 @@ describe('utils/storage.ts', () => { }, ], }, + settings: { theme: 'DARK' }, }), ); const result = loadState(); @@ -33,6 +34,7 @@ describe('utils/storage.ts', () => { expect(result.auth.token).toBeUndefined(); expect(result.auth.enterpriseAccounts).toBeUndefined(); expect(result.auth.user).toBeUndefined(); + expect(result.settings.theme).toBe('DARK'); }); it('should load the state from localstorage - empty', () => { From 66676b576b0e9a2e98736de9ad30ade26d847540 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 31 May 2024 11:01:51 -0400 Subject: [PATCH 28/30] feat: refresh user on migration --- src/context/App.tsx | 4 ++-- src/utils/auth/migration.test.ts | 35 ++++++++++++++++++++++++-------- src/utils/auth/migration.ts | 24 +++++++++++++++++----- 3 files changed, 48 insertions(+), 15 deletions(-) diff --git a/src/context/App.tsx b/src/context/App.tsx index 149166adc..835073682 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -197,9 +197,9 @@ export const AppProvider = ({ children }: { children: ReactNode }) => { clearState(); }, []); - const restoreSettings = useCallback(() => { + const restoreSettings = useCallback(async () => { // Migrate authenticated accounts - migrateAuthenticatedAccounts(); + await migrateAuthenticatedAccounts(); const existing = loadState(); diff --git a/src/utils/auth/migration.test.ts b/src/utils/auth/migration.test.ts index c735f8dc6..39fd4c6bc 100644 --- a/src/utils/auth/migration.test.ts +++ b/src/utils/auth/migration.test.ts @@ -1,3 +1,5 @@ +import axios from 'axios'; +import nock from 'nock'; import { mockGitifyUser, mockToken } from '../../__mocks__/state-mocks'; import type { AuthState } from '../../types'; import Constants from '../constants'; @@ -41,8 +43,16 @@ describe('utils/auth/migration.ts', () => { }); describe('convertAccounts', () => { - it('should convert accounts - personal access token only', () => { - const result = convertAccounts({ + beforeEach(() => { + // axios will default to using the XHR adapter which can't be intercepted + // by nock. So, configure axios to use the node adapter. + axios.defaults.adapter = 'http'; + }); + + it('should convert accounts - personal access token only', async () => { + nock('https://api.github.com').get('/user').reply(200, mockGitifyUser); + + const result = await convertAccounts({ token: mockToken, user: mockGitifyUser, } as AuthState); @@ -58,8 +68,12 @@ describe('utils/auth/migration.ts', () => { ]); }); - it('should convert accounts - oauth app only', () => { - const result = convertAccounts({ + it('should convert accounts - oauth app only', async () => { + nock('https://github.gitify.io/api/v3') + .get('/user') + .reply(200, mockGitifyUser); + + const result = await convertAccounts({ enterpriseAccounts: [ { hostname: 'github.gitify.io', token: mockToken }, ], @@ -71,13 +85,18 @@ describe('utils/auth/migration.ts', () => { platform: 'GitHub Enterprise Server', method: 'OAuth App', token: mockToken, - user: null, + user: mockGitifyUser, }, ]); }); - it('should convert accounts - combination', () => { - const result = convertAccounts({ + it('should convert accounts - combination', async () => { + nock('https://api.github.com').get('/user').reply(200, mockGitifyUser); + nock('https://github.gitify.io/api/v3') + .get('/user') + .reply(200, mockGitifyUser); + + const result = await convertAccounts({ token: mockToken, user: mockGitifyUser, enterpriseAccounts: [ @@ -98,7 +117,7 @@ describe('utils/auth/migration.ts', () => { platform: 'GitHub Enterprise Server', method: 'OAuth App', token: mockToken, - user: null, + user: mockGitifyUser, }, ]); }); diff --git a/src/utils/auth/migration.ts b/src/utils/auth/migration.ts index 4050cc688..d8afe6e9e 100644 --- a/src/utils/auth/migration.ts +++ b/src/utils/auth/migration.ts @@ -1,19 +1,20 @@ import type { Account, AuthState } from '../../types'; import Constants from '../constants'; import { loadState, saveState } from '../storage'; +import { getUserData } from './utils'; /** * Migrate authenticated accounts from old data structure to new data structure (v5.7.0+). * * @deprecated We plan to remove this migration logic in a future major release. */ -export function migrateAuthenticatedAccounts() { +export async function migrateAuthenticatedAccounts() { const existing = loadState(); if (hasAccountsToMigrate(existing.auth)) { console.log('Commencing authenticated accounts migration'); - const migratedAccounts = convertAccounts(existing.auth); + const migratedAccounts = await convertAccounts(existing.auth); saveState({ auth: { ...existing.auth, accounts: migratedAccounts }, @@ -28,6 +29,7 @@ export function hasAccountsToMigrate(existingAuthState: AuthState): boolean { return false; } + // Don't attempt migration if there are already accounts in the new structure if (existingAuthState?.accounts?.length > 0) { return false; } @@ -42,27 +44,39 @@ export function hasAccountsToMigrate(existingAuthState: AuthState): boolean { return false; } -export function convertAccounts(existingAuthState: AuthState): Account[] { +export async function convertAccounts( + existingAuthState: AuthState, +): Promise { const migratedAccounts: Account[] = []; if (existingAuthState?.token) { + const user = await getUserData( + existingAuthState.token, + Constants.DEFAULT_AUTH_OPTIONS.hostname, + ); + migratedAccounts.push({ hostname: Constants.DEFAULT_AUTH_OPTIONS.hostname, platform: 'GitHub Cloud', method: 'Personal Access Token', token: existingAuthState.token, - user: existingAuthState.user, + user: user, }); } if (existingAuthState?.enterpriseAccounts) { for (const legacyEnterpriseAccount of existingAuthState.enterpriseAccounts) { + const user = await getUserData( + legacyEnterpriseAccount.token, + legacyEnterpriseAccount.hostname, + ); + migratedAccounts.push({ hostname: legacyEnterpriseAccount.hostname, platform: 'GitHub Enterprise Server', method: 'OAuth App', token: legacyEnterpriseAccount.token, - user: null, + user: user, }); } } From 9b51869a51ee7815bec4bc122394a82245a2a68f Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 31 May 2024 12:41:05 -0400 Subject: [PATCH 29/30] Merge remote-tracking branch 'origin/main' into refactor/new-account-type --- src/context/App.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/context/App.tsx b/src/context/App.tsx index 835073682..b84c8b3ec 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -198,7 +198,6 @@ export const AppProvider = ({ children }: { children: ReactNode }) => { }, []); const restoreSettings = useCallback(async () => { - // Migrate authenticated accounts await migrateAuthenticatedAccounts(); const existing = loadState(); From f784058514aa9998de5872ae9371bdb492a1d7ff Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 31 May 2024 13:04:37 -0400 Subject: [PATCH 30/30] test: add coverage --- src/utils/auth/migration.test.ts | 43 ++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/src/utils/auth/migration.test.ts b/src/utils/auth/migration.test.ts index 39fd4c6bc..9a65101bd 100644 --- a/src/utils/auth/migration.test.ts +++ b/src/utils/auth/migration.test.ts @@ -3,14 +3,49 @@ import nock from 'nock'; import { mockGitifyUser, mockToken } from '../../__mocks__/state-mocks'; import type { AuthState } from '../../types'; import Constants from '../constants'; -import { convertAccounts, hasAccountsToMigrate } from './migration'; +import { + convertAccounts, + hasAccountsToMigrate, + migrateAuthenticatedAccounts, +} from './migration'; describe('utils/auth/migration.ts', () => { + beforeEach(() => { + // axios will default to using the XHR adapter which can't be intercepted + // by nock. So, configure axios to use the node adapter. + axios.defaults.adapter = 'http'; + }); + + describe('migrateAuthenticatedAccounts', () => { + it('migrate and save legacy accounts', async () => { + jest.spyOn(localStorage.__proto__, 'getItem').mockReturnValueOnce( + JSON.stringify({ + auth: { + token: mockToken, + }, + settings: { theme: 'DARK' }, + }), + ); + + nock('https://api.github.com').get('/user').reply(200, mockGitifyUser); + + jest.spyOn(localStorage.__proto__, 'setItem'); + + await migrateAuthenticatedAccounts(); + + expect(localStorage.setItem).toHaveBeenCalledTimes(1); + }); + }); + describe('hasAccountsToMigrate', () => { it('should not migrate if none exist', () => { expect(hasAccountsToMigrate(null)).toBe(false); }); + it('should not migrate if empty', () => { + expect(hasAccountsToMigrate({} as AuthState)).toBe(false); + }); + it('should migrate if token exist', () => { expect( hasAccountsToMigrate({ @@ -43,12 +78,6 @@ describe('utils/auth/migration.ts', () => { }); describe('convertAccounts', () => { - beforeEach(() => { - // axios will default to using the XHR adapter which can't be intercepted - // by nock. So, configure axios to use the node adapter. - axios.defaults.adapter = 'http'; - }); - it('should convert accounts - personal access token only', async () => { nock('https://api.github.com').get('/user').reply(200, mockGitifyUser);