Skip to content
24 changes: 24 additions & 0 deletions apps/backend/src/users/dtos/updateUserInfo.dto.ts
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 {
Copy link
Collaborator

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.ts and the class UpdateUserInfoDto?

@IsOptional()
@IsString()
@IsNotEmpty()
@MaxLength(255)
firstName?: string;

@IsOptional()
@IsString()
@IsNotEmpty()
@MaxLength(255)
lastName?: string;

@IsOptional()
@IsString()
@IsNotEmpty()
@IsPhoneNumber('US', {
message:
'phone must be a valid phone number (make sure all the digits are correct)',
})
phone?: string;
}
22 changes: 17 additions & 5 deletions apps/backend/src/users/user.entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,25 @@ export class User {
})
role: Role;

@Column()
@Column({
type: 'varchar',
name: 'first_name',
length: 255,
})
firstName: string;

@Column()

@Column({
type: 'varchar',
name: 'last_name',
length: 255,
})
lastName: string;

@Column()

@Column({
type: 'varchar',
name: 'email',
length: 255,
})
email: string;

@Column({
Expand Down
38 changes: 24 additions & 14 deletions apps/backend/src/users/users.controller.spec.ts
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>();
Expand Down Expand Up @@ -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 () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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> = {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need the Partial since all the DTO fields are optional anyway

const result = await controller.updateInfo(1, updateUserSchema);

expect(result).toEqual(mockUser1);
expect(mockUserService.update).toHaveBeenCalledWith(1, updateUserSchema);
});
});

Expand Down
16 changes: 6 additions & 10 deletions apps/backend/src/users/users.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -43,15 +42,12 @@ export class UsersController {
return this.usersService.remove(userId);
}

@Put('/:id/role')
async updateRole(
@Patch(':id/info')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the /info here since we are patching the user object itself

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('/')
Expand Down
114 changes: 110 additions & 4 deletions apps/backend/src/users/users.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -224,4 +227,107 @@ describe('UsersService', () => {
expect(result).toEqual([]);
});
});

describe('update', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please correct me if I'm wrong, but it looks like you added these new service tests this week? If so, can we write them according to the new style where we use the real testing database rather than mocking TypeORM? (You can use order.service.spec.ts as an example, but also see the change Dalton made here)
  2. Can we combine the update tests earlier in the file into these?

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);

Copy link

@dburkhart07 dburkhart07 Feb 21, 2026

Choose a reason for hiding this comment

The 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();
});
});
});
21 changes: 16 additions & 5 deletions apps/backend/src/users/users.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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);
}
Expand Down