Fixes/userdeletionerrorfix#14
Conversation
- Added validation to prevent user deletion with active loans. - Automatically capitalized users name during creation and updates. - Improved trim handling for user input.
📝 WalkthroughWalkthroughThese changes implement safer user deletion by blocking deletions when active loans exist, normalize user input formatting by capitalizing first letters and lowercasing the rest, and improve input handling by trimming and treating blank input as abort signals. Changes
Sequence DiagramsequenceDiagram
participant User
participant UserCLI
participant JpaUserRepository
participant LoanServices
User->>UserCLI: Request delete user
UserCLI->>JpaUserRepository: deleteUserById(userId)
JpaUserRepository->>LoanServices: Check for active loans
alt Active loans exist
LoanServices-->>JpaUserRepository: Active loans found
JpaUserRepository-->>UserCLI: Return false
UserCLI-->>User: Display loan error message
else No active loans
LoanServices-->>JpaUserRepository: No active loans
JpaUserRepository->>JpaUserRepository: Begin transaction
JpaUserRepository->>JpaUserRepository: Remove user
JpaUserRepository->>JpaUserRepository: Commit transaction
JpaUserRepository-->>UserCLI: Return true
UserCLI-->>User: Confirm deletion
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/user/repository/JpaUserRepository.java (1)
34-51: Critical: Callingrollback()without an active transaction will throw an exception.When
useris null, the code falls through to line 49 and callsem.getTransaction().rollback(). However,em.getTransaction().begin()is only called on line 44 inside theif (user != null)block. This will throw anIllegalStateExceptionbecause there's no active transaction to rollback.🐛 Proposed fix
@Override public boolean deleteUserById(Long id) { try (EntityManager em = EMFactory.getEntityManager()) { LoanServices loanServices = new LoanServices(); User user = em.find(User.class, id); if (user != null) { // Check for active loans if (!loanServices.activeLoans(user, em).isEmpty()) { return false; } em.getTransaction().begin(); em.remove(user); em.getTransaction().commit(); return true; } - em.getTransaction().rollback(); return false; } }src/main/java/org/example/user/UserCLI.java (1)
214-223: Critical: Blank input abort breaks the update user flow.The
prompt()method now throws on blank input (line 219), butupdateUserMenu()(lines 171-176) expects blank input to mean "keep current value." Users can no longer leave fields unchanged during updates because pressing Enter without input will abort the entire operation instead of preserving existing values.🐛 Proposed fix: Create separate prompt methods or accept blank in update context
Option 1: Create a
promptOptional()method for update scenarios:private String promptOptional(String message) { System.out.print(message); String input = scanner.nextLine().trim(); if (input.equalsIgnoreCase("avbryt")) { throw new RuntimeException("AVBRUTET"); } return input; // Allow blank }Option 2: Pass a flag to allow blank:
-private String prompt(String message) { +private String prompt(String message, boolean allowBlank) { System.out.print(message); String input = scanner.nextLine().trim(); - if (input.equalsIgnoreCase("avbryt") || input.isBlank()) { + if (input.equalsIgnoreCase("avbryt") || (!allowBlank && input.isBlank())) { throw new RuntimeException("AVBRUTET"); } return input; } + +private String prompt(String message) { + return prompt(message, false); +}Then use
prompt("Nytt förnamn: ", true)inupdateUserMenu().
🧹 Nitpick comments (1)
src/main/java/org/example/user/repository/JpaUserRepository.java (1)
36-36: Consider injectingLoanServicesfor better testability.Instantiating
LoanServicesdirectly inside the method creates tight coupling. Consider injecting it via constructor or method parameter to improve testability and follow dependency inversion principles.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/user/UserCLI.javasrc/main/java/org/example/user/repository/JpaUserRepository.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/org/example/user/UserCLI.java (1)
src/main/java/org/example/user/SessionManager.java (1)
SessionManager(5-33)
src/main/java/org/example/user/repository/JpaUserRepository.java (1)
src/main/java/org/example/LoanServices.java (1)
LoanServices(7-99)
🔇 Additional comments (4)
src/main/java/org/example/user/repository/JpaUserRepository.java (1)
40-43: LGTM!The active loans check correctly prevents deletion of users with outstanding loans, addressing the PR objective.
src/main/java/org/example/user/UserCLI.java (3)
124-127: Name capitalization logic looks good.The capitalization safely handles single-character names since
substring(1)on a single char returns an empty string. Theprompt()method's blank check on line 219 ensures this code won't receive empty strings.
196-211: LGTM!The deletion flow is improved with proper result checking and informative error messaging. The integration with the repository-level active loans check is correct.
170-176: This blank-input handling is unreachable.As noted above, the
prompt()method will throw before these blank checks can execute. Onceprompt()is fixed to allow blank input in update scenarios, this logic will work as intended.
1 participant
Fixed an error where the app crashed when delting user while there were active loans.
Made it so you can't delete if you've got active loans.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.