From dfffd6d4e513d0f33321a3df0010793adb52f07e Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 8 Jul 2025 09:22:20 -0400 Subject: [PATCH 1/5] restructure users hooks to use a queries file --- frontends/api/src/hooks/user/index.ts | 21 +++++++++----------- frontends/api/src/hooks/user/queries.ts | 17 ++++++++++++++++ frontends/api/src/ssr/usePrefetchWarnings.ts | 3 ++- frontends/main/src/test-utils/index.tsx | 3 ++- 4 files changed, 30 insertions(+), 14 deletions(-) create mode 100644 frontends/api/src/hooks/user/queries.ts diff --git a/frontends/api/src/hooks/user/index.ts b/frontends/api/src/hooks/user/index.ts index aa79ab845e..603246e399 100644 --- a/frontends/api/src/hooks/user/index.ts +++ b/frontends/api/src/hooks/user/index.ts @@ -1,6 +1,6 @@ import { useQuery } from "@tanstack/react-query" -import { usersApi } from "../../clients" import type { User } from "../../generated/v0/api" +import { userQueries } from "./queries" enum Permission { ArticleEditor = "is_article_editor", @@ -8,16 +8,7 @@ enum Permission { LearningPathEditor = "is_learning_path_editor", } -const useUserMe = () => - useQuery({ - queryKey: ["userMe"], - queryFn: async (): Promise => { - const response = await usersApi.usersMeRetrieve() - return { - ...response.data, - } - }, - }) +const useUserMe = () => useQuery(userQueries.me()) const useUserIsAuthenticated = () => { const { data: user } = useUserMe() @@ -29,5 +20,11 @@ const useUserHasPermission = (permission: Permission) => { return !!user?.[permission] } -export { useUserMe, useUserIsAuthenticated, useUserHasPermission, Permission } +export { + userQueries, + useUserMe, + useUserIsAuthenticated, + useUserHasPermission, + Permission, +} export type { User } diff --git a/frontends/api/src/hooks/user/queries.ts b/frontends/api/src/hooks/user/queries.ts new file mode 100644 index 0000000000..776d489652 --- /dev/null +++ b/frontends/api/src/hooks/user/queries.ts @@ -0,0 +1,17 @@ +import { queryOptions } from "@tanstack/react-query" +import { usersApi } from "../../clients" + +const userKeys = { + root: ["users"], + me: () => [...userKeys.root, "me"], +} + +const userQueries = { + me: () => + queryOptions({ + queryKey: userKeys.me(), + queryFn: () => usersApi.usersMeRetrieve().then((res) => res.data), + }), +} + +export { userQueries, userKeys } diff --git a/frontends/api/src/ssr/usePrefetchWarnings.ts b/frontends/api/src/ssr/usePrefetchWarnings.ts index 05fbc4c6b3..ad7eb50f7e 100644 --- a/frontends/api/src/ssr/usePrefetchWarnings.ts +++ b/frontends/api/src/ssr/usePrefetchWarnings.ts @@ -1,5 +1,6 @@ import { useEffect } from "react" import type { Query, QueryClient, QueryKey } from "@tanstack/react-query" +import { userQueries } from "../hooks/user/queries" const logQueries = (...args: [...string[], Query[]]) => { const queries = args.pop() as Query[] @@ -16,7 +17,7 @@ const logQueries = (...args: [...string[], Query[]]) => { ) } -const PREFETCH_EXEMPT_QUERIES = [["userMe"]] +const PREFETCH_EXEMPT_QUERIES = [userQueries.me().queryKey] /** * Call this as high as possible in render tree to detect query usage on diff --git a/frontends/main/src/test-utils/index.tsx b/frontends/main/src/test-utils/index.tsx index 20b540a25f..47643fed6d 100644 --- a/frontends/main/src/test-utils/index.tsx +++ b/frontends/main/src/test-utils/index.tsx @@ -9,6 +9,7 @@ import { makeQueryClient } from "@/app/getQueryClient" import { render } from "@testing-library/react" import { factories, setMockResponse } from "api/test-utils" import type { User } from "api/hooks/user" +import { userQueries } from "api/hooks/user" import { mockRouter, createDynamicRouteParser, @@ -67,7 +68,7 @@ const renderWithProviders = ( if (allOpts.user) { const user = { ...defaultUser, ...allOpts.user } - queryClient.setQueryData(["userMe"], { ...user }) + queryClient.setQueryData(userQueries.me().queryKey, { ...user }) } mockRouter.setCurrentUrl(url) From 9317dade19f9ff4eb64206e609c15f7cc456af14 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 8 Jul 2025 11:39:41 -0400 Subject: [PATCH 2/5] ensure fresh data during auth check --- .../api/src/test-utils/factories/user.ts | 11 +++ .../app-pages/ErrorPage/ErrorPageTemplate.tsx | 27 ++++++- .../ErrorPage/ForbiddenPage.test.tsx | 79 ++++++++++++------- .../src/app-pages/ErrorPage/ForbiddenPage.tsx | 22 ++++-- frontends/main/src/app/error.test.tsx | 11 ++- 5 files changed, 110 insertions(+), 40 deletions(-) diff --git a/frontends/api/src/test-utils/factories/user.ts b/frontends/api/src/test-utils/factories/user.ts index a277913561..82211e88df 100644 --- a/frontends/api/src/test-utils/factories/user.ts +++ b/frontends/api/src/test-utils/factories/user.ts @@ -25,6 +25,17 @@ const profile: PartialFactory = (overrides = {}): Profile => ({ const enforcerId = new UniqueEnforcer() const user: PartialFactory = (overrides = {}): User => { + if (overrides?.is_authenticated === false) { + return { + is_authenticated: false, + is_article_editor: false, + is_learning_path_editor: false, + // @ts-expect-error API Response can include anonymous user + id: null, + username: "", + } + } + const result: User = { id: enforcerId.enforce(faker.number.int), first_name: faker.person.firstName(), diff --git a/frontends/main/src/app-pages/ErrorPage/ErrorPageTemplate.tsx b/frontends/main/src/app-pages/ErrorPage/ErrorPageTemplate.tsx index dd30b038df..f77e04b1b7 100644 --- a/frontends/main/src/app-pages/ErrorPage/ErrorPageTemplate.tsx +++ b/frontends/main/src/app-pages/ErrorPage/ErrorPageTemplate.tsx @@ -1,5 +1,5 @@ import React from "react" -import { Typography, styled } from "ol-components" +import { Container, Skeleton, Typography, styled } from "ol-components" import { ButtonLink } from "@mitodl/smoot-design" import { HOME } from "@/common/urls" import Image from "next/image" @@ -10,9 +10,10 @@ import timSpeechBubble from "@/public/images/tim_speech_bubble.svg" type ErrorPageTemplateProps = { title: string timSays?: string + loading?: boolean } -const Page = styled.div(({ theme }) => ({ +const Page = styled(Container)(({ theme }) => ({ backgroundImage: `url(${backgroundImage.src})`, backgroundAttachment: "fixed", backgroundRepeat: "no-repeat", @@ -83,7 +84,29 @@ const Button = styled(ButtonLink)({ const ErrorPageTemplate: React.FC = ({ title, timSays, + loading = false, }) => { + if (loading) { + return ( + + + + + + + + +
+ +
+ + + ) + } return ( diff --git a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx index 84440f1639..635c9182c4 100644 --- a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx +++ b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx @@ -1,44 +1,63 @@ import React from "react" -import { renderWithProviders, screen } from "../../test-utils" -import { HOME } from "@/common/urls" +import { renderWithProviders, screen, waitFor } from "../../test-utils" +import { HOME, login } from "@/common/urls" import ForbiddenPage from "./ForbiddenPage" -import { setMockResponse, urls } from "api/test-utils" -import { Permission } from "api/hooks/user" +import { setMockResponse, urls, factories } from "api/test-utils" +import { useUserMe } from "api/hooks/user" +import { redirect } from "next/navigation" -const oldWindowLocation = window.location - -beforeAll(() => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - delete (window as any).location - - window.location = Object.defineProperties({} as Location, { - ...Object.getOwnPropertyDescriptors(oldWindowLocation), - assign: { - configurable: true, - value: jest.fn(), - }, - }) +jest.mock("next/navigation", () => { + const actual = jest.requireActual("next/navigation") + return { + ...actual, + redirect: jest.fn(), + } }) +const mockedRedirect = jest.mocked(redirect) -afterAll(() => { - window.location = oldWindowLocation -}) +const makeUser = factories.user.user -test("The ForbiddenPage loads with Correct Title", () => { - setMockResponse.get(urls.userMe.get(), { - [Permission.Authenticated]: true, - }) +test("The ForbiddenPage loads with Correct Title", async () => { + setMockResponse.get(urls.userMe.get(), makeUser({ is_authenticated: true })) renderWithProviders() - screen.getByRole("heading", { + await screen.findByRole("heading", { name: "You do not have permission to access this resource.", }) }) -test("The ForbiddenPage loads with a link that directs to HomePage", () => { - setMockResponse.get(urls.userMe.get(), { - [Permission.Authenticated]: true, - }) +test("The ForbiddenPage loads with a link that directs to HomePage", async () => { + setMockResponse.get(urls.userMe.get(), makeUser({ is_authenticated: true })) renderWithProviders() - const homeLink = screen.getByRole("link", { name: "Home" }) + const homeLink = await screen.findByRole("link", { name: "Home" }) expect(homeLink).toHaveAttribute("href", HOME) }) + +test("Fetches auth data afresh and redirects unauthenticated users to auth", async () => { + const user = makeUser() + setMockResponse.get(urls.userMe.get(), user) + + const FakeHeader = ({ children }: { children?: React.ReactNode }) => { + const user = useUserMe() + return ( +
+ {user.data?.first_name} + {children} +
+ ) + } + const { view } = renderWithProviders() + + await screen.findByText(user.first_name) + + setMockResponse.get(urls.userMe.get(), makeUser({ is_authenticated: false })) + + view.rerender( + + + , + ) + + await waitFor(() => { + expect(mockedRedirect).toHaveBeenCalledWith(login()) + }) +}) diff --git a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx index dc966705fb..d81b7dce34 100644 --- a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx +++ b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx @@ -1,20 +1,32 @@ import React, { useEffect } from "react" import ErrorPageTemplate from "./ErrorPageTemplate" -import { useUserMe } from "api/hooks/user" +import { userQueries } from "api/hooks/user" import { redirect } from "next/navigation" import * as urls from "@/common/urls" +import { useQuery } from "@tanstack/react-query" const ForbiddenPage: React.FC = () => { - const { data: user } = useUserMe() + const user = useQuery({ + ...userQueries.me(), + staleTime: 0, // Force refetch on mount + }) + + const shouldRedirect = + user.isFetchedAfterMount && !user.data?.is_authenticated useEffect(() => { - if (!user?.is_authenticated) { + if (shouldRedirect) { const loginUrl = urls.login() redirect(loginUrl) } - }, [user]) + }, [shouldRedirect]) + return ( - + ) } diff --git a/frontends/main/src/app/error.test.tsx b/frontends/main/src/app/error.test.tsx index 3f20768c77..374ba8957c 100644 --- a/frontends/main/src/app/error.test.tsx +++ b/frontends/main/src/app/error.test.tsx @@ -3,6 +3,9 @@ import { renderWithProviders, screen } from "@/test-utils" import { HOME } from "@/common/urls" import ErrorPage from "./error" import { ForbiddenError } from "@/common/errors" +import { setMockResponse, urls, factories } from "api/test-utils" + +const makeUser = factories.user.user test("The error page shows error message", () => { const error = new Error("Ruh roh") @@ -13,10 +16,12 @@ test("The error page shows error message", () => { expect(homeLink).toHaveAttribute("href", HOME) }) -test("The NotFoundPage loads with a link that directs to HomePage", () => { +test("The Forbidden loads with a link that directs to HomePage", async () => { const error = new ForbiddenError() - renderWithProviders(, { user: {} }) - screen.getByRole("heading", { + setMockResponse.get(urls.userMe.get(), makeUser()) + + renderWithProviders() + await screen.findByRole("heading", { name: "You do not have permission to access this resource.", }) screen.getByText("Oops!") From 21562c59d734a0da835b7e3a10e55f09f0a1aded Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 8 Jul 2025 14:38:54 -0400 Subject: [PATCH 3/5] refactor login url --- .../ErrorPage/ForbiddenPage.test.tsx | 19 +++++++------ .../src/app-pages/ErrorPage/ForbiddenPage.tsx | 6 ++--- .../src/app-pages/HomePage/HomePage.test.tsx | 1 + .../app-pages/HomePage/PersonalizeSection.tsx | 2 +- frontends/main/src/common/urls.test.ts | 6 ++--- frontends/main/src/common/urls.ts | 27 +++++++++++-------- frontends/main/src/common/utils.ts | 20 ++++++++++++++ .../RestrictedRoute/RestrictedRoute.tsx | 5 ++-- .../src/page-components/Header/UserMenu.tsx | 6 ++--- .../SignupPopover/SignupPopover.tsx | 10 +++---- 10 files changed, 59 insertions(+), 43 deletions(-) diff --git a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx index 635c9182c4..4e6f817b5f 100644 --- a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx +++ b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx @@ -6,13 +6,6 @@ import { setMockResponse, urls, factories } from "api/test-utils" import { useUserMe } from "api/hooks/user" import { redirect } from "next/navigation" -jest.mock("next/navigation", () => { - const actual = jest.requireActual("next/navigation") - return { - ...actual, - redirect: jest.fn(), - } -}) const mockedRedirect = jest.mocked(redirect) const makeUser = factories.user.user @@ -45,8 +38,9 @@ test("Fetches auth data afresh and redirects unauthenticated users to auth", asy ) } - const { view } = renderWithProviders() - + const { view } = renderWithProviders(, { + url: "/foo?cat=meow", + }) await screen.findByText(user.first_name) setMockResponse.get(urls.userMe.get(), makeUser({ is_authenticated: false })) @@ -58,6 +52,11 @@ test("Fetches auth data afresh and redirects unauthenticated users to auth", asy ) await waitFor(() => { - expect(mockedRedirect).toHaveBeenCalledWith(login()) + expect(mockedRedirect).toHaveBeenCalledWith( + login({ + pathname: "/foo", + searchParams: new URLSearchParams({ cat: "meow" }), + }), + ) }) }) diff --git a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx index d81b7dce34..4307933b51 100644 --- a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx +++ b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx @@ -2,8 +2,8 @@ import React, { useEffect } from "react" import ErrorPageTemplate from "./ErrorPageTemplate" import { userQueries } from "api/hooks/user" import { redirect } from "next/navigation" -import * as urls from "@/common/urls" import { useQuery } from "@tanstack/react-query" +import { useLoginToCurrent } from "@/common/utils" const ForbiddenPage: React.FC = () => { const user = useQuery({ @@ -14,12 +14,12 @@ const ForbiddenPage: React.FC = () => { const shouldRedirect = user.isFetchedAfterMount && !user.data?.is_authenticated + const loginUrl = useLoginToCurrent() useEffect(() => { if (shouldRedirect) { - const loginUrl = urls.login() redirect(loginUrl) } - }, [shouldRedirect]) + }, [shouldRedirect, loginUrl]) return ( { "href", routes.login({ pathname: routes.DASHBOARD_HOME, + searchParams: null, }), ) }) diff --git a/frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx b/frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx index a3e37babfa..ed1238afb9 100644 --- a/frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx +++ b/frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx @@ -76,7 +76,7 @@ const AUTH_TEXT_DATA = { text: "As a member, get personalized recommendations, curate learning lists, and follow your areas of interest.", linkProps: { children: "Sign Up for Free", - href: urls.login({ pathname: urls.DASHBOARD_HOME }), + href: urls.login({ pathname: urls.DASHBOARD_HOME, searchParams: null }), }, }, } diff --git a/frontends/main/src/common/urls.test.ts b/frontends/main/src/common/urls.test.ts index 8b5e063678..3a762250ee 100644 --- a/frontends/main/src/common/urls.test.ts +++ b/frontends/main/src/common/urls.test.ts @@ -3,16 +3,14 @@ import { login } from "./urls" const MITOL_API_BASE_URL = process.env.NEXT_PUBLIC_MITOL_API_BASE_URL test("login encodes the next parameter appropriately", () => { - expect(login()).toBe( - `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/`, - ) - expect(login({})).toBe( + expect(login({ pathname: null, searchParams: null })).toBe( `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/`, ) expect( login({ pathname: "/foo/bar", + searchParams: null, }), ).toBe( `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/foo/bar`, diff --git a/frontends/main/src/common/urls.ts b/frontends/main/src/common/urls.ts index 985ab3d4a7..1937457001 100644 --- a/frontends/main/src/common/urls.ts +++ b/frontends/main/src/common/urls.ts @@ -61,16 +61,21 @@ export const LOGOUT = `${MITOL_API_BASE_URL}/logout/` /** * Returns the URL to the login page, with a `next` parameter to redirect back * to the given pathname + search parameters. + * + * NOTES: + * 1. useLoginToCurrent() is a convenience function that uses the current + * pathname and search parameters to generate the next URL. + * 2. `next` is required to encourage its use. You can explicitly pass `null` + * for values to skip them if desired. */ -export const login = ({ - pathname = "/", - searchParams = new URLSearchParams(), - hash = "", -}: { - pathname?: string | null - searchParams?: URLSearchParams +export const login = (next: { + pathname: string | null + searchParams: URLSearchParams | null hash?: string | null -} = {}) => { +}) => { + const pathname = next.pathname ?? "/" + const searchParams = next.searchParams ?? new URLSearchParams() + const hash = next.hash ?? "" /** * To include search parameters in the next URL, we need to encode them. * If we pass `?next=/foo/bar?cat=meow` directly, Django receives two separate @@ -79,9 +84,9 @@ export const login = ({ * There's no need to encode the path parameter (it might contain slashes, * but those are allowed in search parameters) so let's keep it readable. */ - const search = searchParams.toString() ? `?${searchParams.toString()}` : "" - const next = `${ORIGIN}${pathname}${encodeURIComponent(search)}${encodeURIComponent(hash as string)}` - return `${LOGIN}?next=${next}` + const search = searchParams?.toString() ? `?${searchParams.toString()}` : "" + const nextHref = `${ORIGIN}${pathname}${encodeURIComponent(search)}${encodeURIComponent(hash as string)}` + return `${LOGIN}?next=${nextHref}` } export const DASHBOARD_VIEW = "/dashboard/[tab]" diff --git a/frontends/main/src/common/utils.ts b/frontends/main/src/common/utils.ts index d8896fe47a..1316067b6b 100644 --- a/frontends/main/src/common/utils.ts +++ b/frontends/main/src/common/utils.ts @@ -1,4 +1,7 @@ import { ChannelCounts } from "api/v0" +import { login } from "./urls" +import { usePathname, useSearchParams } from "next/navigation" + const getSearchParamMap = (urlParams: URLSearchParams) => { const params: Record = {} for (const [key] of urlParams.entries()) { @@ -48,9 +51,26 @@ const getCsrfToken = () => { ) } +const useLoginToCurrent = () => { + /** + * NOTE: In contrast to, say + * login({ + * pathname: window.location.pathname, + * searchParams: new URLSearchParams(window.location.search) + * }), + * the version here is reactive: when pathname/searchParams change, the values + * here update automatically and will (appropriately) trigger a re-render. + * + */ + const pathname = usePathname() + const searchParams = useSearchParams() + return login({ pathname, searchParams }) +} + export { getSearchParamMap, aggregateProgramCounts, aggregateCourseCounts, getCsrfToken, + useLoginToCurrent, } diff --git a/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx b/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx index 1aba2ec935..aa9e339c6e 100644 --- a/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx +++ b/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx @@ -4,7 +4,7 @@ import React from "react" import { ForbiddenError } from "@/common/errors" import { Permission, useUserMe } from "api/hooks/user" import { redirect } from "next/navigation" -import * as urls from "@/common/urls" +import { useLoginToCurrent } from "@/common/utils" type RestrictedRouteProps = { children?: React.ReactNode @@ -42,10 +42,9 @@ const RestrictedRoute: React.FC = ({ requires, }) => { const { isLoading, data: user } = useUserMe() + const loginUrl = useLoginToCurrent() if (isLoading) return null if (!user?.is_authenticated) { - // Redirect unauthenticated users to login - const loginUrl = urls.login() redirect(loginUrl) return null } diff --git a/frontends/main/src/page-components/Header/UserMenu.tsx b/frontends/main/src/page-components/Header/UserMenu.tsx index 8d226f16d6..4bbb21bf1c 100644 --- a/frontends/main/src/page-components/Header/UserMenu.tsx +++ b/frontends/main/src/page-components/Header/UserMenu.tsx @@ -11,8 +11,8 @@ import { RiArrowDownSLine, } from "@remixicon/react" import { useUserMe, User } from "api/hooks/user" -import { usePathname, useSearchParams } from "next/navigation" import MITLogoLink from "@/components/MITLogoLink/MITLogoLink" +import { useLoginToCurrent } from "@/common/utils" const FlexContainer = styled.div({ display: "flex", @@ -125,14 +125,12 @@ type UserMenuProps = { const UserMenu: React.FC = ({ variant }) => { const [visible, setVisible] = useState(false) - const pathname = usePathname() - const searchParams = useSearchParams() + const loginUrl = useLoginToCurrent() const { isLoading, data: user } = useUserMe() if (isLoading) { return null } - const loginUrl = urls.login({ pathname, searchParams }) const items: UserMenuItem[] = [ { diff --git a/frontends/main/src/page-components/SignupPopover/SignupPopover.tsx b/frontends/main/src/page-components/SignupPopover/SignupPopover.tsx index f3ec0afbe7..0ba15d563f 100644 --- a/frontends/main/src/page-components/SignupPopover/SignupPopover.tsx +++ b/frontends/main/src/page-components/SignupPopover/SignupPopover.tsx @@ -2,8 +2,7 @@ import React from "react" import { Popover, Typography, styled } from "ol-components" import { ButtonLink } from "@mitodl/smoot-design" import type { PopoverProps } from "ol-components" -import * as urls from "@/common/urls" -import { usePathname, useSearchParams } from "next/navigation" +import { useLoginToCurrent } from "@/common/utils" const StyledPopover = styled(Popover)({ width: "300px", @@ -32,8 +31,7 @@ type SignupPopoverProps = Pick< "anchorEl" | "onClose" | "placement" > const SignupPopover: React.FC = (props) => { - const pathname = usePathname() - const searchParams = useSearchParams() + const loginUrl = useLoginToCurrent() return ( @@ -43,9 +41,7 @@ const SignupPopover: React.FC = (props) => { and follow your areas of interest.
- - Sign Up - + Sign Up
) From 82fd1a92b014cd5bd85545f31dafdf8e2d863516 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 8 Jul 2025 15:10:27 -0400 Subject: [PATCH 4/5] refetch user data on tab focus --- frontends/api/src/hooks/user/queries.ts | 5 +++++ frontends/main/src/app/getQueryClient.ts | 1 - 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/frontends/api/src/hooks/user/queries.ts b/frontends/api/src/hooks/user/queries.ts index 776d489652..9f198346d6 100644 --- a/frontends/api/src/hooks/user/queries.ts +++ b/frontends/api/src/hooks/user/queries.ts @@ -11,6 +11,11 @@ const userQueries = { queryOptions({ queryKey: userKeys.me(), queryFn: () => usersApi.usersMeRetrieve().then((res) => res.data), + /** + * Always refetch on window focus in case the user has logged in or out + * in another tab or window while this tab was inactive. + */ + refetchOnWindowFocus: "always", }), } diff --git a/frontends/main/src/app/getQueryClient.ts b/frontends/main/src/app/getQueryClient.ts index f1c33ff900..c8b28b0bd7 100644 --- a/frontends/main/src/app/getQueryClient.ts +++ b/frontends/main/src/app/getQueryClient.ts @@ -19,7 +19,6 @@ const makeQueryClient = (): QueryClient => { return new QueryClient({ defaultOptions: { queries: { - refetchOnWindowFocus: false, staleTime: Infinity, /** From c4da25fff363ce64ba9f113af78b022d653cbaf5 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Wed, 9 Jul 2025 08:30:14 -0400 Subject: [PATCH 5/5] refactor login url more; fix tests --- .../src/app-pages/ErrorPage/ForbiddenPage.tsx | 8 ++- frontends/main/src/common/utils.ts | 32 +++++++++--- .../RestrictedRoute/RestrictedRoute.test.tsx | 49 +++++++++++-------- .../RestrictedRoute/RestrictedRoute.tsx | 31 ++++++++---- frontends/main/src/test-utils/index.tsx | 39 +++++++++++++++ 5 files changed, 116 insertions(+), 43 deletions(-) diff --git a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx index 4307933b51..ab2f834b35 100644 --- a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx +++ b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx @@ -1,9 +1,8 @@ import React, { useEffect } from "react" import ErrorPageTemplate from "./ErrorPageTemplate" import { userQueries } from "api/hooks/user" -import { redirect } from "next/navigation" import { useQuery } from "@tanstack/react-query" -import { useLoginToCurrent } from "@/common/utils" +import { redirectLoginToCurrent } from "@/common/utils" const ForbiddenPage: React.FC = () => { const user = useQuery({ @@ -14,12 +13,11 @@ const ForbiddenPage: React.FC = () => { const shouldRedirect = user.isFetchedAfterMount && !user.data?.is_authenticated - const loginUrl = useLoginToCurrent() useEffect(() => { if (shouldRedirect) { - redirect(loginUrl) + redirectLoginToCurrent() } - }, [shouldRedirect, loginUrl]) + }, [shouldRedirect]) return ( { const params: Record = {} @@ -53,13 +53,12 @@ const getCsrfToken = () => { const useLoginToCurrent = () => { /** - * NOTE: In contrast to, say - * login({ - * pathname: window.location.pathname, - * searchParams: new URLSearchParams(window.location.search) - * }), - * the version here is reactive: when pathname/searchParams change, the values - * here update automatically and will (appropriately) trigger a re-render. + * NOTES: + * 1. This is reactive; if current URL changes, the result of this hook + * will update and trigger re-renders. This makes it suitable for use as + * an href. + * 2. However, the use of search params / pathname hooks may require Suspense + * or NextJS's dynamic rendering. * */ const pathname = usePathname() @@ -67,10 +66,27 @@ const useLoginToCurrent = () => { return login({ pathname, searchParams }) } +/** + * Redirect user to login route with ?next=. + */ +const redirectLoginToCurrent = (): never => { + redirect( + /** + * Calculating the ?next= via window.location is appropriate + * here since it happens time of redirect call. + */ + login({ + pathname: window.location.pathname, + searchParams: new URLSearchParams(window.location.search), + }), + ) +} + export { getSearchParamMap, aggregateProgramCounts, aggregateCourseCounts, getCsrfToken, useLoginToCurrent, + redirectLoginToCurrent, } diff --git a/frontends/main/src/components/RestrictedRoute/RestrictedRoute.test.tsx b/frontends/main/src/components/RestrictedRoute/RestrictedRoute.test.tsx index f48c239c05..338ee3dd85 100644 --- a/frontends/main/src/components/RestrictedRoute/RestrictedRoute.test.tsx +++ b/frontends/main/src/components/RestrictedRoute/RestrictedRoute.test.tsx @@ -1,42 +1,51 @@ import React from "react" -import { renderWithProviders, screen } from "../../test-utils" +import { + renderWithProviders, + screen, + TestingErrorBoundary, + waitFor, +} from "../../test-utils" import RestrictedRoute from "./RestrictedRoute" import { Permission } from "api/hooks/user" import { allowConsoleErrors } from "ol-test-utilities" +import { setMockResponse, urls, factories } from "api/test-utils" +import { ForbiddenError } from "@/common/errors" -test("Renders children if permission check satisfied", () => { +const makeUser = factories.user.user + +test("Renders children if permission check satisfied", async () => { const errors: unknown[] = [] + setMockResponse.get(urls.userMe.get(), makeUser({ is_authenticated: true })) renderWithProviders( Hello, world! , - - { - user: { [Permission.Authenticated]: true }, - }, ) - screen.getByText("Hello, world!") + await screen.findByText("Hello, world!") expect(!errors.length).toBe(true) }) -test.each(Object.values(Permission))( +test.each( + Object.values(Permission).filter((p) => p !== Permission.Authenticated), +)( "Throws error if and only if lacking required permission", async (permission) => { - // if a user is not authenticated they are redirected to login before an error is thrown - if (permission === Permission.Authenticated) { - return - } allowConsoleErrors() + setMockResponse.get(urls.userMe.get(), makeUser({ [permission]: false })) + + const onError = jest.fn() + renderWithProviders( + + Hello, world! + , + ) - expect(() => - renderWithProviders( - Hello, world!, - { - user: { [permission]: false }, - }, - ), - ).toThrow("Not allowed.") + await waitFor(() => { + expect(onError).toHaveBeenCalled() + }) + const error = onError.mock.calls[0][0] + expect(error).toEqual(new ForbiddenError("Not allowed.")) }, ) diff --git a/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx b/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx index aa9e339c6e..4d2694c961 100644 --- a/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx +++ b/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx @@ -1,10 +1,10 @@ "use client" -import React from "react" +import React, { useEffect } from "react" import { ForbiddenError } from "@/common/errors" -import { Permission, useUserMe } from "api/hooks/user" -import { redirect } from "next/navigation" -import { useLoginToCurrent } from "@/common/utils" +import { Permission, userQueries } from "api/hooks/user" +import { redirectLoginToCurrent } from "@/common/utils" +import { useQuery } from "@tanstack/react-query" type RestrictedRouteProps = { children?: React.ReactNode @@ -41,13 +41,24 @@ const RestrictedRoute: React.FC = ({ children, requires, }) => { - const { isLoading, data: user } = useUserMe() - const loginUrl = useLoginToCurrent() + const { isLoading, data: user } = useQuery({ + ...userQueries.me(), + staleTime: 0, // Force refetch on mount + }) + const shouldRedirect = !isLoading && !user?.is_authenticated + useEffect(() => { + /** + * Note: If user data exists in query cache, user might see content + * while refetching fresh user data to verify auth. + * This is optimistic: since the cached data will almost always be valid + * and any "secret" data is gated via API auth checks anyway. + */ + if (shouldRedirect) { + redirectLoginToCurrent() + } + }, [shouldRedirect]) if (isLoading) return null - if (!user?.is_authenticated) { - redirect(loginUrl) - return null - } + if (shouldRedirect) return null if (!isLoading && !user?.[requires]) { // This error should be caught by an [`errorElement`](https://reactrouter.com/en/main/route/error-element). throw new ForbiddenError("Not allowed.") diff --git a/frontends/main/src/test-utils/index.tsx b/frontends/main/src/test-utils/index.tsx index 47643fed6d..310cb9aa72 100644 --- a/frontends/main/src/test-utils/index.tsx +++ b/frontends/main/src/test-utils/index.tsx @@ -226,6 +226,44 @@ const assertPartialMetas = (expected: Partial) => { ) } +type ErrorBoundaryProps = { + onError?: (error: unknown) => void + children?: React.ReactNode +} +type ErrorBoundaryState = { hasError: boolean } +/** + * Useful in rare circumstances to test an error throw during subsequent + * renders: + * + * const Fallback = jest.fn() + * renderWithProviders( + * }> + * + * + * ) + */ +class TestingErrorBoundary extends React.Component< + ErrorBoundaryProps, + ErrorBoundaryState +> { + constructor(props: ErrorBoundaryProps) { + super(props) + this.state = { hasError: false } + } + + static getDerivedStateFromError() { + return { hasError: true } + } + + componentDidCatch(error: Error): void { + this.props.onError?.(error) + } + + render() { + return this.state.hasError ? null : this.props.children + } +} + export { renderWithProviders, renderWithTheme, @@ -235,6 +273,7 @@ export { ignoreError, getMetas, assertPartialMetas, + TestingErrorBoundary, } // Conveniences export { setMockResponse }