-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add names to retroactive attendance form #407
Conversation
Thanks for contributing! |
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.
Just a small nit. You'll need to fix your tests and run the linter before being able to merge though, make sure those pass before opening a PR
Nit: Since you will be returning an array of strings that concatenate names and emails, it would be cleaner and more correct to define a new type for such strings. Maybe make the field name "namesAndEmails" or something. |
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.
lgtm. please bump package version before merging
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.
sorry I have one quick but important nit
This is the final version I would like to submit for the PR. Let me know if any more changes need to be made! |
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.
One quick question
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.
See my other comment
…into add-name-to-getAllEmails merging from master.
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.
@ryanDing26 looks good to me, just threw in a return type and did some linting
Info
Closes #322.
Description
What changes did you make? List all distinct problems that this PR addresses. Explain any relevant
motivation or context.
Faris wanted to change the functionality on the retroactive attendance form (within the admin dashboard) so that it displayed names in the format
firstName lastName (email)
so that it would be easier to autocomplete manual attendance by name instead of asking people for their email for points.Changes
getAllEmails
in UserRepository.ts such that it built a query with the firstName and lastName of a user from the UserModel object alongside the existing email selectiongetAllEmails
in UserRepository.ts such that instead of just mapping all the emails, it would mapfirstName lastName (email)
Type of Change
expected)
workflows, linting, etc.)
If you've selected Patch, Minor, or Major as your change type, make sure to bump the version before merging in
package.json
!Testing
I have tested that my changes fully resolve the linked issue ...
Checklist
package.json
file.Screenshots
Please include a screenshot of your Postman testing passing successfully.