refactor: ♻️ Tasks module #125
Conversation
WalkthroughThe pull request introduces a significant refactoring of the task retrieval mechanism in the application. The changes involve shifting from a direct repository-based approach to a use case-driven architecture. The Changes
Sequence DiagramsequenceDiagram
participant Resolver as TasksResolver
participant UseCase as GetAllTasksUseCase
participant Service as TasksService
participant Repository as TasksRepository
Resolver->>UseCase: execute()
UseCase->>Service: findAll()
Service->>Repository: findAll()
Repository-->>Service: return tasks
Service-->>UseCase: return tasks
UseCase-->>Resolver: return tasks
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
libs/tasks/domain/src/index.ts (1)
1-10: LGTM! Clean organization of domain exports.The separation of concerns between tasks and user tasks is clear. The shift to interface exports aligns well with the dependency inversion principle.
Consider adding a newline between the task exports (lines 4-5) for consistent spacing with other sections.
libs/tasks/interface-adapters/src/lib/resolver/tasks.resolver.ts (1)
13-15: Consider adding error handling.The resolver should handle potential errors from the use case execution to ensure proper GraphQL error responses.
@Query(() => [TaskDto]) async getTasks(): Promise<TaskDto[]> { - return this.getAllTasksUseCase.execute(); + try { + return await this.getAllTasksUseCase.execute(); + } catch (error) { + throw new Error(`Failed to retrieve tasks: ${error.message}`); + } }libs/tasks/domain/src/lib/tasks-repository.interface.spec.ts (1)
Line range hint
17-22: Enhance test coverage with more specific assertions.The current test only verifies the array type and Task instances. Consider adding tests for:
- Non-empty task arrays
- Task property validations
- Error scenarios
describe('findAll', () => { it('should return all tasks', async () => { const tasks = await repository.findAll(); expect(Array.isArray(tasks)).toBe(true); expect(tasks.every((task) => task instanceof Task)).toBe(true); }); + + it('should return tasks with valid properties', async () => { + const mockTask = Task.create('test-id', 'Test Task', 'Description', ['category']); + jest.spyOn(repository, 'findAll').mockResolvedValue([mockTask]); + + const tasks = await repository.findAll(); + expect(tasks[0].id).toBe('test-id'); + expect(tasks[0].title).toBe('Test Task'); + expect(tasks[0].description).toBe('Description'); + expect(tasks[0].categories).toEqual(['category']); + }); });libs/tasks/application/src/lib/tasks.service.spec.ts (1)
31-43: Add tests for edge cases and error scenarios.While the happy path is well tested, consider adding tests for:
- Empty task list
- Repository errors
- Invalid task data
describe('findMany', () => { it('should return all tasks', async () => { const mockTasks = [ Task.create('task-1', 'Task 1', 'Description 1', ['category-1']), Task.create('task-2', 'Task 2', 'Description 2', ['category-2']), ]; mockTasksRepository.findAll = jest.fn().mockResolvedValue(mockTasks); const result = await service.findAll(); expect(result).toEqual(mockTasks); expect(mockTasksRepository.findAll).toHaveBeenCalled(); }); + + it('should handle empty task list', async () => { + mockTasksRepository.findAll = jest.fn().mockResolvedValue([]); + const result = await service.findAll(); + expect(result).toEqual([]); + }); + + it('should handle repository errors', async () => { + mockTasksRepository.findAll = jest.fn().mockRejectedValue(new Error('DB error')); + await expect(service.findAll()).rejects.toThrow('DB error'); + }); });libs/tasks/application/src/lib/use-cases/get-all-tasks.use-case.spec.ts (2)
6-25: Consider enhancing the test setup for better type safety and cleanup.While the setup is functional, consider these improvements:
let tasksService: TasksService; +const mockTasksService = { + findAll: jest.fn(), +} as jest.Mocked<TasksService>; beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ providers: [ GetAllTasksUseCase, { provide: TasksService, - useValue: { - findAll: jest.fn(), - }, + useValue: mockTasksService, }, ], }).compile(); useCase = module.get<GetAllTasksUseCase>(GetAllTasksUseCase); tasksService = module.get<TasksService>(TasksService); }); + +afterEach(() => { + jest.clearAllMocks(); +});
27-29: Enhance the initialization test coverage.Consider adding assertions to verify that the injected dependencies are properly initialized.
it('should be defined', () => { expect(useCase).toBeDefined(); + expect(tasksService).toBeDefined(); + expect(tasksService.findAll).toBeDefined(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
libs/tasks/application/src/lib/tasks.service.spec.ts(1 hunks)libs/tasks/application/src/lib/tasks.service.ts(1 hunks)libs/tasks/application/src/lib/use-cases/get-all-tasks.use-case.spec.ts(1 hunks)libs/tasks/application/src/lib/use-cases/get-all-tasks.use-case.ts(1 hunks)libs/tasks/domain/src/index.ts(1 hunks)libs/tasks/domain/src/lib/tasks-repository.interface.spec.ts(1 hunks)libs/tasks/domain/src/lib/user-tasks-repository.interface.spec.ts(1 hunks)libs/tasks/interface-adapters/src/lib/resolver/tasks.resolver.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/tasks/domain/src/lib/user-tasks-repository.interface.spec.ts
🔇 Additional comments (5)
libs/tasks/interface-adapters/src/lib/resolver/tasks.resolver.ts (1)
3-3: LGTM! Good architectural improvement.
The change from TasksService to GetAllTasksUseCase follows clean architecture principles by making the resolver depend directly on its specific use case rather than a general service.
Also applies to: 10-10, 14-14
libs/tasks/domain/src/lib/tasks-repository.interface.spec.ts (1)
2-2: LGTM! Good basic test structure.
The mock implementation and basic interface contract testing are well-structured.
Also applies to: 4-7
libs/tasks/application/src/lib/tasks.service.spec.ts (1)
10-12: LGTM! Good mock setup.
The TasksRepository mock is properly implemented using Jest's mocking capabilities.
libs/tasks/application/src/lib/use-cases/get-all-tasks.use-case.spec.ts (2)
1-5: LGTM! Clean and well-structured imports.
The imports follow NestJS conventions and use path aliases appropriately.
1-50: Verify integration with the new architecture.
The test suite supports the move to a use case-driven architecture. Let's verify the integration with other components:
✅ Verification successful
Let me try another verification strategy with correct command syntax.
The test implementation correctly aligns with the use case-driven architecture
The verification results show proper architectural alignment:
- The TasksResolver correctly uses GetAllTasksUseCase for task retrieval
- TasksService is properly decoupled and serves as an application service layer between use cases and repository
- The repository pattern is consistently implemented with proper dependency injection
- Other modules (Users, Auth) follow similar architectural patterns with their respective use cases and resolvers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the architectural changes across the codebase
# Check if TasksService is properly decoupled from use cases
rg -t typescript "class TasksService" -A 10
# Verify that other resolvers follow the same pattern
rg -t typescript "class.*Resolver" -A 10
# Check for any remaining direct repository usage
rg -t typescript "TasksRepository" -A 5
# Look for other use case implementations
fd -e ts "use-case.ts$"
Length of output: 465
Script:
#!/bin/bash
# Check TasksService implementation and its dependencies
rg "class TasksService" --type ts -A 10
# Check resolver implementations
rg "class.*Resolver" --type ts -A 10
# Check for TasksRepository usage
rg "TasksRepository" --type ts -A 5
# Check use case implementations
ast-grep --pattern 'export class $_UseCase {
$$$
execute($$$) {
$$$
}
}'
Length of output: 16789
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
libs/tasks/application/src/lib/user-tasks.service.ts (1)
Line range hint
10-24: Update return types and implement methods.Both createSome and updateSome methods need proper implementation and should return more meaningful response types to align with the use case improvements.
Consider this enhancement:
async createSome( userId: string, tasks: { id: string; createdAt: Date }[], - ): Promise<string> { + ): Promise<{ success: boolean; createdCount: number }> { - // TODO: Implement this - return 'success'; + const created = await this.userTasksRepository.createMany(userId, tasks); + return { success: true, createdCount: created.length }; } async updateSome( userId: string, userTasks: { id: string; updatedAt: Date }[], - ): Promise<string> { + ): Promise<{ success: boolean; updatedCount: number }> { - // TODO: Implement this - return 'success'; + const updated = await this.userTasksRepository.updateMany(userId, userTasks); + return { success: true, updatedCount: updated.length }; }
🧹 Nitpick comments (14)
libs/tasks/interface-adapters/src/lib/resolver/tasks.resolver.spec.ts (1)
15-18: Consider adding type information to the mockThe mock setup is correct, but could benefit from explicit typing for better maintainability.
Consider this improvement:
provide: GetAllTasksUseCase, useValue: { - execute: jest.fn().mockResolvedValue([]), + execute: jest.fn<Promise<Task[]>, []>().mockResolvedValue([]), },libs/tasks/interface-adapters/src/lib/resolver/user-tasks.resolver.ts (3)
17-17: Align variable name with class name for consistency.The variable
getUserTasksUseCasein the constructor does not match the class nameGetAllUserTasksUseCase. For consistency and readability, consider renaming it togetAllUserTasksUseCase.
30-30: Address the TODO: Implement error codes in exception handling.The TODO comment indicates an intention to use error codes instead of plain messages. Implementing standardized error codes can improve error handling and client understanding.
Would you like assistance in defining error codes and updating the exception handling?
Line range hint
11-12: Add error handling for use case execution.Currently, any exceptions thrown by
getAllUserTasksUseCase.execute(userId)will bubble up without specific handling. Consider wrapping the call in a try-catch block to handle known exceptions and provide more informative error responses to the client.libs/tasks/interface-adapters/src/lib/resolver/user-tasks.resolver.spec.ts (1)
85-95: Consider reducing duplication by extracting mapping logic.In the test, the mapping of
mockUserTaskstoUserTaskDtomirrors the mapping logic in the resolver. To adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this mapping into a shared utility function used by both the resolver and the test.libs/tasks/application/src/lib/use-cases/get-all-user-tasks.use-case.ts (1)
11-11: Remove unnecessaryawaitin the return statement.The
awaitinreturn await this.userTasksService.findMany(userId);is unnecessary. Since the function is alreadyasync, you can directly return the promise, and the caller will handle the resolution.Apply this change:
- return await this.userTasksService.findMany(userId); + return this.userTasksService.findMany(userId);libs/tasks/domain/src/lib/user-tasks-repository.interface.ts (1)
4-4: Add JSDoc documentation for findMany method.For consistency with other methods in the interface, please add documentation explaining the method's purpose and possible exceptions.
+ /** + * Retrieves tasks for a specific user + * @throws {UserNotFoundError} If user doesn't exist + */ findMany(userId: string): Promise<UserTask[]>;libs/tasks/interface-adapters/src/lib/tasks.module.ts (1)
7-9: LGTM! Good shift to use case pattern.The addition of specific use cases (GetAllUserTasks, CreateSomeUserTasks, UpdateSomeUserTasks) aligns well with clean architecture principles and improves separation of concerns.
Consider organizing providers by type for better readability:
@Module({ providers: [ + // Resolvers TasksResolver, - TasksService, UserTasksResolver, + + // Services + TasksService, UserTasksService, + + // Use Cases GetAllTasksUseCase, GetAllUserTasksUseCase, CreateSomeUserTasksUseCase, UpdateSomeUserTasksUseCase, + + // Repositories { provide: TASKS_REPOSITORY, useClass: MongooseTasksRepository, }, ],Also applies to: 29-31
libs/tasks/domain/src/lib/user-tasks-repository.interface.spec.ts (1)
30-35: Improve test descriptions and add edge cases.The test description "should return all tasks for a given user" doesn't match the method name
findMany. Consider:
- Updating test descriptions to match new method names
- Adding edge cases such as:
- Invalid user ID
- Large number of tasks
- Error scenarios
- describe('findAll', () => { - it('should return all tasks for a given user', async () => { + describe('findMany', () => { + it('should return empty array when user has no tasks', async () => { const userTasks = await repository.findMany('user-1'); expect(userTasks).toEqual([]); }); + + it('should handle invalid user ID', async () => { + const userTasks = await repository.findMany(''); + expect(userTasks).toEqual([]); + });libs/tasks/application/src/lib/use-cases/create-some-user-tasks.use-case.spec.ts (1)
33-36: Consider freezing test dates for consistency.Using
new Date()in test data can lead to inconsistent test results. Consider using a fixed date string or mock date.const tasks = [ - { id: '1', createdAt: new Date() }, - { id: '2', createdAt: new Date() }, + { id: '1', createdAt: new Date('2024-01-01T00:00:00.000Z') }, + { id: '2', createdAt: new Date('2024-01-01T00:00:00.000Z') }, ];libs/tasks/application/src/lib/use-cases/get-all-user-tasks.use-case.spec.ts (1)
45-46: Consider using more specific assertions.Replace
toBewithtoEqualfor object comparison and add type checking.- expect(result).toBe(expectedTasks); + expect(result).toEqual(expectedTasks); + expect(Array.isArray(result)).toBe(true); + expect(result[0]).toMatchObject({ + id: expect.any(String), + userId: expect.any(String), + status: expect.any(String), + });libs/tasks/application/src/lib/use-cases/update-some-user-tasks.use-case.spec.ts (3)
1-24: Enhance test setup with proper typing and complete mock implementation.Consider improving the test setup:
- Add type safety to the mock implementation
- Consider mocking all methods of UserTasksService to prevent undefined behavior
const module: TestingModule = await Test.createTestingModule({ providers: [ UpdateSomeUserTasksUseCase, { provide: UserTasksService, - useValue: { - updateSome: jest.fn(), - }, + useValue: { + updateSome: jest.fn().mockResolvedValue('success'), + // Mock other methods that might be called + findMany: jest.fn(), + findOne: jest.fn(), + } as jest.Mocked<UserTasksService>, }, ], }).compile();
26-28: Enhance basic test coverage.Consider adding tests to verify:
- The UserTasksService dependency is properly injected
- The use case instance has the expected methods
it('should be defined', () => { expect(useCase).toBeDefined(); + expect(userTasksService).toBeDefined(); + expect(useCase.execute).toBeDefined(); });
1-47: Consider verifying use case independence from implementation details.The current test structure focuses on functionality but doesn't verify the architectural boundaries. Consider adding tests to ensure:
- The use case remains independent of implementation details
- The interface contract between the use case and service is properly maintained
This could be achieved by:
- Moving the service interface to a separate file
- Adding tests to verify the use case only depends on the interface
- Adding contract tests to ensure the service implementation matches the interface
Would you like assistance in implementing these architectural improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
libs/tasks/application/src/index.ts(1 hunks)libs/tasks/application/src/lib/use-cases/create-some-user-tasks.use-case.spec.ts(1 hunks)libs/tasks/application/src/lib/use-cases/create-some-user-tasks.use-case.ts(1 hunks)libs/tasks/application/src/lib/use-cases/get-all-user-tasks.use-case.spec.ts(1 hunks)libs/tasks/application/src/lib/use-cases/get-all-user-tasks.use-case.ts(1 hunks)libs/tasks/application/src/lib/use-cases/update-some-user-tasks.use-case.spec.ts(1 hunks)libs/tasks/application/src/lib/use-cases/update-some-user-tasks.use-case.ts(1 hunks)libs/tasks/application/src/lib/user-tasks.service.ts(1 hunks)libs/tasks/domain/src/lib/entities/user-task.entity.spec.ts(0 hunks)libs/tasks/domain/src/lib/entities/user-task.entity.ts(0 hunks)libs/tasks/domain/src/lib/user-tasks-repository.interface.spec.ts(2 hunks)libs/tasks/domain/src/lib/user-tasks-repository.interface.ts(1 hunks)libs/tasks/interface-adapters/src/lib/dto/user-task.dto.spec.ts(0 hunks)libs/tasks/interface-adapters/src/lib/dto/user-task.dto.ts(1 hunks)libs/tasks/interface-adapters/src/lib/resolver/tasks.resolver.spec.ts(1 hunks)libs/tasks/interface-adapters/src/lib/resolver/user-tasks.resolver.spec.ts(1 hunks)libs/tasks/interface-adapters/src/lib/resolver/user-tasks.resolver.ts(5 hunks)libs/tasks/interface-adapters/src/lib/tasks.module.ts(2 hunks)
💤 Files with no reviewable changes (3)
- libs/tasks/domain/src/lib/entities/user-task.entity.spec.ts
- libs/tasks/interface-adapters/src/lib/dto/user-task.dto.spec.ts
- libs/tasks/domain/src/lib/entities/user-task.entity.ts
🔇 Additional comments (6)
libs/tasks/interface-adapters/src/lib/resolver/tasks.resolver.spec.ts (2)
2-2: LGTM! Clean dependency injection setup
The changes properly reflect the architectural shift from service-based to use case-based approach, with clear import path and consistent naming conventions.
Also applies to: 8-8
24-24: LGTM! Proper test module initialization
The GetAllTasksUseCase is correctly retrieved from the test module.
libs/tasks/application/src/index.ts (1)
5-9: LGTM! Exports are structured appropriately.
The new exports for user tasks are correctly added and organized under the // user tasks section.
libs/tasks/domain/src/lib/user-tasks-repository.interface.ts (1)
1-1: Verify all consumers of findAll have been updated.
The method rename from findAll to findMany and return type change from Task[] to UserTask[] are breaking changes.
Also applies to: 4-4
libs/tasks/interface-adapters/src/lib/dto/user-task.dto.ts (1)
36-43: LGTM! Clean removal of task property.
The constructor parameters and class properties are properly aligned after removing the task property. All necessary decorators and validations remain intact.
libs/tasks/domain/src/lib/user-tasks-repository.interface.spec.ts (1)
1-2: LGTM! Clean transition to UserTask domain model.
The import changes correctly reflect the new domain model structure, maintaining a clear separation of concerns.
libs/tasks/application/src/lib/use-cases/create-some-user-tasks.use-case.ts
Show resolved
Hide resolved
libs/tasks/application/src/lib/use-cases/update-some-user-tasks.use-case.ts
Show resolved
Hide resolved
libs/tasks/application/src/lib/use-cases/create-some-user-tasks.use-case.spec.ts
Show resolved
Hide resolved
libs/tasks/application/src/lib/use-cases/get-all-user-tasks.use-case.spec.ts
Show resolved
Hide resolved
libs/tasks/application/src/lib/use-cases/update-some-user-tasks.use-case.spec.ts
Show resolved
Hide resolved
|
🎉 This PR is included in version 1.19.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
GetAllUserTasksUseCase,CreateSomeUserTasksUseCase, andUpdateSomeUserTasksUseCase.CreateSomeUserTasksUseCaseandGetAllUserTasksUseCase.Bug Fixes
UserTasksServiceto ensure correct return types for task retrieval methods.Documentation
Refactor
UserTasksResolverandTasksResolverclasses to utilize use cases instead of services.Tests