refactor/services-into-application-module #135
Conversation
WalkthroughThis change is a large-scale codebase refactor focused on stylistic consistency, import path standardization, and modularization. It updates import paths to reflect a deeper module structure, converts all string literals to double quotes, adds trailing commas and consistent formatting, and removes or relocates Soroban-related service files and documentation. No functional or logic changes are introduced. Changes
Sequence Diagram(s)Omitted—no control flow or feature changes introduced. Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (6)
✨ Finishing Touches
🪧 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: 13
🔭 Outside diff range comments (6)
src/modules/project/application/services/ProjectService.ts (1)
111-118: Possible data-loss:createMany+ name-based re-query can miss duplicates
createManyreturns only a count, so we re-query bynameto fetch the inserted records.
If two volunteers share the same name (or the client retries the call), the subsequentfindManymay pull more rows than the ones just inserted and the mapping will be wrong.- const volunteersData = data.initialVolunteers.map((volunteer) => ({ - ...volunteer, - projectId: project.id, - })); - await tx.volunteer.createMany({ data: volunteersData }); - const projectWithVolunteers = await tx.project.findUnique({ … }); +const createdVolunteers = await Promise.all( + data.initialVolunteers.map((v) => + tx.volunteer.create({ + data: { ...v, projectId: project.id }, + include: { project: true }, + }), + ), +); + +// project already has volunteers loaded, just concat +project.volunteers = createdVolunteers;Keeps the transaction atomic and guarantees that only the freshly created rows are returned.
src/controllers/Project.controller.ts (1)
21-23:new Date(string)silently producesInvalid DateWhen the client sends an invalid or locale-specific timestamp,
new Date()will
returnInvalid Datewithout throwing, and Prisma will reject later inside the
service. Validate the input early and return a 400.const start = new Date(startDate); if (Number.isNaN(start.getTime())) { return res.status(400).json({ error: "startDate must be ISO-8601" }); }src/modules/organization/application/services/OrganizationService.ts (1)
32-34: Avoid creating a newPrismaClientper service instance
private prisma = new PrismaClient();inside a service that may be instantiated multiple times (e.g. per request) risks leaking TCP connections and hitting the 10-connection limit on many Postgres deployments.- private prisma = new PrismaClient(); + // Re-use the singleton defined in src/config/prisma.ts + private prisma = prisma; // import { prisma } from "../../../../config/prisma";Couple this with dependency injection so tests can pass a mocked client.
src/modules/soroban/__tests__/services/SorobanService.test.ts (1)
1-10: Expand test coverage beyond basic instantiation.While the basic instantiation test is a good start, consider adding tests for the actual functionality of
SorobanService. This would provide better confidence in the service behavior and catch potential regressions.Would you like me to help generate more comprehensive test cases for the
SorobanServicebased on its actual methods and functionality?src/modules/soroban/application/services/SorobanService.ts (1)
52-57: Migrate tostellar-sdkand remove the@ts-ignoreThe legacy
soroban-clientAPI is deprecated—TypeScript support and contract invocation have moved into thestellar-sdkpackage. You should replace the uncheckedinvokeContractcall with the strongly-typed workflow fromstellar-sdk, then drop the@ts-ignore.• File:
src/modules/soroban/application/services/SorobanService.ts
Lines: 52–57Suggested changes:
- // @ts-ignore - Ignoring type checking until we know the exact API - const result = await this.server.invokeContract( - contractId, - methodName, - args - ); + // Build, sign, and submit a Soroban transaction via stellar-sdk + import { SorobanServer, TransactionBuilder, Networks } from 'stellar-sdk'; + import { ScVal } from 'stellar-sdk'; // or helper conversions + + const server = new SorobanServer(this.rpcUrl); + const tx = new TransactionBuilder(this.sourceAccount, Networks.TESTNET) + .fee(1000) + .addOperation( + // replace with the proper contract-call helper from stellar-sdk + server.contractCall(contractId, methodName, args.map(arg => ScVal.from(arg))) + ) + .build(); + const result = await server.sendTransaction(tx);• Remove the
@ts-ignoreonce the correct types are in place.
• See the stellar-sdk Soroban migration guide for full examples.src/middlewares/errorHandler.ts (1)
24-37: UnusederrorInfoobject – remove or log it
errorInfois fully built but never used, creating dead code every request. Either:
- Feed it to
logger.error(if you want a condensed payload), or- Drop the allocation altogether.
- const errorInfo: ErrorLogInfo = { ... }; + // removed – structured meta is already sent below
🧹 Nitpick comments (23)
tests/Photo.test.ts (2)
23-31: Clarify test description to match actual inputThe description says “when no props are provided”, yet you pass an empty object (
{}) rather than omitting the argument.
Either rename the test to something like “when empty props are provided” or add an additional test that truly callsnew Photo()(and guard the constructor accordingly).
88-100: Description implies undefined metadata, but constructor always defaults to{}
Photo’s constructor already initializesmetadatato{}when it is absent, so the metadata is neverundefined.
Rename the test to reflect this behaviour (e.g. “should initialise metadata when not provided”) or add an assertion in the constructor unit test to explicitly show the defaulting.src/modules/project/application/services/ProjectService.ts (1)
195-201: ManualupdatedAtoverride defeats Prisma’s auto-updatePrisma can auto-manage
updatedAtfields. Explicitly setting it risks
clock-skew bugs and hides schema defaults.- data: { - ...data, - updatedAt: new Date(), - }, + data,src/modules/shared/application/services/LoggerService.ts (2)
55-58: Nestedmetaobject makes querying logs harder
{ …context, meta: { … } }forces consumers to drill one level deeper.
Flattening keeps the structure consistent with many log viewers.-return { - ...context, - ...(meta && { meta }), -}; +return { ...context, ...(meta ?? {}) };
84-90: Parameter name shadows the built-inErrorsymbolUsing
errorboth as parameter and inside the template hinders stack-trace tools
and ESLint’sno-shadowrules.-error(message: string, error?: Error | any, req?: Request, meta?: LogMeta): void { +error(message: string, cause?: unknown, req?: Request, meta?: LogMeta): void { const context = this.createRequestContext(req); - const logMeta = this.createLogMeta(context, { - ...meta, - ...(error && { error: { … } }), + const logMeta = this.createLogMeta(context, { + ...meta, + ...(cause instanceof Error && { + error: { + message: cause.message, + stack: cause.stack, + name: cause.name, + ...(cause as any).code && { code: (cause as any).code }, + ...(cause as any).statusCode && { statusCode: (cause as any).statusCode }, + }, + }), });src/controllers/UserController.ts (1)
8-9: Consider injectingUserServiceinstead of instantiating per controllerIn typical Express setups the controller instance is cached, but if this file is
imported multiple times (e.g., in tests) each import will create a brand-new
UserService, making mocking harder. Passing the service via the constructor
keeps the controller testable and respects inversion of control.src/controllers/userVolunteer.controller.ts (1)
2-2: Consider switching to a path alias to avoid fragile../..importsThe relative path is correct, but as the project grows these deep traversals become error-prone whenever files are moved.
If you already have (or plan to add) a TS path alias such as@volunteer/services, importing with:import { UserVolunteerService } from "@volunteer/services/UserVolunteerService";keeps future refactors painless and improves readability.
src/controllers/VolunteerController.ts (1)
2-2: Same alias suggestion as aboveReplacing long relative hops with a configured alias (e.g.
@volunteer/services) will make these imports shorter and less brittle.src/modules/organization/application/services/OrganizationService.ts (1)
58-66: Race condition in uniqueness checks
findUnique+createcan still allow two concurrent requests with the same email/wallet to slip through. Rely on DB unique constraints and catch theP2002Prisma error instead of manual pre-checks.src/modules/nft/__tests__/services/NFTService.test.ts (1)
4-14: Test only checks definition – expand coverageThe current test merely asserts that the class exists, providing no behavioural guarantee. Consider:
- let nftService: typeof NFTService; + let nftService: NFTService; beforeEach(() => { - nftService = NFTService; + nftService = new NFTService(); }); it("should be defined", () => { expect(nftService).toBeDefined(); }); + it("createNFT should throw on missing fields", async () => { + await expect( + nftService.createNFT({ userId: "", organizationId: "", description: "" }) + ).rejects.toThrow("missing_required_fields"); + });This increases real coverage and removes the unused
jestimport.src/modules/user/application/services/UserService.ts (1)
39-39: Minor formatting: Remove extra blank line.There's an unnecessary blank line that could be removed for consistency with the rest of the file.
-src/modules/organization/__tests__/services/OrganizationService.test.ts (1)
1-14: Good basic test structure, consider expanding coverage.The test file follows good practices with proper Jest setup and correct import paths for the new DDD structure. However, consider adding more comprehensive tests beyond just checking if the service is defined.
Consider adding tests for the actual service methods:
it("should create organization successfully", async () => { // Test createOrganization method }); it("should get organization by id", async () => { // Test getOrganizationById method });src/modules/project/__tests__/services/ProjectService.test.ts (1)
1-14: Fix quote inconsistency and consider expanding test coverage.The test structure is good, but there are style inconsistencies with the rest of the codebase. This file uses single quotes while other files in this PR use double quotes consistently.
Apply this diff to maintain consistency:
-import { describe, it, expect, beforeEach, jest } from '@jest/globals'; -import ProjectService from '../../application/services/ProjectService'; +import { describe, it, expect, beforeEach, jest } from "@jest/globals"; +import ProjectService from "../../application/services/ProjectService"; -describe('ProjectService', () => { +describe("ProjectService", () => { let projectService: ProjectService; beforeEach(() => { projectService = new ProjectService(); }); - it('should be defined', () => { + it("should be defined", () => { expect(projectService).toBeDefined(); }); -}); +});Also consider adding more comprehensive tests for the service methods.
src/scripts/sorobanDemo.ts (3)
1-1: Avoid fragile relative import – introduce a path alias instead
../modules/soroban/application/services/SorobanServicebreaks once the script is transpiled todist/(the relative depth changes).
Add apathsalias (e.g.@soroban/services) intsconfig.jsonand leverage it here to keep the demo runnable from bothsrcanddist.-import { sorobanService } from "../modules/soroban/application/services/SorobanService"; +import { sorobanService } from "@soroban/services";
20-32: Prefer project logger overconsole.logfor consistencyAll other layers already route logs through
LoggerService; using plainconsole.loghere bypasses log-level filtering, formatting, and trace-ID correlation.- console.log("Starting SorobanService demo..."); + import { globalLogger } from "../modules/shared/application/services/LoggerService"; + globalLogger.info("Starting SorobanService demo");
87-95: Exit handling swallows non-0 exit codesInside the
catch, you alreadyprocess.exit(1).
But inside the happy-path.then, you exit with0even when the demo emitted a non-handled rejection. Prefer letting Node’s default behaviour surface or move theexit(0)into thetry/catchitself.src/middlewares/errorHandler.ts (1)
38-45: Meta duplication – timestamp calculated twiceA new timestamp is generated here even though one exists in
errorInfo. If you keeperrorInfo, reuse itstimestampto avoid drift; otherwise drop the earlier calculation.scripts/test-logging.ts (1)
80-87:globalLogger.infoshould passreqfor correlationYou omit the third arg (
req) here, so trace-ID & HTTP context are lost for the startup log. Passundefinedexplicitly or create a synthetic context object.src/modules/wallet/__tests__/WalletAuthIntegration.test.ts (2)
41-60: Mixingrequireand ESM imports – stay consistentYou switch to
requireonly for the Prisma mock. Preferjest.mock("@prisma")or an import statement to avoid TS/eslint interop warnings and keep tree-shaking intact.
95-146: Large inline mock objects – extract into helpers for reuseThe verbose mock wiring (Prisma repository, email use-case, wallet verification result) is duplicated in other auth tests. Extract factories into
tests/__mocks__/to keep this file focused on behaviour.src/tests/sorobanService.test.ts (1)
62-83: UsemockRejectedValueOncebut never assert fallback behaviourAfter forcing a rejection you only assert the thrown error, not that
invokeContractwas called. Add an expectation to guard against silent path skips.expect(mockServer.invokeContract).toHaveBeenCalledWith( mockContractId, mockMethodName, mockArgs );src/modules/user/__tests__/services/UserService.test.ts (1)
35-45: Consider expanding test coverage.While the basic tests are good, consider adding tests for error scenarios, edge cases, and other UserService methods like
getUserByEmail,getUsers, anddeleteUserto ensure comprehensive coverage.src/modules/volunteer/__tests__/services/VolunteerService.test.ts (1)
33-37: Consider expanding test coverage.The test suite only covers basic create and get operations. Consider adding tests for
getVolunteersByProjectId,updateVolunteer,deleteVolunteer, and error scenarios to ensure comprehensive coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
scripts/test-logging.ts(4 hunks)scripts/validate-logging.ts(13 hunks)src/controllers/Auth.controller.ts(6 hunks)src/controllers/NFTController.ts(1 hunks)src/controllers/OrganizationController.ts(1 hunks)src/controllers/Project.controller.ts(1 hunks)src/controllers/UserController.ts(1 hunks)src/controllers/VolunteerController.ts(1 hunks)src/controllers/userVolunteer.controller.ts(1 hunks)src/examples/sorobanExample.ts(1 hunks)src/index.ts(4 hunks)src/middleware/rateLimitMiddleware.ts(2 hunks)src/middlewares/errorHandler.ts(3 hunks)src/middlewares/requestLogger.middleware.ts(2 hunks)src/modules/auth/__tests__/services/AuthService.test.ts(1 hunks)src/modules/auth/application/services/AuthService.ts(6 hunks)src/modules/messaging/repositories/implementations/message-prisma.repository.ts(4 hunks)src/modules/nft/__tests__/services/NFTService.test.ts(1 hunks)src/modules/nft/application/services/NFTService.ts(1 hunks)src/modules/organization/__tests__/services/OrganizationService.test.ts(1 hunks)src/modules/organization/application/services/OrganizationService.ts(2 hunks)src/modules/project/__tests__/services/ProjectService.test.ts(1 hunks)src/modules/project/application/services/ProjectService.ts(13 hunks)src/modules/shared/__tests__/services/LoggerService.test.ts(1 hunks)src/modules/shared/application/services/LoggerService.ts(10 hunks)src/modules/soroban/__tests__/services/SorobanService.test.ts(1 hunks)src/modules/soroban/application/services/SorobanService.ts(6 hunks)src/modules/user/__tests__/services/UserService.test.ts(1 hunks)src/modules/user/application/services/UserService.ts(2 hunks)src/modules/volunteer/__tests__/services/VolunteerService.test.ts(1 hunks)src/modules/volunteer/__tests__/services/userVolunteer.service.test.ts(3 hunks)src/modules/volunteer/application/services/UserVolunteerService.ts(1 hunks)src/modules/volunteer/application/services/VolunteerService.ts(1 hunks)src/modules/wallet/__tests__/WalletAuthIntegration.test.ts(5 hunks)src/scripts/sorobanDemo.ts(2 hunks)src/services/README.md(0 hunks)src/services/soroban/README.md(0 hunks)src/services/soroban/demo.ts(0 hunks)src/services/soroban/index.ts(0 hunks)src/services/sorobanService.ts(0 hunks)src/tests/sorobanService.test.ts(2 hunks)src/utils/db-monitor.ts(0 hunks)tests/Photo.test.ts(2 hunks)tests/logging/winston-logger.test.ts(1 hunks)tests/middlewares/errorHandler.test.ts(6 hunks)tests/prisma/testprisma.test.ts(1 hunks)tests/soroban/sorobanService.test.ts(2 hunks)tests/sorobanService.test.ts(2 hunks)tests/wallet/WalletAuthIntegration.test.ts(6 hunks)
💤 Files with no reviewable changes (6)
- src/utils/db-monitor.ts
- src/services/soroban/index.ts
- src/services/soroban/README.md
- src/services/README.md
- src/services/soroban/demo.ts
- src/services/sorobanService.ts
🧰 Additional context used
🧬 Code Graph Analysis (12)
src/modules/nft/__tests__/services/NFTService.test.ts (1)
src/modules/nft/application/services/NFTService.ts (1)
NFTService(8-35)
src/scripts/sorobanDemo.ts (2)
src/modules/soroban/application/services/SorobanService.ts (1)
sorobanService(69-69)src/modules/shared/application/services/LoggerService.ts (1)
error(84-105)
src/modules/soroban/application/services/SorobanService.ts (1)
src/modules/shared/application/services/LoggerService.ts (1)
error(84-105)
src/modules/shared/__tests__/services/LoggerService.test.ts (1)
src/modules/shared/application/services/LoggerService.ts (1)
LoggerService(25-185)
src/examples/sorobanExample.ts (2)
src/modules/soroban/application/services/SorobanService.ts (1)
sorobanService(69-69)src/modules/shared/application/services/LoggerService.ts (1)
error(84-105)
tests/Photo.test.ts (1)
src/entities/Photo.ts (1)
Photo(1-26)
src/modules/user/__tests__/services/UserService.test.ts (4)
src/modules/user/application/services/UserService.ts (1)
UserService(13-47)src/modules/user/repositories/PrismaUserRepository.ts (1)
PrismaUserRepository(7-78)src/modules/user/dto/CreateUserDto.ts (1)
CreateUserDto(1-7)src/modules/user/dto/UpdateUserDto.ts (1)
UpdateUserDto(1-8)
src/modules/volunteer/__tests__/services/VolunteerService.test.ts (3)
src/modules/volunteer/application/services/VolunteerService.ts (1)
VolunteerService(10-62)src/modules/volunteer/repositories/implementations/volunteer-prisma.repository.ts (1)
VolunteerPrismaRepository(5-91)src/modules/volunteer/dto/volunteer.dto.ts (1)
CreateVolunteerDTO(1-7)
src/index.ts (1)
src/modules/shared/application/services/LoggerService.ts (1)
globalLogger(188-188)
scripts/validate-logging.ts (4)
src/modules/shared/application/services/LoggerService.ts (2)
createLogger(191-193)error(84-105)src/config/winston.config.ts (1)
createLogger(115-115)scripts/validate-implementation.js (2)
path(9-9)fs(8-8)src/modules/project/domain/Project.ts (1)
status(57-59)
src/modules/volunteer/__tests__/services/userVolunteer.service.test.ts (2)
src/modules/volunteer/domain/volunteer.entity.ts (1)
Volunteer(3-121)src/errors/VolunteerRegistrationError.ts (3)
VolunteerNotFoundError(22-27)VolunteerAlreadyRegisteredError(15-20)VolunteerPositionFullError(8-13)
src/middlewares/requestLogger.middleware.ts (1)
src/modules/shared/application/services/LoggerService.ts (1)
createLogger(191-193)
🔇 Additional comments (41)
tests/prisma/testprisma.test.ts (4)
5-5: LGTM: Model reference updated from plural to singular.The change from
prisma.userstoprisma.usercorrectly aligns with Prisma's convention of using singular model names. This is consistent throughout all CRUD operations.Also applies to: 16-16, 19-19, 26-26, 36-36
7-7: LGTM: String quotes standardized to double quotes.The string literals have been consistently updated to use double quotes throughout the file, which improves code consistency.
Also applies to: 8-8, 9-9, 10-10, 11-11, 14-14, 17-17, 21-21, 24-24, 31-31, 34-34, 41-41
1-1: No remaininguserstype imports – safe to removeAll references to the old
userstype from@prisma/clienthave been removed. The only match found was a runtime variable usage insrc/routes/testRoutes.ts, not a type annotation.– No import of
userstype remains
– No: userstype annotations found
7-11: All user model fields in tests align with the Prisma schema.The fields
name,password,wallet, andisVerifiedintests/prisma/testprisma.test.tsmatch the currentmodel Userdefinition inschema.prisma(and the generated SQL). No changes required.src/modules/project/application/services/ProjectService.ts (1)
451-457: Timezone edge-case in “start date cannot be in the past”
data.startDate < new Date()compares raw Date objects, which may fail just after midnight in different TZs.
Safer: compare on UTC or strip the time part.const now = new Date(); if (data.startDate.getTime() < now.getTime()) { throw new ValidationError("Start date cannot be in the past"); }src/controllers/NFTController.ts (1)
2-2: Verify thatNFTServiceexposes static methodsThe controller calls
NFTService.createNFT()directly, implyingNFTService
exports either a singleton object or a class with static methods. If it is a
class meant to be instantiated, these calls will throw at runtime.src/modules/volunteer/application/services/UserVolunteerService.ts (1)
6-6: Import path correctly updated for DDD structure.The import path has been properly adjusted to reflect the service's new location in the domain module hierarchy. The relative path
../../../../errors/VolunteerRegistrationErrorcorrectly navigates fromsrc/modules/volunteer/application/services/to the errors directory.tests/logging/winston-logger.test.ts (2)
2-2: Import path correctly updated for shared module structure.The import path has been properly updated to reference the
LoggerServicefrom its new location in the shared module (src/modules/shared/application/services/LoggerService), aligning with the DDD refactoring.
6-6: Good practice: Explicit Jest imports for clarity.Explicitly importing Jest globals from
@jest/globalsimproves code clarity and helps with IDE support and type checking.src/modules/user/application/services/UserService.ts (1)
1-11: Import paths correctly updated for DDD module structure.All import paths have been properly adjusted to reflect the service's new location in the user domain module. The relative paths correctly navigate from
src/modules/user/application/services/to the DTOs, use cases, and repository files within the user module.src/modules/nft/application/services/NFTService.ts (1)
1-6: LGTM! Import paths correctly updated for DDD structure.The import path changes accurately reflect the new module positioning. All relative paths correctly point to their respective directories within the NFT module structure.
src/modules/soroban/application/services/SorobanService.ts (2)
2-2: Import path correctly updated for new module structure.The updated import path with additional directory traversals correctly reflects the new positioning of the SorobanService within the DDD structure.
12-12: Good style consistency with double quotes.The quote changes align with the project's coding standards for consistent use of double quotes.
Also applies to: 29-29, 60-60
src/middleware/rateLimitMiddleware.ts (2)
3-3: Import path correctly updated for DDD structure.The import path change properly reflects the new location of LoggerService in the shared module's application services directory.
33-33: Good formatting improvements with trailing commas.The addition of trailing commas improves code consistency and reduces future diff noise when adding new properties.
Also applies to: 41-41
src/modules/volunteer/application/services/VolunteerService.ts (1)
1-8: LGTM! Import paths correctly implement DDD structure.The updated relative import paths properly reflect the new Domain-Driven Design architecture, maintaining appropriate separation between domain, application, and infrastructure layers.
src/controllers/Auth.controller.ts (2)
2-2: LGTM! Import path correctly updated for DDD structure.The updated import path properly references the AuthService in its new modular location.
25-27: Good formatting improvement in error handling.The conversion from multiline to single-line error response formatting improves code conciseness while maintaining the same functionality.
Also applies to: 48-50, 69-72, 83-85, 104-109
src/modules/messaging/repositories/implementations/message-prisma.repository.ts (1)
1-3: LGTM! Good stylistic improvements for consistency.The formatting changes improve code consistency with double quotes, concise array syntax, and explicit typing without altering functionality.
Also applies to: 45-45, 48-48, 69-69, 144-144, 150-150
src/examples/sorobanExample.ts (2)
1-1: LGTM! Import path updated for DDD structure.The import path correctly references the relocated SorobanService. Based on the relevant code snippets, the
sorobanServicesingleton instance is properly exported from the new location.
9-50: Good formatting improvements for consistency.The conversion to double quotes and improved formatting enhances code consistency across the project.
src/modules/auth/application/services/AuthService.ts (2)
1-8: LGTM! Import paths correctly reflect DDD module structure.The updated import paths properly navigate the new domain-driven architecture, correctly accessing shared configuration and cross-module dependencies.
19-21: Good formatting improvements throughout the service.The formatting changes enhance code readability and consistency, including improved method signatures, object literals, and string literal standardization.
Also applies to: 23-25, 49-55, 59-61, 67-67, 75-75, 84-84, 102-103, 119-119
src/middlewares/errorHandler.ts (1)
48-54:traceIdproperty is not part of Express typings
req.traceIdcompiles only if you have an ambient declaration augmentingexpress.Request. Verify that@types/expressis augmented (e.g.src/types/express/index.d.ts) in this branch; otherwise TS will error in strict mode.src/index.ts (2)
23-25: LGTM: Import path updates align with DDD refactoring.The updated import paths for
messageRoutesandglobalLoggercorrectly reflect the new module-based structure. The migration from centralized services to domain-specific modules improves maintainability and separation of concerns.
34-34: Good: String quote standardization and formatting improvements.The consistent use of double quotes and improved formatting for logging calls enhances code readability and maintains style consistency across the codebase.
Also applies to: 42-43, 187-194
tests/middlewares/errorHandler.test.ts (3)
22-24: Improved mock completeness.Adding
url,ip, and thegetmethod to the mock request makes the test more realistic and better simulates actual Express request objects.
107-139: Updated logging test approach is more accurate.Mocking the Winston logger instead of
console.errorbetter reflects the actual logging implementation used in the error handler middleware.
45-45: Error response format verified ✅The
errorHandlerinsrc/middlewares/errorHandler.tsalready returns a JSON body with a singlecodefield (fromerr.codeorinternalError.toJSON()), matching the tests’ expectations. No further changes are needed.tests/soroban/sorobanService.test.ts (2)
1-1: LGTM: Import path updated for DDD structure.The import path correctly reflects the new module-based structure where Soroban services are now located in
src/modules/soroban/application/services/.
4-31: Good: Consistent formatting and quote standardization.The standardization to double quotes and improved mock formatting enhances code consistency while preserving all test functionality.
Also applies to: 33-94
scripts/validate-logging.ts (2)
6-9: LGTM! Import path updated correctly for module restructuring.The import path has been properly updated to reflect the new DDD module structure, moving from the centralized services directory to the domain-specific module location.
10-467: Code formatting standardized successfully.All string literals have been consistently converted to double quotes and object/function formatting has been standardized with proper multiline structure and trailing commas. The validation logic remains intact and comprehensive.
tests/wallet/WalletAuthIntegration.test.ts (2)
1-2: Import paths updated correctly for module restructuring.The import paths have been properly updated to reflect the new domain-driven module structure while maintaining consistent import style.
5-247: Test logic preserved with consistent style formatting.All test cases, mock implementations, and assertions remain functionally identical. The string quote standardization and formatting improvements enhance code consistency without altering test coverage.
tests/sorobanService.test.ts (2)
1-1: Import path correctly updated for Soroban service module.The import path reflects the new module structure, moving the Soroban service into its dedicated domain module under
application/services.
4-94: Mock setup and test logic preserved with improved formatting.All test cases maintain their original coverage for transaction submission and contract invocation scenarios. The formatting standardization with double quotes and consistent object structure improves readability.
src/modules/volunteer/__tests__/services/userVolunteer.service.test.ts (2)
1-15: Import paths updated correctly for module structure.All import paths have been properly updated to reflect the new domain module organization, including the volunteer service and error classes.
33-239: Comprehensive test coverage preserved with improved formatting.All test scenarios for volunteer registration are maintained, including edge cases for concurrent registrations and error conditions. The formatting standardization with double quotes, trailing commas, and consistent spacing enhances code readability.
src/middlewares/requestLogger.middleware.ts (2)
2-2: Import path updated correctly for shared logger service.The import path properly reflects the new module structure, moving the logger service to the shared domain module.
10-75: Middleware functionality preserved with formatting improvements.The core request/response logging logic remains intact. The multiline function signatures and standardized object formatting improve readability. The addition of
errorResponse: trueflag in the error-only middleware provides better log categorization.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/modules/auth/__tests__/services/AuthService.test.ts (2)
347-368: Add error handling test for wallet validation.The validateWalletFormat test only covers the success case. Following the previous review feedback, an error handling test case is still missing to ensure proper error handling.
Add this test case after the existing validateWalletFormat test:
+ it("should handle wallet format validation failure", async () => { + const walletAddress = "invalid-format-wallet"; + const expectedResult = { + success: false, + isValid: false, + error: "Invalid wallet format" + }; + + mockWalletService.validateWalletFormat.mockResolvedValue(expectedResult); + + const result = await authService.validateWalletFormat(walletAddress); + + expect(result).toEqual(expectedResult); + expect(mockWalletService.validateWalletFormat).toHaveBeenCalledWith( + walletAddress + ); + expect(mockWalletService.validateWalletFormat).toHaveBeenCalledTimes(1); + });
137-256: Add test for wallet address already in use scenario.The register method tests are missing the critical edge case where a user tries to register with a wallet address that's already associated with another account, as identified in previous reviews.
Add this test case after the existing register tests:
+ it("should throw error when wallet address is already in use", async () => { + const userData = { + name: "Test", + lastName: "User", + email: "[email protected]", + password: "password123", + wallet: "existing-wallet-address", + }; + + const mockWalletVerification = { + success: true, + isValid: true, + accountExists: true, + walletAddress: userData.wallet, + verifiedAt: new Date(), + message: "Wallet verified", + }; + + mockWalletService.verifyWallet.mockResolvedValue(mockWalletVerification); + mockUserRepository.findByEmail.mockResolvedValue(null); + mockPrisma.user.findUnique.mockResolvedValue({ + id: "existing-user", + wallet: userData.wallet, + }); + + await expect( + authService.register( + userData.name, + userData.lastName, + userData.email, + userData.password, + userData.wallet + ) + ).rejects.toThrow("User with this wallet address already exists"); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)src/modules/auth/__tests__/services/AuthService.test.ts(1 hunks)src/modules/user/__tests__/services/UserService.test.ts(1 hunks)src/modules/volunteer/__tests__/services/VolunteerService.test.ts(1 hunks)src/modules/volunteer/__tests__/services/userVolunteer.service.test.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/modules/volunteer/tests/services/userVolunteer.service.test.ts
- src/modules/volunteer/tests/services/VolunteerService.test.ts
- src/modules/user/tests/services/UserService.test.ts
|
@shamoo53 pls solve conflicts. |
…into refactor/services-into-application-module
|
@Villarley conflict resolved |
|
@Villarley conflict resolved |
2 participants
🚀 Volunchain Pull Request
Mark with an
xall the checkboxes that apply (like[x])Service Refactoring: Centralized to Domain-Driven Design
Overview
Successfully refactored the VolunChain-Backend codebase from a centralized service architecture to a clean Domain-Driven Design (DDD) structure by moving all services from src/services/ into their respective domain modules under src/modules//application/services/.
Implementation Steps
Analyzed the centralized src/services/ directory to identify all service files
Mapped each service to its corresponding domain module:
AuthService.ts → src/modules/auth/application/services/
NFTService.ts → src/modules/nft/application/services/
OrganizationService.ts → src/modules/organization/application/services/
ProjectService.ts → src/modules/project/application/services/
VolunteerService.ts → src/modules/volunteer/application/services/
WalletService.ts → src/modules/wallet/application/services/
LoggerService.ts → src/modules/shared/application/services/
Created application/services/ directories in each domain module where they didn't exist
Ensured proper DDD folder structure: src/modules//application/services/
Moved each service file to its designated domain module
Updated internal imports within each service to reference new module paths
Maintained service functionality while relocating to domain-specific locations
Controllers: Updated all controller imports to reference new service locations
Middleware: Fixed middleware imports for services like LoggerService
Scripts: Updated utility scripts and test scripts
Tests: Updated existing test files to import from new service locations
Routes: Updated route files that imported services directly
Created unit test stubs for each service in src/modules//tests/services/
Ensured test files follow the same DDD structure as the services
Updated existing tests to reference new service locations
Deleted the old src/services/ directory after confirming all services were moved
Verified no remaining references to the old service directory using codebase search
Ran tests to identify and fix any remaining import issues
📌 Type of Change
📝 Changes description
Refactored the entire service architecture from a centralized src/services/ directory to a modular, Domain-Driven Design (DDD) structure.
Moved each service into its respective domain module under src/modules//application/services/.
Updated all references and imports across the codebase (controllers, tests, middleware, scripts).
Removed the obsolete src/services/ directory after successful migration and validation.
Added proper test stubs and verified existing test compatibility.
Ensured improved maintainability, scalability, and separation of concerns by aligning with DDD best practices.
📸 Evidence (A photo is required as evidence)
⏰ Time spent breakdown
Code Analysis & Planning: 30 minutes
Migration & Refactor: 2 hours
Test Updates: 1 hr
Validation & Cleanup: 1 hr
Total: ~4:30 hrs
🌌 Comments
This refactor sets the stage for cleaner separation of concerns, improved scalability, and better collaboration as the codebase grows. It aligns with modern software design patterns and simplifies future feature development.
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
Refactor
Documentation
Tests
Chores