Skip to content
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

Merged
merged 19 commits into from
Oct 31, 2023

Conversation

ananyar807
Copy link

@ananyar807 ananyar807 commented Oct 18, 2023

ℹ️ 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:

{
  userId: number,
  createdAt: Date,
  lastUpdatedAt: Date,
  cycleSemester: enum Semester('Fall', 'Spring'),
  cycleYear: number,
  status: enum ApplicationStatus('Submitted', 'Reviewed', 'Accepted', 'Rejected'),
  application: Response[],
  notes: Note[],
}

Briefly list the changes made to the code:

  1. Added application entity
  2. Updated user entity to include an array of applications (in case an applicant resubmits)
  3. Created Response and Note types
  4. Refactored the getUser endpoint so that admin & recruiter can access all users, alumni and member can access all except for applicants, and applicants can only access themselves. If the targetUser is not an applicant- their application is empty.

✔️ Verification

To verify, I used Postman to check the routes (http://localhost:3000/api/users/) and tested different userIds from MongoDB Compass database collections.
Screenshot 2023-10-23 at 10 46 38 AM

@ananyar807 ananyar807 marked this pull request as ready for review October 20, 2023 18:26
@chromium-52 chromium-52 changed the title Ar/get application Create getApplication endpoint Oct 23, 2023
@chromium-52 chromium-52 changed the title Create getApplication endpoint Update getUser and Create getApplication endpoint Oct 23, 2023
@chromium-52 chromium-52 changed the title Update getUser and Create getApplication endpoint Update getUser and create getApplication endpoint Oct 23, 2023
@ananyar807 ananyar807 requested review from kimharr24 and ojn03 October 23, 2023 14:47
@Column()
@IsDateString()
createdAt: Date;

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?

Copy link
Member

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)

apps/backend/src/applications/applications.service.ts Outdated Show resolved Hide resolved
}

const applicant = await this.usersService.findOne(userId);
const application = applicant.applications[0];

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.

Copy link
Member

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

apps/backend/src/applications/applications.service.ts Outdated Show resolved Hide resolved
apps/backend/src/applications/applications.service.ts Outdated Show resolved Hide resolved
apps/backend/src/applications/utils.ts Show resolved Hide resolved
apps/backend/src/applications/utils.ts Show resolved Hide resolved
@@ -0,0 +1,54 @@
import {

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?

Copy link
Member

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!

@chromium-52 chromium-52 changed the base branch from AR/getUser to main October 25, 2023 01:00
@chromium-52 chromium-52 added the backend Backend label Oct 30, 2023
@chromium-52
Copy link
Member

Merging this to fix the errors in main for now

@chromium-52 chromium-52 merged commit b3303c7 into main Oct 31, 2023
4 checks passed
@chromium-52 chromium-52 deleted the AR/getApplication branch October 31, 2023 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants