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

Anonymous warning modal #458

Merged
merged 15 commits into from
Oct 24, 2024
Merged

Anonymous warning modal #458

merged 15 commits into from
Oct 24, 2024

Conversation

qiandrewj
Copy link
Contributor

@qiandrewj qiandrewj commented Sep 27, 2024

Summary

This PR works on the anonymous warning feature, and fixes a bug with the admin management modal. @leihelen did most of the heavy lifting on implementing the anonymous modal on the front-end, I mainly finished integrating the feature and adding the backend functionality we want.

PR Type

  • 🍕 Feature
  • 🐛 Bug Fix

Mobile + Desktop Screenshots & Recordings

logged out logged in

QA - Test Plan

I tested the functionality in three cases:

  • When the user is logged out, the anonymous modal is meant to pop up to let them know to login with their Cornell net-id. After logging in, their review is submitted and they get redirected to the profile page
  • When the user is logged in and they are "new user" (we define this as someone who has never reviewed before), they should get the anonymous warning on their first review. This reminds them that we do not publish their name on the review and allows them to submit
  • When the user is logged in but they are not a new user, they should be able to submit a review as before

Breaking Changes & Notes

  • Initially, we took local session data that was stored in the site to submit the review when the user clicks the button on the Course page. Now, this functionality occurs in the Profile page since we redirect the user there while also properly sending the review to the database and displaying a success message. There is some redundant code that might be better moved into a separate file and exported. Note: some endpoints used will need to be updated if the endpoint re-naming branch gets merged
  • Previously, we had an issue in staging with duplicate profiles being created when we attempted to update admin privileges through the admin management modal. To resolve this, I removed the ability to create a user through the modal directly. Now, it will only be able to grant an existing user admin privilege, so if one of our team members wants it they must first login with Google -- I think it shouldn't be too big of a deal.

Added to documentation?

  • 🍕 Documentation for new methods

What GIF represents this PR?

gif

@qiandrewj qiandrewj requested a review from a team as a code owner September 27, 2024 04:09
@dti-github-bot
Copy link
Member

dti-github-bot commented Sep 27, 2024

[diff-counting] Significant lines: 268.

Copy link
Contributor

@jacquelinecai jacquelinecai left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this feature! The UI looks super clean and I can't wait to see it on the deployed website! I added some comments of some bugs/fixes I found through my testing :)

Login
</button>
<div className={styles.pastReviewsMessage}>
You will be redirected to your past reviews page
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is a bug on my end, but after redirecting to the profile, the submitted reviews don't show up in the main panel (but the number is updated on the left side):
IMG_7752

client/src/modules/Course/Components/AnonymousWarning.tsx Outdated Show resolved Hide resolved
client/src/modules/Course/Components/ReviewModal.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@wizhaaa wizhaaa left a comment

Choose a reason for hiding this comment

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

Awesome work on this feature Andrew! I think it works pretty smoothly. Just gotta fix the merge conflicts and which endpoints the frontend is hitting.

client/src/modules/Course/Components/AnonymousWarning.tsx Outdated Show resolved Hide resolved
@qiandrewj qiandrewj merged commit 42ca524 into main Oct 24, 2024
4 checks passed
@qiandrewj qiandrewj deleted the aq/anonymous branch October 24, 2024 01:57
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.

4 participants