Feat/move all services #140
Conversation
WalkthroughThis update introduces stub controllers for several modules, restructures service and entity imports to align with a modular architecture, and removes legacy folders. Numerous files are refactored for stylistic consistency, including string literal formatting and semicolon usage. Integration and unit tests are updated or stubbed to reflect the ongoing migration, and repository implementations are adjusted to use factory methods and modular imports. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StubController
participant ExpressApp
Client->>ExpressApp: HTTP request (e.g., POST /users)
ExpressApp->>StubController: Route handler call
StubController-->>ExpressApp: res.status(501).json({ error: "Service temporarily disabled..." })
ExpressApp-->>Client: HTTP 501 response with error message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 12
🔭 Outside diff range comments (5)
src/modules/nft/use-cases/createNFT.ts (1)
8-16: Constructor misuse & ID generation could be improved.
Creating an entity vianew NFT(...)coupled withDate.now().toString()for IDs bypasses any invariants a domain factory might enforce and risks ID collisions across processes. Prefer a factory/static creator that encapsulates validation and uses a UUID:-const nft = new NFT( - Date.now().toString(), - data.userId, - data.organizationId, - data.description, - new Date() -); +const nft = NFT.create({ + id: uuidv4(), + userId: data.userId, + organizationId: data.organizationId, + description: data.description, + createdAt: new Date(), +});This also keeps the constructor private, reinforcing aggregate integrity.
src/modules/project/repositories/PrismaProjectRepository.ts (1)
1-96: Remove large blocks of commented-out code or convert to a proper stub.Keeping 90+ lines of commented implementation bloats the repo and hinders readability. If the class is intentionally out of scope during the service migration, replace it with a minimal stub (or delete the file) and rely on git history for retrieval.
src/modules/nft/repositories/nft.repository.ts (1)
15-17: InstantiatePrismaClientonce, not per repository instanceCreating a new client for every repository inflates connection pools and prolongs cold-start time. Inject a shared client or use a singleton:
-class NFTRepository implements INFTRepository { - private prisma = new PrismaClient(); +const prisma = new PrismaClient(); +export class NFTRepository implements INFTRepository {Remember to call
prisma.$disconnect()on app shutdown.src/repository/IUserRepository.ts (1)
3-21: Add missingverifyUsermethod toIUserRepositoryA search confirms
verifyUser(user.id)is called inverify-email.usecase.tsbut isn’t declared on the interface. Please update the interface accordingly:Locations to update:
- File:
src/repository/IUserRepository.tsSuggested diff:
isUserVerified(userId: string): Promise<boolean>; + + /** + * Marks the user as verified and clears the verification token. + */ + verifyUser(userId: string): Promise<void>; }src/modules/auth/use-cases/verify-email.usecase.ts (1)
43-44: Interface-implementation mismatch (verifyUser)
await this.userRepository.verifyUser(user.id);relies on a method that the reviewedIUserRepositorydoes not declare. Ensure both interface and concrete repositories expose the same contract.
🧹 Nitpick comments (51)
src/docs/transaction-helper.md (3)
24-30: Re-indent the example for clearer readabilityThe inner statements have lost their two-space indentation, making the snippet harder to scan quickly. Consider re-adding indentation so nested blocks stand out.
const result = await withTransaction(async (tx) => { -const user = await tx.user.create({ data: userData }); -const profile = await tx.profile.create({ -data: { ...profileData, userId: user.id } -}); -return { user, profile }; + const user = await tx.user.create({ data: userData }); + const profile = await tx.profile.create({ + data: { ...profileData, userId: user.id } + }); + return { user, profile }; });
38-44: Keep object-literal & callback bodies indentedThe block following
transactionHelper.executeInTransactionand theoptionsobject read more naturally with indentation:const result = await transactionHelper.executeInTransaction(async (tx) => { -// Your transactional operations here -return someResult; + // Your transactional operations here + return someResult; }, { -timeout: 15000, -isolationLevel: 'Serializable' + timeout: 15000, + isolationLevel: 'Serializable', });
78-87: Restore indentation inside the decorator exampleSame readability issue in the decorated service method:
@WithTransaction({ timeout: 20000 }) async createProjectWithVolunteers(projectData: any, volunteersData: any[]) { -// This method automatically runs in a transaction -const project = await this.prisma.project.create({ data: projectData }); -const volunteers = await this.prisma.volunteer.createMany({ -data: volunteersData.map(v => ({ ...v, projectId: project.id })) -}); -return { project, volunteers }; + // This method automatically runs in a transaction + const project = await this.prisma.project.create({ data: projectData }); + const volunteers = await this.prisma.volunteer.createMany({ + data: volunteersData.map(v => ({ ...v, projectId: project.id })) + }); + return { project, volunteers }; }src/modules/shared/domain/entities/test-item.entity.ts (2)
16-35: Consider domain-specific error types & DRY validation
create()throws rawErrorobjects three times. Replacing these with domain-level error classes (e.g.,InvalidTestItemError) improves stack-trace filtering and makes the contract explicit for callers / tests.You can also collapse the three nearly-identical negative-number checks into a small private validator to avoid repetition and centralise future constraints.
Example sketch:
+class InvalidTestItemError extends Error {} + private static ensureNonNegative(field: string, value: number): void { if (value < 0) { - throw new Error(`${field} must be non-negative`); + throw new InvalidTestItemError(`${field} must be non-negative`); } } public static create(name: string, value: number, age: number): TestItem { if (!name?.trim()) { - throw new Error("Name is required"); + throw new InvalidTestItemError("Name is required"); } - if (value < 0) { … } - if (age < 0) { … } + this.ensureNonNegative("Value", value); + this.ensureNonNegative("Age", age); … }
37-42: Reuse existing validation to keep logic single-sourced
updateValue()re-implements the non-negative guard that already exists increate(). Extracting the guard (see previous comment) lets you call the same helper here and prevents future divergence.src/modules/shared/__tests__/domain/entities/test-item.entity.test.ts (1)
1-1: Path import can leverage ts-alias for clarity
Relative path"../../../domain/entities/..."becomes fragile after refactors. Iftsconfig.jsonalready defines something like@shared/*, prefer:import { TestItem } from "@shared/domain/entities/test-item.entity";This shortens imports and insulates tests from directory moves.
src/config/redis.ts (1)
3-13: Expose explicitconnect()hook for predictable startup
createClient()performs lazy connection; if the first Redis command is issued before network access is ready it can throw. Exporting an async init makes the lifecycle explicit and lets callers fail fast (e.g., during app bootstrap).redisClient.on("error", (err) => console.error("Redis Client Error", err)); -redisClient.on("connect", () => console.log("Redis Client Connected")); - -export { redisClient }; +/** + * Call once during application bootstrap. + * Example: + * await initRedis(); + */ +export const initRedis = async (): Promise<void> => { + await redisClient.connect(); + console.log("Redis Client Connected"); +}; + +export { redisClient };Ensure every entry-point (CLI, server, tests) invokes
await initRedis()exactly once before issuing Redis commands.src/modules/nft/use-cases/createNFT.ts (1)
2-2: Alias naming may add cognitive load.
ImportingNFTDomain as NFTshortens the identifier but hides the domain context. Consider either:import { NFTDomain } from "../domain/entities/nft.entity"; // … // const nft = NFTDomain.create(/* props */);or keep the alias but add a quick comment clarifying that this is the domain entity to avoid future confusion.
src/modules/user/domain/entities/User.entity.ts (1)
1-30: Confirm entity persistence strategy – TypeORM decorators remain while project migrates to Prisma.All changes here are purely stylistic (semicolon insertion) and look fine.
However, the file still relies on TypeORM decorators (@Entity,@Column, etc.). The PR description states the codebase is moving toward Prisma; keeping TypeORM entities around can create twin-model drift and maintenance overhead.Please verify that:
- This entity is still instantiated through TypeORM at runtime, or
- A follow-up task exists to translate / delete these classes once the Prisma schema is authoritative.
If (2) applies, consider adding a TODO with an issue reference to avoid the entity silently diverging from the canonical data model.
src/modules/project/use-cases/ListProjectsUseCase.ts (1)
1-13: Minor readability tweak – avoid double repository call path.Current logic is fine, but you can tighten it a bit:
- async execute(organizationId?: string): Promise<Project[]> { - if (organizationId) { - return this.projectRepository.findByOrganizationId(organizationId); - } - return this.projectRepository.findAll(); - } + async execute(organizationId?: string): Promise<Project[]> { + return organizationId + ? this.projectRepository.findByOrganizationId(organizationId) + : this.projectRepository.findAll(); + }Same behaviour, slightly shorter branch surface.
src/repository/IPhotoRepository.ts (1)
1-1: Path change works but consider reverting to an alias for long-term maintainability.Deep relative imports (
../modules/photo/domain/entities/photo.entity) get brittle as folder depth changes.
If@/entities/Photo(or similar) is still configured intsconfig.jsonpaths, prefer the alias to keep refactors painless.src/modules/project/use-cases/CreateProjectUseCase.ts (1)
9-14: Minor readability tweak: use nullish-coalescing for default status
dto.status || ProjectStatus.DRAFTalso triggers whendto.statusis0,false, or''(unlikely for an enum, but possible if the DTO is widened later).
Using??makes the intent explicit:- status: dto.status || ProjectStatus.DRAFT, + status: dto.status ?? ProjectStatus.DRAFT,src/modules/wallet/domain/value-objects/StellarAddress.ts (1)
35-37: Swallowed error makes debugging harderThe empty
catch {}prevents logging the underlyingstellar-sdkvalidation error.
Consider at least logging it under a debug level to aid future diagnosis:- } catch { + } catch (err) { + // Optional: logger.debug('Invalid Stellar address check failed', err); return false; }src/modules/auth/use-cases/email-verification.usecase.ts (1)
13-13: Minor: extract magic number for token length.
randomBytes(32)is fine, but consider aconst EMAIL_TOKEN_BYTES = 32at module scope to make intent explicit and configurable.src/modules/wallet/dto/WalletVerificationResponseDto.ts (1)
35-40: Inlineimport()type is hard to read — move to top-levelimport type.- public static fromWalletVerification( - verification: import("../domain/entities/WalletVerification").WalletVerification + import type { WalletVerification } from "../domain/entities/WalletVerification"; + public static fromWalletVerification( + verification: WalletVerificationThis keeps the method signature concise and avoids repeated relative paths if used elsewhere.
src/config/horizon.config.ts (1)
11-14: Redundant “HORIZON_URL” runtime checkBecause
horizonConfig.urlalready falls back to the default test-net URL when the env var is falsy, this guard can never throw.
Either drop the check or replace the default-value logic with a strict “must-exist” requirement.- url: process.env.HORIZON_URL || "https://horizon-testnet.stellar.org", + // Fail fast if the variable is missing or empty + url: (() => { + const val = process.env.HORIZON_URL; + if (!val) { + throw new Error("HORIZON_URL environment variable is required"); + } + return val; + })(),src/modules/user/domain/entities/user-volunteer.entity.ts (1)
21-25: Redundant manual timestamp
@CreateDateColumnwill setjoinedAtautomatically on insert; setting it manually risks clock skew and is unnecessary.- userVolunteer.joinedAt = new Date();src/modules/photo/interfaces/photo.interface.ts (1)
2-7: Consider stricter typing ofmetadatafor better type-safetyEverything looks good functionally; however, declaring
metadataasRecord<string, any>forfeits compile-time checks.
If you don’t need fully dynamic values, preferunknown(and narrow later) or define a dedicated metadata interface.- metadata?: Record<string, any>; + metadata?: Record<string, unknown>;This minor change prevents accidental misuse while keeping flexibility.
Also applies to: 9-19
src/modules/project/use-cases/GetProjectUseCase.ts (1)
10-12: Replace genericErrorwith a domain-specific Not-Found errorThrowing a bare
Errorloses semantic information and forces callers to string-match. Introduce / reuse aNotFoundError(orProjectNotFoundError) so the presentation layer can map it to the correct HTTP 404.- throw new Error("Project not found"); + throw new NotFoundError("Project not found");This will also simplify global error-handling middleware.
src/modules/user/domain/interfaces/IUserRepository.ts (1)
14-18: AvoidanyinsetVerificationTokenreturn type
Promise<any>masks the actual contract. If callers don’t use the return value, declarePromise<void>; otherwise specify the concrete DTO/entity.- ): Promise<any>; + ): Promise<void>;This small change improves correctness and aids auto-completion.
src/modules/nft/repositories/nft.repository.ts (1)
18-26: Casting throughunknownhides type mismatches
as unknown as PrismaNFTcircumvents the compiler rather than fixing the typing issue.
Prefer defining a mapper or extending Prisma types to the domain model.const persisted = await this.prisma.nFT.create({ data: {/* … */} }); const newNFT = toDomainNFT(persisted); // typed mapperThis keeps the repository strongly-typed and future-proof.
src/modules/metrics/controllers/MetricsController.ts (1)
20-22: Consider harmonising error payloadsAll three handlers now return JSON objects with an
errorstring only. If you foresee adding i18n, error codes, or logging correlation IDs, this is a good place to standardise the shape (e.g.{ code, message }).
No action required if out of scope, but worth keeping in mind to avoid future breaking changes.Also applies to: 46-50, 75-80
src/modules/nft/__tests__/domain/entities/nft.entity.test.ts (1)
60-63: Avoid brittle string matching intoThrowassertions
expect(() => nft.mint(...)).toThrow("NFT is already minted");couples the test to the exact error message.
Prefer checking the error class or a regular expression so wording tweaks don’t break tests:-expect(() => { - nft.mint("token-123", "0x123..."); -}).toThrow("NFT is already minted"); +expect(() => nft.mint("token-123", "0x123...")) + .toThrow(/already minted/i);src/middlewares/auth.middleware.ts (1)
53-54: Named + default export – double-check consumer usageExporting both
optionalAuthMiddlewareanddefaultis fine, but if some files import withimport optionalAuthMiddleware from ...and others use named import, tree-shaking may duplicate bindings.
Consider choosing one style and applying a lint rule to enforce it.src/modules/wallet/__tests__/StellarAddress.test.ts (1)
5-7: Consolidate repeated constant & add muxed-account positive case
validAddressis declared three times and uses the same literal. Extract it once at file scope to remove duplication and improve maintainability.
While at it, consider adding a happy-path test for a muxed key (prefixM…) since the VO explicitly supports it.- it("should create a valid StellarAddress with a valid public key", () => { - const validAddress = - "GCKFBEIYTKP5RDBQMUTAPDCFZDFNVTQNXUCUZMAQYVWLQHTQBDKTQRQY"; +const VALID_G_ADDRESS = + "GCKFBEIYTKP5RDBQMUTAPDCFZDFNVTQNXUCUZMAQYVWLQHTQBDKTQRQY"; + + it("should create a valid StellarAddress with a valid public key", () => { + const stellarAddress = new StellarAddress(VALID_G_ADDRESS); + expect(stellarAddress.value).toBe(VALID_G_ADDRESS); }); + + it("should create a valid StellarAddress with a valid muxed key", () => { + const muxed = + "MAAAAAAAAAAAAAB7BQ2LIZQXU4S6SLXXRWTXXRWTXXRWTXXRWTXXRWTXXRWTXXRW"; + const stellarAddress = new StellarAddress(muxed); + expect(stellarAddress.value).toBe(muxed); + });Also applies to: 36-38, 52-54, 60-64
src/modules/wallet/use-cases/VerifyWalletUseCase.ts (2)
6-8: Mark injected repository asreadonlyThe dependency is immutable after construction; marking it
readonlyprevents accidental reassignment and communicates intent.-export class VerifyWalletUseCase { - constructor(private walletRepository: IWalletRepository) {} +export class VerifyWalletUseCase { + constructor(private readonly walletRepository: IWalletRepository) {}
21-26: Surface error context & avoid shadowing unknown structures
(error: any)swallows the original stack-trace and may convert non-Errorvalues intoundefinedmessages. Propagate unknown errors upwards or at least log them; the use-case should not silently downgrade fatal infra issues to a 2xx-looking DTO.- } catch (error: any) { - return WalletVerificationResponseDto.createError( - dto.walletAddress, - error.message || "Wallet verification failed" - ); + } catch (err) { + // TODO: inject Logger and log `err` with stack + const message = + err instanceof Error ? err.message : "Wallet verification failed"; + return WalletVerificationResponseDto.createError(dto.walletAddress, message); }src/modules/project/domain/entities/project.entity.ts (1)
1-4: Domain coupling:Projectdirectly importsVolunteerentityThe project aggregate now depends on another bounded-context entity via
import { Volunteer } from "@/modules/volunteer/domain/entities/volunteer.entity";.
This tight coupling hinders modular independence and can introduce circular dependencies during compilation. Consider modelling the relation with a foreign-key (volunteerId) and mapping in the infrastructure layer instead, or extract a shared interface in a higher package.src/modules/project/__tests__/domain/entities/project.entity.test.ts (1)
14-16: Date literals can be timezone-sensitive
new Date("2024-01-01")parses as UTC, which may roll back a day on some locales when coerced to local time, causing flaky tests. Prefernew Date(Date.UTC(2024,0,1))or construct with individual parts.Also applies to: 24-26
src/modules/messaging/routes/messaging.routes.ts (1)
18-23: Wrap async route handlers to propagate errorsIf the controller methods return promises, un-handled rejections will be swallowed and the request will hang.
Consider marking the callbacksasyncand forwarding errors tonext, or use a small helper wrapper.-router.post("/", (req, res) => messagingController.sendMessage(req, res)); +router.post("/", async (req, res, next) => { + try { + await messagingController.sendMessage(req, res); + } catch (err) { + next(err); + } +});src/modules/volunteer/__tests__/domain/volunteer.entity.test.ts (1)
14-22: DRY up repeatedVolunteer.createcallsMultiple tests duplicate the entity construction. Extracting a
buildVolunteer()helper in abeforeEach(or using factory utilities liketest-data-bot) will cut noise and make failures easier to diagnose.src/modules/wallet/domain/entities/WalletVerification.ts (1)
14-16: Consider numeric-safe type forbalanceandsequenceStoring amounts and sequences as plain strings invites subtle bugs (string math, lexical sorting).
Usingbigint,Decimal.js, or a dedicatedAmountvalue-object would make arithmetic explicit and safer.src/modules/wallet/__tests__/HorizonWalletRepository.test.ts (1)
18-24: Mock path coupling to config locationThe hard-coded relative path (
../../../config/horizon.config) will break if the config is moved again during the ongoing DDD re-structure. Import via the module alias or expose the config through an index barrel to decouple tests from filesystem layout.src/repository/IUserRepository.ts (1)
1-1: Consider using the project-wide path alias instead of a long relative importImporting with
../modules/user/...may break once the file is moved again. Iftsconfigdefines an alias such as@user/*, prefer that to improve resilience to refactors.src/modules/messaging/services/MessagingService.ts (2)
20-25: Prefer passing an object for long parameter listsSix positional parameters invite ordering mistakes. Consider:
return this.messageRepository.create({ content, senderId, receiverId, volunteerId, });This also makes future additions non-breaking.
88-93: Use domain-specific error classes instead of genericErrorThrowing plain
Errorstrings complicates error handling at upper layers. Define e.g.ParticipantNotFoundErrorand map it to a 403/404 in the controller.src/modules/messaging/repositories/implementations/message-prisma.repository.ts (2)
69-70: Avoidany– leverage Prisma’s generated types
(msg: any)drops compile-time safety. Prisma generatesPrisma.MessageGetPayload<...>that can be used instead.- (msg: any) => + (msg): Message =>Or declare an explicit type beforehand.
45-49: Ordering bysentAtascending may starve paginationFetching the oldest messages first can cause clients to request many pages before reaching recent chats. Consider defaulting to
descand reversing on the client.src/modules/user/presentation/controllers/UserController.stub.ts (1)
8-43: LGTM! Well-structured stub controller.The stub controller implementation is appropriate for the migration phase:
- Consistent HTTP 501 responses across all methods
- Clear error messages indicating temporary service disruption
- Proper TODO comment for future implementation
Consider adding explicit return type annotations for consistency:
- async createUser(req: Request, res: Response) { + async createUser(req: Request, res: Response): Promise<void> {Apply similar changes to all methods for better type safety.
src/repository/PhotoRepository.ts (2)
42-63: Consider simplifying the create method logic.The current implementation creates two Photo entities and relies on non-null assertions which could be risky:
- Non-null assertions risk: Lines 44-45 use
!operators assumingurlanduserIdexist without validation- Double entity creation: The method creates a Photo entity, then creates another from the database result
Consider adding validation before the non-null assertions:
+ if (!data.url || !data.userId) { + throw new Error('url and userId are required'); + } const photo = Photo.create({ - url: data.url!, - userId: data.userId!, + url: data.url, + userId: data.userId, uploadedAt: new Date(), metadata: data.metadata || {}, });
1-1: Consider more targeted ESLint disables.The file-level ESLint disable affects the entire file. Consider using inline disables for specific lines where
anyis needed:- /* eslint-disable @typescript-eslint/no-explicit-any */Then add inline disables where needed:
metadata: any; // eslint-disable-line @typescript-eslint/no-explicit-anysrc/modules/user/presentation/controllers/userVolunteer.controller.stub.ts (1)
8-38: LGTM! Consistent stub implementation with export pattern note.The stub controller implementation follows the same pattern as other migration stubs with appropriate 501 responses.
Consider consistency in export patterns. Other stub controllers export the class as default:
export default class UserVolunteerController { // ... }While this exports an instance:
export default new UserVolunteerController();Choose one pattern for consistency across all stub controllers.
src/middleware/rateLimitMiddleware.ts (1)
20-20: Replace console.log with structured logging.This console.log should use the logger instance for consistency with the rest of the application's logging strategy.
- console.log(allowed, remaining); + this.logger.info("Rate limit check result", { + allowed, + remaining, + ip: req.ip, + path: req.path, + traceId: req.traceId, + });src/modules/organization/presentation/controllers/OrganizationController.stub.ts (1)
52-52: Consider export consistency across stub controllers.This stub exports an instance while
Project.controller.stub.tsexports the class. Consider standardizing the export pattern across all stub controllers for consistency.For consistency with other stub controllers, consider:
-export default new OrganizationController(); +export default OrganizationController;src/modules/auth/presentation/controllers/Auth.controller.stub.ts (3)
9-50: Consider reducing code duplication.All methods have identical implementations. While this is temporary stub code, you could reduce duplication with a shared response method:
+ private stubResponse(res: Response) { + res.status(501).json({ + message: "Auth service temporarily disabled during migration", + error: "Service migration in progress" + }); + } async register(req: Request, res: Response) { - res.status(501).json({ - message: "Auth service temporarily disabled during migration", - error: "Service migration in progress" - }); + this.stubResponse(res); } async login(req: Request, res: Response) { - res.status(501).json({ - message: "Auth service temporarily disabled during migration", - error: "Service migration in progress" - }); + this.stubResponse(res); }Apply similar changes to the remaining methods.
52-52: Consider export pattern for future implementation.The singleton export pattern works for stubs but may hinder testing and dependency injection in the real implementation. Consider exporting the class itself for better testability:
- export default new AuthController(); + export { AuthController }; + export default AuthController;This allows both
new AuthController()for dependency injection and default import for convenience.
8-50: Consider adding basic structure for real implementation.For the upcoming real implementation, consider:
- Type safety: Define interfaces for request/response bodies
- Logging: Add request logging for debugging during migration
- Validation: Basic request validation even in stubs
Would you like me to generate a more structured stub that includes these elements as a foundation for the real implementation?
src/__tests__/integration/system-boot.test.ts (4)
13-40: Improve application startup test approach.The current approach works but could be more robust:
- Consider using dynamic import instead of require for better TypeScript support:
- require("../../index"); + await import("../../index");
- Make the test more explicit about what constitutes success:
- expect(true).toBe(true); + expect("Application imported successfully").toBeTruthy();
- Consider extracting error validation to a helper function for reusability:
function isExpectedRuntimeError(error: unknown): boolean { const errorMessage = error instanceof Error ? error.message : String(error); const allowedErrors = [ 'prisma_1.prisma.$connect is not a function', 'connect ECONNREFUSED', 'Redis connection error', 'Database connection failed' ]; return allowedErrors.some(allowed => errorMessage.includes(allowed)); }
44-61: LGTM! Clean module structure verification.The test effectively verifies the new modular structure. Consider extracting the expected modules list to a constant at the top of the file for easier maintenance:
+ const EXPECTED_MODULES = ["auth", "user", "project", "volunteer", "organization"]; + describe("Module Structure", () => { it("should have modular structure in place", () => { // ... existing code ... - const expectedModules = ["auth", "user", "project", "volunteer", "organization"]; - expectedModules.forEach(module => { + EXPECTED_MODULES.forEach(module => { const modulePath = path.join(modulesPath, module); expect(fs.existsSync(modulePath)).toBe(true); });
63-77: LGTM! Effective legacy cleanup verification.The test properly verifies that legacy folders have been removed. Similar to the module test, consider extracting the legacy folders list:
+ const LEGACY_FOLDERS = ["controllers", "services", "entities", "errors", "dtos", "useCase"]; + it("should not have legacy folders", () => { // ... existing code ... - const legacyFolders = ["controllers", "services", "entities", "errors", "dtos", "useCase"]; - legacyFolders.forEach(folder => { + LEGACY_FOLDERS.forEach(folder => { const folderPath = path.join(srcPath, folder); expect(fs.existsSync(folderPath)).toBe(false); });
80-80: Remove trailing empty line.}); }); -
| private static swaggerDocument = YAML.parse( | ||
| fs.readFileSync("./openapi.yaml", "utf8") | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Relative path to openapi.yaml is fragile
fs.readFileSync("./openapi.yaml", "utf8") depends on the process’ CWD.
Resolve the file relative to this module to prevent runtime ENOENT when the app is started from a different folder:
-import YAML from "yaml";
+import YAML from "yaml";
+import path from "path";
...
- private static swaggerDocument = YAML.parse(
- fs.readFileSync("./openapi.yaml", "utf8")
- );
+ private static swaggerDocument = YAML.parse(
+ fs.readFileSync(
+ path.resolve(__dirname, "../../openapi.yaml"),
+ "utf8"
+ )
+ );📝 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.
| private static swaggerDocument = YAML.parse( | |
| fs.readFileSync("./openapi.yaml", "utf8") | |
| ); | |
| import YAML from "yaml"; | |
| import path from "path"; | |
| import fs from "fs"; | |
| export class SwaggerConfig { | |
| private static swaggerDocument = YAML.parse( | |
| fs.readFileSync( | |
| path.resolve(__dirname, "../../openapi.yaml"), | |
| "utf8" | |
| ) | |
| ); | |
| // …other members… | |
| } |
🤖 Prompt for AI Agents
In src/config/swagger.config.ts around lines 7 to 9, the code reads openapi.yaml
using a relative path which depends on the current working directory and can
cause ENOENT errors. Fix this by resolving the path to openapi.yaml relative to
the module file using Node's path utilities, such as path.resolve or path.join
with __dirname, to ensure the file is correctly located regardless of where the
app is started.
| if (!user) throw new Error("User not found"); | ||
| if (user.isVerified) throw new Error("User is already verified"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer domain-specific error classes over raw Error.
Using plain Error makes it difficult for higher layers (HTTP handlers, loggers, etc.) to differentiate between business errors and unexpected ones. Introduce something like UserNotFoundError / UserAlreadyVerifiedError extending Error (or a shared DomainError) so consumers can map to proper status codes.
🤖 Prompt for AI Agents
In src/modules/auth/use-cases/email-verification.usecase.ts around lines 10 to
11, replace the generic Error throws with domain-specific error classes such as
UserNotFoundError and UserAlreadyVerifiedError that extend Error or a shared
DomainError base class. Define these custom error classes to represent the
specific business errors, then throw instances of them instead of raw Error.
This allows higher layers to distinguish error types and handle them
appropriately.
| import { IUserRepository } from "../../../repository/IUserRepository"; | ||
| import { randomBytes } from "crypto"; | ||
| // import { sendVerificationEmail } from "../utils/email.utils"; // Function not found, commented out |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the updated import path and address missing email functionality.
The import path was updated to a deeper relative path, and the email sending functionality has been commented out and replaced with a console log. This breaks the actual email verification functionality.
🏁 Script executed:
#!/bin/bash
# Description: Verify the new IUserRepository import path exists and check for email utils
# Check if the new import path exists
fd -t f "IUserRepository.ts" src/repository/
# Look for email utility functions that might have been moved
rg -i "sendVerificationEmail" --type tsLength of output: 3703
Restore email functionality in ResendVerificationEmailUseCase
The sendVerificationEmail util exists at src/utils/email.utils.ts; the import in resend-email-verification.usecase.ts was incorrectly commented out and pointed to the wrong path. We need to re-enable the email send call and correct its import.
• File: src/modules/auth/use-cases/resend-email-verification.usecase.ts
- Replace the commented-out import with the correct path.
- Swap the
console.logplaceholder for the realsendVerificationEmailcall.
Suggested diff:
import { IUserRepository } from "../../../repository/IUserRepository";
-import { randomBytes } from "crypto";
-// import { sendVerificationEmail } from "../utils/email.utils"; // Function not found, commented out
+import { randomBytes } from "crypto";
+import { sendVerificationEmail } from "../../../utils/email.utils";
export class ResendVerificationEmailUseCase {
constructor(private readonly userRepository: IUserRepository) {}
async resendVerificationEmail(email: string): Promise<ResendVerificationEmailResponseDTO> {
const user = await this.userRepository.findByEmail(email);
if (!user) throw new Error("User not found");
const token = randomBytes(32).toString("hex");
await this.userRepository.updateVerificationToken(user.id, token);
const verificationLink = `https://example.com/verify?token=${token}`;
- console.log(`Resending verification email to ${email} with link ${verificationLink}`);
+ await sendVerificationEmail(email, verificationLink);
return { success: true };
}
}After this change, verify that existing tests mock sendVerificationEmail appropriately.
📝 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.
| import { IUserRepository } from "../../../repository/IUserRepository"; | |
| import { randomBytes } from "crypto"; | |
| // import { sendVerificationEmail } from "../utils/email.utils"; // Function not found, commented out | |
| import { IUserRepository } from "../../../repository/IUserRepository"; | |
| import { randomBytes } from "crypto"; | |
| import { sendVerificationEmail } from "../../../utils/email.utils"; | |
| export class ResendVerificationEmailUseCase { | |
| constructor(private readonly userRepository: IUserRepository) {} | |
| async resendVerificationEmail(email: string): Promise<ResendVerificationEmailResponseDTO> { | |
| const user = await this.userRepository.findByEmail(email); | |
| if (!user) throw new Error("User not found"); | |
| const token = randomBytes(32).toString("hex"); | |
| await this.userRepository.updateVerificationToken(user.id, token); | |
| const verificationLink = `https://example.com/verify?token=${token}`; | |
| await sendVerificationEmail(email, verificationLink); | |
| return { success: true }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/modules/auth/use-cases/resend-email-verification.usecase.ts lines 1 to 3,
the import for sendVerificationEmail is commented out and points to the wrong
path. Fix this by importing sendVerificationEmail from the correct path
src/utils/email.utils.ts, then replace the console.log placeholder in the code
with a call to sendVerificationEmail. Finally, ensure that existing tests mock
sendVerificationEmail properly to avoid test failures.
| console.log(`Verification email would be sent to ${user.email} with link: ${verificationLink}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Email functionality replaced with console logging.
The actual email sending functionality has been replaced with a console.log statement, which means users will not receive verification emails. This breaks a core authentication feature.
This needs to be addressed before merging:
- // TODO: Implement email sending functionality
- console.log(`Verification email would be sent to ${user.email} with link: ${verificationLink}`);
+ // TODO: Re-implement email sending functionality after service migration
+ // For now, this functionality is disabled during migration
+ throw new Error("Email verification service temporarily unavailable during migration");Consider adding a feature flag or proper error handling instead of silently failing to send emails.
📝 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.
| // TODO: Implement email sending functionality | |
| console.log(`Verification email would be sent to ${user.email} with link: ${verificationLink}`); | |
| } | |
| // TODO: Re-implement email sending functionality after service migration | |
| // For now, this functionality is disabled during migration | |
| throw new Error("Email verification service temporarily unavailable during migration"); | |
| } |
🤖 Prompt for AI Agents
In src/modules/auth/use-cases/resend-email-verification.usecase.ts around lines
21 to 23, the code currently uses console.log instead of sending the actual
verification email, which breaks the email verification feature. Replace the
console.log with the real email sending logic using the appropriate email
service or utility. Additionally, implement error handling to catch and log any
failures during email sending, and consider adding a feature flag to enable or
disable email sending in different environments.
| } catch (error) { | ||
| throw new Error("Invalid or expired verification token"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Catching and re-throwing loses root cause
The catch block swallows every error and re-throws a generic one, making debugging harder and hiding unexpected bugs (e.g., DB failures).
- } catch (error) {
- throw new Error("Invalid or expired verification token");
- }
+ } catch (error) {
+ // Preserve original message and stack for observability
+ throw error instanceof Error ? error : new Error(String(error));
+ }📝 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.
| } catch (error) { | |
| throw new Error("Invalid or expired verification token"); | |
| } | |
| } | |
| } catch (error) { | |
| // Preserve original message and stack for observability | |
| throw error instanceof Error ? error : new Error(String(error)); | |
| } |
🤖 Prompt for AI Agents
In src/modules/auth/use-cases/verify-email.usecase.ts around lines 50 to 52, the
catch block currently catches all errors and re-throws a generic error, which
hides the original error details and complicates debugging. Modify the catch
block to preserve the original error information by either including the
original error message in the new error or by re-throwing the original error
when it is not related to an invalid or expired token, so that unexpected errors
like database failures are not masked.
| startDate: Date; | ||
|
|
||
| @Column({ type: "date", nullable: false }) | ||
| endDate: Date | ||
| endDate: Date; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing invariant: startDate should precede endDate
Nothing prevents persisting a project whose startDate is after endDate. Add a domain guard in the constructor or a validate method to enforce chronological consistency.
🤖 Prompt for AI Agents
In src/modules/project/domain/entities/project.entity.ts between lines 24 and
28, the startDate and endDate fields lack validation to ensure startDate is
before endDate. Add a domain guard either in the constructor or a dedicated
validate method that checks if startDate is earlier than endDate, and throw an
error or prevent object creation if this invariant is violated to enforce
chronological consistency.
|
|
||
| if (!project) { | ||
| throw new Error('Project not found'); | ||
| throw new Error("Project not found"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer domain-specific exception over generic Error
Throwing new Error("Project not found") leaks a technical error into the application layer and complicates error handling in controllers/middleware.
Introduce a ProjectNotFoundError (or reuse a generic NotFoundError) and propagate that instead:
+import { ProjectNotFoundError } from "../errors/project-not-found.error";
@@
- if (!project) {
- throw new Error("Project not found");
+ if (!project) {
+ throw new ProjectNotFoundError();
}This keeps the domain boundary clean and facilitates consistent HTTP mapping (e.g., 404).
🤖 Prompt for AI Agents
In src/modules/project/use-cases/DeleteProjectUseCase.ts around lines 8 to 11,
replace the generic Error thrown when the project is not found with a
domain-specific exception like ProjectNotFoundError or a reusable NotFoundError.
Define or import this specific error class and throw it instead to maintain
clear domain boundaries and enable consistent error handling and HTTP response
mapping.
| public equals(entity: Entity<T>): boolean { | ||
| if (entity === null || entity === undefined) { | ||
| return false | ||
| return false; | ||
| } | ||
|
|
||
| if (this === entity) { | ||
| return true | ||
| return true; | ||
| } | ||
|
|
||
| return JSON.stringify(this.props) === JSON.stringify(entity.props) | ||
| return JSON.stringify(this.props) === JSON.stringify(entity.props); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
equals() comparison via JSON.stringify is brittle & slow
Serialising objects to JSON for equality:
- Fails when property order differs.
- Breaks on non-serialisable props (e.g., Dates, Maps).
- Adds unnecessary performance overhead on large graphs.
Consider a deep-comparison helper instead (e.g., lodash isEqual) or implement a domain-specific equality method.
- return JSON.stringify(this.props) === JSON.stringify(entity.props);
+ // Prefer a robust deep-equality check
+ return isEqual(this.props, entity.props); // requires `import { isEqual } from "lodash";`Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/shared/domain/entities/base.entity.ts around lines 25 to 35, the
equals() method uses JSON.stringify for comparison, which is brittle and
inefficient. Replace the JSON.stringify comparison with a deep equality check
using a helper like lodash's isEqual or implement a custom deep comparison
method that correctly handles property order and non-serializable types such as
Dates and Maps to ensure accurate and performant equality checks.
| const user = new User(); | ||
| user.name = "John"; | ||
| user.lastName = "Doe"; | ||
| user.email = "[email protected]"; | ||
| user.password = "hashedPassword"; | ||
| user.wallet = "GXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"; | ||
| user.isVerified = false; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Direct property mutation bypasses entity invariants
Setting entity fields one-by-one sidesteps any validation normally enforced inside the constructor/factory. Prefer using the public factory or constructor with arguments so tests represent real-world usage, e.g.:
const user = User.create({
name: "John",
lastName: "Doe",
email: "[email protected]",
password: "hashedPassword",
wallet: "GXXXXXXXX...",
});🤖 Prompt for AI Agents
In src/modules/user/__tests__/domain/entities/user.entity.test.ts between lines
6 and 13, the test code directly assigns values to User entity properties,
bypassing any validation or invariants enforced by the constructor or factory
methods. To fix this, replace the direct property assignments with a call to the
User factory or constructor that accepts an object with all required fields,
such as User.create({ name, lastName, email, password, wallet }), ensuring the
test uses the entity as it would be used in real scenarios and respects
validation rules.
| @PrimaryColumn("uuid") | ||
| userId: string | ||
| userId: string; | ||
|
|
||
| @PrimaryColumn("uuid") | ||
| volunteerId: string | ||
| volunteerId: string; | ||
|
|
||
| @CreateDateColumn() | ||
| joinedAt: Date | ||
| joinedAt: Date; | ||
|
|
There was a problem hiding this comment.
Properties violate strictPropertyInitialization
userId, volunteerId, and joinedAt are declared without definite-assignment assertions, triggering TS2339 when strict mode is on.
- userId: string;
+ userId!: string;
...
- volunteerId: string;
+ volunteerId!: string;
...
- joinedAt: Date;
+ joinedAt!: Date;📝 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.
| @PrimaryColumn("uuid") | |
| userId: string | |
| userId: string; | |
| @PrimaryColumn("uuid") | |
| volunteerId: string | |
| volunteerId: string; | |
| @CreateDateColumn() | |
| joinedAt: Date | |
| joinedAt: Date; | |
| @PrimaryColumn("uuid") | |
| userId!: string; | |
| @PrimaryColumn("uuid") | |
| volunteerId!: string; | |
| @CreateDateColumn() | |
| joinedAt!: Date; |
🤖 Prompt for AI Agents
In src/modules/user/domain/entities/user-volunteer.entity.ts between lines 6 and
14, the properties userId, volunteerId, and joinedAt are declared without
definite-assignment assertions, causing TypeScript errors under strict mode. To
fix this, add definite-assignment assertions (!) after each property name to
inform TypeScript that these properties will be initialized elsewhere,
preventing the strictPropertyInitialization error.
1 participant
🚀 Volunchain Pull Request
Mark with an
xall the checkboxes that apply (like[x])application/services#126📌 Type of Change
📝 Changes description
This PR refactors the entire
src/servicesstructure by moving all domain-specific services into their respective module folders undersrc/modules/<domain>/application/services. All dependent imports across the codebase have been updated accordingly. In addition, unit tests were added or migrated tosrc/modules/<domain>/__tests__/services/for each affected service to ensure consistent functionality. Once the migration was complete, the oldsrc/servicesfolder was deleted. This change enforces the DDD (Domain-Driven Design) structure and improves maintainability.⏰ Time spent breakdown
⏳ Total time spent: ~4h 30min
🌌 Comments
Thank you for contributing to Volunchain, we are glad that you have chosen us as your project of choice and we hope that you continue to contribute to this great project, so that together we can make our mark at the top!
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores