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.

Sign up for GitHub

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

feat: ✨ update user module #72

Closed
wants to merge 2 commits into from
Closed

feat: ✨ update user module #72

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d6023d
feat: ✨ update user module
zhumeisongsong Nov 25, 2024
ef3f8ca
feat: ✨ Update UserDocument
zhumeisongsong Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion libs/users/application/src/lib/use-cases/get-user.use-case.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,18 @@ describe('GetUserUseCase', () => {
beforeEach(() => {
usersRepository = {
findById: jest.fn(),
findByEmail: jest.fn()
};
getUserUseCase = new GetUserUseCase(usersRepository);
});

describe('execute', () => {
it('should return user when found', async () => {
const mockUser = { id: '1', name: 'John Doe' };
const mockUser = { id: '1',
firstName: 'Test',
lastName: 'User',
email: '[email protected]'
};
(usersRepository.findById as jest.Mock).mockResolvedValue(mockUser);

const result = await getUserUseCase.execute('1');
Expand Down
8 changes: 7 additions & 1 deletion libs/users/application/src/lib/users.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,13 @@ describe('UsersService', () => {

describe('findById', () => {
it('should return a user if found', async () => {
const user: User = { id: '1', name: 'John Doe' };
const user: User = {
id: '1',
firstName: 'John',
lastName: 'Doe',
email: '[email protected]',
password: 'password123',
};
jest.spyOn(getUserUseCase, 'execute').mockResolvedValue(user);

const result = await service.findById('1');
Expand Down
4 changes: 4 additions & 0 deletions libs/users/application/src/lib/users.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,8 @@ export class UsersService {
async findById(id: string): Promise<User | null> {
return this.getUserUseCase.execute(id);
}

async findByEmail(email: string): Promise<User | null> {
return this.getUserUseCase.execute(email);
}
Comment on lines +14 to +16
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

GetUserUseCase is not compatible with email lookups

The implementation confirms that GetUserUseCase.execute() specifically uses findById() internally, making it incompatible with email-based lookups. The current implementation will fail as it passes the email to a method expecting a valid MongoDB ObjectId.

Required changes:

  • Create a dedicated GetUserByEmailUseCase that uses the repository's findByEmail method
  • Update UsersService to use the new use case for email lookups
  • Implement the missing findByEmail method in MongooseUsersRepository
🔗 Analysis chain

Review the use case implementation for email lookup

The new findByEmail method is reusing getUserUseCase.execute() which was originally designed for ID-based lookups. This could lead to incorrect behavior if the use case isn't properly handling different types of identifiers.

Consider one of these approaches:

  1. Create a dedicated use case for email lookup (preferred)
  2. Modify the existing use case to explicitly handle different identifier types

Let's verify the use case implementation:

Suggested implementation if using approach #1:

@Injectable()
export class UsersService {
  constructor(
    private readonly getUserUseCase: GetUserUseCase,
    private readonly getUserByEmailUseCase: GetUserByEmailUseCase
  ) {}

  async findByEmail(email: string): Promise<User | null> {
    return this.getUserByEmailUseCase.execute(email);
  }
}
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the GetUserUseCase implementation to understand how it handles different identifier types

# Look for the use case implementation
ast-grep --pattern 'class GetUserUseCase {
  $$$
  execute($_) {
    $$$
  }
}'

# Check for any email-specific repository methods
ast-grep --pattern 'interface $UsersRepository {
  $$$
  findByEmail($_) {
    $$$
  }
}'

Length of output: 173

Script:

#!/bin/bash
# Let's try a different approach to find the relevant implementations

# Search for GetUserUseCase implementation
rg -l "GetUserUseCase" --type ts

# Search for UsersRepository interface
rg -l "UsersRepository" --type ts

# Search for any email-related methods in the codebase
rg "findByEmail|getByEmail" --type ts -A 3

Length of output: 2107

Script:

#!/bin/bash
# Let's check the implementation of GetUserUseCase to see how it handles the identifier

cat libs/users/application/src/lib/use-cases/get-user.use-case.ts

# Also check the mongoose repository implementation to see how it handles email lookups
cat libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts

Length of output: 1292

}
40 changes: 32 additions & 8 deletions libs/users/domain/src/lib/user.entity.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,39 @@
import { User } from './user.entity';

describe('User Entity', () => {
it('should create a user with id and name', () => {
const user = new User('1', 'John Doe');
expect(user.id).toBe('1');
expect(user.name).toBe('John Doe');
it('should create a user with all required properties', () => {
const user = new User(
'test-id',
'John',
'Doe',
'[email protected]',
'password123',
);

expect(user.id).toBe('test-id');
expect(user.firstName).toBe('John');
expect(user.lastName).toBe('Doe');
expect(user.email).toBe('[email protected]');
expect(user.password).toBe('password123');
});

it('should allow updating the name', () => {
const user = new User('1', 'John Doe');
user.name = 'Jane Doe';
expect(user.name).toBe('Jane Doe');
it('should have all properties as defined in the constructor', () => {
const userData = {
id: 'user-123',
firstName: 'Jane',
lastName: 'Smith',
email: '[email protected]',
password: 'securepass',
};

const user = new User(
userData.id,
userData.firstName,
userData.lastName,
userData.email,
userData.password,
);

expect(user).toEqual(userData);
});
});
5 changes: 4 additions & 1 deletion libs/users/domain/src/lib/user.entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
export class User {
constructor(
public readonly id: string,
public name: string,
public firstName: string,
public lastName: string,
public email: string,
Comment on lines +4 to +6
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for user properties

The domain entity should enforce business rules through validation.

Consider adding validation for:

  1. Email format
  2. Required fields
  3. Name length constraints
export class User {
  constructor(
    public readonly id: string,
    public firstName: string,
    public lastName: string,
    public email: string,
    private readonly hashedPassword: string,
  ) {
    this.validateEmail(email);
    this.validateNames(firstName, lastName);
  }

  private validateEmail(email: string): void {
    if (!email || !email.includes('@')) {
      throw new Error('Invalid email format');
    }
  }

  private validateNames(firstName: string, lastName: string): void {
    if (!firstName || !lastName) {
      throw new Error('First name and last name are required');
    }
    if (firstName.length < 2 || lastName.length < 2) {
      throw new Error('Names must be at least 2 characters long');
    }
  }
}

public password: string,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

⚠️ Security Risk: Avoid storing plain text passwords in domain entities

Storing passwords as plain text in domain entities poses a significant security risk. The password should be hashed before storage and the hashing logic should be encapsulated in a separate service.

Consider one of these approaches:

  1. Remove the password from the domain entity and handle it separately in an auth-specific service
  2. Store only the hashed password and move the hashing logic to a factory method:
export class User {
  private constructor(
    public readonly id: string,
    public firstName: string,
    public lastName: string,
    public email: string,
    private readonly hashedPassword: string,
  ) {}

  static async create(
    id: string,
    firstName: string,
    lastName: string,
    email: string,
    plainTextPassword: string
  ): Promise<User> {
    const hashedPassword = await hashPassword(plainTextPassword);
    return new User(id, firstName, lastName, email, hashedPassword);
  }
}

Comment on lines +4 to +7
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Breaking changes detected in User entity constructor

The verification reveals significant inconsistencies and breaking changes:

  • The User entity constructor requires 5 parameters (id, firstName, lastName, email, password)
  • However, the mongoose repository's findById method creates users with only 2 parameters (id, name)
  • The mongoose schema only defines a single name field, not matching the entity structure

Key locations requiring updates:

  • libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts: Incorrect User instantiation
  • libs/users/infrastructure/mongoose/src/lib/user.schema.ts: Schema doesn't match entity fields
  • libs/users/domain/src/lib/user.entity.spec.ts: Contains the only correct usage of the new constructor
🔗 Analysis chain

Verify the impact of constructor signature change

The constructor signature has changed significantly. This is a breaking change that will affect all code that creates User instances.

Let's verify the impact:

Let me gather more information about the actual implementation of the User class and its usage in the repository to better understand the impact.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find all direct instantiations of the User class
ast-grep --pattern 'new User($$$)'

# Find all test files that might need updates
fd -e spec.ts -e test.ts

# Look for potential factories or repositories that create users
rg -l "User['\"].*factory|repository" --type ts

Length of output: 2438

Script:

#!/bin/bash
# Check the User class implementation
ast-grep --pattern 'class User {
  $$$
}'

# Check the mongoose repository implementation to see how it creates users
cat libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts

# Check the user schema to understand the data structure
cat libs/users/infrastructure/mongoose/src/lib/user.schema.ts

Length of output: 1729

) {}
}
28 changes: 25 additions & 3 deletions libs/users/domain/src/lib/users.repository.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,29 @@ import { User } from './user.entity';

class MockUsersRepository implements UsersRepository {
private users: User[] = [
{ id: '1', name: 'John Doe' },
{ id: '2', name: 'Jane Doe' },
{
id: '1',
firstName: 'John',
lastName: 'Doe',
email: '[email protected]',
password: 'password123'
},
{
id: '2',
firstName: 'Jane',
lastName: 'Smith',
email: '[email protected]',
password: 'password456'
}
];

async findById(id: string): Promise<User | null> {
return this.users.find((user) => user.id === id) || null;
}

async findByEmail(email: string): Promise<User | null> {
return this.users.find((user) => user.email === email) || null;
}
Comment on lines +26 to +28
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add test cases for findByEmail method.

The new method lacks test coverage for:

  • Successfully finding a user by email
  • Case-sensitivity handling
  • Invalid email format
  • User not found scenario

Would you like me to help generate the missing test cases?

}

describe('UsersRepository', () => {
Expand All @@ -21,7 +37,13 @@ describe('UsersRepository', () => {

test('findById should return a user by id', async () => {
const user = await usersRepository.findById('1');
expect(user).toEqual({ id: '1', name: 'John Doe' });
expect(user).toEqual({
id: '1',
firstName: 'John',
lastName: 'Doe',
email: '[email protected]',
password: 'password123'
});
});

test('findById should return null if user not found', async () => {
Expand Down
1 change: 1 addition & 0 deletions libs/users/domain/src/lib/users.repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { User } from './user.entity';

export interface UsersRepository {
findById(id: string): Promise<User | null>;
findByEmail(email: string): Promise<User | null>;
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Implementation and tests are incomplete for findByEmail method

The MongooseUsersRepository is missing the required findByEmail implementation, and its test suite lacks coverage for this method. Here are the specific issues:

  • MongooseUsersRepository needs to implement the findByEmail method as required by the interface
  • mongoose-users.repository.spec.ts needs test cases for the findByEmail functionality
🔗 Analysis chain

Verify implementation completeness.

Let's ensure all repository implementations and related tests have been updated with the new method.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all repository implementations include findByEmail method
# and corresponding test coverage exists

# Check for repository implementations
echo "Checking repository implementations..."
ast-grep --pattern 'class $_ implements UsersRepository {
  $$$
  findByEmail($_): $_  {
    $$$
  }
  $$$
}'

# Check for test coverage
echo "Checking test coverage..."
rg -A 5 'describe.*UsersRepository' 
rg -A 2 'test.*findByEmail'

Length of output: 3491

Script:

#!/bin/bash
# Let's check for the actual implementation of MongooseUsersRepository
echo "Checking MongooseUsersRepository implementation..."
ast-grep --pattern 'class MongooseUsersRepository implements UsersRepository {
  $$$
  findByEmail($_): $_  {
    $$$
  }
  $$$
}'

# Let's also check for findByEmail tests specifically
echo "Checking findByEmail test cases..."
rg -B 2 -A 5 'test.*findByEmail'

# Let's verify if there are any other repository implementations
echo "Checking for other repository implementations..."
rg -l "implements UsersRepository"

Length of output: 624

Script:

#!/bin/bash
# Let's examine the MongooseUsersRepository implementation directly
echo "Content of mongoose-users.repository.ts:"
cat libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts

# Let's also check the test file content
echo -e "\nContent of mongoose-users.repository.spec.ts:"
cat libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.spec.ts

Length of output: 2978

}

export const USERS_REPOSITORY = Symbol('USERS_REPOSITORY');
11 changes: 9 additions & 2 deletions libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ describe('MongooseUsersRepository', () => {
it('should return a user when found', async () => {
const mockUser = {
id: '507f1f77bcf86cd799439011',
name: 'Test User',
firstName: 'John',
lastName: 'Doe',
email: '[email protected]',
password: 'password123',
name: 'John Doe'
};

jest.spyOn(userModel, 'findById').mockReturnValue({
Expand All @@ -39,7 +43,10 @@ describe('MongooseUsersRepository', () => {
const user = await repository.findById(mockUser.id);
expect(user).toBeInstanceOf(User);
expect(user?.id).toBe(mockUser.id);
expect(user?.name).toBe(mockUser.name);
expect(user?.firstName).toBe(mockUser.firstName);
expect(user?.lastName).toBe(mockUser.lastName);
expect(user?.email).toBe(mockUser.email);
expect(user?.password).toBe(mockUser.password);
Comment on lines +46 to +49
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test assertions for better security and validation

  1. Avoid asserting the exact password value to prevent exposing sensitive data in test outputs
  2. Consider adding validation for the email format
  3. Consider verifying the complete User instance structure
 expect(user?.firstName).toBe(mockUser.firstName);
 expect(user?.lastName).toBe(mockUser.lastName);
 expect(user?.email).toBe(mockUser.email);
-expect(user?.password).toBe(mockUser.password);
+expect(user?.password).toBeDefined();
+expect(user?.password).not.toBe('password123');
+expect(user?.email).toMatch(/^[^\s@]+@[^\s@]+\.[^\s@]+$/);
+
+// Verify complete structure
+expect(Object.keys(user!)).toEqual(
+  expect.arrayContaining(['id', 'firstName', 'lastName', 'email', 'password'])
+);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
expect(user?.firstName).toBe(mockUser.firstName);
expect(user?.lastName).toBe(mockUser.lastName);
expect(user?.email).toBe(mockUser.email);
expect(user?.password).toBe(mockUser.password);
expect(user?.firstName).toBe(mockUser.firstName);
expect(user?.lastName).toBe(mockUser.lastName);
expect(user?.email).toBe(mockUser.email);
expect(user?.password).toBeDefined();
expect(user?.password).not.toBe('password123');
expect(user?.email).toMatch(/^[^\s@]+@[^\s@]+\.[^\s@]+$/);
// Verify complete structure
expect(Object.keys(user!)).toEqual(
expect.arrayContaining(['id', 'firstName', 'lastName', 'email', 'password'])
);

});

it('should return null when user is not found', async () => {
Expand Down
11 changes: 8 additions & 3 deletions libs/users/interface-adapters/src/lib/dto/user.dto.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,16 @@ import { UserDto } from './user.dto';
describe('UserDto', () => {
it('should create a new UserDto instance', () => {
const id = '123';
const name = 'Test User';
const userDto = new UserDto(id, name);
const firstName = 'Test';
const lastName = 'User';
const email = '[email protected]';

const userDto = new UserDto(id, firstName, lastName, email);

expect(userDto).toBeDefined();
expect(userDto.id).toBe(id);
expect(userDto.name).toBe(name);
expect(userDto.firstName).toBe(firstName);
expect(userDto.lastName).toBe(lastName);
expect(userDto.email).toBe(email);
});
});
14 changes: 11 additions & 3 deletions libs/users/interface-adapters/src/lib/dto/user.dto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,18 @@ export class UserDto {
id: string;

@Field()
name: string;
firstName: string;

constructor(id: string, name: string) {
@Field()
lastName: string;

@Field()
email: string;
Comment on lines +14 to +15
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add email format validation and non-null constraint.

Consider adding validation for the email field using class-validator decorators and making it non-nullable in the GraphQL schema.

- @Field()
+ @Field({ nullable: false })
+ @IsEmail()
  email: string;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Field()
email: string;
@Field({ nullable: false })
@IsEmail()
email: string;


constructor(id: string, firstName: string, lastName: string, email: string) {
this.id = id;
this.name = name;
this.firstName = firstName;
this.lastName = lastName;
this.email = email;
}
}
1 change: 0 additions & 1 deletion libs/users/interface-adapters/src/lib/resolver/users.resolver.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Test, TestingModule } from '@nestjs/testing';
import { UsersService } from '@users/application';
import { User } from '@users/domain';

import { UsersResolver } from './users.resolver';

Expand Down
6 changes: 4 additions & 2 deletions libs/users/interface-adapters/src/lib/resolver/users.resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ export class UsersResolver {
constructor(private usersService: UsersService) {}

@Query(() => UserDto, { nullable: true })
async getUser(@Args({ name: 'id', type: () => ID }) id: string): Promise<UserDto | null> {
async getUser(
@Args({ name: 'id', type: () => ID }) id: string,
): Promise<UserDto | null> {
const user = await this.usersService.findById(id);

if (!user) {
return null;
}

return new UserDto(user.id, user.name);
return new UserDto(user.id, user.firstName, user.lastName, user.email);
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

The UserDto structure change is compatible but requires database schema update

The verification reveals:

  • UserDto class correctly defines firstName, lastName, and email fields
  • User domain entity matches with the expected structure (id, firstName, lastName, email)
  • UsersService.findById returns the User entity with compatible fields
  • However, the UserDocument schema in Mongoose still uses a 'name' field instead of firstName/lastName

Issues found:

  • libs/users/infrastructure/mongoose/src/lib/user.schema.ts: Schema needs to be updated to use firstName and lastName fields instead of name
🔗 Analysis chain

Verify compatibility with updated UserDto structure

The return statement has been updated to use the new UserDto structure with firstName, lastName, and email. Let's verify the compatibility of these changes.

Let me gather more information about the UserDto class definition and its usage.

Let me check one more thing to verify the User entity structure from the GetUserUseCase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the compatibility of the updated UserDto structure

# Check UserDto constructor definition
echo "Checking UserDto constructor definition..."
ast-grep --pattern 'class UserDto {
  constructor($$$) {
    $$$
  }
}'

# Check if UsersService.findById returns compatible structure
echo "Checking UsersService.findById implementation..."
ast-grep --pattern 'class UsersService {
  $$$
  async findById($$$) {
    $$$
  }
  $$$
}'

# Look for other UserDto instantiations that might need updates
echo "Checking other UserDto instantiations..."
rg "new UserDto\(" -A 1

Length of output: 883

Script:

#!/bin/bash
# Check UserDto class definition
echo "Checking UserDto class definition..."
rg "class UserDto" -A 10

# Check for any test cases that might reveal the expected structure
echo "Checking UserDto test cases..."
rg "describe.*UserDto" -A 20

# Check UsersService implementation
echo "Checking UsersService implementation..."
rg "class UsersService" -A 20

Length of output: 3317

Script:

#!/bin/bash
# Check User entity and GetUserUseCase implementation
echo "Checking User entity definition..."
rg "class User" -A 10

echo "Checking GetUserUseCase implementation..."
rg "class GetUserUseCase" -A 10

Length of output: 4946

}
}
Loading