Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ const learningResourceBaseDepartment: Factory<
return {
department_id: uniqueEnforcerWords.enforce(() => faker.lorem.words()),
name: uniqueEnforcerWords.enforce(() => faker.lorem.words()),
channel_url: faker.internet.url(),
channel_url: `${faker.internet.url({ appendSlash: false })}${faker.system.directoryPath()}`,
...overrides,
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,10 @@ describe("Channel Pages, Topic only", () => {
name: (name) => subTopics?.results.map((t) => t.name).includes(name),
})
links.forEach((link, i) => {
expect(link).toHaveAttribute("href", subTopics.results[i].channel_url)
expect(link).toHaveAttribute(
"href",
new URL(subTopics.results[i].channel_url!).pathname,
)
})
})
})
Expand Down
2 changes: 1 addition & 1 deletion frontends/main/src/app-pages/ChannelPage/ChannelPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ const SubTopicsDisplay: React.FC<SubTopicDisplayProps> = (props) => {
size="large"
variant="outlinedWhite"
key={topic.id}
href={topic.channel_url}
href={new URL(topic.channel_url).pathname}
label={topic.name}
/>
))}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,6 @@ describe("ChannelSearch", () => {
setMockResponse.get(urls.userMe.get(), {})

const initialSearch = "?q=meow&page=2"
const finalSearch = "?q=woof"

const { location } = renderWithProviders(<ChannelPage />, {
url: `/c/${channel.channel_type}/${channel.name}${initialSearch}`,
Expand All @@ -313,8 +312,8 @@ describe("ChannelSearch", () => {
expect(queryInput.value).toBe("meow")
await user.clear(queryInput)
await user.paste("woof")
expect(location.current.search).toBe(initialSearch)
expect(location.current.searchParams.get("q")).toBe("meow")
await user.click(screen.getByRole("button", { name: "Search" }))
expect(location.current.search).toBe(finalSearch)
expect(location.current.searchParams.get("q")).toBe("woof")
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ describe("DepartmentListingPage", () => {
name: (name) => name.includes(dept.name),
})

expect(link).toHaveAttribute("href", dept.channel_url)
expect(link).toHaveAttribute("href", new URL(dept.channel_url!).pathname)
})

test("headings", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,12 @@ const SchoolDepartments: React.FC<SchoolDepartmentProps> = ({
if (!department.channel_url) return null
return (
<ListItem disablePadding key={department.department_id}>
<DepartmentLink href={department.channel_url}>
<DepartmentLink
href={
department.channel_url &&
new URL(department.channel_url).pathname
}
>
<ListItemText
primary={department.name}
secondary={counts
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,10 @@ const BrowseTopicsSection: React.FC = () => {
{topics?.results.map(
({ id, name, channel_url: channelUrl, icon }) => {
return (
<TopicBox key={id} href={new URL(channelUrl!).pathname}>
<TopicBox
key={id}
href={channelUrl ? new URL(channelUrl!).pathname : ""}
>
<TopicBoxContent>
<RootTopicIcon icon={icon} />
<TopicBoxName>{name}</TopicBoxName>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,6 @@ const OnboardingPage: React.FC = () => {
}
values={formik.values.goals}
onChange={(event) => {
console.log(event)
formik.handleChange(event)
}}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ describe("SearchPage", () => {
},
},
})

const { location } = renderWithProviders(<SearchPage />, {
url: "?topic=Physics&topic=Chemistry",
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,17 @@ describe("TopicsListingPage", () => {
expect(subtopics1[1]).toHaveTextContent(sortedSubtopics1[0].name)
expect(subtopics1[2]).toHaveTextContent(sortedSubtopics1[1].name)
expect(subtopics1[3]).toHaveTextContent(sortedSubtopics1[2].name)
expect(subtopics1.map((el) => el.href)).toEqual([
topics.t1.channel_url,
...sortedSubtopics1.map((t) => t.channel_url),
expect(subtopics1.map((el) => new URL(el.href).pathname)).toEqual([
new URL(topics.t1.channel_url!).pathname,
...sortedSubtopics1.map((t) => new URL(t.channel_url!).pathname),
])

expect(subtopics2[0]).toHaveTextContent(topics.t2.name)
expect(subtopics2[1]).toHaveTextContent(sortedSubtopics2[0].name)
expect(subtopics2[2]).toHaveTextContent(sortedSubtopics2[1].name)
expect(subtopics2.map((el) => el.href)).toEqual([
topics.t2.channel_url,
...sortedSubtopics2.map((t) => t.channel_url),
expect(subtopics2.map((el) => new URL(el.href).pathname)).toEqual([
new URL(topics.t2.channel_url!).pathname,
...sortedSubtopics2.map((t) => new URL(t.channel_url!).pathname),
])
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ const TopicBox = ({
{ label: "Programs", count: programCount },
].filter((item) => item.count)
const { title, href, icon, channels } = topicGroup

return (
<li className={className}>
<TopicBoxHeader title={title} href={href} icon={icon} />
Expand All @@ -149,7 +150,7 @@ const TopicBox = ({
size="large"
variant="outlinedWhite"
key={c.id}
href={c.channel_url}
href={c.channel_url && new URL(c.channel_url).pathname}
label={c.name}
/>
))}
Expand All @@ -160,7 +161,7 @@ const TopicBox = ({
size="medium"
variant="outlinedWhite"
key={c.id}
href={c.channel_url}
href={c.channel_url && new URL(c.channel_url).pathname}
label={c.name}
/>
))}
Expand Down
2 changes: 1 addition & 1 deletion frontends/main/src/app-pages/UnitsListingPage/UnitCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ const UnitCard: React.FC<UnitCardProps> = (props) => {
return channelDetailQuery.isLoading ? (
<UnitCardLoading />
) : (
<CardStyled href={new URL(unitUrl!).pathname}>
<CardStyled href={unitUrl && new URL(unitUrl!).pathname}>
<Card.Content>
<UnitCardContainer>
<UnitCardContent>
Expand Down
10 changes: 2 additions & 8 deletions frontends/main/src/components/ErrorBoundary/ErrorBoundary.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
"use client"

import React, { Component } from "react"
import NotFoundPage from "@/app-pages/ErrorPage/NotFoundPage"
import ForbiddenPage from "@/app-pages/ErrorPage/ForbiddenPage"
import { ForbiddenError } from "@/common/permissions"
import { usePathname } from "next/navigation"
Expand Down Expand Up @@ -57,14 +56,9 @@ class ErrorBoundaryHandler extends Component<
}

render() {
if (this.state.hasError) {
if (isForbiddenError(this.state.error)) {
return <ForbiddenPage />
} else {
return <NotFoundPage />
}
if (this.state.hasError && isForbiddenError(this.state.error)) {
return <ForbiddenPage />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change for? If I visit http://localhost:8062/c/unit/cow now I get 500 error.

Which... apparently I also get at https://next.rc.learn.mit.edu/c/unit/cow, though the UI shows the 404 page

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened https://github.com/mitodl/hq/issues/5734 for the current 500 situation

Copy link
Contributor Author

@jonkafton jonkafton Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was that it was trapping all errors and displaying the 404 page, not the error.

We have a not-found.tsx in the app root to display the 404 page for routes not matched.

The request to a /c/unit/noexist (a matched route for a non existing channel) results in an error thrown while making the API request server side to fetch the channel details. We get a 404 from the API, though as it is not handled it produces a 500 at the browser with the error shown.

Here we need to handle and replay the 404. Errors thrown in server components or root layouts are not caught by the error boundary - this is likely fixed by adding an app/error.ts (server components) and app/global-error.ts, the widest catch-all (info).

Our current wraps children within the inner page - we should widen this for the whole page or remove if app/error.ts has the same effect.

Copied above to https://github.com/mitodl/hq/issues/5734

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's handle #5734 in a dedicated error-handling PR? Though maybe leave this as-was for now

Copy link
Contributor Author

@jonkafton jonkafton Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - it's deserving of a separate issue. We need to:

  • Reinstate 404 handling in the client component error boundary, but not for all errors (Check instance of AxiosError and status 404). This is to display a 404 if we open a not found resource drawer. I was finding this resulted in infinite retry (Is React Query retrying after errors?)

  • Add a catch all error handler for server components (app/error.ts) to render the 404 page or display the error.

}

return this.props.children
}
}
Expand Down
3 changes: 3 additions & 0 deletions frontends/main/src/page-components/Header/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ const StyledSearchButton = styled(ActionButtonLink)(({ theme }) => ({
[theme.breakpoints.down("sm")]: {
padding: "0",
},
alignItems: "center",
display: "inline-flex",
justifyContent: "center",
}))

const StyledSearchIcon = styled(RiSearch2Line)(({ theme }) => ({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
import React from "react"
import {
act,
expectProps,
renderWithProviders,
screen,
waitFor,
within,
} from "@/test-utils"
import LearningResourceDrawer, {
useOpenLearningResourceDrawer,
} from "./LearningResourceDrawer"
import LearningResourceDrawer from "./LearningResourceDrawer"
import { urls, factories, setMockResponse } from "api/test-utils"
import { LearningResourceExpanded } from "ol-components"
import { RESOURCE_DRAWER_QUERY_PARAM } from "@/common/urls"
Expand Down Expand Up @@ -77,29 +74,6 @@ describe("LearningResourceDrawer", () => {
expect(LearningResourceExpanded).not.toHaveBeenCalled()
})

test("useOpenLearningResourceDrawer sets correct parameter", () => {
let openDrawer = (_id: number): void => {
throw new Error("Not implemented")
}
const TestComponent = () => {
openDrawer = useOpenLearningResourceDrawer()
return null
}
const { location } = renderWithProviders(<TestComponent />, {
url: "?dog=woof",
})

act(() => {
openDrawer(123)
})

const params = new URLSearchParams(location.current.search)
expect(Object.fromEntries(params)).toEqual({
[RESOURCE_DRAWER_QUERY_PARAM]: "123",
dog: "woof",
})
})

test.each([
{
isLearningPathEditor: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,7 @@ import type {
RoutedDrawerProps,
} from "ol-components"
import { useLearningResourcesDetail } from "api/hooks/learningResources"
import {
useSearchParams,
useRouter,
ReadonlyURLSearchParams,
} from "next/navigation"
import { useSearchParams, ReadonlyURLSearchParams } from "next/navigation"

import { RESOURCE_DRAWER_QUERY_PARAM } from "@/common/urls"
import { useUserMe } from "api/hooks/user"
Expand Down Expand Up @@ -142,29 +138,17 @@ const getOpenDrawerSearchParams = (
return newSearchParams
}

const useOpenLearningResourceDrawer = () => {
const searchParams = useSearchParams()
const router = useRouter()

const openLearningResourceDrawer = useCallback(
(resourceId: number) => {
router.push(`?${getOpenDrawerSearchParams(searchParams, resourceId)}`)
},
[router, searchParams],
)
return openLearningResourceDrawer
}

const useResourceDrawerHref = () => {
const searchParams = useSearchParams()

return useCallback(
(resourceId: number) => {
return `?${getOpenDrawerSearchParams(searchParams, resourceId)}`
const hash = window?.location.hash
return `?${getOpenDrawerSearchParams(searchParams, resourceId)}${hash || ""}`
},
[searchParams],
)
}

export default LearningResourceDrawer
export { useOpenLearningResourceDrawer, useResourceDrawerHref }
export { useResourceDrawerHref }
12 changes: 10 additions & 2 deletions frontends/main/src/test-utils/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const renderWithProviders = (
const user = { ...defaultUser, ...allOpts.user }
queryClient.setQueryData(["userMe"], { ...user })
}

mockRouter.setCurrentUrl(url)

const view = render(
Expand All @@ -78,8 +79,15 @@ const renderWithProviders = (

const location = {
get current() {
const url = new URL(mockRouter.asPath, "http://localhost")
return { pathname: mockRouter.pathname, search: url.search }
const searchParams = new URLSearchParams(
mockRouter.query as unknown as URLSearchParams,
)
const search = searchParams.toString()
return {
pathname: mockRouter.pathname,
searchParams,
search: search ? `?${search}` : "",
}
},
}

Expand Down
Loading
Loading