-
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
Update getUser and create getApplication endpoint #15
Conversation
…ty/c4c-ops into AR/getApplication
@Column() | ||
@IsDateString() | ||
createdAt: Date; | ||
|
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.
❓ I noticed that the application schema you posted has a lastUpdatedAt
field. I'm assuming that the lastUpdatedAt
field is now obsolete/no longer necessary, and the current application entity I'm looking at is the most recent application schema?
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.
Yup, the lastUpdatedAt
field isn't needed anymore bc we made the decision to not allow applicants to update their app after they submit it (which is what we do atm)
} | ||
|
||
const applicant = await this.usersService.findOne(userId); | ||
const application = applicant.applications[0]; |
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.
❓ Are we assuming that the application at index 0 will always be the latest application for a particular user? We should probably write something more robust so that we don't have to remember this assumption in the long-term.
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.
Hmm yeah we do something like this for SFTT and it's been fine so I don't think it's too big of an issue but any given user won't have more than a couple of apps (so doing more complex operations for this won't affect its runtime to a noticeable extent) and this is a common operation so I think this is a good idea! I did change the logic so that it actually gets the app for the current cycle instead of the latest app because we shouldn't need to look at apps for past cycles often. Maybe in a future iteration we can modify this endpoint so that it takes in a cycle year and semester so that we can also get an app for an arbitrary cycle
@@ -0,0 +1,54 @@ | |||
import { |
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.
❓ What is the purpose of this dto?
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.
The purpose of this dto is to define the response schema for the getUser
endpoint, which is basically the user entity minus the applications
field (since we have a separate endpoint to get the application for an applicant and the getUser
endpoint shouldn't return a user's applications). I checked the code now tho and it looks like the getUser
endpoint actually returns a user of type User
and not GetUserResponse
so we'll change our code to reflect this!
Merging this to fix the errors in main for now |
ℹ️ Issue
Closes #15
📝 Description
I updated the getUser endpoint to include a user application - this will recruiters and admins to pull up information submitted as part of an application for the current recruitment cycle. While adding this field, I also had to add the application entity, which includes information from this application schema:
Briefly list the changes made to the code:
✔️ Verification
To verify, I used Postman to check the routes (http://localhost:3000/api/users/) and tested different userIds from MongoDB Compass database collections.