CATcher-org / CATcher

CATcher is a software application used for peer-testing of software projects.
https://catcher-org.github.io/CATcher/
MIT License
71 stars 66 forks source link

Redirect invalid routes to 404 not found page #1238

Closed NereusWB922 closed 5 months ago

NereusWB922 commented 5 months ago

Summary:

Fixes #1171

Changes Made:

Screenshot:

image

Proposed Commit Message:

Redirect invalid routes to 404 not found page

There is an uncaught error when the application tries to redirect to invalid routes. 

Redirecting users to a 404 not found page enhances the user experience.

Let's create a PageNotFoundModule and redirect invalid routes to it.
codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (013fbfa) 53.86% compared to head (f85bde0) 53.89%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1238 +/- ## ========================================== + Coverage 53.86% 53.89% +0.03% ========================================== Files 101 101 Lines 2885 2885 Branches 534 534 ========================================== + Hits 1554 1555 +1 Misses 988 988 + Partials 343 342 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

luminousleek commented 5 months ago

Also @CATcher-org/2324s2 @damithc might need your opinion - would you prefer a 404 page or an error toaster when there's a bad link? Personally I'm leaning towards an error toaster but it's also probably more work to implement.

NereusWB922 commented 5 months ago

I just tried to directly edit the link. It seems like it will also redirect to the login page for a valid route. I think this might be because this action causes the website to reload, and the isAuthenticated state is lost. My idea is that we might want to use JWT for authentication, so every time the website reloads, it will check whether the stored JWT is valid instead of resetting the isAuthenticated state. What do you all think?

https://github.com/CATcher-org/CATcher/assets/107099783/2c1fc21e-8444-477a-9985-72f2fa4a59cc

gycgabriel commented 5 months ago

I just tried to directly edit the link. It seems like it will also redirect to the login page for a valid route. I think this might be because this action causes the website to reload, and the isAuthenticated state is lost. My idea is that we might want to use JWT for authentication, so every time the website reloads, it will check whether the stored JWT is valid instead of resetting the isAuthenticated state. What do you all think?

This digs into why CATcher doesn't have persistent login on reload. For security reasons, CATcher doesn't store a JWT on the browser/client-side because CATcher does not have a server-side db to verify the JWT.

See https://github.com/CATcher-org/CATcher/pull/923. Feel free to take up this investigation, furthering this solution or other solutions, and reach out to @chunweii if you have any questions regarding the experimental PR.

NereusWB922 commented 5 months ago

I just tried to directly edit the link. It seems like it will also redirect to the login page for a valid route. I think this might be because this action causes the website to reload, and the isAuthenticated state is lost. My idea is that we might want to use JWT for authentication, so every time the website reloads, it will check whether the stored JWT is valid instead of resetting the isAuthenticated state. What do you all think?

This digs into why CATcher doesn't have persistent login on reload. For security reasons, CATcher doesn't store a JWT on the browser/client-side because CATcher does not have a server-side db to verify the JWT.

See #923. Feel free to take up this investigation, furthering this solution or other solutions, and reach out to @chunweii if you have any questions regarding the experimental PR.

Got it. Will look into this.

chunweii commented 5 months ago

Also @CATcher-org/2324s2 @damithc might need your opinion - would you prefer a 404 page or an error toaster when there's a bad link? Personally I'm leaning towards an error toaster but it's also probably more work to implement.

I'd prefer an error toaster as well. It is unlikely for a student to enter a bad url in the browser anyways. In the issue #1171, the uncaught error happened because the user created an incorrect markdown url hyperlink. If we redirect the user to a 404 page, it would cause them inconvenience and they may be logged out too.

NereusWB922 commented 5 months ago

Also @CATcher-org/2324s2 @damithc might need your opinion - would you prefer a 404 page or an error toaster when there's a bad link? Personally I'm leaning towards an error toaster but it's also probably more work to implement.

I'd prefer an error toaster as well. It is unlikely for a student to enter a bad url in the browser anyways. In the issue #1171, the uncaught error happened because the user created an incorrect markdown url hyperlink. If we redirect the user to a 404 page, it would cause them inconvenience and they may be logged out too.

Yes, I agree with that. I will create another PR for the error toaster.

luminousleek commented 5 months ago

Also @CATcher-org/2324s2 @damithc might need your opinion - would you prefer a 404 page or an error toaster when there's a bad link? Personally I'm leaning towards an error toaster but it's also probably more work to implement.

I'd prefer an error toaster as well. It is unlikely for a student to enter a bad url in the browser anyways. In the issue #1171, the uncaught error happened because the user created an incorrect markdown url hyperlink. If we redirect the user to a 404 page, it would cause them inconvenience and they may be logged out too.

Yes, I agree with that. I will create another PR for the error toaster.

Ok please then close this PR then, thanks!