Move controllers to module structure and add controller-level tests #136
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 updates import paths for controller files across multiple modules to reflect their relocation into Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant Controller
participant ExpressRequest
participant ExpressResponse
TestSuite->>Controller: Call method (e.g., createX) with mock Request/Response
Controller->>ExpressRequest: Validate input
alt Missing Required Fields
Controller->>ExpressResponse: res.status(400).json({ error: ... })
else Valid Input
Controller->>ExpressResponse: res.status(200).json({ ... })
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. 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: 3
🔭 Outside diff range comments (2)
src/modules/certificate/presentation/controllers/certificate.controller.ts (2)
105-105: Avoid exposing internal error details.The error response includes the raw error object which might expose sensitive internal information.
- res.status(500).json({ error: "Failed to create certificate", err }); + res.status(500).json({ error: "Failed to create certificate" });
35-35: Guard against missing certificate before logging downloadThe first
prisma.certificate.findUniqueresult isn’t null-checked before usingcert!.id, which can crash at runtime. Add a guard like in the second branch:• File: src/modules/certificate/presentation/controllers/certificate.controller.ts
• Lines: around 29–35Suggested diff:
- const cert = await prisma.certificate.findUnique({ where: { id: volunteerId } }); - await certificateService.logDownload(cert!.id, userId); + const cert = await prisma.certificate.findUnique({ where: { id: volunteerId } }); + if (!cert) { + return res.status(404).json({ error: "Certificate not found" }); + } + await certificateService.logDownload(cert.id, userId);
🧹 Nitpick comments (8)
src/modules/nft/presentation/controllers/NFTController.ts (3)
5-5: Clean up comment formatting.The comment uses underscores instead of spaces, which is inconsistent with standard commenting practices.
- // Creates_a_new_NFT_and_returns_the_created_NFT_data_OKK!! + // Creates a new NFT and returns the created NFT data
41-41: Fix method naming inconsistency.The method name
DeleteNFTdoesn't follow camelCase convention used by other methods in this controller.- await NFTService.DeleteNFT(req.params.id); + await NFTService.deleteNFT(req.params.id);
42-42: Fix typo in response message.There's a spelling error in the success message.
- res.json(`succefully delete NFT ${req.params.id}`); + res.json(`Successfully deleted NFT ${req.params.id}`);src/modules/nft/__tests__/controllers/NFTController.int.test.ts (1)
1-23: Basic test structure is good but could be more comprehensive.The integration test correctly verifies method existence and basic error handling. However, consider expanding the test coverage to include more scenarios and edge cases.
Consider adding tests for:
- Valid NFT creation scenarios
- Different types of validation errors
- Authorization scenarios
- Edge cases for getNFTById and other methods
+ test("should create NFT successfully with valid data", async () => { + const validNFTData = { name: "Test NFT", description: "Test" }; + const req = { body: validNFTData } as Partial<Request>; + const res = { + status: jest.fn().mockReturnThis(), + json: jest.fn(), + } as Partial<Response>; + + await NFTController.createNFT(req as Request, res as Response); + expect(res.status).toHaveBeenCalledWith(201); + });src/modules/volunteer/__tests__/controllers/VolunteerController.int.test.ts (1)
8-12: Remove unnecessary method existence test.Testing for method existence is redundant since TypeScript compilation would catch missing methods. This test doesn't add meaningful value to the test suite.
- it("should have a createVolunteer method", () => { - expect(typeof VolunteerController.prototype.createVolunteer).toBe( - "function" - ); - });src/modules/certificate/__tests__/controllers/certificate.controller.int.test.ts (1)
6-8: Remove unnecessary function existence test.Testing for function existence adds no value since TypeScript compilation ensures the function exists.
- it("should have a downloadCertificate function", () => { - expect(typeof CertificateController.downloadCertificate).toBe("function"); - });src/modules/project/__tests__/controllers/ProjectController.int.test.ts (2)
6-9: Inconsistent test pattern: mixing instance and prototype.The test creates a controller instance but then checks the prototype method. Consider using a consistent approach - either test all methods on the prototype or test instance methods consistently.
- const controller = new ProjectController(); - - it("should have a createProject method", () => { - expect(typeof ProjectController.prototype.createProject).toBe("function"); + it("should have a createProject method", () => { + expect(typeof new ProjectController().createProject).toBe("function");
20-22: Inconsistent error property expectation.This test expects an
errorproperty while other similar tests in the codebase expect amessageproperty. Consider standardizing the error response format across all controllers.- expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: expect.any(String) }) - ); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ message: expect.any(String) }) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/modules/auth/__tests__/controllers/AuthController.int.test.ts(1 hunks)src/modules/auth/presentation/controllers/Auth.controller.ts(6 hunks)src/modules/certificate/__tests__/controllers/certificate.controller.int.test.ts(1 hunks)src/modules/certificate/presentation/controllers/certificate.controller.ts(1 hunks)src/modules/nft/__tests__/controllers/NFTController.int.test.ts(1 hunks)src/modules/nft/presentation/controllers/NFTController.ts(1 hunks)src/modules/organization/__tests__/controllers/OrganizationController.int.test.ts(1 hunks)src/modules/organization/presentation/controllers/OrganizationController.ts(1 hunks)src/modules/project/__tests__/controllers/ProjectController.int.test.ts(1 hunks)src/modules/project/presentation/controllers/Project.controller.ts(1 hunks)src/modules/user/__tests__/controllers/UserController.int.test.ts(1 hunks)src/modules/user/presentation/controllers/UserController.ts(1 hunks)src/modules/user/presentation/controllers/userVolunteer.controller.ts(1 hunks)src/modules/volunteer/__tests__/controllers/VolunteerController.int.test.ts(1 hunks)src/modules/volunteer/presentation/controllers/VolunteerController.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/modules/organization/__tests__/controllers/OrganizationController.int.test.ts (1)
src/modules/organization/presentation/controllers/organization.controller.ts (1)
OrganizationController(12-132)
src/modules/volunteer/__tests__/controllers/VolunteerController.int.test.ts (1)
src/modules/volunteer/presentation/controllers/VolunteerController.ts (1)
VolunteerController(5-52)
🔇 Additional comments (12)
src/modules/nft/presentation/controllers/NFTController.ts (1)
2-2: Import path update looks correct.The import path change aligns with the module restructuring objective and correctly navigates from the new controller location to the services directory.
src/modules/volunteer/presentation/controllers/VolunteerController.ts (1)
2-3: Import path updates are correct.Both import paths have been appropriately updated to reflect the new module structure, correctly navigating from the controller's new location to the services and DTO directories.
src/modules/user/presentation/controllers/UserController.ts (1)
1-5: All import path updates are correct.The import paths have been systematically updated to reflect the new module structure, properly navigating from the controller's new location to the respective modules, services, and types directories.
src/modules/certificate/presentation/controllers/certificate.controller.ts (1)
1-5: Import path updates are correct.All import paths have been appropriately updated to reflect the new module structure. The reordering of the Response import is a minor change that doesn't affect functionality.
src/modules/certificate/__tests__/controllers/certificate.controller.int.test.ts (1)
10-33: Verified status code for non-existent volunteerThe
downloadCertificatecontroller indeed returns a 403 when the volunteer isn’t found (lines 21–24), so the test’s expectation of 403 is correct. If you’d rather treat “volunteer not found” as a 404, update both the controller (tores.status(404)) and the corresponding test assertion; otherwise, no change is needed.src/modules/project/presentation/controllers/Project.controller.ts (1)
2-2: Import path is correctThe
ProjectServicefile exists atsrc/services/ProjectService.ts, and the four-level relative import from
src/modules/project/presentation/controllers/Project.controller.tscorrectly resolves to it. No further updates are needed.src/modules/user/presentation/controllers/userVolunteer.controller.ts (1)
2-3: Import paths verified and correctBoth imports (
UserVolunteerServiceandprisma) correctly resolve tosrc/services/userVolunteer.service.tsandsrc/config/prisma.tsfromsrc/modules/user/presentation/controllers/userVolunteer.controller.ts. No changes needed.src/modules/organization/presentation/controllers/OrganizationController.ts (1)
2-3: Import paths correctly updated for new module structure.The import paths have been properly adjusted to reflect the controller's new location in the domain-driven module structure. The deep relative paths correctly traverse from
src/modules/organization/presentation/controllers/to the root-level services and utils directories.src/modules/auth/presentation/controllers/Auth.controller.ts (2)
2-3: Import paths correctly updated for new module structure.The import paths have been properly adjusted to traverse from the new controller location to the services and types directories.
25-27: Good refactor: Consolidated error handling responses.The consolidation of
res.status().json()calls into single chained statements improves readability and follows Express.js best practices. The error handling logic remains intact with properinstanceof Errorchecks and fallback messages.Also applies to: 48-50, 69-72, 83-85, 104-109
src/modules/auth/__tests__/controllers/AuthController.int.test.ts (1)
1-22: Well-structured integration test with proper mocking.The test correctly verifies method existence and error handling behavior using appropriate Express request/response mocks. The async/await handling is proper, and the assertions check for expected HTTP status codes and error message structure.
src/modules/organization/__tests__/controllers/OrganizationController.int.test.ts (1)
10-26: I’d like to confirm whichOrganizationControllerthe test is importing to ensure it matches the intended implementation. Can you show the import statement(s) at the top ofsrc/modules/organization/__tests__/controllers/OrganizationController.int.test.ts? Specifically, the lines whereOrganizationControlleris brought in.
src/modules/volunteer/__tests__/controllers/VolunteerController.int.test.ts
Outdated
Show resolved
Hide resolved
src/modules/organization/__tests__/controllers/OrganizationController.int.test.ts
Outdated
Show resolved
Hide resolved
Move controllers to module structure and add controller-level tests
|
The Test CI/test is failing because I observed that on line 33 in the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/modules/user/__tests__/controllers/UserController.int.test.ts (2)
11-24: Enhance DTO mocks to reflect real validation behavior.The current DTO mocks return empty objects, which may not accurately simulate the validation and transformation logic of the actual DTOs. Consider mocking them to return more realistic data or validation results.
-jest.mock("../../../../modules/user/dto/CreateUserDto", () => { - return { - CreateUserDto: function () { - return {}; - }, - }; -}); +jest.mock("../../../../modules/user/dto/CreateUserDto", () => { + return { + CreateUserDto: function (data: any) { + // Mock validation logic + if (!data.email) throw new Error("Email is required"); + return { email: data.email, ...data }; + }, + }; +});
48-60: Consider extracting mock setup to reduce duplication.The UserService mock setup is repeated across multiple tests. Consider extracting this to a helper function or using beforeEach for cleaner test organization.
+ const mockUserService = (methods: Partial<any>) => { + (UserService as jest.Mock).mockImplementation(() => methods); + }; + describe("POST /users", () => { it("should create a user and return 201", async () => { const mockUser = { id: "1", email: "[email protected]" }; - (UserService as jest.Mock).mockImplementation(() => ({ - createUser: jest.fn().mockResolvedValue(mockUser), - })); + mockUserService({ + createUser: jest.fn().mockResolvedValue(mockUser), + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (16)
.github/workflows/test.yml(3 hunks)src/modules/auth/__tests__/controllers/AuthController.int.test.ts(1 hunks)src/modules/auth/presentation/controllers/Auth.controller.ts(6 hunks)src/modules/certificate/__tests__/controllers/certificate.controller.int.test.ts(1 hunks)src/modules/certificate/presentation/controllers/certificate.controller.ts(1 hunks)src/modules/nft/__tests__/controllers/NFTController.int.test.ts(1 hunks)src/modules/nft/presentation/controllers/NFTController.ts(1 hunks)src/modules/organization/__tests__/controllers/OrganizationController.int.test.ts(1 hunks)src/modules/organization/presentation/controllers/OrganizationController.ts(1 hunks)src/modules/project/__tests__/controllers/ProjectController.int.test.ts(1 hunks)src/modules/project/presentation/controllers/Project.controller.ts(1 hunks)src/modules/user/__tests__/controllers/UserController.int.test.ts(1 hunks)src/modules/user/presentation/controllers/UserController.ts(1 hunks)src/modules/user/presentation/controllers/userVolunteer.controller.ts(1 hunks)src/modules/volunteer/__tests__/controllers/VolunteerController.int.test.ts(1 hunks)src/modules/volunteer/presentation/controllers/VolunteerController.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/modules/project/tests/controllers/ProjectController.int.test.ts
- src/modules/nft/tests/controllers/NFTController.int.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/modules/project/presentation/controllers/Project.controller.ts
- src/modules/organization/presentation/controllers/OrganizationController.ts
- src/modules/nft/presentation/controllers/NFTController.ts
- src/modules/user/presentation/controllers/userVolunteer.controller.ts
- src/modules/volunteer/presentation/controllers/VolunteerController.ts
- src/modules/user/presentation/controllers/UserController.ts
- src/modules/auth/presentation/controllers/Auth.controller.ts
- src/modules/auth/tests/controllers/AuthController.int.test.ts
- src/modules/certificate/presentation/controllers/certificate.controller.ts
- src/modules/certificate/tests/controllers/certificate.controller.int.test.ts
- src/modules/organization/tests/controllers/OrganizationController.int.test.ts
- src/modules/volunteer/tests/controllers/VolunteerController.int.test.ts
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/test.yml
33-33: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
src/modules/user/__tests__/controllers/UserController.int.test.ts (1)
45-155: Excellent comprehensive test coverage!This integration test suite significantly improves upon the previous minimal implementation by providing thorough coverage of all UserController endpoints with both success and error scenarios. The test structure is well-organized and covers:
- POST /users: Creation with success/error cases
- GET /users/:id: Retrieval by ID with success/not found/error cases
- GET /users?email=: Retrieval by email with validation and error handling
- PUT /users/:id: Update operations with proper error handling
The HTTP status codes and response assertions are appropriate for each scenario.
GideonBature
force-pushed
the
move
branch
2 times, most recently
from
6eae6fc to
1396b71
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/modules/user/__tests__/controllers/UserController.int.test.ts (3)
22-25: Improve UpdateUserDto mock to be more realistic.The current mock returns an empty object, which doesn't reflect realistic DTO behavior and may not catch potential validation issues in the controller.
jest.mock("../../dto/UpdateUserDto", () => { return { - UpdateUserDto: function () { - return {}; + UpdateUserDto: function (data: Record<string, unknown>) { + // Mock basic validation similar to CreateUserDto + return { ...data }; }, }; });
39-41: Consider renaming for clarity.The function name
mockUserServicecould be more descriptive to indicate it's setting up the mock implementation.-const mockUserService = (methods: Partial<Record<string, unknown>>) => { +const setupMockUserService = (methods: Partial<Record<string, unknown>>) => { (UserService as jest.Mock).mockImplementation(() => methods); };Don't forget to update the function calls in the test cases accordingly.
64-74: Consider adding more specific error scenarios.While the current error handling tests are good, consider adding tests for more specific error types that might occur in production, such as validation errors, database connection issues, or business logic violations.
Example addition for more specific error testing:
it("should handle validation errors with specific status codes", async () => { (UserService as jest.Mock).mockImplementation(() => ({ createUser: jest.fn().mockRejectedValue(new ValidationError("Invalid email format")), })); const res = await request(app) .post("/users") .send({ email: "invalid-email" }); expect(res.status).toBe(422); // Unprocessable Entity for validation errors expect(res.body.error).toContain("Invalid email format"); });Also applies to: 96-104, 132-140, 152-160
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(3 hunks)src/modules/user/__tests__/controllers/UserController.int.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/modules/user/__tests__/controllers/UserController.int.test.ts (1)
src/services/UserService.ts (1)
UserService(12-45)
🔇 Additional comments (1)
src/modules/user/__tests__/controllers/UserController.int.test.ts (1)
43-161: Excellent improvement in test coverage!This comprehensive integration test suite successfully addresses the previous review comment concerns by providing meaningful tests that cover:
- All CRUD operations (Create, Read by ID/email, Update)
- Success and error scenarios for each endpoint
- Proper HTTP status codes and response validation
- Input validation testing
- Service layer error handling
The tests now provide real value by verifying controller behavior rather than just checking method existence.
src/modules/user/__tests__/controllers/UserController.int.test.ts
Outdated
Show resolved
Hide resolved
|
Thank you ser! |
2 participants
🚀 Volunchain Pull Request
Mark with an
xall the checkboxes that apply (like[x])src/modules/*/presentation/controllers#125📌 Type of Change
📝 Changes description
🏗️ Structural Reorganization
src/controllers/tosrc/modules/{module-name}/presentation/controllers/../services/→../../../../services/)🧪 Added Integration Tests
AuthController- Tests registration validationCertificateController- Tests certificate download authorizationNFTController- Tests NFT creation validationOrganizationController- Tests organization creation validationProjectController- Tests project creation validationUserController- Basic controller method verificationVolunteerController- Tests volunteer creation validation📸 Evidence (A photo is required as evidence)
⏰ Time spent breakdown
🌌 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
Tests
Refactor
Chores