injiweb-1733-no-userid-in-session used utils to add a check#1036
injiweb-1733-no-userid-in-session used utils to add a check#1036cyber-titan wants to merge 1 commit intoinji:developfrom
Conversation
Signed-off-by: cyber-titan <saiabhi2309@gmail.com>
WalkthroughThe changes add user ID validation to the wallet creation process by introducing a WalletValidator dependency into WalletsController. The validator is invoked before wallet creation to ensure the user ID is present and valid. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/java/io/mosip/mimoto/controller/WalletsControllerTest.java (2)
510-513: ExplicitdoNothing()is redundant for Mockito mocks.Mockito mocks default to doing nothing for void methods. This call can be removed for cleaner test code.
🧹 Suggested cleanup
`@Test` public void shouldCreateWalletWhenUserIdIsValidAndValidatorPasses() throws Exception { - // validator is void -> use doNothing() - org.mockito.Mockito.doNothing() - .when(walletValidator) - .validateUserId(userId); - when(walletService.createWallet(userId, walletName, walletPin, confirmWalletPin)) .thenReturn(new WalletResponseDto(walletId, walletName, null));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/mosip/mimoto/controller/WalletsControllerTest.java` around lines 510 - 513, Remove the unnecessary explicit Mockito.doNothing() setup in WalletsControllerTest: delete the org.mockito.Mockito.doNothing().when(walletValidator).validateUserId(userId); line because mocks already do nothing for void methods; leave any other stubbing/verification for walletValidator and keep validateUserId untouched so the test behavior remains the same.
131-146: Existing test may be testing an unrealistic scenario after this PR.With the new validation flow,
walletValidator.validateUserId(null)is called beforewalletService.createWallet(). This test doesn't configure the mock forwalletValidator, so it silently passes (does nothing), and then expectswalletServiceto throw.In production,
walletValidatorwould throw first, making thewalletServicemock configuration unreachable. Consider updating this test to set upwalletValidatormock or removing it since the new testshouldReturnUnauthorizedWhenUserIdIsInvalidForCreateWalletcovers the correct flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/mosip/mimoto/controller/WalletsControllerTest.java` around lines 131 - 146, The test should mock the validator instead of relying on walletService to throw; update shouldReturnUnauthorizedWhenUserIdIsMissingForCreateWallet to configure walletValidator.validateUserId(null) to throw the UnauthorizedAccessException (same error code/message used now) so the controller's validation path is exercised (or remove this test and rely on shouldReturnUnauthorizedWhenUserIdIsInvalidForCreateWallet), and remove or adjust the walletService.createWallet(null, walletName, walletPin, confirmWalletPin) stubbing because it is unreachable once walletValidator.validateUserId throws.src/main/java/io/mosip/mimoto/controller/WalletsController.java (1)
92-93: Validation is already consistent at the service layer across all endpoints.The
validateUserId()check is performed inWalletServiceImplfor all wallet operations (createWallet,getWallets,unlockWallet,deleteWallet). However, thecreateWalletcontroller adds an additional layer of validation before calling the service, which is redundant since the service layer validates again. For consistency, either remove the controller-level validation increateWalletor apply it uniformly across all controller endpoints. Since validation already occurs at the service layer for all operations, the current code functions correctly but the pattern could be simplified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/mosip/mimoto/controller/WalletsController.java` around lines 92 - 93, Remove the redundant controller-level userId validation in WalletsController.createWallet to match the existing pattern where WalletServiceImpl performs validateUserId for all wallet operations; specifically, delete the call to walletValidator.validateUserId(userId) in the createWallet method so the controller delegates validation responsibility to WalletServiceImpl (createWallet, getWallets, unlockWallet, deleteWallet) and avoid duplicating logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/io/mosip/mimoto/controller/WalletsController.java`:
- Around line 92-93: Remove the redundant controller-level userId validation in
WalletsController.createWallet to match the existing pattern where
WalletServiceImpl performs validateUserId for all wallet operations;
specifically, delete the call to walletValidator.validateUserId(userId) in the
createWallet method so the controller delegates validation responsibility to
WalletServiceImpl (createWallet, getWallets, unlockWallet, deleteWallet) and
avoid duplicating logic.
In `@src/test/java/io/mosip/mimoto/controller/WalletsControllerTest.java`:
- Around line 510-513: Remove the unnecessary explicit Mockito.doNothing() setup
in WalletsControllerTest: delete the
org.mockito.Mockito.doNothing().when(walletValidator).validateUserId(userId);
line because mocks already do nothing for void methods; leave any other
stubbing/verification for walletValidator and keep validateUserId untouched so
the test behavior remains the same.
- Around line 131-146: The test should mock the validator instead of relying on
walletService to throw; update
shouldReturnUnauthorizedWhenUserIdIsMissingForCreateWallet to configure
walletValidator.validateUserId(null) to throw the UnauthorizedAccessException
(same error code/message used now) so the controller's validation path is
exercised (or remove this test and rely on
shouldReturnUnauthorizedWhenUserIdIsInvalidForCreateWallet), and remove or
adjust the walletService.createWallet(null, walletName, walletPin,
confirmWalletPin) stubbing because it is unreachable once
walletValidator.validateUserId throws.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/io/mosip/mimoto/controller/WalletsController.javasrc/test/java/io/mosip/mimoto/controller/WalletsControllerTest.java
Summary by CodeRabbit