-
Couldn't load subscription status.
- Fork 2
Updated coffee chat suggestions script #1068
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
base: main
Are you sure you want to change the base?
Conversation
|
|
|
[diff-counting] Significant lines: 140. |
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.
Everything looks good Adrienne, just fixing the formatting of some lines is needed!
|
|
||
| require('dotenv').config(); | ||
|
|
||
| import { configureAccount } from '../src/utils/firebase-utils'; |
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.
Would move this to the top of the file along with the other imports.
|
|
||
| import { configureAccount } from '../src/utils/firebase-utils'; | ||
|
|
||
| //const serviceAcc = require('../resources/cornelldti-idol-firebase-adminsdk-ifi28-9aaca97159.json'); |
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.
For comments, make sure to add a space after '//'
| .map((mem) => ({ name: `${mem.firstName} ${mem.lastName}`, netid: mem.netid })); | ||
|
|
||
| const getMembersByCategory = async () => { | ||
| //update csv path to current suggestions |
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.
Same suggestion here with the comments .
| const main = async () => { | ||
| const membersByCategory = await getMembersByCategory(); | ||
|
|
||
| const members = await memberPromise; |
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.
This looks really great Adrienne! I did notice that you fetch members both here and in the getMembersByCategory function (line 45). I would suggest passing members as a parameter instead to avoid redundant fetching. Otherwise, great job!
Summary
Linear
coffee chat suggestions
Test plan
Note
I wanted to make a pr to merge the script since we would need to use it every semester (originally from Chris's branch).
Also right now, we need to manually check for members who make multiple submissions to the google form or submit their name in lowercase (generally just validate the data). Do we want to handle those cases in the script? Or since it's not too much work it's fine?