WEB-117 Loan Account Charge(Amount% charges) Doubling on Application Modification #2690
Conversation
WalkthroughAdds deduplication of loan charges both when initializing the charges step and before submitting edits. Charges are normalized and deduped by Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as LoansAccountChargesStepComponent
participant E as EditLoansAccountComponent
participant API as Backend
U->>C: Open or edit loan account
Note over C: Initialize charges\n• Prefer product template when datasource empty\n• If modifying (loanId present), may use account template\n• Overdue-charges branch retained
C->>C: getUniqueCharges(charges)\n(deduplicate by id/chargeId)
U->>E: Submit edited loan account
E->>E: Build uniqueCharges map\n(skip malformed entries, normalize id/chargeId)
E->>API: POST payload with deduplicated `charges`
API-->>E: Response
E-->>U: Confirmation / result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)src/app/**⚙️ CodeRabbit configuration file
Files:
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: 2
🧹 Nitpick comments (5)
src/app/loans/edit-loans-account/edit-loans-account.component.ts (2)
141-145: Include feeOnMonthDay when present to avoid losing Annual Fee datesThe step component sets feeOnMonthDay for 'Annual Fee'. The payload currently only sends dueDate, which may drop that info.
- charges: Array.from(uniqueCharges.values()).map((charge: any) => ({ - chargeId: charge.id || charge.chargeId, - amount: charge.amount, - dueDate: charge.dueDate && this.dateUtils.formatDate(charge.dueDate, dateFormat) - })), + charges: Array.from(uniqueCharges.values()).map((charge: any) => { + const payload: any = { + chargeId: charge.id ?? charge.chargeId, + amount: charge.amount + }; + if (charge.dueDate) { + payload.dueDate = this.dateUtils.formatDate(charge.dueDate, dateFormat); + } + if (charge.feeOnMonthDay) { + payload.feeOnMonthDay = this.dateUtils.formatDate(charge.feeOnMonthDay, dateFormat); + } + if (charge.feeInterval != null) { + payload.feeInterval = charge.feeInterval; + } + return payload; + }),Please confirm Fineract expects feeOnMonthDay/feeInterval for your charge types in PUT /v1/loans. If not, we can pare this back. Based on learnings.
132-137: Avoid double-dedup: rely on a single, shared helper to keep behavior consistentWe dedup in the step getter and again here. Prefer one source of truth (e.g., a shared util), and keep a server-side guard (filter invalids) here.
I can extract a shared function (e.g., loans-charge-utils.ts) and update both call sites. Want me to draft it?
Also applies to: 141-145
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (3)
165-176: Prefer existing loan charges on modification; current order may transiently show product chargesOnChanges runs before OnInit; with empty chargesDataSource, the first branch loads product charges even in edit mode, then OnInit overwrites with existing charges. This causes redundant work and potential UI flicker.
- const isModification = this.loanId != null; - if ( - this.loansAccountProductTemplate.charges && - this.loansAccountProductTemplate.charges.length > 0 && - this.chargesDataSource.length === 0 - ) { - this.chargesDataSource = - this.loansAccountProductTemplate.charges.map((charge: any) => ({ ...charge, id: charge.chargeId })) || []; - } else if (isModification && this.loansAccountTemplate && this.loansAccountTemplate.charges) { - this.chargesDataSource = - this.loansAccountTemplate.charges.map((charge: any) => ({ ...charge, id: charge.chargeId })) || []; - } + const isModification = this.loanId != null; + if (isModification && this.loansAccountTemplate?.charges?.length) { + this.chargesDataSource = + this.loansAccountTemplate.charges.map((charge: any) => ({ ...charge, id: charge.chargeId })) || []; + } else if (this.chargesDataSource.length === 0 && this.loansAccountProductTemplate?.charges?.length) { + this.chargesDataSource = + this.loansAccountProductTemplate.charges.map((charge: any) => ({ ...charge, id: charge.chargeId })) || []; + }Follow-up: consider moving init from ngOnInit to ngOnChanges and deleting the ngOnInit block that sets chargesDataSource to avoid duplication. Based on coding guidelines.
320-323: Confirm business rule: should duplicate chargeId entries ever be allowed?If some charge types legitimately appear multiple times (e.g., specified due dates), hard dedup by id will drop them. If duplicates are only the bug scenario (WEB-117), proceed; otherwise, key by a composite (id + dueDate) when the chargeTimeType allows multiples.
I can implement conditional dedup (by id, or by id+date when charge.allowMultiple) if you confirm the rule.
141-146: *Add a trackBy function to the charges table’s ngFor
Define and reference a trackBy inloans-account-charges-step.component.html(e.g.<tr *ngFor="let charge of chargesDataSource; trackBy: trackByChargeId">) and add to the component:trackByChargeId(_: number, charge: any) { return charge.id; }This ensures row identity is preserved after array updates, avoiding unnecessary DOM churn.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/loans/edit-loans-account/edit-loans-account.component.ts(1 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts(2 hunks)
🧰 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/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.tssrc/app/loans/edit-loans-account/edit-loans-account.component.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
src/app/loans/edit-loans-account/edit-loans-account.component.ts
Outdated
Show resolved
Hide resolved
...ans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-117-loan-account-charge-amount-charges-doubling-on-application-modification
branch
from
097e45d to
f31b61e
Compare
|
@steinwinde Please Review . |
| const loanType = 'individual'; | ||
| const uniqueCharges = new Map<number | string, any>(); | ||
| (this.loansAccount.charges ?? []).forEach((charge: any) => { | ||
| const chargeId = charge.id ?? charge.chargeId; |
There was a problem hiding this comment.
Why we are doing this? I mean the Charge has an attribute for the Id and that's all
|
@alberto-art3ch Initially, I had fixed the charge doubling issue by using a Map to track unique charges based on both id and chargeId fields. After that coderabbit has review the code and it suggested to add proper typing and null checking to handle edege cases . |
|
@IOhacker , @steinwinde , @alberto-art3ch Please Review . |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai is this correct solution? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 19970 @gkbishnoi07, the solution partially addresses the issue but has some concerns: Issues with Current Implementation
Recommendations
The solution will prevent duplicate charges from being submitted, but understanding and fixing the root cause would be more robust. Would you like me to help investigate where the duplication originates? |
3 participants
Changes Made :-
-Fixed issue where charges were doubling when editing a loan application.
-Added code to prevent duplicate charges by checking for existing ones.
-Updated how charges are handled during loan modifications to maintain correct values.
WEB-117
Summary by CodeRabbit
Bug Fixes
Improvements