Skip to content

Conversation

@abeglova
Copy link
Contributor

@abeglova abeglova commented Sep 26, 2024

What are the relevant tickets?

closes https://github.com/mitodl/hq/issues/5479

Description (What does it do?)

This pr implements error catching and permission checking on the next js app

How can this be tested?

Go to http://open.odl.local:8062/c/unit/not-a-unit. You should see a 404 Not Found Error

As a not logged in user:
http://open.odl.local:8062/dashboard
http://open.odl.local:8062/dashboard/my-lists
http://open.odl.local:8062/dashboard/my-lists/id
http://open.odl.local:8062/dashboard/profile
http://open.odl.local:8062/dashboard/settings

Should redirect you to the ssl page

As a logged in admin
http://open.odl.local:8062/dashboard/my-lists/80000000 should show a 404 Not Found Error

As a logged in non-admin
http://open.odl.local:8062/dashboard/my-lists/
http://open.odl.local:8062/dashboard/my-lists/1
Should display "Not Allowed"

@abeglova abeglova marked this pull request as ready for review September 27, 2024 21:34
Copy link
Contributor

@jonkafton jonkafton left a comment

Choose a reason for hiding this comment

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

Looks and works great. Small request - could you remove the RestrictedRoute and ErrorPage components from the frontends/mit-learn workspace as we're using anything left in there to indicate that it still needs to be migrated.

}
const isForbiddenError = (error: unknown) => error instanceof ForbiddenError

class ErrorBoundaryHandler extends Component<
Copy link
Contributor

Choose a reason for hiding this comment

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

I was interested to see a class component here and realize it's so we can use the error boundary lifecycle methods. It says here there's no equivalent for function components, https://legacy.reactjs.org/docs/hooks-faq.html#how-do-lifecycle-methods-correspond-to-hooks, though that doc is legacy... and these do appear in the up to date docs: https://react.dev/reference/react/Component#catching-rendering-errors-with-an-error-boundary (on the same page as a bunch of warnings "We recommend defining components as functions instead of classes")!

Looks like we can assume this is the recommended approach until they maybe make a hook available in future versions. The latest v19-beta does still recommend a single class component error boundary "if you’d like to avoid creating class components" - https://19.react.dev/reference/react/Component#componentdidcatch.

@abeglova abeglova merged commit 8d2a220 into nextjs Sep 30, 2024
11 checks passed
@odlbot odlbot mentioned this pull request Oct 22, 2024
74 tasks
@rhysyngsun rhysyngsun deleted the ab/nextjs-errors branch February 7, 2025 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants