From 782e9b4909d395c9ed3fbbf1831a3ca3595bf18b Mon Sep 17 00:00:00 2001 From: ajay-sentry <159853603+ajay-sentry@users.noreply.github.com> Date: Mon, 29 Apr 2024 10:05:06 -0700 Subject: [PATCH] fix: use session if exists when doing initial redirect for user (#2810) * add a test, convert to TS, update to homepage redirect component * try this out * add error case --- src/{App.spec.jsx => App.spec.tsx} | 49 ++++++++++++++++------ src/{App.jsx => App.tsx} | 34 ++++++++++----- src/services/user/useInternalUser.spec.tsx | 15 ++++++- src/services/user/useInternalUser.ts | 37 +++++++++------- 4 files changed, 94 insertions(+), 41 deletions(-) rename src/{App.spec.jsx => App.spec.tsx} (91%) rename src/{App.jsx => App.tsx} (84%) diff --git a/src/App.spec.jsx b/src/App.spec.tsx similarity index 91% rename from src/App.spec.jsx rename to src/App.spec.tsx index f8136cb9a6..66728f4867 100644 --- a/src/App.spec.jsx +++ b/src/App.spec.tsx @@ -2,6 +2,7 @@ import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import { render, screen, waitFor } from '@testing-library/react' import { graphql, rest } from 'msw' import { setupServer } from 'msw/node' +import React from 'react' import { MemoryRouter, Route } from 'react-router-dom' import config from 'config' @@ -43,9 +44,9 @@ const internalUser = { integrationId: null, name: null, ownerid: 123, - service: 'github', + service: 'cool-service', stats: null, - username: 'codecov', + username: 'cool-guy', }, ], termsAgreement: true, @@ -73,9 +74,9 @@ const queryClient = new QueryClient({ }) const server = setupServer() -let testLocation +let testLocation: any const wrapper = - (initialEntries = []) => + (initialEntries = ['']): React.FC => ({ children }) => ( @@ -94,9 +95,10 @@ const wrapper = beforeAll(() => { server.listen({ onUnhandledRequest: 'warn' }) + console.error = () => {} }) -afterEach(() => { +beforeEach(() => { config.IS_SELF_HOSTED = false queryClient.clear() server.resetHandlers() @@ -107,10 +109,20 @@ afterAll(() => { }) describe('App', () => { - function setup(hasLoggedInUser = true) { + function setup({ + hasLoggedInUser, + hasSession, + }: { + hasLoggedInUser?: boolean + hasSession?: boolean + }) { server.use( rest.get('/internal/user', (_, res, ctx) => { - return res(ctx.status(200), ctx.json(internalUser)) + if (hasSession) { + return res(ctx.status(200), ctx.json(internalUser)) + } else { + return res(ctx.status(200), ctx.json({})) + } }), graphql.query('DetailOwner', (_, res, ctx) => res(ctx.status(200), ctx.data({ owner: 'codecov' })) @@ -319,17 +331,18 @@ describe('App', () => { ({ testLabel, pathname, expected }) => { beforeEach(() => { config.IS_SELF_HOSTED = false - setup() + setup({ hasLoggedInUser: true, hasSession: true }) }) it(`renders the ${testLabel} page`, async () => { render(, { wrapper: wrapper([pathname]) }) - await waitFor( - () => expect(testLocation.pathname).toBe(expected.location), - expect(testLocation.search).toBe(expected.search) + await waitFor(() => + expect(testLocation.pathname).toBe(expected.location) ) + await waitFor(() => expect(testLocation.search).toBe(expected.search)) + const page = await screen.findByText(expected.page) expect(page).toBeInTheDocument() }) @@ -504,7 +517,7 @@ describe('App', () => { ({ testLabel, pathname, expected }) => { beforeEach(() => { config.IS_SELF_HOSTED = true - setup() + setup({ hasLoggedInUser: true, hasSession: true }) }) it(`renders the ${testLabel} page`, async () => { @@ -521,9 +534,19 @@ describe('App', () => { describe('user not logged in', () => { it('redirects to login', async () => { - setup(false) + setup({ hasLoggedInUser: true, hasSession: false }) render(, { wrapper: wrapper(['*']) }) await waitFor(() => expect(testLocation.pathname).toBe('/login')) }) }) + describe('user has session, not logged in', () => { + it('redirects to session default', async () => { + setup({ hasLoggedInUser: false, hasSession: true }) + + render(, { wrapper: wrapper(['/blah']) }) + await waitFor(() => + expect(testLocation.pathname).toBe('/cool-service/cool-guy') + ) + }) + }) }) diff --git a/src/App.jsx b/src/App.tsx similarity index 84% rename from src/App.jsx rename to src/App.tsx index ee9903e99c..24cb1c6ec9 100644 --- a/src/App.jsx +++ b/src/App.tsx @@ -13,7 +13,8 @@ import LoginLayout from 'layouts/LoginLayout' import { useLocationParams } from 'services/navigation' import { ToastNotificationProvider } from 'services/toastNotification' import { useUTM } from 'services/tracking/utm' -import { useUser } from 'services/user' +import { useInternalUser, useUser } from 'services/user' +import { isProvider } from 'shared/api/helpers' const AccountSettings = lazy(() => import('./pages/AccountSettings')) const AdminSettings = lazy(() => import('./pages/AdminSettings')) @@ -28,21 +29,32 @@ const PullRequestPage = lazy(() => import('./pages/PullRequestPage')) const RepoPage = lazy(() => import('./pages/RepoPage')) const SyncProviderPage = lazy(() => import('./pages/SyncProviderPage')) +interface URLParams { + provider: string +} + const HomePageRedirect = () => { - const { provider } = useParams() + const { provider } = useParams() const { data: currentUser } = useUser() + const { data: internalUser } = useInternalUser({}) const { params } = useLocationParams() - if (!provider || !currentUser) { - return + let redirectURL = '/login' + + if (internalUser && internalUser.owners) { + redirectURL = `/${internalUser.owners[0]?.service}/${internalUser.owners[0]?.username}` } - const defaultOrg = - currentUser.owner?.defaultOrgUsername ?? currentUser.user?.username - let redirectURL = `/${provider}/${defaultOrg}` - const { setup_action: setupAction } = params - if (setupAction) { - redirectURL += `?setup_action=${setupAction}` + if (provider && isProvider(provider) && currentUser) { + const defaultOrg = + currentUser.owner?.defaultOrgUsername ?? currentUser.user?.username + redirectURL = `/${provider}/${defaultOrg}` + + // @ts-expect-error useLocationParams needs to be typed + const { setup_action: setupAction } = params + if (setupAction) { + redirectURL += `?setup_action=${setupAction}` + } } return @@ -148,7 +160,7 @@ const MainAppRoutes = () => ( ) : ( - + )} diff --git a/src/services/user/useInternalUser.spec.tsx b/src/services/user/useInternalUser.spec.tsx index 32142fea5d..d6a7a6e5ef 100644 --- a/src/services/user/useInternalUser.spec.tsx +++ b/src/services/user/useInternalUser.spec.tsx @@ -26,9 +26,12 @@ afterAll(() => { }) describe('useInternalUser', () => { - function setup() { + function setup(hasError = false) { server.use( rest.get('/internal/user', (req, res, ctx) => { + if (hasError) { + return res(ctx.status(400), ctx.json({})) + } return res( ctx.status(200), ctx.json({ @@ -60,4 +63,14 @@ describe('useInternalUser', () => { ) }) }) + + describe('when hook call errors', () => { + it('returns empty object', async () => { + setup(true) + + const { result } = renderHook(() => useInternalUser({}), { wrapper }) + + await waitFor(() => expect(result.current.data).toStrictEqual({})) + }) + }) }) diff --git a/src/services/user/useInternalUser.ts b/src/services/user/useInternalUser.ts index 79fe1c2683..2ae5590776 100644 --- a/src/services/user/useInternalUser.ts +++ b/src/services/user/useInternalUser.ts @@ -40,22 +40,27 @@ export interface UseInternalUserArgs { export const useInternalUser = (opts: UseInternalUserArgs) => useQuery({ queryKey: ['InternalUser'], - queryFn: ({ signal }) => { - return Api.get({ - path: '/user', - signal, - }).then((res) => { - const parsedData = InternalUserSchema.safeParse(res) - - if (!parsedData.success) { - return Promise.reject({ - status: 404, - data: null, - }) - } - - return parsedData.data - }) + queryFn: async ({ signal }) => { + let response + try { + response = await Api.get({ + path: '/user', + signal, + }) + } catch (e) { + return {} as InternalUserData + } + + const parsedData = InternalUserSchema.safeParse(response) + + if (!parsedData.success) { + return Promise.reject({ + status: 404, + data: null, + }) + } + + return parsedData.data }, ...(!!opts && opts), })