-
Notifications
You must be signed in to change notification settings - Fork 0
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
Bug fixes for wi24 #185
Bug fixes for wi24 #185
Conversation
Justin Kong seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
[diff-counting] Significant lines: 7. |
Visit the preview URL for this PR (updated for commit fc9bc36): https://zing-lsc--pr185-bug-fixes-for-wi24-hei1p4s7.web.app (expires Tue, 16 Jan 2024 17:44:49 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 1fc18307b178c916e2663810a6932f60b173c01b |
@@ -112,7 +112,7 @@ export const addStudentSurveyResponse = async ( | |||
} | |||
|
|||
// 0. Check if email is valid cornell.edu email. | |||
const emailRegex = /^\[email protected]$/ | |||
const emailRegex = /^\w+@cornell\.edu$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idr how regex works oop, does this like, make sure emails end with explicitly .edu and not like .education?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably has to do with the dot matching any character other than line terminators lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok. btw, hi Changyuan!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe mention what dependency changes u made in package.lock/yarn.lock
Just added that to the pull request! Basically I think for frontend there was an update which removed quotation marks around certain parts of the file (with no meaningful dependency changes), and for the backend our packages had version upgrades. There was no addition or deletion of any packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 👍
Summary
This pull request fixes a few bugs and prepares LSC for the winter semester, as well as
cornell.edu
Test Plan
Metrics page also has winter '24 as a valid option
Now correctly identifies invalid emails
Notes
In the future we will need to make the metrics semesters update automatically.
Breaking Changes
yarn.lock
was updated when I didyarn install
, it seems to me that the changes primarily consists of a new syntax update which just removed quotation marks.package-lock.json
was also updated when I didnpm install
, due to me working off of a new machine. There were no deletions or additions, just standard upgrades.