forked from Code-4-Community/scaffolding
-
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
feat: getAllApplications #50
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kimharr24
reviewed
Feb 23, 2024
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.
Nice work! Added a few comments addressing your TODOs.
- For testing, you can sign up using your email and sign in, at which point your user information should be stored as a row in the Users table. You can directly modify your status field to be an Admin/Recruiter in pgAdmin for testing this endpoint. You can also add applications to the Applications table directly in pgAdmin.
- Yes
- Not sure, generally keeping fields private is good practice - I think @chromium-52 made this design decision
- The number of times an applicant has applied in the past
kimharr24
changed the title
feat: getAllAplications endpoint resolves #44
feat: getAllApplications
Mar 17, 2024
This was
linked to
issues
Mar 17, 2024
Closed
chromium-52
reviewed
Mar 19, 2024
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 some quick comments but lgtm otherwise!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ℹ️ Issue
Closes #44 #59
📝 Description
Added GET endpoint at route "api/apps/"
If calling user is not a recruiter or admin, return 401 unauthurized. otherwise returns all applications of current cycle
response is an array of GetApplicationResponseDTO:
[
GetApplicationResponseDTO,
GetApplicationResponseDTO,
...
]
this is a bit different from what was specified in the ticket:
{
"applications": GetApplicationResponseDTO[]
}
because I believe it falls more in line with the existing endpoints and type definitions. If necessary I can change it to follow the ticket more exactly.
Comments/Questions:
I was unsure how to test this with postman. Im not sure how to emulate a recruiter/admin/other role as the calling user through postman. How would I do so?
Does req.user.status hold the calling users role (admin, Member, recruiter, etc)? I assumed so by looking at other endpoints, but Im not certain
Is there a reason the fields in the Cycle class were made private? I had to make them public to make this work
What is the numapps parameter in the application.toGetApplicationResponseDTO() function supposed to represent?