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

Use Google Account for Student Sign In #122

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

Conversation

CollinWoo
Copy link
Contributor

@CollinWoo CollinWoo commented Nov 18, 2022

Summary

This pull request allows students to sign in using their cornell.edu account with Google Sign-in while filling out the student survey.

  • Implemented design for new login verification page
  • Implemented google sign in for students
  • Implemented checking that student email ends in cornell.edu

Test Plan

Testing was performed by filling out surveys using the new system and verifying that they were submitted correctly (new students show up on the dashboards and can be emailed to). Additional testing was also performed using admin accounts to make sure that email sending and dashboard access still work as intended despite the new sign in method.

Screenshot 2022-11-17 at 6 58 58 PM

Screenshot 2022-11-17 at 6 59 23 PM

Notes

  • Signing in as an admin using google authentication still gives all admin privileges, but you cannot send emails properly since the email authentication requires Microsoft
  • Because of a change in the user account linking setting, getting the logged in user's email now requires a different method. Currently, this setting change is performed by changing every instance of user?.email to the new format. This is probably not good coding style and not good for future extensibility. Suggestions on how to globally change the email access without manually changing every instance would be appreciated. I also might have missed some instances so not everything might work correctly
  • The new log in verification screen is not perfectly responsive for wider/smaller screens
  • There might be a bug with the authentication because it seems that sometimes people logged in as admin cannot access the dashboard, but I am not sure how to recreate the error

Breaking Changes

  • The setting for user account linking in Firebase Authentication has been changed from "link accounts that use the same email" to "create multiple accounts for each identity provider"

@CollinWoo CollinWoo requested a review from a team as a code owner November 18, 2022 00:20
Copy link
Contributor

@jewang25 jewang25 left a comment

Choose a reason for hiding this comment

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

The updated login page looks really good! I like how your testing plan is really thorough and how the new feature makes sure the student's email is a cornell email. When I was testing the first time, it wouldn't let me go to the dashboard from the survey page for some reason. I'm not sure if it's just me or a random situation, but if you have time you could look into that. However, it's minor so good job with implementing this new feature!

@dti-github-bot
Copy link
Member

[diff-counting] Significant lines: 212.

@github-actions
Copy link

Visit the preview URL for this PR (updated for commit d47939d):

https://zing-lsc--pr122-student-sign-in-e5wa3p9s.web.app

(expires Fri, 25 Nov 2022 04:32:41 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 1fc18307b178c916e2663810a6932f60b173c01b

Copy link
Contributor

@rgu0114 rgu0114 left a comment

Choose a reason for hiding this comment

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

This looks great Colin! I like how visually pleasing the new sign in page is, as well as how you check that the emails are valid Cornell emails using the existing infrastructure.

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