feature: Refine wallet, payment component #129
Conversation
WalkthroughThis update modifies the payment and wallet functionalities across backend and frontend modules. In the backend, payment queries now use an enterprise ID instead of a name, and wallet operations have been refactored to return a numerical balance. Role-based access restrictions have been added to both payment and wallet endpoints. On the frontend, components now extend a subscription-aware base class and retrieve wallet balance dynamically using a new WalletService, and subtle UI text and style updates have been applied. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
🧹 Nitpick comments (7)
sep490-enterprise/src/main/java/enterprise/repositories/PaymentRepository.java (1)
12-19: Good architectural choice using UUID for enterprise identification.The shift from name-based to UUID-based enterprise identification is a robust architectural decision that:
- Ensures better uniqueness guarantees
- Aligns with best practices for entity identification
- Improves security by not exposing potentially sensitive name information
sep490-enterprise/src/main/java/enterprise/services/impl/PaymentServiceImpl.java (1)
27-28: Add error handling and logging.Consider adding:
- Specific exception handling for the
orElseThrow()- Logging for audit and debugging purposes
Example implementation:
@Override public Page<PaymentEntity> search(SearchCriteriaDTO<PaymentCriteriaDTO> searchCriteria, Pageable pageable) { - UUID enterpriseId = SecurityUtils.getCurrentUserEnterpriseId().orElseThrow(); - return payRepo.findByEnterpriseId(enterpriseId, pageable); + UUID enterpriseId = SecurityUtils.getCurrentUserEnterpriseId() + .orElseThrow(() -> new SecurityException("Enterprise ID not found in security context")); + log.debug("Searching payments for enterprise ID: {}", enterpriseId); + Page<PaymentEntity> results = payRepo.findByEnterpriseId(enterpriseId, pageable); + log.debug("Found {} payments", results.getTotalElements()); + return results; }sep490-frontend/src/app/modules/enterprise/services/wallet.service.ts (2)
1-5: Imports are well-structured for typical Angular development.Using
HttpClient,Observable, and application routing constants is standard practice. Consider storing external URLs in environment-based configs for clarity across different environments.
10-15: Consider adding error handling.While this method is fine for simple usage, you might want to handle potential errors or failed requests (e.g., using
catchError) to provide a more robust user experience.sep490-enterprise/src/main/java/enterprise/services/impl/WalletServiceImpl.java (1)
22-28: LGTM! The return type change improves data encapsulation.The change from returning
WalletEntitytoLongis a good improvement as it reduces unnecessary data exposure by returning only the balance.However, consider improving error handling by providing a custom exception with a clear error message.
- .orElseThrow(); + .orElseThrow(() -> new WalletNotFoundException("Wallet not found for enterprise ID: " + enterpriseId));sep490-frontend/src/app/modules/enterprise/components/plan/plan.component.ts (1)
50-54: Add error handling to the wallet balance subscription.Consider handling potential errors in the subscription to provide better user feedback.
this.registerSubscription( this.walletService.getWalletBalance().subscribe(result => { this.balance = result; - }) + }, + error => { + console.error('Failed to fetch wallet balance:', error); + this.applicationService.showErrorMessage('Failed to fetch wallet balance'); + }) );sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.ts (1)
87-91: Add error handling and consider extracting shared wallet balance logic.
- Add error handling to provide better user feedback.
- Consider extracting the wallet balance retrieval logic to a base class since it's shared with
PlanComponent.this.registerSubscription( this.walletService.getWalletBalance().subscribe(result => { this.balance = result; - }) + }, + error => { + console.error('Failed to fetch wallet balance:', error); + this.applicationService.showErrorMessage('Failed to fetch wallet balance'); + }) );Consider creating a base class:
export abstract class WalletAwareComponent extends SubscriptionAwareComponent { protected balance: number = 0; constructor(protected readonly walletService: WalletService, protected readonly applicationService: ApplicationService) { super(); } protected getBalance(): void { this.registerSubscription( this.walletService.getWalletBalance().subscribe( result => { this.balance = result; }, error => { console.error('Failed to fetch wallet balance:', error); this.applicationService.showErrorMessage('Failed to fetch wallet balance'); } ) ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
sep490-enterprise/src/main/java/enterprise/repositories/PaymentRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/WalletRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/rest/PaymentController.java(2 hunks)sep490-enterprise/src/main/java/enterprise/rest/WalletController.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/WalletService.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/impl/PaymentServiceImpl.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/impl/WalletServiceImpl.java(1 hunks)sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.ts(5 hunks)sep490-frontend/src/app/modules/enterprise/components/plan/plan.component.ts(4 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.module.ts(2 hunks)sep490-frontend/src/app/modules/enterprise/services/wallet.service.ts(1 hunks)sep490-frontend/src/assets/i18n/en.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- sep490-frontend/src/assets/i18n/en.json
- sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.html
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Java Modules (sep490-enterprise)
- GitHub Check: Build Java Modules (sep490-idp)
🔇 Additional comments (14)
sep490-enterprise/src/main/java/enterprise/repositories/WalletRepository.java (2)
6-8: LGTM!The new imports are correctly added and necessary for the new functionality.
9-11: Well-designed method signature!The new
findByEnterpriseIdmethod follows best practices:
- Returns
Optionalfor null-safety- Uses
UUIDfor reliable entity identification- Follows Spring Data JPA naming conventions for automatic query generation
This change aligns well with the shift from name-based to ID-based queries.
sep490-enterprise/src/main/java/enterprise/services/impl/PaymentServiceImpl.java (1)
27-28: LGTM! Good security improvement.The change to use enterprise ID from the security context enforces proper access control at the enterprise level.
sep490-enterprise/src/main/java/enterprise/rest/PaymentController.java (2)
10-11: LGTM! Clean security imports added.The new imports correctly support the role-based access control implementation.
29-30:❓ Verification inconclusive
Good security enhancement with role-based access control!
The @RolesAllowed annotation appropriately restricts payment search functionality to enterprise owners, following the principle of least privilege.
Let's verify if other payment-related endpoints are consistently protected:
🏁 Script executed:
#!/bin/bash # Search for other payment-related endpoints in the codebase ast-grep --pattern 'class $_ { $$$ @$_Mapping($_) $$$ public $_ $_Payment($$$) { $$$ } $$$ }'Length of output: 111
Security enhancement approved—further manual validation required!
The application of the @RolesAllowed annotation on the searchPayment endpoint in PaymentController correctly restricts access to enterprise owners. However, the initial automated search for additional payment-related endpoints produced no output, so we cannot conclusively confirm that all payment endpoints are consistently protected. Please manually verify that any other endpoints handling payment data include appropriate access controls.
- PaymentController (lines 29-30): @RolesAllowed applied correctly to the searchPayment endpoint.
- Action Needed: Manually audit related payment endpoints to ensure consistent protection.
sep490-enterprise/src/main/java/enterprise/rest/WalletController.java (3)
4-6: New imports align with role-based access needs and wallet operations.These imports for
WalletService,UserRole, andRolesAllowedsupport the newly introduced role-based restriction and delegated wallet logic.
21-22: Constructor-based injection is properly used.Using
private finalfields with@RequiredArgsConstructoris a recommended practice, ensuring immutability and a clean dependency injection pattern.
24-26: Verify that the correct enterprise context is used inwalletService.getBalance().Currently,
userContextDatais not passed into the service. Confirm that the service is deriving the correct enterprise ID or user context internally. Otherwise, consider updating the method signature to ensure correctness.sep490-enterprise/src/main/java/enterprise/services/WalletService.java (1)
5-5: Returning a numeric balance matches the new approach.Switching from returning an entity to a simple
Longfor the balance is consistent with the updated controller logic and improves clarity.sep490-frontend/src/app/modules/enterprise/services/wallet.service.ts (2)
6-7: Angular service initialization is correct.Decorating the class with
@Injectable()and exporting it is the conventional approach for DI in Angular.
8-9: Constructor injection of HttpClient is standard.No concerns here. This is a best practice to keep your service testable and maintainable.
sep490-frontend/src/app/modules/enterprise/enterprise.module.ts (1)
12-12: LGTM! The WalletService is properly configured.The service is correctly imported and added to the providers array, making it available for dependency injection throughout the module.
Also applies to: 22-28
sep490-frontend/src/app/modules/enterprise/components/plan/plan.component.ts (1)
15-18: LGTM! The component properly extends SubscriptionAwareComponent.The inheritance is correctly implemented with proper constructor initialization.
Also applies to: 34-36
sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.ts (1)
24-27: LGTM! The component properly extends SubscriptionAwareComponent.The inheritance is correctly implemented with proper constructor initialization.
Also applies to: 41-47
sep490-enterprise/src/main/java/enterprise/repositories/PaymentRepository.java
Outdated
Show resolved
Hide resolved
sep490-enterprise/src/main/java/enterprise/services/impl/PaymentServiceImpl.java
Show resolved
Hide resolved
GiaBaorr
force-pushed
the
feature/i-dont-know-what-is-this
branch
from
922d111 to
2a05631
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.ts (1)
86-92: Add error handling to the wallet balance subscription.While the subscription is properly managed, consider adding error handling to gracefully handle potential API failures:
getBalance(): void { this.registerSubscription( this.walletService.getWalletBalance().subscribe( - result => { - this.balance = result; - } + { + next: (result) => { + this.balance = result; + }, + error: (error) => { + console.error('Failed to fetch wallet balance:', error); + // Consider showing a user-friendly error message + this.balance = 0; // or any other fallback value + } + } ) ); }sep490-enterprise/src/main/java/enterprise/services/impl/WalletServiceImpl.java (1)
22-28: Enhance error handling with custom exceptions.While the implementation is clean and functional, consider improving error handling by:
- Using custom exceptions with meaningful messages
- Handling specific failure cases separately
Here's a suggested improvement:
public Long getBalance(){ - UUID enterpriseId = SecurityUtils.getCurrentUserEnterpriseId().orElseThrow(); + UUID enterpriseId = SecurityUtils.getCurrentUserEnterpriseId() + .orElseThrow(() -> new UnauthorizedException("User not associated with an enterprise")); return walRepo .findByEnterpriseId(enterpriseId) .map(WalletEntity::getBalance) - .orElseThrow(); + .orElseThrow(() -> new ResourceNotFoundException("Wallet not found for enterprise: " + enterpriseId)); }sep490-enterprise/src/main/java/enterprise/rest/WalletController.java (1)
23-27: Add API documentation and consider specific response status.Consider the following improvements:
- Add OpenAPI/Swagger documentation
- Use specific HTTP status codes for different scenarios
Here's a suggested improvement:
@GetMapping("/balance") @RolesAllowed(UserRole.RoleNameConstant.ENTERPRISE_OWNER) + @Operation(summary = "Get enterprise wallet balance", + description = "Retrieves the current balance for the authenticated enterprise owner's wallet") + @ApiResponse(responseCode = "200", description = "Balance retrieved successfully") + @ApiResponse(responseCode = "404", description = "Wallet not found") + @ApiResponse(responseCode = "403", description = "User not authorized") public ResponseEntity<Long> getBalance(@AuthenticationPrincipal UserContextData userContextData) { - return ResponseEntity.ok().body(walletService.getBalance()); + return ResponseEntity.status(HttpStatus.OK).body(walletService.getBalance()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
sep490-enterprise/src/main/java/enterprise/repositories/PaymentRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/WalletRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/rest/PaymentController.java(2 hunks)sep490-enterprise/src/main/java/enterprise/rest/WalletController.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/WalletService.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/impl/PaymentServiceImpl.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/impl/WalletServiceImpl.java(1 hunks)sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.ts(5 hunks)sep490-frontend/src/app/modules/enterprise/components/plan/plan.component.ts(4 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.module.ts(2 hunks)sep490-frontend/src/app/modules/enterprise/services/wallet.service.ts(1 hunks)sep490-frontend/src/assets/i18n/en.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- sep490-frontend/src/assets/i18n/en.json
- sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.html
- sep490-frontend/src/app/modules/enterprise/enterprise.module.ts
- sep490-frontend/src/app/modules/enterprise/services/wallet.service.ts
- sep490-frontend/src/app/modules/enterprise/components/plan/plan.component.ts
- sep490-enterprise/src/main/java/enterprise/repositories/WalletRepository.java
- sep490-enterprise/src/main/java/enterprise/rest/PaymentController.java
- sep490-enterprise/src/main/java/enterprise/services/WalletService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Java Modules (sep490-enterprise)
🔇 Additional comments (8)
sep490-enterprise/src/main/java/enterprise/repositories/PaymentRepository.java (1)
13-13: Well-designed repository method!The new
findByEnterpriseIdmethod follows Spring Data JPA conventions and improves the codebase by:
- Using UUID instead of name for more reliable identification
- Supporting pagination for better performance with large datasets
- Implementing enterprise-scoped queries for proper multi-tenant data isolation
sep490-enterprise/src/main/java/enterprise/services/impl/PaymentServiceImpl.java (2)
27-28: LGTM! Secure enterprise ID retrieval and proper transaction handling.The implementation correctly retrieves the enterprise ID from SecurityUtils and uses proper transaction management.
26-29: Consider handling the unused search criteria parameter.The
searchCriteriaparameter is still unused in the implementation.Previous suggestions remain valid:
- Remove it if search filtering is no longer needed, or
- Incorporate it into the query to maintain filtering capability while still scoping by enterprise ID.
sep490-frontend/src/app/modules/enterprise/components/payment/payment.component.ts (3)
2-2: LGTM!The new imports are correctly added and follow Angular's module organization best practices.
Also applies to: 12-12
24-27: LGTM! Good improvement in subscription management.The component now properly extends
SubscriptionAwareComponentfor better subscription handling and explicitly implementsOnInit. This change:
- Improves memory management by properly handling subscription cleanup.
- Makes the interface implementation more explicit, enhancing code clarity.
41-47: LGTM! Constructor properly initialized.The constructor changes are well-implemented:
- Correctly injects the
WalletServicefor wallet balance functionality.- Properly calls
super()as required when extendingSubscriptionAwareComponent.sep490-enterprise/src/main/java/enterprise/services/impl/WalletServiceImpl.java (1)
20-20: LGTM!Good use of spacing for improved readability.
sep490-enterprise/src/main/java/enterprise/rest/WalletController.java (1)
21-22: LGTM!Good use of constructor injection via @requiredargsconstructor for the WalletService dependency.
sep490-enterprise/src/main/java/enterprise/repositories/WalletRepository.java
Outdated
Show resolved
Hide resolved
sep490-enterprise/src/main/java/enterprise/services/WalletService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sep490-enterprise/src/main/java/enterprise/repositories/WalletRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/WalletService.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/impl/WalletServiceImpl.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sep490-enterprise/src/main/java/enterprise/repositories/WalletRepository.java
- sep490-enterprise/src/main/java/enterprise/services/WalletService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Java Modules (sep490-enterprise)
🔇 Additional comments (1)
sep490-enterprise/src/main/java/enterprise/services/impl/WalletServiceImpl.java (1)
20-20: LGTM!
sep490-enterprise/src/main/java/enterprise/services/impl/WalletServiceImpl.java
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Refactor
Style
UI Text