Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix page redirects across all pages #515

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

victoriavo256
Copy link

Checklist:

Co-authored by: Dylan Uribe [email protected]
Co-authored by: Ann Pan [email protected]

Thank you to SenthilKumar Ilango [email protected] for mentoring us.

Resolved Issue #497 and refactored all page redirects. Redirects were previously handled with ctx.writeHead, but we replaced it with our method called redirectUser which uses the recommended page redirect instruction provided by next.js. (https://nextjs.org/docs/pages/api-reference/functions/get-server-side-props#redirect) Tested changes locally.

Closes #497

victoriavo256 and others added 5 commits December 1, 2023 14:58
Co-authored by: Dylan Uribe <[email protected]>
Co-authored by: Ann Pan <[email protected]>
Co-authored-by: Dylan Uribe <[email protected]>
Co-authored-by: Ann Pan <[email protected]>
Co-authored-by: Dylan Uribe  <[email protected]>
Co-authored-by: Ann Pan <[email protected]>
@victoriavo256 victoriavo256 requested a review from a team as a code owner December 2, 2023 00:20
Copy link

@senthil1288 senthil1288 left a comment

Choose a reason for hiding this comment

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

Good Work!! Looks like you guys manually tested these changes, hope there was a UI test framework in place to test this. Nevertheless, the code and documentation is pretty clean!!

};
};

export default redirectUser;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested the PR by doing the following (all checks resulted as expected):

  1. Initially signing in as user ‘NONE’ which is the default user when first signing on to the app -> this correctly showed the ‘Access Denied’ UI as intended
Screenshot 2024-02-10 at 6 33 54 PM
  1. Initially signing in as user ‘STUDENT’ -> this correctly showed the ‘Access Denied’ UI as intended
Screenshot 2024-02-10 at 6 33 54 PM
  1. Initially signing in as user ‘TEACHER’ -> this correctly shows the class creation UI as intended
Screenshot 2024-02-10 at 6 33 02 PM
  1. Initially signing in as user ‘ADMIN’ -> this correctly displays the admin dashboard
Screenshot 2024-02-10 at 6 38 55 PM

Edge case to consider:
When updating the user privileges in Prisma, the UI does not update immediately as the link is still set to https://{CODESPACES}.app.github.dev/error. This could mean that logic may need to be added which would update the URL (‘error’, ‘admin’, ‘teacher’, ‘student’) once a user privilege is updated.

Mentioning @utsab for further view.

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.

Unauthorized access to the /classes page should show a more descriptive error message
3 participants