-
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
Added getAllMembers Endpoint #9
Changes from all commits
029cd2f
a5940ba
f3df40b
da9c7f5
f07fcca
54e9192
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
export enum Status { | ||
MEMBER = 'MEMBER', | ||
RECRUITER = 'RECRUITER', | ||
ADMIN = 'ADMIN', | ||
ALUMNI = 'ALUMNI', | ||
APPLICANT = 'APPLICANT', | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,12 @@ | ||
import { Entity, Column, PrimaryGeneratedColumn } from 'typeorm'; | ||
|
||
import { Entity, Column, ObjectIdColumn, ObjectId } from 'typeorm'; | ||
import { Status } from './types'; | ||
@Entity() | ||
export class User { | ||
@PrimaryGeneratedColumn() | ||
id: number; | ||
@ObjectIdColumn() // https://github.com/typeorm/typeorm/issues/1584 | ||
userId: ObjectId; | ||
|
||
@Column() | ||
status: Status; | ||
|
||
@Column() | ||
firstName: string; | ||
|
@@ -13,4 +16,19 @@ export class User { | |
|
||
@Column() | ||
email: string; | ||
|
||
@Column() | ||
profilePicture: string; | ||
|
||
@Column() | ||
linkedin: string | null; | ||
|
||
@Column() | ||
github: string | null; | ||
|
||
@Column() | ||
team: string | null; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️ Can we define enums for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, we can also define enums for these fields. To account for people with multiple roles, would we want to change the data type to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, that sounds good! |
||
@Column() | ||
role: string | null; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,10 @@ | ||
import { Controller, Get } from '@nestjs/common'; | ||
import { | ||
DefaultValuePipe, | ||
ParseBoolPipe, | ||
Query, | ||
Controller, | ||
Get, | ||
} from '@nestjs/common'; | ||
|
||
import { UsersService } from './users.service'; | ||
|
||
|
@@ -7,7 +13,10 @@ export class UsersController { | |
constructor(private readonly usersService: UsersService) {} | ||
|
||
@Get() | ||
getAllUsers() { | ||
return this.usersService.findAll(); | ||
getAllMembers( | ||
@Query('getAllMembers', new DefaultValuePipe(false), ParseBoolPipe) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 Just thinking out loud but I have a feeling we'll want this route to be a more generic route that can be used to retrieve users of an arbitrary list of statuses instead of having separate routes to get different subsets of users There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For sure, we can go back to this and change it to be more generic in the future (unless you want us to refactor it now) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah let's change it in a future PR for sure |
||
getAllMembers: boolean, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that breaks the endpoint when you don't pass in any query parameters (i.e., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh yep, makes sense! |
||
) { | ||
return this.usersService.findAll(getAllMembers); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,44 @@ | ||
import { Injectable } from '@nestjs/common'; | ||
import { UnauthorizedException, Injectable } from '@nestjs/common'; | ||
import { InjectRepository } from '@nestjs/typeorm'; | ||
import { Repository } from 'typeorm'; | ||
import { MongoRepository } from 'typeorm'; | ||
|
||
import { User } from './user.entity'; | ||
import { Status } from './types'; | ||
import { ObjectId } from 'mongodb'; | ||
|
||
@Injectable() | ||
export class UsersService { | ||
constructor( | ||
@InjectRepository(User) | ||
private usersRepository: Repository<User>, | ||
private usersRepository: MongoRepository<User>, | ||
) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ Is there a reason for preferring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I understand, |
||
|
||
findAll(): Promise<User[]> { | ||
return this.usersRepository.find(); | ||
async findAll(getAllMembers: boolean): Promise<User[]> { | ||
if (!getAllMembers) return []; | ||
|
||
const exampleUser: User = { | ||
userId: new ObjectId('a0f3efa0f3efa0f3efa0f3ef'), | ||
status: Status.ADMIN, | ||
firstName: 'jimmy', | ||
lastName: 'jimmy2', | ||
email: 'jimmy.jimmy2@mail.com', | ||
profilePicture: null, | ||
linkedin: null, | ||
github: null, | ||
team: null, | ||
role: null, | ||
}; | ||
|
||
if (exampleUser.status == Status.APPLICANT) { | ||
throw new UnauthorizedException(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️ Can we use console.log([1] == true) // true
console.log([2] == true) // false
console.log(![] == []) // true |
||
} | ||
|
||
const users: User[] = await this.usersRepository.find({ | ||
where: { | ||
status: { $not: { $eq: Status.APPLICANT } }, | ||
}, | ||
}); | ||
|
||
return users; | ||
} | ||
} |
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.
🔧 Can we allow
profilePicture
to benull
? We probably don't need/want the pictures of applicantsThere 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.
Yes! We can go back and change this to allow the
profilePicture
to benull
.