Relocate All Entities to Their Corresponding Modules (domain/entities) #134
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (4)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis change migrates all entity classes from the shared Changes
Changes Table (Grouped)
Sequence Diagram(s)sequenceDiagram
participant Service/UseCase
participant Domain Entity (in domain/entities)
participant Database (via TypeORM)
Service/UseCase->>Domain Entity (in domain/entities): Import and instantiate entity
Domain Entity (in domain/entities)->>Domain Entity (in domain/entities): Apply validation/business logic
Domain Entity (in domain/entities)->>Database (via TypeORM): Persist or query entity
Database (via TypeORM)-->>Domain Entity (in domain/entities): Return entity instance/data
Domain Entity (in domain/entities)-->>Service/UseCase: Return processed entity
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 19
🔭 Outside diff range comments (2)
src/modules/volunteer/use-cases/update-volunteer.use-case.ts (1)
16-18: Replace genericErrorwith a domain-specific exception
Throwingnew Error("Volunteer not found")couples callers to a string literal and makes typed error handling impossible. Introduce aVolunteerNotFoundError extends Error(or equivalent) and throw that instead.- throw new Error("Volunteer not found"); + throw new VolunteerNotFoundError(id);src/modules/organization/domain/entities/organization.entity.ts (1)
29-35: Remove unused constructor parameters.The constructor accepts
id,createdAt, andupdatedAtparameters but doesn't use them. Sincesuper()is called without arguments (indicating the new BaseEntity uses TypeORM decorators), these parameters should be removed for consistency.constructor( - props: OrganizationProps, - id?: string, - createdAt?: Date, - updatedAt?: Date + props: OrganizationProps ) { super();Also update the static factory method and update method calls accordingly:
public static create(props: OrganizationProps, id?: string): Organization { - return new Organization(props, id); + return new Organization(props); } public update(props: Partial<OrganizationProps>): Organization { return new Organization( { id: this.id, name: props.name ?? this.name, email: props.email ?? this.email, description: props.description ?? this.description, category: props.category ?? this.category, website: props.website ?? this.website, address: props.address ?? this.address, phone: props.phone ?? this.phone, isVerified: props.isVerified ?? this.isVerified, logoUrl: props.logoUrl ?? this.logoUrl, walletAddress: props.walletAddress ?? this.walletAddress, - }, - this.id, - this.createdAt, - new Date() + } ); }
🧹 Nitpick comments (9)
src/modules/volunteer/use-cases/create-volunteer.use-case.ts (1)
8-12: Surface domain-level validation errors instead of raw exceptions
Volunteer.create()may throw validation errors. Consider catching these and mapping them to a domain-specific error (e.g.,VolunteerValidationError) so the application layer can react appropriately instead of leaking low-level exceptions.src/modules/volunteer/use-cases/get-volunteers-by-project.use-case.ts (1)
17-24: Potential N+1 database hit—consider batching
findByProjectId(...)andcount(...)are executed sequentially. If your Prisma client supports aggregate queries, you can fetch both the list and the count in a single round-trip to reduce latency.src/modules/user/use-cases/userUseCase.ts (1)
11-13: Switch to non-blocking password hashing
bcrypt.hashSyncblocks the event loop. Use the async variant to avoid performance degradation under load.-const hashedPassword = bcrypt.hashSync(data.password, 10); +const hashedPassword = await bcrypt.hash(data.password, 10);src/modules/volunteer/repositories/implementations/volunteer-prisma.repository.ts (1)
8-10: InstantiatePrismaClientonce, reuse via DI
Creating a newPrismaClientper repository instance can exhaust database connections. Inject a shared client (e.g., via NestJS provider or singleton) instead.src/modules/user/__tests__/domain/entities/user-volunteer.entity.test.ts (2)
17-33: Enhance validation test coverage for edge cases.Consider adding test cases for
nullandundefinedvalues to ensure robust validation:+ it("should throw error if userId is null or undefined", () => { + expect(() => UserVolunteer.create(null as any, validVolunteerId)).toThrow("User ID and Volunteer ID are required") + expect(() => UserVolunteer.create(undefined as any, validVolunteerId)).toThrow("User ID and Volunteer ID are required") + }) + + it("should throw error if volunteerId is null or undefined", () => { + expect(() => UserVolunteer.create(validUserId, null as any)).toThrow("User ID and Volunteer ID are required") + expect(() => UserVolunteer.create(validUserId, undefined as any)).toThrow("User ID and Volunteer ID are required") + })
54-63: Consider improving timestamp test reliability.The current timestamp test may be flaky due to timing precision. Consider using Jest's fake timers or a more robust approach:
- describe("Timestamps", () => { - it("should set joinedAt timestamp on creation", () => { - const beforeCreation = new Date() - const userVolunteer = UserVolunteer.create(validUserId, validVolunteerId) - const afterCreation = new Date() - - expect(userVolunteer.joinedAt.getTime()).toBeGreaterThanOrEqual(beforeCreation.getTime()) - expect(userVolunteer.joinedAt.getTime()).toBeLessThanOrEqual(afterCreation.getTime()) - }) - }) + describe("Timestamps", () => { + it("should set joinedAt timestamp on creation", () => { + const userVolunteer = UserVolunteer.create(validUserId, validVolunteerId) + const now = new Date() + const timeDifference = Math.abs(userVolunteer.joinedAt.getTime() - now.getTime()) + + expect(userVolunteer.joinedAt).toBeInstanceOf(Date) + expect(timeDifference).toBeLessThan(1000) // Allow 1 second tolerance + }) + })src/modules/user/__tests__/domain/entities/user.entity.test.ts (1)
1-60: Consider adopting factory method pattern for consistency.Unlike other entity tests in this PR, the User entity tests use direct constructor instantiation instead of a factory method. Consider implementing a factory method like other entities for consistency and better validation:
- it("should create a user with valid properties", () => { - const user = new User() - user.name = "John" - user.lastName = "Doe" - user.email = "[email protected]" - user.password = "hashedPassword" - user.wallet = "GXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" - user.isVerified = false + it("should create a user with valid properties", () => { + const userProps = { + name: "John", + lastName: "Doe", + email: "[email protected]", + password: "hashedPassword", + wallet: "GXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" + } + const user = User.create(userProps)This would also enable adding validation tests for required fields and invalid inputs.
src/modules/nft/domain/entities/nft.entity.ts (2)
49-51: Add validation and business logic to updateMetadata.The updateMetadata method lacks validation and doesn't check if the NFT is minted before allowing updates.
Consider adding validation and business logic:
public updateMetadata(metadataUri: string): void { + if (!this.isMinted) { + throw new Error("Cannot update metadata for unminted NFT") + } + + if (!metadataUri || !metadataUri.trim()) { + throw new Error("Metadata URI cannot be empty") + } + this.metadataUri = metadataUri }
58-67: Clarify the purpose of NFTDomain class and consider deprecation strategy.Having both
NFTandNFTDomainclasses in the same file could cause confusion. The comment mentions it's kept for use cases, but this dual structure needs clearer documentation.Consider adding deprecation notice or clearer documentation:
// Keep the simple domain class for use cases +/** + * @deprecated This class is maintained for backward compatibility with existing use cases. + * New code should use the NFT entity class instead. + * TODO: Migrate use cases to use the NFT entity and remove this class. + */ export class NFTDomain {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.DS_Storeis excluded by!**/.DS_Storepackage-lock.jsonis excluded by!**/package-lock.jsonsrc/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (40)
package.json(1 hunks)src/config/prisma.ts(1 hunks)src/entities/BaseEntity.ts(0 hunks)src/entities/NFT.ts(0 hunks)src/entities/Organization.ts(0 hunks)src/entities/Photo.ts(0 hunks)src/entities/Project.ts(0 hunks)src/entities/TestItem.ts(0 hunks)src/entities/User.ts(0 hunks)src/entities/Volunteer.ts(0 hunks)src/entities/userVolunteer.entity.ts(0 hunks)src/modules/nft/__tests__/domain/entities/nft.entity.test.ts(1 hunks)src/modules/nft/domain/entities/nft.entity.ts(1 hunks)src/modules/organization/domain/entities/organization.entity.ts(3 hunks)src/modules/photo/__tests__/domain/entities/photo.entity.test.ts(1 hunks)src/modules/photo/__tests__/photo.entity.test.ts(0 hunks)src/modules/photo/domain/entities/photo.entity.ts(1 hunks)src/modules/photo/entities/photo.entity.ts(1 hunks)src/modules/project/__tests__/domain/entities/project.entity.test.ts(1 hunks)src/modules/project/domain/Project.ts(1 hunks)src/modules/project/domain/entities/project.entity.ts(1 hunks)src/modules/shared/__tests__/domain/entities/test-item.entity.test.ts(1 hunks)src/modules/shared/domain/entities/base.entity.ts(1 hunks)src/modules/shared/domain/entities/test-item.entity.ts(1 hunks)src/modules/user/__tests__/domain/entities/user-volunteer.entity.test.ts(1 hunks)src/modules/user/__tests__/domain/entities/user.entity.test.ts(1 hunks)src/modules/user/__tests__/user.test.ts(2 hunks)src/modules/user/domain/entities/User.entity.ts(1 hunks)src/modules/user/domain/entities/User.ts(1 hunks)src/modules/user/domain/entities/user-volunteer.entity.ts(1 hunks)src/modules/user/use-cases/userUseCase.ts(1 hunks)src/modules/volunteer/__tests__/domain/volunteer.entity.test.ts(3 hunks)src/modules/volunteer/domain/entities/volunteer.entity.ts(1 hunks)src/modules/volunteer/domain/volunteer.entity.ts(0 hunks)src/modules/volunteer/repositories/implementations/volunteer-prisma.repository.ts(1 hunks)src/modules/volunteer/repositories/interfaces/volunteer-repository.interface.ts(1 hunks)src/modules/volunteer/use-cases/create-volunteer.use-case.ts(1 hunks)src/modules/volunteer/use-cases/get-volunteers-by-project.use-case.ts(1 hunks)src/modules/volunteer/use-cases/update-volunteer.use-case.ts(1 hunks)src/services/VolunteerService.ts(1 hunks)
💤 Files with no reviewable changes (11)
- src/entities/Project.ts
- src/entities/Volunteer.ts
- src/entities/BaseEntity.ts
- src/entities/NFT.ts
- src/entities/Organization.ts
- src/entities/User.ts
- src/entities/Photo.ts
- src/entities/TestItem.ts
- src/modules/photo/tests/photo.entity.test.ts
- src/entities/userVolunteer.entity.ts
- src/modules/volunteer/domain/volunteer.entity.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/modules/photo/domain/entities/photo.entity.ts (1)
src/modules/user/domain/entities/User.entity.ts (1)
Entity(3-31)
src/modules/nft/__tests__/domain/entities/nft.entity.test.ts (1)
src/modules/organization/domain/entities/organization.entity.ts (1)
Organization(17-72)
src/modules/project/__tests__/domain/entities/project.entity.test.ts (1)
src/modules/project/domain/Project.ts (1)
Project(20-60)
src/modules/nft/domain/entities/nft.entity.ts (2)
src/modules/user/domain/entities/User.entity.ts (1)
Entity(3-31)src/modules/organization/domain/entities/organization.entity.ts (1)
Organization(17-72)
🔇 Additional comments (27)
package.json (1)
72-75: ESLint & @typescript-eslint versions are compatible
@typescript-eslint/[email protected]and@typescript-eslint/[email protected]both declare a peerDependency of"eslint": "^8.57.0 || ^9.0.0", so usingeslint@^9.30.0is valid—no need to downgrade ESLint or move to a non-existent v9 of the TypeScript-ESLint packages.Please note:
- package.json lines 72–75: version alignment is already correct.
- package.json line 86: there’s a
"typescript-eslint": "^8.22.0"entry—there’s no such npm package, so remove it if it was added unintentionally.Optionally, you may bump both
@typescript-eslintpackages to the latest v8.x (currently 8.35.0) for the newest bug fixes.Likely an incorrect or invalid review comment.
src/modules/volunteer/use-cases/create-volunteer.use-case.ts (1)
1-1: Import path update looks correct
The new path reflects the relocation todomain/entities; compilation should succeed.src/modules/volunteer/use-cases/update-volunteer.use-case.ts (1)
1-1: Import path update confirmed
No further action required.src/modules/volunteer/use-cases/get-volunteers-by-project.use-case.ts (1)
1-1: Import path update verified
The change is minimal and correct.src/modules/user/use-cases/userUseCase.ts (1)
13-19: Bypassing entity factory may skip validations
Instantiatingnew User()directly circumvents any invariants enforced by aUser.create()factory method introduced in the refactor. Prefer the factory for consistency and validation.src/modules/volunteer/repositories/implementations/volunteer-prisma.repository.ts (1)
2-2: Import path update acknowledged
No issues detected.src/modules/volunteer/repositories/interfaces/volunteer-repository.interface.ts (1)
1-1: Import path correctly updated for entity relocation.The import path update properly reflects the new directory structure where entities are organized under
domain/entities/. This change aligns with the DDD refactoring objectives.src/services/VolunteerService.ts (1)
1-1: Import path correctly updated for entity relocation.The import path update properly reflects the new entity location. The service correctly uses the entity's domain methods (like
update()on line 53), which demonstrates good domain-driven design.src/modules/user/__tests__/user.test.ts (1)
1-62: Code formatting improvements enhance consistency.The removal of trailing semicolons and standardization of line endings improves code consistency. The test logic remains unchanged and properly covers user CRUD operations with appropriate setup/teardown.
src/modules/volunteer/__tests__/domain/volunteer.entity.test.ts (2)
1-1: Import path correctly updated for entity relocation.The import path update properly reflects the new nested directory structure for entities.
96-102: Excellent addition of validation testing for update method.The new test case ensures that the entity's
update()method properly validates input data and throws appropriate errors for invalid values. This strengthens the test coverage for domain validation logic.src/modules/user/domain/entities/User.ts (1)
1-2: Clean re-export proxy pattern approved
- Verified that
src/modules/user/domain/entities/User.entity.tsexists.- Confirmed it contains the full TypeORM entity definition with
@Entity,@PrimaryGeneratedColumn,@Columndecorators.No further changes are needed.
src/modules/user/__tests__/domain/entities/user-volunteer.entity.test.ts (1)
1-64: Well-structured test suite with good DDD practices.The test suite effectively covers the core functionality of the UserVolunteer entity with clear separation of concerns and proper use of factory methods.
src/modules/photo/__tests__/domain/entities/photo.entity.test.ts (2)
1-118: Excellent comprehensive test suite with robust validation.This test suite demonstrates excellent coverage of the Photo entity's functionality, including proper validation, metadata management, and serialization. The URL validation is particularly well-implemented, covering both HTTP and HTTPS protocols and invalid formats.
88-99: Well-designed metadata merging test.The test properly verifies that metadata updates merge with existing metadata without overwriting, which is crucial for maintaining data integrity.
src/modules/shared/__tests__/domain/entities/test-item.entity.test.ts (1)
1-90: Well-structured test suite with comprehensive validation coverage.The TestItem entity tests demonstrate good practices with thorough validation testing, including edge cases like whitespace-only names and zero values. The business logic methods are properly tested with clear expectations.
src/modules/photo/entities/photo.entity.ts (1)
1-2: Excellent architectural improvement following DDD principles.The refactoring to re-export from the domain layer centralizes entity logic and improves code organization while maintaining backward compatibility. This pattern aligns well with Domain-Driven Design principles.
src/modules/organization/domain/entities/organization.entity.ts (1)
52-71: LGTM: Proper immutable update pattern.The update method correctly preserves entity identity by including the current
idand creates a new instance with updated properties. This immutable approach is excellent for domain entities.src/modules/project/__tests__/domain/entities/project.entity.test.ts (1)
31-81: Excellent test coverage for domain behavior.The test suite comprehensively covers:
- Status transitions with validation
- Error conditions with specific error messages
- Status check methods
- Business rule enforcement
This demonstrates good domain-driven design testing practices.
src/modules/shared/domain/entities/test-item.entity.ts (2)
16-35: Excellent validation and factory method implementation.The static
createmethod demonstrates good domain validation practices:
- Input validation with specific error messages
- Clear business rules enforcement
- Proper entity instantiation
The validation logic is comprehensive and user-friendly.
37-46: Well-designed domain methods.The domain methods maintain consistency with the validation rules established in the factory method. The
updateValuemethod properly validates input, andincrementAgeprovides a clear domain operation.src/modules/nft/__tests__/domain/entities/nft.entity.test.ts (1)
43-81: Comprehensive NFT domain behavior testing.The test suite effectively covers:
- Minting functionality with validation
- Error handling for invalid operations
- Metadata management
- Ownership verification
This provides good confidence in the NFT entity's domain logic.
src/modules/user/domain/entities/User.entity.ts (1)
8-30: Well-designed column structure for user domain.The column definitions demonstrate good domain modeling:
- Appropriate unique constraints on email and wallet
- Sensible defaults for verification status
- Proper nullable fields for verification workflow
- Clear field types and constraints
src/modules/project/domain/Project.ts (1)
1-60: LGTM! Clean domain model implementation.The formatting improvements and domain model structure follow good DDD practices with private constructor, factory method, and proper encapsulation of domain logic.
src/modules/shared/domain/entities/base.entity.ts (1)
3-12: LGTM! Clean TypeORM base entity.The TypeORM decorators properly handle common entity fields with automatic UUID generation and timestamp management.
src/modules/project/domain/entities/project.entity.ts (1)
1-74: Excellent domain entity implementation!This is a well-designed TypeORM entity that properly encapsulates business logic:
- Clean status transition management with proper validation
- Good use of enum for type safety
- Appropriate error handling for invalid state transitions
- Clear domain methods that enforce business rules
The implementation follows DDD principles effectively.
src/modules/volunteer/domain/entities/volunteer.entity.ts (1)
41-52: Well-implemented factory method pattern.The static
createmethod properly validates inputs before constructing the entity, following good domain-driven design practices.
2 participants
Volunchain Pull Reques
domain/entities) #127Type of Change
Changes description
This PR implements a comprehensive refactoring to relocate all entities from the centralized
src/entities/directory to their corresponding domain modules undersrc/modules/<domain>/domain/entities/. This change aligns with Domain-Driven Design (DDD) principles and significantly improves code organization.Key Changes Made:
Entity Relocations:
src/modules/user/domain/entities/user.entity.tssrc/modules/project/domain/entities/project.entity.tssrc/modules/volunteer/domain/entities/volunteer.entity.tssrc/modules/nft/domain/entities/nft.entity.tssrc/modules/photo/domain/entities/photo.entity.tssrc/modules/user/domain/entities/user-volunteer.entity.tssrc/modules/shared/domain/entities/test-item.entity.tssrc/modules/shared/domain/entities/base.entity.tsDomain Logic Enhancements:
activate(),complete(),cancel()create(),update()method with validation, andtoObject()serializationmint()functionality, metadata management, and ownership verificationupdateMetadata()Comprehensive Test Coverage:
Created unit tests for all entities covering:
Technical Improvements:
Breaking Changes:
Migration Guide:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores