-
Notifications
You must be signed in to change notification settings - Fork 0
[SSF-102] Add endpoints to update user info #79
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
base: main
Are you sure you want to change the base?
Changes from all commits
b968ae0
5830938
bd3f846
061c4a8
8a8420c
6411d03
dc91943
3f6119c
9df9b00
9a7f68f
50ffbec
4d9140d
f9ae01b
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,24 @@ | ||
| import { IsString, IsPhoneNumber, IsOptional, IsNotEmpty, MaxLength } from 'class-validator'; | ||
|
|
||
| export class updateUserInfo { | ||
| @IsOptional() | ||
| @IsString() | ||
swarkewalia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| @IsNotEmpty() | ||
| @MaxLength(255) | ||
| firstName?: string; | ||
|
|
||
| @IsOptional() | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| @MaxLength(255) | ||
| lastName?: string; | ||
|
|
||
| @IsOptional() | ||
| @IsString() | ||
swarkewalia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| @IsNotEmpty() | ||
| @IsPhoneNumber('US', { | ||
| message: | ||
| 'phone must be a valid phone number (make sure all the digits are correct)', | ||
| }) | ||
| phone?: string; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,11 @@ | ||
| import { BadRequestException } from '@nestjs/common'; | ||
| import { UsersController } from './users.controller'; | ||
| import { UsersService } from './users.service'; | ||
| import { User } from './user.entity'; | ||
| import { Role } from './types'; | ||
| import { userSchemaDto } from './dtos/userSchema.dto'; | ||
|
|
||
| import { Test, TestingModule } from '@nestjs/testing'; | ||
| import { mock } from 'jest-mock-extended'; | ||
| import { updateUserInfo } from './dtos/updateUserInfo.dto'; | ||
| import { Pantry } from '../pantries/pantries.entity'; | ||
|
|
||
| const mockUserService = mock<UsersService>(); | ||
|
|
@@ -137,24 +136,35 @@ describe('UsersController', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('PUT /:id/role', () => { | ||
| it('should update user role with valid role', async () => { | ||
| const updatedUser = { ...mockUser1, role: Role.ADMIN }; | ||
| describe('PATCH :id/info', () => { | ||
| it('should update user info with valid information', async () => { | ||
| const updatedUser = { | ||
| ...mockUser1, | ||
| firstName: 'UpdatedFirstName', | ||
| lastName: 'UpdatedLastName', | ||
| phone: '777-777-7777', | ||
| }; | ||
| mockUserService.update.mockResolvedValue(updatedUser as User); | ||
|
|
||
| const result = await controller.updateRole(1, Role.ADMIN); | ||
| const updateUserSchema: updateUserInfo = { | ||
| firstName: 'UpdatedFirstName', | ||
| lastName: 'UpdatedLastName', | ||
| phone: '777-777-7777', | ||
| }; | ||
| const result = await controller.updateInfo(1, updateUserSchema); | ||
|
|
||
| expect(result).toEqual(updatedUser); | ||
| expect(mockUserService.update).toHaveBeenCalledWith(1, { | ||
| role: Role.ADMIN, | ||
| }); | ||
| expect(mockUserService.update).toHaveBeenCalledWith(1, updateUserSchema); | ||
| }); | ||
|
|
||
| it('should throw BadRequestException for invalid role', async () => { | ||
| await expect(controller.updateRole(1, 'invalid_role')).rejects.toThrow( | ||
| BadRequestException, | ||
| ); | ||
| expect(mockUserService.update).not.toHaveBeenCalled(); | ||
| it('should update user info with defaults', async () => { | ||
|
Collaborator
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. If we're adding a test where we try to call the controller method with an empty DTO, shouldn't the expected behavior be that we throw an exception? |
||
| mockUserService.update.mockResolvedValue(mockUser1 as User); | ||
|
|
||
| const updateUserSchema: Partial<updateUserInfo> = {}; | ||
|
Collaborator
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. We shouldn't need the |
||
| const result = await controller.updateInfo(1, updateUserSchema); | ||
|
|
||
| expect(result).toEqual(mockUser1); | ||
| expect(mockUserService.update).toHaveBeenCalledWith(1, updateUserSchema); | ||
| }); | ||
| }); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,15 +4,14 @@ import { | |
| Get, | ||
| Param, | ||
| ParseIntPipe, | ||
| Put, | ||
| Post, | ||
| BadRequestException, | ||
| Body, | ||
| Patch, | ||
| } from '@nestjs/common'; | ||
| import { UsersService } from './users.service'; | ||
| import { User } from './user.entity'; | ||
| import { Role } from './types'; | ||
| import { userSchemaDto } from './dtos/userSchema.dto'; | ||
| import { updateUserInfo } from './dtos/updateUserInfo.dto'; | ||
| import { Pantry } from '../pantries/pantries.entity'; | ||
|
|
||
| @Controller('users') | ||
|
|
@@ -43,15 +42,12 @@ export class UsersController { | |
| return this.usersService.remove(userId); | ||
| } | ||
|
|
||
| @Put('/:id/role') | ||
| async updateRole( | ||
| @Patch(':id/info') | ||
|
Collaborator
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. We don't need the |
||
| async updateInfo( | ||
| @Param('id', ParseIntPipe) id: number, | ||
| @Body('role') role: string, | ||
| @Body() dto: updateUserInfo, | ||
| ): Promise<User> { | ||
| if (!Object.values(Role).includes(role as Role)) { | ||
| throw new BadRequestException('Invalid role'); | ||
| } | ||
| return this.usersService.update(id, { role: role as Role }); | ||
| return this.usersService.update(id, dto); | ||
| } | ||
|
|
||
| @Post('/') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { mock } from 'jest-mock-extended'; | |
| import { In } from 'typeorm'; | ||
| import { BadRequestException } from '@nestjs/common'; | ||
| import { PantriesService } from '../pantries/pantries.service'; | ||
| import { updateUserInfo } from './dtos/updateUserInfo.dto'; | ||
|
|
||
| const mockUserRepository = mock<Repository<User>>(); | ||
| const mockPantriesService = mock<PantriesService>(); | ||
|
|
@@ -143,16 +144,18 @@ describe('UsersService', () => { | |
|
|
||
| describe('update', () => { | ||
| it('should update user attributes', async () => { | ||
| const updateData = { firstName: 'Updated', role: Role.ADMIN }; | ||
| const updatedUser = { ...mockUser, ...updateData }; | ||
| const dto: updateUserInfo = { firstName: 'Updated' }; | ||
| const updatedUser = { ...mockUser, firstName: 'Updated' }; | ||
|
|
||
| mockUserRepository.findOneBy.mockResolvedValue(mockUser as User); | ||
| mockUserRepository.save.mockResolvedValue(updatedUser as User); | ||
|
|
||
| const result = await service.update(1, updateData); | ||
| const result = await service.update(1, dto); | ||
|
|
||
| expect(result).toEqual(updatedUser); | ||
| expect(mockUserRepository.save).toHaveBeenCalledWith(updatedUser); | ||
| expect(mockUserRepository.save).toHaveBeenCalledWith( | ||
| expect.objectContaining({ firstName: 'Updated' }), | ||
| ); | ||
| }); | ||
|
|
||
| it('should throw NotFoundException when user is not found', async () => { | ||
|
|
@@ -224,4 +227,107 @@ describe('UsersService', () => { | |
| expect(result).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('update', () => { | ||
|
Collaborator
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.
|
||
| it('should update firstName', async () => { | ||
| const dto: updateUserInfo = { firstName: 'Updated' }; | ||
| const updatedUser = { ...mockUser, firstName: 'Updated' }; | ||
|
|
||
| mockUserRepository.findOneBy.mockResolvedValue(mockUser as User); | ||
| mockUserRepository.save.mockResolvedValue(updatedUser as User); | ||
|
|
||
| const result = await service.update(1, dto); | ||
|
|
||
| expect(result.firstName).toBe('Updated'); | ||
| expect(mockUserRepository.save).toHaveBeenCalledWith( | ||
| expect.objectContaining({ firstName: 'Updated' }), | ||
| ); | ||
| }); | ||
|
|
||
| it('should update lastName', async () => { | ||
| const dto: updateUserInfo = { lastName: 'Smith' }; | ||
| const updatedUser = { ...mockUser, lastName: 'Smith' }; | ||
|
|
||
| mockUserRepository.findOneBy.mockResolvedValue(mockUser as User); | ||
| mockUserRepository.save.mockResolvedValue(updatedUser as User); | ||
|
|
||
| const result = await service.update(1, dto); | ||
|
|
||
| expect(result.lastName).toBe('Smith'); | ||
| }); | ||
|
|
||
| it('should update phone', async () => { | ||
| const dto: updateUserInfo = { phone: '0987654321' }; | ||
| const updatedUser = { ...mockUser, phone: '0987654321' }; | ||
|
|
||
| mockUserRepository.findOneBy.mockResolvedValue(mockUser as User); | ||
| mockUserRepository.save.mockResolvedValue(updatedUser as User); | ||
|
|
||
| const result = await service.update(1, dto); | ||
|
|
||
| expect(result.phone).toBe('0987654321'); | ||
| }); | ||
|
|
||
| it('should update multiple fields at once', async () => { | ||
| const dto: updateUserInfo = { firstName: 'Updated', lastName: 'Smith' }; | ||
| const updatedUser = { | ||
| ...mockUser, | ||
| firstName: 'Updated', | ||
| lastName: 'Smith', | ||
| }; | ||
|
|
||
| mockUserRepository.findOneBy.mockResolvedValue(mockUser as User); | ||
| mockUserRepository.save.mockResolvedValue(updatedUser as User); | ||
|
|
||
| const result = await service.update(1, dto); | ||
|
|
||
| expect(result.firstName).toBe('Updated'); | ||
| expect(result.lastName).toBe('Smith'); | ||
| }); | ||
|
|
||
| it('should not overwrite fields absent from the DTO', async () => { | ||
| const dto: updateUserInfo = { firstName: 'OnlyFirst' }; | ||
| const updatedUser = { ...mockUser, firstName: 'OnlyFirst' }; | ||
|
|
||
| mockUserRepository.findOneBy.mockResolvedValue(mockUser as User); | ||
| mockUserRepository.save.mockResolvedValue(updatedUser as User); | ||
|
|
||
| const result = await service.update(1, dto); | ||
|
|
||
|
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. we should check here that the updatedUser's firstName is now 'OnlyFirst' |
||
| expect(result.firstName).toBe('OnlyFirst'); | ||
| expect(result.lastName).toBe(mockUser.lastName); | ||
| expect(result.email).toBe(mockUser.email); | ||
| expect(result.phone).toBe(mockUser.phone); | ||
| expect(result.role).toBe(mockUser.role); | ||
| }); | ||
|
|
||
| it('should throw BadRequestException when DTO is empty', async () => { | ||
| const dto: updateUserInfo = {}; | ||
|
|
||
| await expect(service.update(1, dto)).rejects.toThrow( | ||
| new BadRequestException( | ||
| 'At least one field must be provided to update', | ||
| ), | ||
| ); | ||
|
|
||
| expect(mockUserRepository.findOneBy).not.toHaveBeenCalled(); | ||
| expect(mockUserRepository.save).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should throw NotFoundException when user is not found', async () => { | ||
| mockUserRepository.findOneBy.mockResolvedValue(null); | ||
|
|
||
| await expect( | ||
| service.update(999, { firstName: 'Updated' }), | ||
| ).rejects.toThrow(new NotFoundException('User 999 not found')); | ||
| }); | ||
|
|
||
| it('should throw BadRequestException for invalid id', async () => { | ||
| await expect( | ||
| service.update(-1, { firstName: 'Updated' }), | ||
| ).rejects.toThrow(new BadRequestException('Invalid User ID')); | ||
|
|
||
| expect(mockUserRepository.findOneBy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import { Role } from './types'; | |
| import { validateId } from '../utils/validation.utils'; | ||
| import { Pantry } from '../pantries/pantries.entity'; | ||
| import { PantriesService } from '../pantries/pantries.service'; | ||
| import { updateUserInfo } from './dtos/updateUserInfo.dto'; | ||
|
|
||
| @Injectable() | ||
| export class UsersService { | ||
|
|
@@ -73,16 +74,26 @@ export class UsersService { | |
| return user; | ||
| } | ||
|
|
||
| async update(id: number, attrs: Partial<User>) { | ||
| async update(id: number, dto: updateUserInfo): Promise<User> { | ||
| validateId(id, 'User'); | ||
|
|
||
| const user = await this.findOne(id); | ||
| const { firstName, lastName, phone } = dto; | ||
|
|
||
| if (!user) { | ||
|
Collaborator
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. Why was this check removed? |
||
| throw new NotFoundException(`User ${id} not found`); | ||
| if ( | ||
| firstName === undefined && | ||
| lastName === undefined && | ||
| phone === undefined | ||
| ) { | ||
| throw new BadRequestException( | ||
| 'At least one field must be provided to update', | ||
| ); | ||
| } | ||
|
|
||
| Object.assign(user, attrs); | ||
| const user = await this.findOne(id); | ||
|
|
||
| if (firstName !== undefined) user.firstName = firstName; | ||
| if (lastName !== undefined) user.lastName = lastName; | ||
| if (phone !== undefined) user.phone = phone; | ||
|
|
||
| return this.repo.save(user); | ||
| } | ||
|
|
||
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 make this more consistent with our other DTOs by naming the file
update-user-info.dto.tsand the classUpdateUserInfoDto?