WEB-536:fix resolve chargeIds parameter error in penalty waiving#2957
Conversation
|
Note
|
| Cohort / File(s) | Change Summary |
|---|---|
Loan Action Components src/app/loans/loans-view/loan-account-actions/loan-reschedule/loan-reschedule.component.ts, src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts |
Modified submission flow to waive penalties first (when enabled and penalties selected) before proceeding with reschedule/repayment. Introduced new private submitReschedule() and submitRepayment() helper methods to encapsulate submission logic. Penalty waiver failures do not block subsequent submission. |
Penalty Management Service src/app/loans/services/penalty-management.service.ts |
Added public waivePenalties(loanId: string, penaltyIds: number[]) method that performs batch penalty waiving using forkJoin, with per-request error handling and early return for empty penalty lists. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant Component as Reschedule/Repayment<br/>Component
participant PenaltyService as PenaltyManagementService
participant LoanService as LoanService
participant Navigation
User->>Component: Submit form
alt Waive Penalties Enabled & Penalties Selected
Component->>PenaltyService: waivePenalties(loanId, penaltyIds)
activate PenaltyService
PenaltyService->>LoanService: executeLoansAccountChargesCommand<br/>(waive) × N
LoanService-->>PenaltyService: Results (success/error)
PenaltyService-->>Component: Aggregated Results
deactivate PenaltyService
Note over Component: Continue regardless<br/>of waiver outcome
end
Component->>LoanService: submitRescheduleData/submitRepaymentData
activate LoanService
LoanService-->>Component: Success Response
deactivate LoanService
Component->>Navigation: Navigate to Loan View
Navigation-->>User: Display Updated Loan
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- WEB-505 feat:implement waive penalties for loan repayment and resch… #2941 — Directly related PR modifying the same loan reschedule/repayment components and integrating penalty-waiver functionality through PenaltyManagementService.
Suggested reviewers
- IOhacker
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main change: adding penalty waiving logic with proper chargeIds parameter handling before repayment/reschedule submission. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
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 help to get the list of available commands and usage tips.
![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
🧹 Nitpick comments (3)
src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts (1)
330-343: Consider adding user feedback for penalty waiving operations.The current implementation proceeds with repayment regardless of whether penalty waiving succeeds or fails. While this ensures the main operation isn't blocked, users receive no indication of the waiver outcome.
Consider:
- Adding a loading indicator during the async penalty waiving operation
- Displaying a success/error message to inform users of the waiver result
- Using a proper logging service instead of
console.errorfor production environments💡 Example with user feedback
if (this.waivePenalties && this.selectedPenalties.length > 0) { // Show loading indicator this.penaltyManagementService.waivePenalties(this.loanId, this.selectedPenalties).subscribe({ next: (results) => { // Check if any penalties failed and show appropriate message const failedCount = results.filter(r => !r || !r.success).length; if (failedCount > 0) { // Show warning: "X penalties failed to waive, proceeding with repayment" } else { // Show success: "Penalties waived successfully" } this.submitRepayment(data); }, error: (error) => { // Use proper logging service // Show error message to user this.submitRepayment(data); } }); } else { this.submitRepayment(data); }src/app/loans/loans-view/loan-account-actions/loan-reschedule/loan-reschedule.component.ts (1)
130-143: Consider adding user feedback for penalty waiving operations.Similar to the make-repayment component, this implementation proceeds with the reschedule regardless of penalty waiving success or failure. Users receive no indication of the waiver outcome.
Consider:
- Adding a loading indicator during the async penalty waiving operation
- Displaying a success/error message to inform users of the waiver result
- Using a proper logging service instead of
console.errorfor production environments💡 Example with user feedback
if (this.waivePenalties.value && this.selectedPenalties.length > 0) { // Show loading indicator this.penaltyManagementService.waivePenalties(this.loanId, this.selectedPenalties).subscribe({ next: (results) => { // Check results and show appropriate message const failedCount = results.filter(r => !r || !r.success).length; if (failedCount > 0) { // Show warning: "X penalties failed to waive, proceeding with reschedule" } else { // Show success: "Penalties waived successfully" } this.submitReschedule(data); }, error: (error) => { // Use proper logging service // Show error message to user this.submitReschedule(data); } }); } else { this.submitReschedule(data); }src/app/loans/services/penalty-management.service.ts (1)
198-215: Improve type safety and error handling for better observability.The method has two areas worth improving:
Type safety: Return type
Observable<any[]>and error parameteranyreduce type safety. Consider defining a result interface to properly represent success/failure for each penalty.Error observability: Using
console.errorand returningnullsilently swallows failures. Callers cannot distinguish which penalties succeeded vs. failed, making it difficult to provide meaningful user feedback.The empty object
{}parameter is correct and consistent with other 'waive' operations in the codebase.🔎 Suggested improvements
Define a result type and improve error handling:
+interface WaivePenaltyResult { + chargeId: number; + success: boolean; + error?: any; +} + - waivePenalties(loanId: string, penaltyIds: number[]): Observable<any[]> { + waivePenalties(loanId: string, penaltyIds: number[]): Observable<WaivePenaltyResult[]> { if (!penaltyIds || penaltyIds.length === 0) { return of([]); } const waiveRequests = penaltyIds.map((chargeId: number) => this.loanService.executeLoansAccountChargesCommand(loanId, 'waive', {}, chargeId).pipe( - catchError((error: any) => { + catchError((error) => { console.error(`Error waiving penalty ${chargeId}:`, error); - return of(null); + return of({ chargeId, success: false, error }); + }), + map((response) => ({ chargeId, success: true, response })) - }) ) ); return forkJoin(waiveRequests); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/loans/loans-view/loan-account-actions/loan-reschedule/loan-reschedule.component.tssrc/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.tssrc/app/loans/services/penalty-management.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/loans/services/penalty-management.service.tssrc/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.tssrc/app/loans/loans-view/loan-account-actions/loan-reschedule/loan-reschedule.component.ts
🧠 Learnings (1)
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts
🔇 Additional comments (2)
src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts (1)
346-351: LGTM! Clean extraction of submission logic.The helper method effectively reduces code duplication and provides a clear separation of concerns between penalty waiving and repayment submission.
src/app/loans/loans-view/loan-account-actions/loan-reschedule/loan-reschedule.component.ts (1)
146-154: LGTM! Clean extraction of submission logic.The helper method effectively reduces code duplication and provides a clear separation of concerns between penalty waiving and reschedule submission. The existing TODO comment is preserved for future navigation improvements.
2 participants
This pr adjusts penalty-waiving behavior so penalties are waived via the dedicated waive endpoint before submitting repayment or reschedule requests.
WEB-536
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.